nitro configuration management #1385
Conversation
…ts conf item which will retrieve secrets
There was a problem hiding this comment.
Overall this seems like a sound implementation with a workable design. When reading through it though I began to realize that there's a few ways that we could simplify the design, but I'm not certain of the requirements that led us to this design or that I'm properly understanding the bigger picture of this. Let's talk through this today during our sync meeting.
| func initService(ctx context.Context) (*Service, error) { | ||
| logger := logging.Logger(ctx, "initService") | ||
| // generate the ed25519 pub/priv keypair for secrets management | ||
| pubKey, privKey, err := box.GenerateKey(rand.Reader) |
There was a problem hiding this comment.
This looks like a good way to generate a new key per service on the fly. We're relying on a well known CSPRNG and using the nacl box implementation (a well known port of Nacl to Go-lang) to perform key generation.
| } | ||
|
|
||
| // initialize the service | ||
| func initService(ctx context.Context) (*Service, error) { |
There was a problem hiding this comment.
The only place that I see this is being called at the moment is via the controllers_test.go file. Is the expectation here that we'd be generating an ephemeral keypair for each time that this is called (e.g. in the GET /conf handler) or would this be called once on initialization of the microservice? This is my first time looking at a Go backend microservice so if I'm missing something obvious my apologies.
There was a problem hiding this comment.
https://github.com/brave-intl/bat-go/blob/nitro-payments-dev/ this is the branch which holds all the collated code, and uses initService in the main microservice.
| } | ||
|
|
||
| var decryptKey [32]byte | ||
| copy(decryptKey[:], key[:32]) |
There was a problem hiding this comment.
Is there other data that's included in this "key" array? Ideally we wouldn't want to copy this key material to a separate piece of memory if not necessary and we'd ideally want to be cleaning up the memory ourselves by overwriting all places where private key material is stored in memory once we're certain they'll no longer be used rather than relying on the garbage collector for this. The main reason for this is to reduce the possiblity of someone discovering the key by inspecting the memory.
There was a problem hiding this comment.
@husobee: reuse key instead of performing copy
There was a problem hiding this comment.
@husobee: zeroize capabilities in constant time.
| var decryptKey [32]byte | ||
| copy(decryptKey[:], key[:32]) | ||
|
|
||
| // decrypted is the encryption key used to decrypt secrets now |
There was a problem hiding this comment.
| // decrypted is the encryption key used to decrypt secrets now | |
| // decryptKey is the symmetrical encryption key used to decrypt configuration secrets now |
| copy(senderKeyT[:], senderKey[:32]) | ||
|
|
||
| var privKey [32]byte | ||
| copy(privKey[:], s.privKey[:32]) |
There was a problem hiding this comment.
Same here - Ideally we wouldn't want to be making a copy of the privateKey if we don't need to. I'm uncertain if this copy is done in constant time which could create a side channel attack for the private key.
Since we didn't generate the s.privKey within the scope of this function we'd not want to overwrite it though so that's fine as is. If we do want to make copies of the key here (I need to dig more into this) then we would want to clean them up after usage on line 70 since we know the scope of usage of this key.
I'll do some digging to see if I can find a good way to overwrite an array with all zeros in constant time and also if there's an issue calling this copy function to copy private keys.
There was a problem hiding this comment.
Double checked the copy operation in the spec. As far as I can tell this shouldn't be an issue. Here's a PoC I wrote as well to check the assembly.
|
|
||
| // PaymentsEncryptionKeyCTXKey - the context key for getting the application secrets file location | ||
| PaymentsEncryptionKeyCTXKey CTXKey = "payments_encryption_key" | ||
| // PaymentsSenderPublicKeyCTXKey - the context key for getting the application secrets file location |
There was a problem hiding this comment.
Same here - can you double check this one as well
There was a problem hiding this comment.
@husobee: adjust comment accordingly
| @@ -0,0 +1,10 @@ | |||
| package service | |||
There was a problem hiding this comment.
nit: Can we rename this file to SecretManager.go to reflect the interface being defined in this file
There was a problem hiding this comment.
@husobee: to fix name file
| ) | ||
|
|
||
| // ConfigurationMiddleware applies the current state of the service's configuration on the ctx | ||
| func (service *Service) ConfigurationMiddleware() func(http.Handler) http.Handler { |
There was a problem hiding this comment.
nit: would be nice to have this named PaymentConfigurationMiddleware to more accurately reflect it's purpose since it's not a generic Configuration middleware
There was a problem hiding this comment.
@husobee to adjust naming to reflect
| } | ||
| } | ||
|
|
||
| func TestGetConfigurationHandler(t *testing.T) { |
There was a problem hiding this comment.
This test looks good to me and easy to read - nice work!
| reqBody := configurationHandlerRequest(map[appctx.CTXKey]interface{}{ | ||
| appctx.CommitCTXKey: "value", // commit is a configuration pushed in | ||
| appctx.SecretsURICTXKey: "secrets uri", // tell configuration to pull new secrets | ||
| appctx.PaymentsEncryptionKeyCTXKey: base64.StdEncoding.EncodeToString(encrypted), // tell configuration to pull new secrets |
There was a problem hiding this comment.
What's the threat model and motivation here that led to us using the nitro_shim to encrypt a blob and store it in an S3 bucket and then encrypt the symmetrical key for the payments service? I can think of a few ways that we could simplify this architecture, but I'm not certain of the requirements that led us to this construction or that I properly understand the design so I want to understand these things first before suggesting changes.
There was a problem hiding this comment.
Possibly we can figure a way to archive/eliminate existing s3 buckets
Summary
configuration handler to dynamically set configurations, with a secrets conf item which will retrieve secrets
Type of change ( select one )
Tested Environments
Before submitting this PR:
Manual Test Plan: