Add Infisical KMS provider#88
Conversation
…nfisical_kms_integration_test.go`, and `infisical_kms_test.go` files. - Add support for Infisical in the secrets management system, including methods for putting, getting, and deleting secrets. - Implement integration tests for full lifecycle operations with Infisical KMS.
…dependency changes, implement `infisical/client.go` for KMS client functionality, and modify tests in `infisical_kms_test.go` to align with new client structure.
|
Hi @Adityadan, can we please get a review on this PR? It should be good to go with no new dependencies! |
adityadani
left a comment
adityadani
left a comment
There was a problem hiding this comment.
In general this library provides three modes:
- Integration with KMS providers that generate a "DEK" which then can be used for encrypting data/credentials etc. -> Vaulttransit, AWS KMS etc
- Integration with KMS providers that actually store the secrets used for encryption with them -> Vault, Azure KV etc
- Google Cloud is another mode and a special case which is similar to what you have implemented which is to encrypt the provided data and store it in the persistence store. But it then supports two KeyContexts: PublicData and CustomData.
Which model suits Infisical better? And if it is 3. can we honor PublicData and CustomData behavior similar to what has been done in Google Cloud so that the clients of this library do not see any changes?
| base += "/api" | ||
| } | ||
| return &kmsClient{ | ||
| httpClient: &http.Client{Timeout: 30 * time.Second}, |
There was a problem hiding this comment.
Does Infisical support TLSConfig? Or can we enforce an "https" URL check to ensure that clientSecret is not passed as plaintext over the wire
There was a problem hiding this comment.
I'll update the code to add HTTPS enforcement.
|
|
||
| c.mu.Lock() | ||
| c.token = result.AccessToken | ||
| c.expiresAt = time.Now().Add(time.Duration(result.ExpiresIn)*time.Second - tokenExpiryBuffer) |
There was a problem hiding this comment.
We wont renew the token if it is revoked before this expiry time. Can we check for 401 Unauthorized errors in doKmsRequest and renew the token?
There was a problem hiding this comment.
If we get a 401 or 403 error (also used for expired tokens in Infisical API), we'll log in again and retry once.
|
|
||
| logrus.WithFields(logrus.Fields{ | ||
| "site": siteURL, | ||
| "kmsKeyID": kmsKeyID, |
There was a problem hiding this comment.
What is a KMS Key ID and is it ok to log it? Is it some sort of a Master Key ?
There was a problem hiding this comment.
This KMS Key ID is not a master key, it's the ID handle that identifies the key on the Infisical KMS server.
I'm dropping this to keep the same pattern used in other providers.
| ciphertext, err := k.client.encrypt(string(jsonBytes)) | ||
| if err != nil { | ||
| return secrets.NoVersion, fmt.Errorf("infisical-kms: encryption failed: %w", err) | ||
| } |
There was a problem hiding this comment.
How do you envision the use of Infisical with this library? The callers of this library in certain cases also expect a KMS provider to store "secret data". I see that Infisical supports that, so in such a case you wont encrypt and return back the ciphertext but instead simply store this "secret data" in the KMS provider.
There was a problem hiding this comment.
Infisical indeed also has a secrets management product. However, for the purposes of Portworx, we are currently only looking to integrate our KMS product. If there's demand for integrating secrets management, we can absolutely look into that in the future.
…ty in `client_test.go`, covering authentication retries, error handling, and secure URL validation.
Hey @adityadani, thank you for the review! The model that suits Infisical is indeed the model 3. I updated the PR to handle PublicData and CustomData behavior as it's done in the Google Cloud provider. I also addressed all the other comments on the PR. |
3 participants
Summary
infisical-kmssecrets backend that delegates encryption/decryption to Infisical KMS