fix race condition in string generator helper

[fairclothjm]

A data race in vault/helper/random.(*StringGenerator).validateConfig() was exposed by new text coverage that was recently introduced.

WARNING: DATA RACE
Read at 0x00c005d77000 by goroutine 10158:
  github.com/hashicorp/vault/helper/random.(*StringGenerator).validateConfig()
      /home/circleci/go/src/github.com/hashicorp/vault/helper/random/string_generator.go:243 +0x3e6

and

Previous write at 0x000006f28ae0 by goroutine 10155:
  github.com/hashicorp/vault/helper/random.(*StringGenerator).validateConfig()
      /home/circleci/go/src/github.com/hashicorp/vault/helper/random/string_generator.go:238 +0x293

Failure: https://app.circleci.com/pipelines/github/hashicorp/vault/51983/workflows/d654323c-baa8-4985-9838-a4131f8900f5/jobs/607405/steps

[robmonte]

Can you explain why locking is needed for setting a variable like this?

[cipherboy]

The lock isn't necessary for the write to the local charset variable, but the read of the g.charset value.

In validateConfig:

if len(g.charset) == 0 {
    g.charset = getChars(g.Rules)
}

so if there's parallel access to both validateConfig and generate, we will race reading this variable. Since this assignment isn't an atomic write (its a []rune slice, which is a pointer type), you might read a bad pointer (like, half of the pointer's value could be copied over when you read, so you'd point to a garbage memory location) and thus fail.

In particular, I think when you first initialize the group and set it up to do a password rotation, if you have a lot of in-flight requests, you'll see multiple try and write this charset via validating the config I believe, and some which will be past it trying to do this generate read already.

Or maybe it is parallel modification + use of the policy...

[fairclothjm]

Thanks @cipherboy! That is a great summary.

@robmonte I am happy to talk more about this if you have more questions. Let me know!

[austingebauer]

👍

[cipherboy]

Makes sense to me!

[cipherboy]

The lock isn't necessary for the write to the local charset variable, but the read of the g.charset value.

In validateConfig:

if len(g.charset) == 0 {
    g.charset = getChars(g.Rules)
}

so if there's parallel access to both validateConfig and generate, we will race reading this variable. Since this assignment isn't an atomic write (its a []rune slice, which is a pointer type), you might read a bad pointer (like, half of the pointer's value could be copied over when you read, so you'd point to a garbage memory location) and thus fail.

In particular, I think when you first initialize the group and set it up to do a password rotation, if you have a lot of in-flight requests, you'll see multiple try and write this charset via validating the config I believe, and some which will be past it trying to do this generate read already.

Or maybe it is parallel modification + use of the policy...