Merge pull request #2292 from buildkite/sign-env-and-plugins

Include plugins in command step signatures

agent/integration/job_verification_integration_test.go

@@ -22,15 +22,52 @@ var (
 	job = api.Job{
 		ChunksMaxSizeBytes: 1024,
 		ID:                 defaultJobID,
-		Step:               pipeline.CommandStep{Command: "echo hello world"},
-		Env:                map[string]string{"BUILDKITE_COMMAND": "echo hello world"},
+		Step: pipeline.CommandStep{
+			Command: "echo hello world",
+			Plugins: pipeline.Plugins{
+				{
+					Name: "some-plugin#v1.0.0",
+					Config: map[string]string{
+						"key": "value",
+					},
+				},
+			},
+		},
+		Env: map[string]string{
+			"BUILDKITE_COMMAND": "echo hello world",
+			"BUILDKITE_PLUGINS": `[{"some-plugin#v1.0.0":{"key":"value"}}]`,
+		},
 	}
 
 	jobWithMismatchedStepAndJob = api.Job{
 		ID:                 defaultJobID,
 		ChunksMaxSizeBytes: 1024,
-		Step:               pipeline.CommandStep{Command: "echo hello world"},
-		Env:                map[string]string{"BUILDKITE_COMMAND": "echo 'THIS ISN'T HELLO WORLD!!!! CRIMES'"},
+		Step: pipeline.CommandStep{
+			Command: "echo hello world",
+		},
+		Env: map[string]string{
+			"BUILDKITE_COMMAND": "echo 'THIS ISN'T HELLO WORLD!!!! CRIMES'",
+		},
+	}
+
+	jobWithMismatchedPlugins = api.Job{
+		ChunksMaxSizeBytes: 1024,
+		ID:                 defaultJobID,
+		Step: pipeline.CommandStep{
+			Command: "echo hello world",
+			Plugins: pipeline.Plugins{
+				{
+					Name: "some-plugin#v1.0.0",
+					Config: map[string]string{
+						"key": "value",
+					},
+				},
+			},
+		},
+		Env: map[string]string{
+			"BUILDKITE_COMMAND": "echo hello world",
+			"BUILDKITE_PLUGINS": `[{"crimes-plugin#v1.0.0":{"steal":"everything"}}]`,
+		},
 	}
 )
 
@@ -115,6 +152,21 @@ func TestJobVerification(t *testing.T) {
 					jobWithMismatchedStepAndJob.Env["BUILDKITE_COMMAND"], jobWithMismatchedStepAndJob.Step.Command),
 			},
 		},
+		{
+			name:                     "when the step signature matches, but the plugins doesn't match the step, it fails signature verification",
+			agentConf:                agent.AgentConfiguration{JobVerificationNoSignatureBehavior: agent.VerificationBehaviourBlock},
+			job:                      jobWithMismatchedPlugins,
+			signingKey:               symmetricJWKFor(t, signingKeyLlamas),
+			verificationJWKS:         jwksFromKeys(t, symmetricJWKFor(t, signingKeyLlamas)),
+			mockBootstrapExpectation: func(bt *bintest.Mock) { bt.Expect().NotCalled() },
+			expectedExitStatus:       "-1",
+			expectedSignalReason:     agent.SignalReasonSignatureRejected,
+			expectLogsContain: []string{
+				"⚠️ ERROR",
+				fmt.Sprintf(`the value of field "plugins" on the job (%q) does not match the value of the field on the step (%q)`,
+					jobWithMismatchedPlugins.Env["BUILDKITE_PLUGINS"], job.Env["BUILDKITE_PLUGINS"]),
+			},
+		},
 		{
 			name:                     "when the step has a signature, but the JobRunner doesn't have a verification key, it fails signature verification",
 			agentConf:                agent.AgentConfiguration{},

api/jobs.go

@@ -32,6 +32,10 @@ func (j *Job) ValuesForFields(fields []string) (map[string]string, error) {
 		switch f {
 		case "command":
 			o[f] = j.Env["BUILDKITE_COMMAND"]
+
+		case "plugins":
+			o[f] = j.Env["BUILDKITE_PLUGINS"]
+
 		default:
 			return nil, fmt.Errorf("unknown or unsupported field on Job struct for signing/verification: %q", f)
 		}

internal/pipeline/sign.go

@@ -24,7 +24,10 @@ type Signature struct {
 func Sign(sf SignedFielder, key jwk.Key) (*Signature, error) {
 	payload := &bytes.Buffer{}
 
-	values := sf.SignedFields()
+	values, err := sf.SignedFields()
+	if err != nil {
+		return nil, err
+	}
 	if len(values) == 0 {
 		return nil, errors.New("sign: no fields to sign")
 	}
@@ -89,7 +92,7 @@ func (s *Signature) Verify(sf SignedFielder, keySet jwk.Set) error {
 type SignedFielder interface {
 	// SignedFields returns the default set of fields to sign, and their values.
 	// This is called by Sign.
-	SignedFields() map[string]string
+	SignedFields() (map[string]string, error)
 
 	// ValuesForFields looks up each field and produces a map of values. This is
 	// called by Verify. The set of fields might differ from the default, e.g.

internal/pipeline/sign_test.go

@@ -7,13 +7,29 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/buildkite/agent/v3/internal/ordered"
 	"github.com/lestrrat-go/jwx/v2/jwa"
 	"github.com/lestrrat-go/jwx/v2/jwk"
 )
 
 func TestSignVerify(t *testing.T) {
 	step := &CommandStep{
 		Command: "llamas",
+		Plugins: Plugins{
+			{
+				Name:   "some-plugin#v1.0.0",
+				Config: nil,
+			},
+			{
+				Name: "another-plugin#v3.4.5",
+				Config: ordered.MapFromItems(
+					ordered.TupleSA{
+						Key:   "llama",
+						Value: "Kuzco",
+					},
+				),
+			},
+		},
 	}
 
 	cases := []struct {
@@ -26,19 +42,19 @@ func TestSignVerify(t *testing.T) {
 			name:                           "HMAC-SHA256",
 			generateSigner:                 func(alg jwa.SignatureAlgorithm) (jwk.Key, jwk.Set) { return newSymmetricKeyPair(t, "alpacas", alg) },
 			alg:                            jwa.HS256,
-			expectedDeterministicSignature: "eyJhbGciOiJIUzI1NiIsImtpZCI6IlRlc3RTaWduVmVyaWZ5In0..f5NQYQtR0Eg-0pzzCon2ykzGy5oDPYtQw0C0fTKGI38",
+			expectedDeterministicSignature: "eyJhbGciOiJIUzI1NiIsImtpZCI6IlRlc3RTaWduVmVyaWZ5In0..NDiUjV0myH279-OQi6eOKjgyhAPUnc5ZmZoynhUUvIo",
 		},
 		{
 			name:                           "HMAC-SHA384",
 			generateSigner:                 func(alg jwa.SignatureAlgorithm) (jwk.Key, jwk.Set) { return newSymmetricKeyPair(t, "alpacas", alg) },
 			alg:                            jwa.HS384,
-			expectedDeterministicSignature: "eyJhbGciOiJIUzM4NCIsImtpZCI6IlRlc3RTaWduVmVyaWZ5In0..HgHltOlatth2TCc4swArP1UL_Zm2Rh2ccEC26s1sFBO6FOW5qfW37uQ9CHAz6dhh",
+			expectedDeterministicSignature: "eyJhbGciOiJIUzM4NCIsImtpZCI6IlRlc3RTaWduVmVyaWZ5In0..XGdZ7TG0lBSg7rXc091A3OaXAjODyI7aFkAjFJblD0YUnC5WW6WHgmJqlrG94x7z",
 		},
 		{
 			name:                           "HMAC-SHA512",
 			generateSigner:                 func(alg jwa.SignatureAlgorithm) (jwk.Key, jwk.Set) { return newSymmetricKeyPair(t, "alpacas", alg) },
 			alg:                            jwa.HS512,
-			expectedDeterministicSignature: "eyJhbGciOiJIUzUxMiIsImtpZCI6IlRlc3RTaWduVmVyaWZ5In0..mcph5zwioGkmx-aPrxExzc9QRzO4afn_kK_89aEuo4xYD0tcUD8OJom09x2xcvK6eRkOpvVlkrKLBzvh-7uu6w",
+			expectedDeterministicSignature: "eyJhbGciOiJIUzUxMiIsImtpZCI6IlRlc3RTaWduVmVyaWZ5In0..GvKR_cGqNcF8EgffnkSoymJORoH60W36O80tYnGwnKXTUTh0XVmnEp0gT03YYRdf39JnwqbMGCticQJFFA_jWg",
 		},
 		{
 			name:           "RSA-PSS 256",
@@ -110,7 +126,7 @@ func TestSignVerify(t *testing.T) {
 
 type testFields map[string]string
 
-func (m testFields) SignedFields() map[string]string { return m }
+func (m testFields) SignedFields() (map[string]string, error) { return m, nil }
 
 func (m testFields) ValuesForFields(fields []string) (map[string]string, error) {
 	out := make(map[string]string, len(fields))

internal/pipeline/step_command.go

@@ -1,7 +1,7 @@
 package pipeline
 
 import (
-	"errors"
+	"encoding/json"
 	"fmt"
 	"strings"
 
@@ -58,25 +58,62 @@ func (c *CommandStep) UnmarshalOrdered(src any) error {
 }
 
 // SignedFields returns the default fields for signing.
-func (c *CommandStep) SignedFields() map[string]string {
+func (c *CommandStep) SignedFields() (map[string]string, error) {
+	plugins := ""
+	if len(c.Plugins) > 0 {
+		// TODO: Reconsider using JSON here - is it stable enough?
+		pj, err := json.Marshal(c.Plugins)
+		if err != nil {
+			return nil, err
+		}
+		plugins = string(pj)
+	}
 	return map[string]string{
 		"command": c.Command,
-	}
+		"plugins": plugins,
+	}, nil
 }
 
 // ValuesForFields returns the contents of fields to sign.
 func (c *CommandStep) ValuesForFields(fields []string) (map[string]string, error) {
+	// Make a set of required fields. As fields is processed, mark them off by
+	// deleting them.
+	required := map[string]struct{}{
+		"command": {},
+		"plugins": {},
+	}
+
 	out := make(map[string]string, len(fields))
 	for _, f := range fields {
+		delete(required, f)
+
 		switch f {
 		case "command":
 			out["command"] = c.Command
+
+		case "plugins":
+			if len(c.Plugins) == 0 {
+				out["plugins"] = ""
+				break
+			}
+			// TODO: Reconsider using JSON here - is it stable enough?
+			val, err := json.Marshal(c.Plugins)
+			if err != nil {
+				return nil, err
+			}
+			out["plugins"] = string(val)
+
 		default:
 			return nil, fmt.Errorf("unknown or unsupported field for signing %q", f)
 		}
 	}
-	if _, ok := out["command"]; !ok {
-		return nil, errors.New("command is required for signature verification")
+
+	if len(required) > 0 {
+		missing := make([]string, 0, len(required))
+		for k := range required {
+			missing = append(missing, k)
+		}
+		return nil, fmt.Errorf("one or more required fields are not present: %v", missing)
 	}
 	return out, nil
 }