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.

Sign up for GitHub

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

Jump to bottom

Support GCP KMS credentials using decryption secretRef #635

Merged
merged 2 commits into from
May 24, 2022
Merged

Support GCP KMS credentials using decryption secretRef #635

merged 2 commits into from
May 24, 2022

Conversation

somtochiama
Copy link
Member

@somtochiama somtochiama commented Apr 22, 2022 •
edited by stefanprodan
Loading

This pull request adds support for specifying the credentials for authenticating to GCP KMS with the decryption secret.
GCP KMS credentials should be provided under sops.gcp-kms

Fix: #360
Ref: #324

hiddeco
hiddeco reviewed Apr 22, 2022
View reviewed changes
internal/sops/gcpkms/keysource.go Outdated Show resolved Hide resolved
internal/sops/keyservice/server.go Outdated
@@ -106,6 +111,16 @@ func (ks Server) Encrypt(ctx context.Context, req *keyservice.EncryptRequest) (*
Ciphertext: ciphertext,
}, nil
}
case *keyservice.Key_GcpKmsKey:
if ks.gcpCredsJSON != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making the credentials optional, I would opt for making the credentials optional for the client within the key. Falling back to defaults (without credentials) there.

In retrospect, that would also have been better for the others that are currently falling back, as SOPS is slow in client updates which we already control.

internal/sops/gcpkms/keysource_test.go
}))
}

func TestMasterKey_createCloudKMSService(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

You can quite easily mock the Google Cloud API for testing purposes to add further tests, this is also done in the source-controller.

@hiddeco hiddeco added enhancement New feature or request area/sops SOPS related issues and pull requests labels Apr 22, 2022
@hiddeco hiddeco changed the title [SOPS] Add support for GCP KMS credentials using decryption secretRef Support fGCP KMS credentials using decryption secretRef Apr 22, 2022
@hiddeco hiddeco changed the title Support fGCP KMS credentials using decryption secretRef Support GCP KMS credentials using decryption secretRef Apr 22, 2022
@somtochiama somtochiama requested a review from hiddeco April 25, 2022 16:45
@somtochiama somtochiama marked this pull request as ready for review April 26, 2022 09:38
@hiddeco
Copy link
Member

hiddeco commented Apr 26, 2022

@somtochiama just wanted to let you know that this is in my queue to get reviewed. But given it would require a minor bump, I am giving priority to some things that can end up patches first. Will get to this on Thursday, hopefully :-)

stefanprodan
stefanprodan reviewed Apr 28, 2022
View reviewed changes
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.

Please explain in the API docs how users can setup static credentials for GCP https://github.com/fluxcd/kustomize-controller/blob/main/docs/spec/v1beta2/kustomization.md#decryption-secret-reference

hiddeco
hiddeco reviewed Apr 28, 2022
View reviewed changes
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 looking good! Couple of tiny nitpicks, other than that it is 💯

internal/sops/keyservice/server.go Outdated Show resolved Hide resolved
internal/sops/gcpkms/keysource_test.go Outdated Show resolved Hide resolved
internal/sops/gcpkms/keysource.go Outdated Show resolved Hide resolved
internal/sops/gcpkms/keysource_test.go Outdated Show resolved Hide resolved
internal/sops/gcpkms/keysource_test.go Show resolved Hide resolved
internal/sops/gcpkms/keysource_test.go Show resolved Hide resolved
internal/sops/gcpkms/keysource.go Outdated Show resolved Hide resolved
internal/sops/gcpkms/keysource.go Show resolved Hide resolved
internal/sops/gcpkms/keysource.go Show resolved Hide resolved
internal/sops/gcpkms/keysource.go Show resolved Hide resolved
@hiddeco hiddeco mentioned this pull request Apr 29, 2022
Merged
@hiddeco hiddeco added this to the GA milestone Apr 29, 2022
@somtochiama somtochiama force-pushed the gcp-kms-cred branch 3 times, most recently from f305cab to 73ceb26 Compare May 12, 2022 21:19
@somtochiama somtochiama requested a review from hiddeco May 16, 2022 09:50
@stefanprodan
Copy link
Member

@somtochiama please rebase

@hiddeco hiddeco force-pushed the gcp-kms-cred branch 2 times, most recently from 5de2996 to 106aae7 Compare May 24, 2022 12:29
@somtochiama somtochiama force-pushed the gcp-kms-cred branch 2 times, most recently from cabfae0 to d4e3cd2 Compare May 24, 2022 18:31
@hiddeco hiddeco force-pushed the gcp-kms-cred branch 4 times, most recently from eca5359 to ffeca02 Compare May 24, 2022 19:45
@somtochiama @hiddeco
sops/gcpkms: add key source implementation
2ac475e 
This adds a SOPS GCP KMS key source which makes use of the latest GCP
client, and supports both injection of master key credentials and a
default client making use of environmental runtime values.

The implementation fully replaces SOPS', and is covered with
compatability tests.

Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@hiddeco
sops/gcpkms: disable integration tests
cbb0fc9 
This temporarily disables the integration tests as we are waiting for
the CNCF to provide us with GCP credits.

Signed-off-by: Hidde Beydals <hello@hidde.co>
hiddeco
hiddeco approved these changes May 24, 2022
View reviewed changes
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.

LGTM. Thanks a lot for following up on the SDK change and tests @somtochiama 🥇

@hiddeco hiddeco merged commit fec5316 into fluxcd:main May 24, 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
Reviewers

@stefanprodan stefanprodan stefanprodan left review comments

@hiddeco hiddeco hiddeco approved these changes

Assignees
No one assigned
Labels
area/sops SOPS related issues and pull requests enhancement New feature or request
Projects
Maintainers' Focus
Archived in project
Milestone
Bootstrap GA
Development

Successfully merging this pull request may close these issues.

Kustomize: Inline cloud provider decryption configuration
3 participants
@somtochiama @hiddeco @stefanprodan