adds support for AWS KMS by ChrisJBurns · Pull Request #1141 · octo-sts/app

ChrisJBurns · 2025-11-16T19:02:16Z

Recreates #892

For some reason the underlying repo that had the original PR code was deleted. This PR restores that contribution because like others I would still very much be interested in hosting a version of the OctoSTS app on AWS using AWS KMS and Secrets Manager.

implements #438

Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com>

ChrisJBurns · 2025-11-30T16:22:45Z

@cpanato @wlynch @wwsean08 @imjasonh @mattmoor Apologies for the ping once again on this, am aiming for AWS Support for Octo-STS as an early Christmas present 😆

pkg/kms/gcp/gcp.go

cpanato · 2025-12-09T16:05:47Z

need a rebase

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

ChrisJBurns · 2025-12-09T17:43:40Z

@cpanato Thanks for the above, just rebased and reverted the copyright date back to 2024.

cpanato

lgtm

seems ok
thank you
would like to have another pair of 👀

ChrisJBurns · 2025-12-10T18:42:58Z

Amazing, thank you @cpanato much appreciated! 🚀

wlynch

Some minor comments, but overall looks good!

pkg/secrets/gcp/gcp_test.go

wlynch · 2025-12-16T16:47:46Z

pkg/secrets/gcp/gcp_test.go

+	// Set up the fake server.
+	impl := &fakeSecretManager{}
+	l, err := net.Listen("tcp", "localhost:0")
+	assert.NoError(t, err)
+
+	gsrv := grpc.NewServer()
+	secretmanagerpb.RegisterSecretManagerServiceServer(gsrv, impl)
+	fakeServerAddr := l.Addr().String()
+	go func() {
+		if err := gsrv.Serve(l); err != nil {
+			panic(err)
+		}
+	}()
+	defer gsrv.Stop()
+
+	// Create a client.
+	client, err := secretmanager.NewClient(ctx,
+		option.WithEndpoint(fakeServerAddr),
+		option.WithoutAuthentication(),
+		option.WithGRPCDialOption(grpc.WithTransportCredentials(insecure.NewCredentials())),
+	)
+	assert.NoError(t, err)


Pull this into a func (use t.Cleanup instead of defer for closing the client)

addressed in commit 1a4be64

wlynch · 2025-12-16T16:48:34Z

pkg/secrets/secrets.go

+	awsSecretManager *awsSM.Client
+}
+
+func (s *secretProvider) GetSecret(keyID string) ([]byte, error) {


Can we plumb the context through here instead of embedding it in the struct?

We only do this for the signers in order to comply with the jwt.SigningMethod interface, but otherwise the preference would be to plumb the context through so it can be configured per-call.

Addressed in commit 44ccfcd

wlynch · 2025-12-16T16:52:36Z

pkg/kms/gcp/gcp.go

+// NewSigner returns a new signer for the provider.
+func (p *Provider) NewSigner() (ghinstallation.Signer, error) {
+	return p, nil
+}


You can delete this - this isn't really adding anything except proving *Provider implements Signer (which we could also do with var _ ghinstallation.Signer = &Provider{}

Addressed in commit 57d1b72

wlynch · 2025-12-16T16:53:34Z

pkg/kms/gcp/gcp.go

 	key    string
 }

-func New(_ context.Context, client *kms.KeyManagementClient, key string) (ghinstallation.Signer, error) {


I'm not sure if anyone is using this externally - keep the function signature in place as a wrapper around NewProvider (you probably need to add a NewProviderWithClient) and mark this as deprecated.

Addressed as part of 12bd48a

ChrisJBurns · 2025-12-16T16:58:05Z

Much appreciated @wlynch ❤️ I'll get right on addressing the PR comments. Wondering if you had a chance to merge a fix for #892 (comment). As far as I know it still happened the last time I checked. I'll be trying it all again anyways with an updated version of OctoSTS incase there were fixes that were merged more recently that resolved it.

ChrisJBurns · 2025-12-18T21:20:57Z

@wlynch I can confirm that I have got this working in AWS now! Not sure if the infinite-loop issue that was detailed in the original PR has been fixed on main or whether it was the fact that I disabled all events except for PR event and push events. Either way - it works!!

I'll get right on the addressing of PR comments above, in addition, I'm not sure if you'd like me to submit an "example" AWS Terraform setup using AWS ECS, Secrets Manager and KMS. I'm happy to do so, but it will obviously likely get outdated as time goes by and the Terraform modules/AWS APIs evolve.

Co-authored-by: Billy Lynch <1844673+wlynch@users.noreply.github.com> Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

wlynch · 2025-12-19T23:39:35Z

I haven't attempted again since #988 - I'll probably have some time over the break to look at this again though! Glad to hear it's working for you now though :)

…erServiceServer Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

ChrisJBurns · 2025-12-21T19:13:34Z

@wlynch No problems, I've just pushed up several commits that address your review comments, let me know what you think! - Happy Christmas 🎅

jgrumboe · 2025-12-22T08:23:18Z

Hi @ChrisJBurns
This PR is really great. I'm also looking into hosting the app on AWS.
Will you also update the IaC in the iac and modules/app folder?

ChrisJBurns · 2025-12-22T15:30:49Z

@jgrumboe I can certainly submit an extra PR after this one if the folks at Chainguard are happy. However I feel it may get outdated as the Terraform and AWS APIs evolve, but maybe as an initial reference its ok.

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

ChrisJBurns · 2026-01-29T18:55:11Z

Hey @wlynch, hope you had a great Christmas and New Year, any chance of a lil review of my new changes?

lf-

A bit of absolutely non-blocking feedback: you can integration test the crypto of the AWS KMS using https://github.com/nsmithuk/local-kms if you'd like :)

I don't, however, think the code is especially possible to screw up, so integration tests aren't necessarily very critical. The reason I know a bit about this is because I just wrote another KMS integration, from which one might be able to borrow some ideas for such an integration test: indygreg/apple-platform-rs#274

ChrisJBurns added 2 commits November 16, 2025 18:57

adds support for aws kms

363a903

Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com>

forgot import

b27ec2d

Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com>

cpanato reviewed Dec 9, 2025

View reviewed changes

pkg/kms/gcp/gcp.go Show resolved Hide resolved

ChrisJBurns added 2 commits December 9, 2025 17:42

Merge remote-tracking branch 'upstream/main'

b38388a

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

revert back to previous copyright date

896f0bf

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

cpanato previously approved these changes Dec 10, 2025

View reviewed changes

cpanato requested review from mattmoor and wlynch December 10, 2025 10:18

wlynch reviewed Dec 16, 2025

View reviewed changes

Update pkg/secrets/gcp/gcp_test.go

5cfde90

Co-authored-by: Billy Lynch <1844673+wlynch@users.noreply.github.com> Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

ChrisJBurns dismissed cpanato’s stale review via 5cfde90 December 18, 2025 21:59

Update pkg/secrets/gcp/gcp_test.go

b3d182f

Co-authored-by: Billy Lynch <1844673+wlynch@users.noreply.github.com> Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

ChrisJBurns added 5 commits December 21, 2025 18:46

removes unneeded functions by using embedded UnimplementedSecretManag…

19c7810

…erServiceServer Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

refactor: simplify gcp secret manager test setup

1a4be64

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

plumb context through secret methods for per call

44ccfcd

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

removed the redundant NewSigner() method

57d1b72

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

adds NewProviderWithClient and deprecates old New()

12bd48a

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

Merge branch 'main' of github.com:octo-sts/app

7f7d1d8

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>

lf- reviewed Feb 4, 2026

View reviewed changes

Conversation

ChrisJBurns commented Nov 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisJBurns commented Nov 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cpanato commented Dec 9, 2025

Uh oh!

ChrisJBurns commented Dec 9, 2025

Uh oh!

cpanato left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisJBurns commented Dec 10, 2025

Uh oh!

wlynch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisJBurns commented Dec 16, 2025

Uh oh!

ChrisJBurns commented Dec 18, 2025

Uh oh!

wlynch commented Dec 19, 2025

Uh oh!

ChrisJBurns commented Dec 21, 2025

Uh oh!

jgrumboe commented Dec 22, 2025

Uh oh!

ChrisJBurns commented Dec 22, 2025

Uh oh!

ChrisJBurns commented Jan 29, 2026

Uh oh!

lf- left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ChrisJBurns commented Nov 16, 2025 •

edited

Loading

ChrisJBurns commented Nov 30, 2025 •

edited

Loading