feature: support per-secret files in S3

[jamestelfer]

At the moment, secrets can be added to a per-pipeline environment file that is stored in S3. This works quite well, however in our case we give engineers that own the pipeline write-only access to the secrets bucket.

This allows them to update secrets as necessary but denies the capacity to read existing secrets (for obvious reasons).

However, when all secrets are in a single file, it is easy to accidentally overwrite one of the secrets. For this reason, we implemented support in the agent environment hook for per-pipeline per-secret files.

Files are downloaded from ${BUILDKITE_SECRETS_BUCKET}/${BUILDKITE_PIPELINE_SLUG}/secret-files. For each of these files:

• its name must be in upper snake case
• its name must match the suffix _(SECRET|SECRET_KEY|PASSWORD|TOKEN|ACCESS_KEY)$ (correlating to the default obfuscation rules for the agent)
• the contents of the file is exported to a variable with the file name as the variable name

For example, consider the following structure in S3:

- pipelineName
   - environment
   - secret-files
      - NEWRELIC_TOKEN
      - AMAZING_AI_TOOL_SECRET
      - another-file-name

1. environment will be loaded first
2. NEWRELIC_TOKEN and AMAZING_AI_TOOL_SECRET will be created as environment variables, where the respective file contents become the variable values
3. the file another-file-name would show an error in the pipeline log and write an annotation informing the user that the secret file is being ignored.

Overall this has worked well for us. Engineers can upload their secrets without fear of unwanted side-effects, and less support intervention is required.

I propose that the secrets hook supports this strategy (or a variation thereof), taking advantage of parallel downloads to make it faster and providing the capability to other customers.

Questions:

• is the sub-path name OK?
• should the allowed filename pattern be configurable?
• are only error logs supported or could this utility write an annotation? (Note that it could "just" write an environment variable containing the skipped file names, and allow an agent hook to handle the issue.)

Love to get some feedback. If it's accepted, we'd be happy to implement it.

[quinn-diesel]

Hey @jamestelfer - this sounds really interesting. We'll chat about it as a group and come back to you with our thoughts.
It does sound like you have already implemented this would it be in a form where you could throw it into a PR so we could have a look? Or did you start from scratch rather than forking this?

[CerealBoy]

Hi @jamestelfer 👋🏼

Your proposal is quite a nice idea, and if you're happy to assemble a PR we'd love to work through it with you. As for your questions, I'll answer them inline below.

is the sub-path name OK?

I think the path you've proposed is fine. There's a mix of - and _ so either is acceptable here.

should the allowed filename pattern be configurable?

It's definitely a good thing to match the pattern that is covered by the agent. Validating that the rest of the name is consistent is also a good thing and would make a lot of sense. I don't think this would need to be configurable, at least until there's a particular case that requires it becoming known.

are only error logs supported or could this utility write an annotation? (Note that it could "just" write an environment variable containing the skipped file names, and allow an agent hook to handle the issue.)

I think your alternate idea of using an ENV with an agent hook to process it would be a sound way of handling the case. This is similar to how the agent handles ignored environment variables (see https://github.com/buildkite/agent/blob/a7662406a86189316675bc4b109bb13c1ff209a8/internal/job/executor.go#L758-L771 for an example of what I mean here).

Please reach out here or via support if you have further questions or would like to discuss deeper on anything!

[jamestelfer]

Awesome!

I have some time over the next couple of weeks that I can put towards this. I'll look to implement a relatively naïve first cut approach (that is, downloading the items 1 by 1 serially), and see where we need to go from there.

One call out: the current code only uses GetObject. This feature requires ListObjects, which will possibly change the IAM pre-requisites for this functionality. I haven't looked at the current permissions to know if this is going to be an issue at all.