Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support AWS KMS credentials using decryption secretRef #641

Merged
merged 7 commits into from
May 20, 2022

Conversation

aryan9600
Copy link
Member

Adds support for using .spec.decryption to refer to custom AWS KMS credentials and override global decryption configuration.

Signed-off-by: Sanskar Jaiswal sanskar.jaiswal@weave.works

@aryan9600 aryan9600 force-pushed the aws-kms-decryption branch 2 times, most recently from 4fb4b50 to 92851a2 Compare April 28, 2022 06:19
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hiddeco hiddeco self-requested a review April 28, 2022 13:38
@hiddeco hiddeco added enhancement New feature or request area/sops SOPS related issues and pull requests labels Apr 28, 2022
@aryan9600
Copy link
Member Author

@stefanprodan I think it might be redundant to explain IRSA in both the sops guide and the kustomization docs, since IRSA is sufficiently explained in the sops guide (the wording can be improved to be clearer though), so maybe we can link that guide in the kustomization docs?

@stefanprodan
Copy link
Member

@aryan9600 it's fine to link to the guide, but we need a section like this one https://github.com/fluxcd/kustomize-controller/blob/main/docs/spec/v1beta2/kustomization.md#gcp-kms for AWS.

@aryan9600
Copy link
Member Author

@stefanprodan we already have that, if i'm not mistaken: https://github.com/fluxcd/kustomize-controller/blob/main/docs/spec/v1beta2/kustomization.md#aws. we could add the link to the sops guide (and make the guide clearer) in this section?

@hiddeco
Copy link
Member

hiddeco commented Apr 28, 2022

There is no documentation which instructs how to supply a SecretRef and what data the Secret must contain for AWS. Which is explained in detail for any other custom implementation we have. See: https://github.com/fluxcd/kustomize-controller/blob/main/docs/spec/v1beta2/kustomization.md#azure-key-vault-secret-entry

@aryan9600
Copy link
Member Author

Yep, I'm working on that, I was talking about the ISRA stuff 😅

@hiddeco hiddeco added this to the GA milestone Apr 29, 2022
@aryan9600
Copy link
Member Author

Fuzzing tests are failing because of invalid fuzzing tests in aws-sdk-go-v2, ref: aws/aws-sdk-go-v2#1693

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is shaping up well, couple of nitpicks mostly around making our lives easier when we look at this again in a couple months 😄

internal/sops/awskms/keysource.go Outdated Show resolved Hide resolved
internal/sops/awskms/keysource.go Outdated Show resolved Hide resolved
internal/sops/awskms/keysource.go Outdated Show resolved Hide resolved
internal/sops/awskms/keysource.go Outdated Show resolved Hide resolved
internal/sops/awskms/keysource.go Outdated Show resolved Hide resolved
internal/sops/awskms/keysource.go Outdated Show resolved Hide resolved
internal/sops/awskms/keysource.go Show resolved Hide resolved
internal/sops/awskms/keysource.go Outdated Show resolved Hide resolved
internal/sops/awskms/keysource.go Outdated Show resolved Hide resolved
internal/sops/awskms/keysource.go Show resolved Hide resolved
@hiddeco hiddeco changed the title Add support for AWS KMS credentials using .spec.decryption Support AWS KMS credentials using decryption secretRef May 3, 2022
@aryan9600 aryan9600 force-pushed the aws-kms-decryption branch 3 times, most recently from 6bfdba4 to 2942001 Compare May 4, 2022 08:13
internal/sops/awskms/keysource.go Show resolved Hide resolved
internal/sops/awskms/keysource.go Show resolved Hide resolved
Comment on lines 66 to 67
// CredsProvider is a wrapper around aws.CredentialsProvider used for authenticating
// when using AWS KMS.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// CredsProvider is a wrapper around aws.CredentialsProvider used for authenticating
// when using AWS KMS.
// CredsProvider is a wrapper around aws.CredentialsProvider used for authenticating
// towards AWS KMS.

// createKMSConfig returns a Config configured with the appropriate credentials.
func (key MasterKey) createKMSConfig() (*aws.Config, error) {
// Uses the credentialsProvider if present, otherwise default to reading credentials
// from the enviornment.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// from the enviornment.
// from the environment.

}

// ToMap converts the MasterKey to a map for serialization purposes.
func (key MasterKey) ToMap() map[string]interface{} {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be above private methods.

Comment on lines 59 to 73
// epResolver IS ONLY MEANT TO BE USED FOR TESTS.
// it can be used to override the endpoint that the AWS client resolves to
// by default. it's hacky but there is no other choice, since you can't
// specify the endpoint as an env var like you can do with an access key.
epResolver aws.EndpointResolver
Copy link
Member

@hiddeco hiddeco May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// epResolver IS ONLY MEANT TO BE USED FOR TESTS.
// it can be used to override the endpoint that the AWS client resolves to
// by default. it's hacky but there is no other choice, since you can't
// specify the endpoint as an env var like you can do with an access key.
epResolver aws.EndpointResolver
// epResolver can be used to override the endpoint the AWS client resolves
// to by default. This is mostly used for testing purposes as it can not be
// injected using e.g. an environment variable. The field is not publicly
// exposed, nor configurable.
epResolver aws.EndpointResolver

@@ -56,6 +57,16 @@ func (o WithAgeIdentities) ApplyToServer(s *Server) {
s.ageIdentities = age.ParsedIdentities(o)
}

// WithAWSKeys configurs the AWS credentials on the Server
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// WithAWSKeys configurs the AWS credentials on the Server
// WithAWSKeys configures the AWS credentials on the Server.

@@ -43,6 +44,11 @@ type Server struct {
// When nil, the request will be handled by defaultServer.
azureToken *azkv.Token

// awsCredsProvider is the Credentials object used for Encrypt and Decrypt
// operations of AWS KMS requests.
// When nil, the request will be handled by defaultServer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer correct.

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stellar job 💯

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two tiny nits, apart from that LGTM

great work @aryan9600!

docs/spec/v1beta2/kustomization.md Outdated Show resolved Hide resolved
internal/sops/awskms/keysource.go Outdated Show resolved Hide resolved
@stefanprodan
Copy link
Member

stefanprodan commented May 9, 2022

What are we going to do about fuzzing? We can't merge this and have all PRs from now on failing. Can we disable the AWS tests?

@pjbgf
Copy link
Member

pjbgf commented May 9, 2022

What are going to do about fuzzing? We can't merge this and have all PRs from now on failing. Can we disable the AWS tests?

We will disable them for the time being whilst we wait for the fix upstream. The approach we discussed so far is picking a different conditional tag for our own fuzz tests.

Sanskar Jaiswal added 5 commits May 19, 2022 14:33
Signed-off-by: Sanskar Jaiswal <sanskar.jaiswal@weave.works>
Signed-off-by: Sanskar Jaiswal <sanskar.jaiswal@weave.works>
Signed-off-by: Sanskar Jaiswal <sanskar.jaiswal@weave.works>
Signed-off-by: Sanskar Jaiswal <sanskar.jaiswal@weave.works>
Signed-off-by: Sanskar Jaiswal <sanskar.jaiswal@weave.works>
@aryan9600 aryan9600 force-pushed the aws-kms-decryption branch 4 times, most recently from 8873d41 to 3f61e59 Compare May 19, 2022 14:13
Signed-off-by: Sanskar Jaiswal <sanskar.jaiswal@weave.works>
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @aryan9600! 👏 👏

LGTM

@hiddeco
Copy link
Member

hiddeco commented May 20, 2022

Can we please change the license on this (internal/sops/awskms) to MPL 2.0 before we merge.

Signed-off-by: Sanskar Jaiswal <sanskar.jaiswal@weave.works>
@hiddeco hiddeco merged commit 4cde465 into fluxcd:main May 20, 2022
@stefanprodan stefanprodan modified the milestones: GA, Bootstrap GA Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sops SOPS related issues and pull requests enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants