Adding Keyvault Shim #1346
Adding Keyvault Shim #1346
Conversation
timraymond
left a comment
timraymond
left a comment
There was a problem hiding this comment.
Looks pretty good. Just have some questions and thoughts.
| // NewShim constructs a Shim for a KeyVault instance located at the provided url. The azcore.TokenCredential will | ||
| // only be used during method calls, it is not verified at initialization. | ||
| func NewShim(vaultURL string, cred azcore.TokenCredential) (*Shim, error) { | ||
| c, err := azsecrets.NewClient(vaultURL, cred, nil) |
There was a problem hiding this comment.
If you make sf public, you can eliminate this constructor and the dep on azcore. It might also let you test with the keyvault_test package as well since you'd no longer need access to private members.
There was a problem hiding this comment.
These days I'm defaulting to provide constructors for types that don't have a useful zero value. Without these dependencies, the methods below are broken. This package also aims to abstract away the azsecrets package thus why the constructor manages the creation of the sdk client, but azcore still seemed to be ok to demand from the client since that seems to be the preferred abstraction for different auth methods to azure services (cli, msi, client creds, etc) and is not necessarily tied to keyvault (i.e. the token credential might already be in use for other sdk clients).
By doing away with the constructor we might be open to potential misuse of the type if the deps are not correctly provided and are requiring the caller to import azsecrets on top of this package. Definitely open to discussion around this. Thoughts @timraymond @rbtr?
There was a problem hiding this comment.
I think a New is appropriate here; my benchmark is: if I incompletely initialize this struct will things break, that seems true here.
There was a problem hiding this comment.
Mmm... true, it would panic otherwise without extra (annoying) checks. I agree with keeping the constructor.
| } | ||
|
|
||
| // Shim provides convenience methods for working with KeyVault. | ||
| type Shim struct { |
There was a problem hiding this comment.
Would CertificateProvider be closer to what this does?
There was a problem hiding this comment.
Yeah, given the api surface that is probably the most apt description. Maybe it's a bit premature to think our keyvault usage will grow beyond retrieving certs, but my original thinking was along those lines: this type is a wrapper over the az keyvault sdk that we can extend to simplify keyvault operations as needed.
| return tls.Certificate{}, errors.Wrap(err, "could not get secret") | ||
| } | ||
|
|
||
| pemBlocks, err := getPEMBlocks(*resp.Properties.ContentType, *resp.Value) |
There was a problem hiding this comment.
Does PEM follow MIME to the letter? Like, could you end up with surprise things after the media type (e.g., as absurd as this would be: application/x-pem-file; charset=utf-8. There's a mime.ParseMediaType that does the right thing, if so.
There was a problem hiding this comment.
For keyvault-generated certs (our target use case), what I've observed are these specific content types, though documentation about kv secrets is not totally clear.
| logger.Info("response from tls server", zap.String("body bytes", string(bs))) | ||
| } | ||
|
|
||
| func createClientTLSConfig(tlsCert tls.Certificate) (*tls.Config, error) { |
There was a problem hiding this comment.
I think this would be nice to have pushed down into the keyvault package... usually when you go to create an http.Server with TLS enabled, you hit that TLSConfig section and have to look up a bunch of documentation on how to assemble the certs, the root CA, etc., etc. Ideally, it would be "just drop whatever gets returned from this into TLSConfig."
* adding az sdk dependencies and tidying mod file * adding keyvault shim * example usage application for kv shim * adding tests, cleaning up * fixing linter errors * updating go mod
3 participants
Reason for Change:
As part of enabling zero touch cert rotation for CNS deployments, we need to be able to fetch TLS certificates from Keyvault using MSI. This PR introduces the
keyvaultpackage, the main export of which is akeyvault.Shimwhich should make it easy to retrieve Keyvault certificates and transform them into the appropriate types for consumption in a Go http server.Issue Fixed:
Requirements:
Notes:
The added modules from the Azure SDK make use of generics, which are known to be not fully compatible with the linter/CI