-
Notifications
You must be signed in to change notification settings - Fork 301
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
Add cli command to redact secrets and redact secrets from Pipelines Secrets #2660
Add cli command to redact secrets and redact secrets from Pipelines Secrets #2660
Conversation
384df11
to
fc05be6
Compare
@@ -361,35 +400,57 @@ func TestGetEnv(t *testing.T) { | |||
}) | |||
} | |||
|
|||
func testAPI[Req, Resp any](t *testing.T, env *env.Environment, req *http.Request, client *http.Client, testCase apiTestCase[Req, Resp]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just moved this above the tests
@@ -101,7 +101,8 @@ func (c *Client) Do(ctx context.Context, method, url string, req, resp any) erro | |||
defer hresp.Body.Close() | |||
dec := json.NewDecoder(hresp.Body) | |||
|
|||
if hresp.StatusCode != http.StatusOK { | |||
switch hresp.StatusCode / 100 { | |||
case 4, 5: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a redaction returns 201.
for _, r := range mux { | ||
r.Reset(needles) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to mux.go
if cfg.Debug { | ||
return err | ||
} | ||
return errSecretParse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i assume that this is here in case any of the errors from ParseSecrets
(and AddToRedactor
below) have sensitive info in them, but how likely is that? is it worth just outputting them regardless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I know the likelihood is remote, but I don't want to make assumptions about the implementation of things like (*json.Decoder).Decode
. I think the impact if very bad, and it's pretty trivial to debug it by adding a --debug
flag.
@@ -0,0 +1,58 @@ | |||
package clicommand_test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍 test package!
const ( | ||
alreadyRedacted = "Guayaquil" | ||
toRedact = "Quito" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very happy to see that "test data that's all related to ecuador" has spread to you
internal/job/api.go
Outdated
@@ -25,7 +25,7 @@ func (e *Executor) startJobAPI() (cleanup func(), err error) { | |||
return cleanup, fmt.Errorf("creating job API socket path: %v", err) | |||
} | |||
|
|||
srv, token, err := jobapi.NewServer(e.shell.Logger, socketPath, e.shell.Env) | |||
srv, token, err := jobapi.NewServer(e.shell.Logger, socketPath, e.shell.Env, e.redactors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without getting too borrow-checker-rewrite-it-in-rust-y, what's the lifetime of e.redactors
as used by the job api? if something adds a new redactor mid-job, what happens? is replacer.Mux
goroutine safe?
i think because all slices are references, if a redactor is added it should be fine, but if someone calls e.redactors = ...
then it'll break? is that true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this code does seem dangerous. Redactors used to be created in each hook, but now they are created for the life of the job executor.
but if someone calls
e.redactors = ...
then it'll break? is that true?
The redactors will still work in that the job logs will still redact the set of values that the redactors had prior to the reassignment. The garbage collector won't clean them up either, as there are still references to the redactors in
e.shell.Writer
e.shell.Logger.WriterLogger
In any case, the Mux
will still be referred to in the job api server.
What will break is that a new hook won't be able to add the value of new environment variables that match the pattern to the redactor.
So as long as the job is still writing to e.shell.Writer
and e.shell.Logger
, previous redaction would still work.
Ultimately, this danger also exists for e.shell.Env
. If someone write e.shell = ...
to a new reference, then a lot of things will break. I think this is an indication that the redactors should be inside the shell, though. I think I will make a change to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually @moskyb, do you mind if I do that in a follow up? This PR is getting quite long, and I would like to do a nice refactor of how shell.Shells are created and tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah no that's totally fine - the risk is largely theoretical as is, happy for this to be done later.
Currently, the failure can only come from a failure to flush the redactor. Though this was previously ignored, I think a debug log is appropriate. I've made sure the log is in the job logs, and not the agent logs.
Previously, the redactors were created in each hook (and the defaultCommandPhase), this make it difficult to retain the redactees between hooks. Now, we only create the redactors once. But we flush them after each hook.
In the future, we may want to redact in than just log output, nesting this under the `log` command would be confusing.
9221131
to
a311dfc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wahoo! awesome work
Description
This adds a new command
buildkite-agent redactor add
that redacts secrets obtained from Pipelines Secrets from job logs. After some consideration, I've chosen this instead of:buildkite-agent secret redact
: Thesecret
group will be reserved for subcommands that interact with Pipelines Secrets, which this does not.buildkite-agent log redact
: In the future, we could redact more than just logsPlease let me know if there are other factors that mean the name of the command should be something else.
This uses a new route added to the Job API to update the redacted values for a job executor. A significant amount of the changes in this PR deal with expanding the scope of the redactor to be at the job executor level, rather than the hook level, so now, the redacted values will be retained for the lifetime of the job executor. Also, the redactor has been modified to only store a set of values internally. Thus, new values may be added to the the redactor without fear of duplication.
I have not done anything explicitly to address concurrency in the redactor Mux. I think it is enough that:
It is theoretically possible for a JobAPI added redaction to occur concurrently with a hook added redaction, but this is unlikely in practice as the hooks mutate the redactor AFTER they have run, meaning there is no opportunity to call the JobAPI at this time from inside a job. Of course, it JobAPI could be called from outside the job, but that's not what it was designed for, and there is a token that's intended to protect it from being called from outside a job.
I've also brushed up the help texts and did some basic re-arrangement of the commands in the
buildkite-agent
help text.Context
The motivation to do this work was to automatically redact secrets that were obtained from the API via the
buildkite-agent secret get
command. But, I think some historical context would be useful to understand what will be changed.Previously, log redaction was limited to configuring some patterns of environment variable names whose values would be redacted from a job's logs. This was a relatively static configuration, but as each hook could export new environment variables, there was a facility to reset the redactor with the new environment variables that matched the configured patterns at the start of each hook. However, this reset process meat that secrets added in one hook would not necessarily be retained in the set of values that would be redacted from subsequent hooks.
After lifting the redactor to the job executor level, there was still no mechanism for a running job to modify the redacted values. For this reason, a route was added to the Job API to allow just that.
Because this reset mechanism removed the redactee values from the redactor, the redactor did retained these in a list without de-duplication. I've changed the redactor (actually it's in a package called
replacer
) to do that. The existing test coverage should be good enought to ensure it functions correctly.Screenshots
With a job that has a post-checkout hook similar to :
and a command similar to:
The following job log is produced:
Changes
buildkite-agent redactor add
that accepts values to redact in multiple formats (just json and none for now) and ensures that they will be redacted from future log output.buildkite-agent secrets get
are redacted from job logsmap[string]struct{}
internally instead of[]string
to store theneedles
POST
to add a redactee to the redactorMux
of redactors that lives for the life of the job execution. It is flushed at the end of each hook, but it will retain any redactee added to it for the rest of its life.Testing
go test ./...
). Buildkite employees may check this if the pipeline has run automatically.go fmt ./...
)