Verify step signatures #2210

moskyb · 2023-07-17T05:55:58Z

This PR: Enables the buildkite-agent to verify incoming jobs against signatures uploaded in their pipeline uploads.

This is done by a process (vaguely):

When accepting a job, the Job sent by the backend will (WIP) now include a step attribute, which contains a subset of information contained in the step upload - at current, it's the step's command and signature, though as we sign more elements of the signature, this will expand to include other elements
After accepting the job, but before kicking off the bootstrap process, the agent computes a signature for the step, and compares it to the signature shipped in the accept payload
With the step verified, we need to check that the Job's elements match the Step's
If they match, run the job
If they don't (or there's a signature mismatch, or any other error happens during this process), fail the job with exit status -1, and an informational signal reason - this is all handled very similarly to the existing pre-bootstrap hook

triarius

Looking good.

I think all my comments are just various degrees of nits.

But I would encourage exploring what putting this stuff in a different package would look like.

triarius · 2023-07-25T05:56:08Z

agent/agent_configuration.go

+	JobVerificationNoSignatureBehavior      string
+	JobVerificationInvalidSignatureBehavior string


Maybe explain what these are. The other one seems obvious, but it deserves a doc comment too.

I see they're explained below, but this is a higher level. I think it's good to talk about them here as well.

agent/integration/test_helpers.go

agent/job_runner.go

agent/run_job.go

moskyb · 2023-07-25T07:37:28Z

agent/integration/test_helpers.go

+		case "/jobs/" + jobID + "/chunks":
+			sequence := req.URL.Query().Get("sequence")
+			seqNo, _ := strconv.Atoi(sequence)
+			r, _ := gzip.NewReader(bytes.NewBuffer(b))
+			uz, _ := io.ReadAll(r)
+			t.logChunks[seqNo] = string(uz)
+			rw.WriteHeader(http.StatusCreated)


error handling? what's that?

DrJosh9000

Good stuff ...but still reviewing...

agent/run_job.go

agent/verify_job.go

DrJosh9000 · 2023-08-02T04:49:08Z

clicommand/agent_start.go

@@ -34,6 +34,7 @@ import (
 	"github.com/mitchellh/go-homedir"
 	"github.com/urfave/cli"
 	"golang.org/x/exp/maps"
+	"golang.org/x/exp/slices"


Oh goodness. When did we start depending on exp packages?

At least these will be making their way into Go 1.21 largely the same.

um, it's, um, it's hard to say, exactly, when (May 5th 2022) this started or who (extremely me) started it

Welp, looks like maps.Keys isn't making it to stdlib maps (yet): golang/go#61538

(slices.Contains is still in, so I'll let this PR slide)

internal/pipeline/command_step.go

triarius

This is shaping up to be amazing!

agent/verify_job.go

clicommand/agent_start.go

agent/integration/test_helpers.go

agent/run_job.go

triarius · 2023-08-02T06:36:10Z

agent/run_job.go

+		exit.Status = -1
+		exit.SignalReason = "job_verification_failed_with_error"
+		return nil
+	}


I do think the if/else chain I suggested last time looks better here.

I, too, generally prefer switches to if/else chains, but here:

There is no added efficiency in using a switch as all the case statements have to be evaluated in order in the worst case as there is no value to switch on.

This statement leaks the ise variable. The if/else chain would scope it exclusively to the appropriate branch

But I'm not going to die on this hill.

If I were to change anything, it would be to replace default: with case err != nil:, and not make err == nil a case in the switch.

As in, the other side of "indent error flow" is "don't indent non-error flow".

agent/verify_job.go

triarius

🎉

triarius · 2023-08-03T01:04:41Z

This is great!

DrJosh9000

Huzzah! 🧃

DrJosh9000 · 2023-08-03T05:44:00Z

internal/pipeline/step_command.go

@@ -18,6 +18,7 @@ type CommandStep struct {
 	Command   string     `yaml:"command"`
 	Plugins   Plugins    `yaml:"plugins,omitempty"`
 	Signature *Signature `yaml:"signature,omitempty"`
+	Matrix    any        `yaml:"matrix,omitempty"`


We'll also have to set the new field in unmarshalMap.

oooh good point

... and interpolate ...

DrJosh9000 · 2023-08-03T05:52:53Z

clicommand/agent_start.go

@@ -34,6 +34,7 @@ import (
 	"github.com/mitchellh/go-homedir"
 	"github.com/urfave/cli"
 	"golang.org/x/exp/maps"
+	"golang.org/x/exp/slices"


Welp, looks like maps.Keys isn't making it to stdlib maps (yet): golang/go#61538

(slices.Contains is still in, so I'll let this PR slide)

DrJosh9000 · 2023-08-03T05:55:00Z

clicommand/agent_start.go

@@ -53,71 +54,90 @@ Example:

   $ buildkite-agent start --token xxx`

+var noSignatureBehaviors = []string{agent.VerificationBehaviourBlock, agent.VerificationBehaviourWarn}


It's used between signature-missing and signature-invalid, so perhaps

Suggested change

var noSignatureBehaviors = []string{agent.VerificationBehaviourBlock, agent.VerificationBehaviourWarn}

var verificationFailureBehaviors = []string{agent.VerificationBehaviourBlock, agent.VerificationBehaviourWarn}

DrJosh9000 · 2023-08-03T06:00:31Z

api/jobs.go

+	ChunksFailedCount  int                  `json:"chunks_failed_count,omitempty"`
+}
+
+func (j Job) ValuesForFields(fields []string) (map[string]string, error) {


Job looks big enough that I'd make this receiver *Job (for not-copying-it-over-stack reasons, not for modifying-the-fields reasons).

DrJosh9000 · 2023-08-03T06:09:20Z

agent/verify_job.go

+	// Now that the signature of the job's step is verified, we need to check if the fields on the job match those on the
+	// step. If they don't, we need to fail the job


I was going to suggest it would be simpler to override BUILDKITE_COMMAND with step.Command, than to carefully compare each field.

But thinking about it, they would only differ if the backend (ours or the hypothetical attacker's) is doing crimes, in which case we probably shouldn't run anything. And the backend can't migrate away from passing BUILDKITE_COMMAND without breaking every old agent version. (Ah, but it could sniff the agent version then vary the job...)

Might be worth documenting that?

just to double check, are saying that it might be good to document that the only reason that step and job fields should differ is if an attacker is doing crimes on the backend?

It might not be the only reason, but it definitely seems like a reason

dabarrell · 2023-08-03T06:20:35Z

agent/run_job.go

+			r.verificationFailureLogs(err, r.InvalidSignatureBehavior)
+			if r.InvalidSignatureBehavior == VerificationBehaviourBlock {
+				exit.Status = -1
+				exit.SignalReason = "job_verification_failed_invalid_signature"


My initial thought was that AGENT_REFUSED might already serve the purpose here, but I imagine you're trying to tick off the objective of showing verification failure in the timeline at the same time.

It feels a little strange to be embedding the failure mode into the signal reason like this. We probably don't want to pack signal reason with a whole bunch of values - more a class of error, rather than an error string, if that makes sense. Beyond feeling a bit wrong, think about how they're used - if a user wanted to catch the errors for an automatic retry, for example, they'd need to list them all. Have you considered one signal reason for these (perhaps VERIFICATION_FAILED, and showing the actual error in the agent/job logs?

If signal reason was designed for users to catch and retry, then I don't think we should be overloading its use to also display information in the timeline. Is there another way of creating a job event for signing failures? If not, I think we should examine building one when the time to build the timeline feature arrives.

Co-authored-by: Narthana Epa <narthana.epa@gmail.com>

moskyb force-pushed the pdp-1125-verify-steps branch 2 times, most recently from 29b475c to ea0f66e Compare July 17, 2023 05:59

DrJosh9000 added the signing label Jul 18, 2023

moskyb force-pushed the pdp-1125-verify-steps branch 3 times, most recently from ece0f77 to 856cce4 Compare July 25, 2023 05:38

triarius requested a review from a team July 25, 2023 06:47

triarius reviewed Jul 25, 2023

View reviewed changes

moskyb force-pushed the pdp-1125-verify-steps branch from 65c156b to f4b2a6b Compare July 25, 2023 07:36

moskyb commented Jul 25, 2023

View reviewed changes

moskyb force-pushed the pdp-1125-verify-steps branch 4 times, most recently from 5a4bc99 to 9ac378a Compare July 27, 2023 01:16

moskyb requested review from DrJosh9000 and a team July 27, 2023 03:05

moskyb force-pushed the pdp-1125-verify-steps branch 2 times, most recently from 8e2c62c to 06fc81e Compare July 31, 2023 23:38

moskyb mentioned this pull request Aug 2, 2023

Fix data race in testAgentEndpoint #2265

Merged

moskyb force-pushed the pdp-1125-verify-steps branch 2 times, most recently from 402843b to 02ffc45 Compare August 2, 2023 00:53

moskyb marked this pull request as ready for review August 2, 2023 03:31

moskyb force-pushed the pdp-1125-verify-steps branch from 02ffc45 to 95c2404 Compare August 2, 2023 03:33

moskyb changed the title ~~[WIP] Verify step signatures~~ Verify step signatures Aug 2, 2023

DrJosh9000 reviewed Aug 2, 2023

View reviewed changes

agent/run_job.go Show resolved Hide resolved

agent/run_job.go Outdated Show resolved Hide resolved

agent/run_job.go Outdated Show resolved Hide resolved

moskyb force-pushed the pdp-1125-verify-steps branch from 95c2404 to 65dc8fd Compare August 2, 2023 04:35

DrJosh9000 reviewed Aug 2, 2023

View reviewed changes

agent/verify_job.go Show resolved Hide resolved

DrJosh9000 reviewed Aug 2, 2023

View reviewed changes

agent/verify_job.go Outdated Show resolved Hide resolved

DrJosh9000 reviewed Aug 2, 2023

View reviewed changes

internal/pipeline/command_step.go Outdated Show resolved Hide resolved

moskyb force-pushed the pdp-1125-verify-steps branch 2 times, most recently from 1f7845a to 6cc495d Compare August 2, 2023 05:31

triarius reviewed Aug 2, 2023

View reviewed changes

triarius approved these changes Aug 3, 2023

View reviewed changes

moskyb force-pushed the pdp-1125-verify-steps branch 3 times, most recently from 5bf9034 to c78f0de Compare August 3, 2023 05:41

DrJosh9000 approved these changes Aug 3, 2023

View reviewed changes

dabarrell reviewed Aug 3, 2023

View reviewed changes

moskyb force-pushed the pdp-1125-verify-steps branch 2 times, most recently from cd13203 to 5777275 Compare August 7, 2023 03:38

moskyb mentioned this pull request Aug 8, 2023

Pipeline Signing/Verification with JWS #2252

Merged

moskyb force-pushed the pdp-1125-verify-steps branch from 5777275 to a279f07 Compare August 9, 2023 03:18

moskyb and others added 11 commits August 10, 2023 10:26

Don't sign matrix steps (yet)

ef92f9d

Sort + Partition Agent config values

ddce856

Move agent integration test helpers into their own file

b0b57f8

Don't leak the computed signature of the job to the logs

7b0c6b4

Job Verification

4c8ab98

Pass job signing config through to pipeline uploads

ae6f939

Steps can have a little matrix, as a treat

eb21468

Update agent/integration/test_helpers.go

efe9c21

Co-authored-by: Narthana Epa <narthana.epa@gmail.com>

Make nil verifier case a little bit clearer

49c09ac

Misc PR feedback fixes

e80217f

Single signal reason for verification failure

c53640e

moskyb force-pushed the pdp-1125-verify-steps branch from a279f07 to c53640e Compare August 9, 2023 22:33

moskyb merged commit f8118ff into main Aug 10, 2023

moskyb deleted the pdp-1125-verify-steps branch August 10, 2023 01:33

triarius mentioned this pull request Aug 15, 2023

Bump CHANGELOG and VERSION for v3.51.0 #2293

Merged

jordandcarter mentioned this pull request Nov 18, 2024

Remove signal reason unable_to_verify_signature and replace with signature_rejected #3094

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify step signatures #2210

Verify step signatures #2210

moskyb commented Jul 17, 2023 •

edited

Loading

triarius left a comment

triarius Jul 25, 2023

triarius Jul 25, 2023

moskyb Jul 25, 2023

DrJosh9000 left a comment

DrJosh9000 Aug 2, 2023

moskyb Aug 2, 2023 •

edited

Loading

DrJosh9000 Aug 3, 2023

triarius left a comment

triarius Aug 2, 2023

DrJosh9000 Aug 3, 2023

triarius left a comment

triarius commented Aug 3, 2023

DrJosh9000 left a comment

DrJosh9000 Aug 3, 2023

moskyb Aug 3, 2023

DrJosh9000 Aug 3, 2023

DrJosh9000 Aug 3, 2023

DrJosh9000 Aug 3, 2023

DrJosh9000 Aug 3, 2023

DrJosh9000 Aug 3, 2023 •

edited

Loading

moskyb Aug 3, 2023

DrJosh9000 Aug 3, 2023

dabarrell Aug 3, 2023

triarius Aug 4, 2023

		JobVerificationNoSignatureBehavior string
		JobVerificationInvalidSignatureBehavior string

		@@ -53,71 +54,90 @@ Example:

		$ buildkite-agent start --token xxx`

		var noSignatureBehaviors = []string{agent.VerificationBehaviourBlock, agent.VerificationBehaviourWarn}

	var noSignatureBehaviors = []string{agent.VerificationBehaviourBlock, agent.VerificationBehaviourWarn}
	var verificationFailureBehaviors = []string{agent.VerificationBehaviourBlock, agent.VerificationBehaviourWarn}

		// Now that the signature of the job's step is verified, we need to check if the fields on the job match those on the
		// step. If they don't, we need to fail the job

Verify step signatures #2210

Verify step signatures #2210

Conversation

moskyb commented Jul 17, 2023 • edited Loading

triarius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrJosh9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moskyb Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triarius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triarius left a comment

Choose a reason for hiding this comment

triarius commented Aug 3, 2023

DrJosh9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrJosh9000 Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moskyb commented Jul 17, 2023 •

edited

Loading

moskyb Aug 2, 2023 •

edited

Loading

DrJosh9000 Aug 3, 2023 •

edited

Loading