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

feat: allow webhook to use env vars or cert files for github app secret #470

Conversation

karlhaworth
Copy link
Contributor

feat: allow webhook to use env vars or cert files for github app secret

Builds on top of #412

Signed-off-by: Karl Haworth <karl.haworth@aa.com>
Signed-off-by: Karl Haworth <karl.haworth@aa.com>
Signed-off-by: Karl Haworth <karl.haworth@aa.com>
@wlynch wlynch self-requested a review August 29, 2024 13:21
@karlhaworth
Copy link
Contributor Author

resp, err := secretmanager.AccessSecretVersion(ctx, &secretmanagerpb.AccessSecretVersionRequest{
Name: name,
})
if baseCfg.KMSKey != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why make this conditional on KMSKey? Webhook validation should be independent of kms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the web hook secret in the original code uses the secretmanager which is google KMS specific? If you're not using the KMSKey with google, then the web hook secret can be inputted as an env var. Does that make sense or am I off?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a little odd because they are technically independent of each other - KMS is used for outbound auth, whereas the webhook secret is use for inbound validation.

I see what you're saying though about it's a good indication that you want to enable Cloud provider support. I suspect we might want to add a separate envvar (or prefix the kms/secret names with a prefix scheme), but for now I'm okay with moving forward with this as-is.

Could you add a comment here with a short description for why we're doing this? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you add a comment here with a short description for why we're doing this? 🙏

comment added

@wlynch
Copy link
Collaborator

wlynch commented Aug 29, 2024

Will look into the IAC issues!

Signed-off-by: Karl Haworth <karl.haworth@aa.com>
…ow-webook-to-use-env-vars-or-cert-files-for-github-app
@karlhaworth
Copy link
Contributor Author

Will look into the IAC issues!

I was bringing in origin when I needed to bring in upstream. Hope that fixes the iac issues

@wlynch
Copy link
Collaborator

wlynch commented Aug 29, 2024

I think it was a breaking change with the upstream Google provider - #472 should fix! 🤞

@wlynch
Copy link
Collaborator

wlynch commented Aug 30, 2024

@karlhaworth can you try rebasing on main now that #472 is in to see if the tf lint errors go away? 🙏

…ow-webook-to-use-env-vars-or-cert-files-for-github-app
@wlynch wlynch merged commit 935faf8 into octo-sts:main Aug 30, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants