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(envbuilder.go): add support for build secrets #391

Merged
merged 16 commits into from
Oct 29, 2024
Merged

Conversation

SasSwart
Copy link
Contributor

@SasSwart SasSwart commented Oct 16, 2024

This PR adopts support for build secrets from our recent updates to Kaniko to provide a safer alternative to this: #93

@SasSwart SasSwart changed the title chore(envbuilder.go): prove the concept of build secret ends chore(envbuilder.go): prove the concept of build secret envs Oct 16, 2024
Copy link
Member

@johnstcn johnstcn 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 somewhat different from what I had in mind, and seems to cross more into the direction of #366

The idea is that we want these secrets to be available during the container build process only. As far as I can tell, this persists them to disk inside Kaniko's working directoryl.

What do you think about the following approach:

  • Fetch all environment variables with the prefix ENVBUILDER_SECRET and store them in a map or similar
  • Before building the image using Kaniko, set all the environment variables from above without ENVBUILDER_SECRET prefix.
  • After building the image and just before exec'ing the init command, unset all of the above environment variables.

If the prefix ENVBUILDER_SECRET_ is misleading, we can rename it to something else also.

@SasSwart
Copy link
Contributor Author

Before building the image using Kaniko, set all the environment variables from above without ENVBUILDER_SECRET prefix.

I did attempt this, but wasn't able to get it to work. each RUN directive starts with a mostly blank environment as far as I can tell. I will revisit this approach though. Perhaps I can get it to work upon closer inspection.

Out of interest, why do you prefer the ENV approach as opposed to persisting them to disk?

@johnstcn
Copy link
Member

johnstcn commented Oct 17, 2024

Out of interest, why do you prefer the ENV approach as opposed to persisting them to disk?

I actually don't have a good answer to this apart from 'environment variables are easy and it seems to be the method of least surprise'.

However, I can see some folks being wary of passing build-time secrets via environment variable, as someone with access to either the container spec or some 'back-door' into the container (e.g. via kubectl exec) would then potentially end up being able to read the env var.

EDIT: a use case that appears to be common is mounting secrets inside the container with ownership root:root and permission 0400 so that a resulting non-root user inside the container is unable to read the secret.

@SasSwart
Copy link
Contributor Author

@johnstcn The updated PR should demonstrate a more flexible approach in line with docker standards. The user can now choose whether they want an ENV, or a file. They can also specify where that file should be.

Copy link
Contributor Author

@SasSwart SasSwart left a comment

Choose a reason for hiding this comment

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

Self Review:

envbuilder.go Outdated Show resolved Hide resolved
envbuilder.go Outdated Show resolved Hide resolved
envbuilder.go Outdated Show resolved Hide resolved
envbuilder.go Outdated Show resolved Hide resolved
@SasSwart SasSwart changed the title chore(envbuilder.go): prove the concept of build secret envs feat(envbuilder.go): add support for build secrets Oct 24, 2024
@SasSwart SasSwart marked this pull request as ready for review October 25, 2024 08:40
@SasSwart SasSwart self-assigned this Oct 25, 2024
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

In general the implementation looks good, had some minor suggestions. Tests need a bit more work though.

integration/integration_test.go Show resolved Hide resolved
options/secrets.go Outdated Show resolved Hide resolved
options/secrets.go Outdated Show resolved Hide resolved
options/secrets_test.go Outdated Show resolved Hide resolved
options/secrets_test.go Outdated Show resolved Hide resolved
@SasSwart
Copy link
Contributor Author

@mafredri I've tended to all review notes. Thanks for all the feedback, it was useful!
Tests should be in a much better space now. Please have another look?

Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

A few minor nits but otherwise looking good. 👍🏻

envbuilder.go Outdated Show resolved Hide resolved
options/secrets.go Show resolved Hide resolved
options/secrets_test.go Outdated Show resolved Hide resolved
options/secrets_test.go Outdated Show resolved Hide resolved
options/secrets_test.go Outdated Show resolved Hide resolved
options/secrets_test.go Show resolved Hide resolved
@SasSwart SasSwart merged commit 416acc6 into main Oct 29, 2024
4 checks passed
@SasSwart SasSwart deleted the jjs/buildsecrets branch October 29, 2024 08:02
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