Vault provider

[vineet-garg]

What this PR does / why we need it: Implements encryption provider based on Vault based KMS as described in proposal: PR:888
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes # 49817

Special notes for your reviewer:

Release note:encryption provider based on Vault based KMS

[tjfontaine]

does this need the target branch updated?

[vineet-garg]

It will be nice to have this branch kept synced with Kubernetes/kubernetes:master to avoid large merge conflicts at a later stage.

[prydie]

A go fmt would be good as there's some mixed/misaligned indentation here and a couple of other places.

[vineet-garg]

corrected the code using gofmt -w, also local verification hack/make-rules/../../hack/verify-gofmt.sh going through now.

[kksriram]

@vineet-garg My initial reaction - this commit is too large (110 files). Let's figure out offline how to break this up into smaller commits.

[jhorwit2]

You should use path.Join for all the url building. That will let you remove all the extra trim/slash checking above. right now if config.AuthPath isn't set authPath below will be auth//. path.Join handles all that for you.

[jhorwit2]

What are your thoughts on supporting ReadEnvironment() out of the box. In my experience, this is typically always a feature everyone wants since it allows you to seamlessly integrate with existing vault deployments that depend on the env vars.

You can see them all here.

[vineet-garg]

Its a good idea, Though we need more configuration parameters than just client parameters, like authentication parameters, key names etc.

[kksriram]

@jhorwit2 perhaps as a follow-on PR. I'd like to get the basic support in first.

[jhorwit2]

Is this validation required? The config is validated after it is initially deserialized. Nothing that I saw modifies these values either.

[vineet-garg]

It is not validation, just error handling in a default case. The Default case is not expected to be reachable, but its still good to have the default case.

[jhorwit2]

Should add a comment then to say this is not an expected code path. The fact that it's there currently makes it seem like it's a possible outcome.

[jhorwit2]

should add the type and/or value to the error

[vineet-garg]

values are secrets and cannot be put in error messages. Type "String" is already mentioned in the err message.

[jhorwit2]

Sorry, i meant add the type of the response that was not a string reflect.TypeOf(resp.Data["plaintext"])

[vineet-garg]

fixed

[jhorwit2]

The way this is structured can cause this to panic if err is non-nil. It should be something like:

resp, err := c.client.RawRequest(req)
if err != nil {
  return nil, err
}

defer resp.Body.close()
if resp.StatusCode == 403 {
  return nil, &forbiddenError{version: c.version, err: err}
}

[jhorwit2]

This constant should be a variable in the vault package and used as the key when stripping / adding it to data.

[vineet-garg]

stripping and adding of the provider name prefix to data stored is done in the core kmstransformer code which is not owned by us, It takes the provider name from here and use it for that particular provider.

Separate from this, there is also prefix processing in client.go, it just strips and adds the prefix used by vault itself.
Both prefixes can vary seperately

[vineet-garg]

@jhorwit2 Thanks for reviewing the code, it was very helpful, I have made most of the changes suggested by you. Does the PR look good to you? Once internal review is over, I will create a new branch which has only 2 commits (one for code changes, one for dependencies and build) sync with latest upstream master and run all local verify/test/integration-test again.

[jhorwit2]

I feel these token methods should return (string, error) and the token should only be set in refreshToken.

[vineet-garg]

changed

[jhorwit2]

The pattern here and withRefreshToken seems odd to me. Why doesn't the clientWrapper store the config that it needs to do authentication? That way you could just call encrypt/decrypt on the clientWrapper and it handles the refreshToken retry logic for you. It appears to me you always want to do the encrypt/decrypt action with the refresh token, so it should go down one level into the client wrapper.

[wu-qiang]

Yes, put retry logic to client is another choice. For current implementation, it expects vault.go handle provider logic (for example parsing config, handle keyNames), client.go handle all details that communicates with vault server. I think "refresh token and retry" belongs to provider logic, because using retry or how many time to retry can be configured by provider if we want.

[jhorwit2]

I'm fairly certain there is a race condition here. If the token can only be used once before having to be renewed that would cause encrypt/decrypt calls to fail since they only retry once and multiple goroutines may block waiting on the same refresh token.

[wu-qiang]

Yes, for those tokens with small number of use, encrypt/decrypt may fail. Actually even with more retry (for example retry 5 times), the failure may also happen. Not use retry is just for simplicity. We can make it configurable if needed.

[jhorwit2]

Is this planning to be addressed prior to opening upstream? That seems like a pretty severe issue.

[vineet-garg]

we will address this. There will be performance impact if num of use/expiry time is short, but it will not fail

[jhorwit2]

Is it possible for these tokens to be renewable? If so, shouldn't we be able to do all this w/o locks by calling Renew on the client?

[wu-qiang]

Not all vault tokens are renewable, so we don't consider Renew on vault client.

[jhorwit2]

Should it support renewable tokens as "first class" though instead of defaulting to a refresh approach? That seems like the best performance especially in a large cluster and the least complexity. (no locking required). If not now, I imagine we would want to in the future, so thinking about that in the current design would help make that easier. If you moved the refresh logic into the client you could have a refreshClientWrapper and in the future we could add a renewalClientWrapper with the same interface that's constructed dynamically based on info returned on the original token.

[vineet-garg]

I would defer this to follow-up PR

[jhorwit2]

You don't need to prefix or suffix this with slashes it can just be path.Join(stuff...)

[vineet-garg]

fixed

[jhorwit2]

path.Join -- same goes for appRoleToken, decrypt and other requests calls if there are any.

[vineet-garg]

fixed

[jhorwit2]

errors should start with a lowercase (this goes for other errors like below): https://github.com/golang/go/wiki/CodeReviewComments#error-strings

[vineet-garg]

fixed

[jhorwit2]

To me this still appears like it can fail due to race conditions if the token has a limited number of uses and/or there are enough goroutines making calls to encrypt/decrypt.

[jhorwit2]

Oh, i see what you changed here. This shouldn't technically error anymore; however, it's going to have potentially bad performance when multiple goroutines all fail during the read lock calls and block on the write mutex. I think in its current state it's fine, but I feel there may be push back upstream.

[vineet-garg]

Given the usage, the contention is less likely:

1. There is caching of DEK in the framework code, so most decrypt operations will not require call to KMS providers.
2. Encrypt operations will not be frequent as encrypt will happen only during creation or update of a secret.
3. It is not expected that multiple secrets will be created or updated concurrently frequently.
4. If token policy is chosen to have a reasonable num of use/expiry, 403 will not be that frequent.

As long as technically it is guaranteed to work in rare worst case scenario at a degraded performance, I think it should be good. I can put the explanation in comments so that someone reads and validates the assumptions.

[jhorwit2]

👍 great explanation. Thanks!

[jhorwit2]

Should use https://golang.org/pkg/testing/#T.Run

[vineet-garg]

fixed

[jhorwit2]

@vineet-garg I still have some worries about the locking with race conditions under enough load causing errors since it only retries once.

nit: I feel the tests could be cleaned up and consolidated using table driven tests which is the de facto standard in k8s. For example, TestOneKey and TestMoreThanOneKeys are largely the same tests. You could have a struct like

testCases := []struct{
  name string
  text string
  encryptKeys []string
}{
  { name: "test one key", text: "foo", encryptKeys: []string{"one"}},
  { name: "test two keys", text: "foo", encryptKeys: []string{"one", "two"}},
}

for _, tc := range testCases {
  t.Run(tc.name, func(t *testing.T) {
    for _, key := range tc.ecnryptKeys
        // encypt & store in slice. 

    check the stored ciphers by decrypting. 
  })
}

Also, the tests are fairly integration testy. Does the vault client not implement interfaces you can use to avoid creating test http servers and thus simplify the unit tests? Seems not to implement one. We can do that later since it'll require a new interface + fake client.

edit: the example above was just something i threw together. Idk if it's correct for those appear very similar.

[vineet-garg]

@jhorwit2
I checked the test cases, the only test that could be factored into subtests was TestInvalidConfiguration.
It had that pattern where same behavior was being tested under different inputs and different configurations. In all other test cases, a different behavior is tested in each test case eg. TestOneKey TestMoreThanOneKey

[vineet-garg]

Closing this pull requests as it has a large number of unorganized commits. review comments incorporated and commits organized in new pull request: #4