fix(executors): surface env var decode failures in the job log

pkg/api/job_request.go

@@ -3,6 +3,7 @@ package agentapi
 import (
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/fs"
 	"io/ioutil"
@@ -92,6 +93,23 @@ func (p *PublicKey) Decode() ([]byte, error) {
 	return base64.StdEncoding.DecodeString(string(*p))
 }
 
+/*
+ * A public key has no name to report, so a decode failure is identified by
+ * its position in the job request. The key material itself is never logged:
+ * these are the keys used to authorize SSH access into the job.
+ */
+func (p *PublicKey) DecodeAt(index int) ([]byte, error) {
+	key, err := p.Decode()
+	if err != nil {
+		return key, fmt.Errorf(
+			"error decoding SSH public key #%d (%s): %w",
+			index, describeBase64Value(string(*p)), err,
+		)
+	}
+
+	return key, nil
+}
+
 type JobRequest struct {
 	JobID         string      `json:"job_id" yaml:"job_id"`
 	Executor      string      `json:"executor" yaml:"executor"`
@@ -146,11 +164,94 @@ func NewRequestFromYamlFile(path string) (*JobRequest, error) {
 }
 
 func (e *EnvVar) Decode() ([]byte, error) {
-	return base64.StdEncoding.DecodeString(e.Value)
+	value, err := base64.StdEncoding.DecodeString(e.Value)
+	if err != nil {
+		return value, fmt.Errorf("error decoding '%s' (%s): %w", e.Name, describeBase64Value(e.Value), err)
+	}
+
+	return value, nil
 }
 
 func (f *File) Decode() ([]byte, error) {
-	return base64.StdEncoding.DecodeString(f.Content)
+	content, err := base64.StdEncoding.DecodeString(f.Content)
+	if err != nil {
+		return content, fmt.Errorf("error decoding '%s' (%s): %w", f.Path, describeBase64Value(f.Content), err)
+	}
+
+	return content, nil
+}
+
+var newLines = strings.NewReplacer("\r", "", "\n", "")
+
+/*
+ * Describes why base64.StdEncoding might reject a value, without ever
+ * including the value itself, since env vars and files hold secrets.
+ * The base64 error alone only points at a byte offset, which is not enough
+ * to tell a truncated value from one encoded with the wrong alphabet.
+ */
+func describeBase64Value(value string) string {
+	description := fmt.Sprintf("length %d", len(value))
+
+	/*
+	 * Only claim the url-safe alphabet when the value is rejected by the
+	 * standard one and accepted by the url-safe one - that combination is
+	 * what points at a producer using the wrong encoder. Plaintext holding
+	 * a '-' (a branch name, a date) decodes as neither.
+	 */
+	if !decodesAs(base64.StdEncoding, value) && decodesAs(base64.URLEncoding, value) {
+		description += ", valid as url-safe base64"
+	}
+
+	// \r and \n are ignored by the decoder, so they don't count towards padding.
+	if len(newLines.Replace(value))%4 != 0 {
+		description += ", not padded to a multiple of 4"
+	}
+
+	if strings.ContainsAny(value, " \t\r\n") {
+		description += ", contains whitespace"
+	}
+
+	return description
+}
+
+func decodesAs(encoding *base64.Encoding, value string) bool {
+	_, err := encoding.DecodeString(value)
+	return err == nil
+}
+
+/*
+ * Decodes every env var and file in the job request, reporting all the ones
+ * that are not valid base64. Only the fields that are always decoded when a
+ * job runs are checked here: container env vars and image pull credentials are
+ * decoded conditionally, or with the error ignored, so a bad value there does
+ * not necessarily break the job.
+ *
+ * SSH public keys are a known gap rather than a safe omission: a key that
+ * fails to decode does fail the job on the docker-compose executor. They are
+ * left out because PublicKey has no name to report, so there is nothing
+ * useful to say about which key is broken.
+ */
+func (j *JobRequest) ValidateEncoding() error {
+	errs := []error{}
+	collect := func(_ []byte, err error) {
+		if err != nil {
+			errs = append(errs, err)
+		}
+	}
+
+	// env vars first: they are exported before files are injected,
+	// so this reports offenders in the order a job hits them.
+	for _, envVar := range j.EnvVars {
+		collect(envVar.Decode())
+	}
+
+	for _, file := range j.Files {
+		collect(file.Decode())
+	}
+
+	// errors.Join returns nil for an empty slice, and keeps every offender
+	// reachable through errors.Is/errors.As instead of flattening to a string.
+	return errors.Join(errs...)
 }
 
 const ImagePullCredentialsStrategyDockerHub = "DockerHub"
@@ -165,7 +266,7 @@ func (c *ImagePullCredentials) ToCmdEnvVars() ([]string, error) {
 		name := env.Name
 		value, err := env.Decode()
 		if err != nil {
-			return envs, fmt.Errorf("error decoding '%s': %v", env.Name, err)
+			return envs, err
 		}
 
 		envs = append(envs, fmt.Sprintf("%s=%s", name, string(value)))
@@ -179,7 +280,7 @@ func (c *ImagePullCredentials) FindFile(path string) (string, error) {
 		if f.Path == path {
 			v, err := f.Decode()
 			if err != nil {
-				return "", fmt.Errorf("error decoding '%s': %v", path, err)
+				return "", err
 			}
 
 			return string(v), nil
@@ -198,7 +299,7 @@ func findEnvVar(envVars []EnvVar, varName string) (string, error) {
 		if envVar.Name == varName {
 			v, err := envVar.Decode()
 			if err != nil {
-				return "", fmt.Errorf("error decoding '%s': %v", varName, err)
+				return "", err
 			}
 
 			return string(v), nil

pkg/api/job_request_test.go

@@ -2,8 +2,10 @@ package agentapi
 
 import (
 	"encoding/base64"
+	"errors"
 	"path/filepath"
 	"runtime"
+	"strings"
 	"testing"
 
 	assert "github.com/stretchr/testify/assert"
@@ -121,3 +123,203 @@ func Test__ImagePullCredentials(t *testing.T) {
 		assert.ErrorContains(t, err, "no file 'does/not/exist' found")
 	})
 }
+
+func Test__DecodeNamesTheOffendingValue(t *testing.T) {
+	t.Run("env var decode error names the variable", func(t *testing.T) {
+		envVar := EnvVar{Name: "SEMAPHORE_GIT_SHA", Value: "abc$def"}
+
+		_, err := envVar.Decode()
+		assert.ErrorContains(t, err, "error decoding 'SEMAPHORE_GIT_SHA'")
+		assert.ErrorContains(t, err, "illegal base64 data")
+	})
+
+	t.Run("env var decode error does not leak the value", func(t *testing.T) {
+		envVar := EnvVar{Name: "SEMAPHORE_GIT_SHA", Value: "super-secret-value"}
+
+		_, err := envVar.Decode()
+		assert.Error(t, err)
+		assert.NotContains(t, err.Error(), "super-secret-value")
+		assert.NotContains(t, err.Error(), "secret")
+	})
+
+	t.Run("file decode error names the path", func(t *testing.T) {
+		file := File{Path: "/home/semaphore/.ssh/id_rsa", Content: "abc$def"}
+
+		_, err := file.Decode()
+		assert.ErrorContains(t, err, "error decoding '/home/semaphore/.ssh/id_rsa'")
+	})
+
+	t.Run("file decode error does not leak the content", func(t *testing.T) {
+		file := File{Path: "a/b/c", Content: "private-key-material"}
+
+		_, err := file.Decode()
+		assert.Error(t, err)
+		assert.NotContains(t, err.Error(), "private-key-material")
+	})
+
+	t.Run("valid values decode without error", func(t *testing.T) {
+		envVar := EnvVar{Name: "A", Value: base64.StdEncoding.EncodeToString([]byte("VALUE_A"))}
+		v, err := envVar.Decode()
+		assert.NoError(t, err)
+		assert.Equal(t, "VALUE_A", string(v))
+
+		file := File{Path: "a/b/c", Content: base64.StdEncoding.EncodeToString([]byte("CONTENT"))}
+		c, err := file.Decode()
+		assert.NoError(t, err)
+		assert.Equal(t, "CONTENT", string(c))
+	})
+}
+
+func Test__PublicKeyDecodeNamesThePosition(t *testing.T) {
+	t.Run("decode error identifies the key by position", func(t *testing.T) {
+		key := PublicKey("abc$def")
+
+		_, err := key.DecodeAt(2)
+		assert.ErrorContains(t, err, "error decoding SSH public key #2")
+		assert.ErrorContains(t, err, "length 7, not padded to a multiple of 4")
+		assert.ErrorContains(t, err, "illegal base64 data")
+	})
+
+	t.Run("decode error does not leak the key material", func(t *testing.T) {
+		key := PublicKey("ssh-rsa AAAAB3NzaC1yc2EAAAA-not-base64")
+
+		_, err := key.DecodeAt(0)
+		assert.Error(t, err)
+		assert.NotContains(t, err.Error(), "ssh-rsa")
+		assert.NotContains(t, err.Error(), "AAAAB3")
+	})
+
+	t.Run("valid key decodes", func(t *testing.T) {
+		key := PublicKey(base64.StdEncoding.EncodeToString([]byte("ssh-rsa AAAA")))
+
+		v, err := key.DecodeAt(0)
+		assert.NoError(t, err)
+		assert.Equal(t, "ssh-rsa AAAA", string(v))
+	})
+}
+
+func Test__DescribeBase64Value(t *testing.T) {
+	t.Run("reports the shape, never the content", func(t *testing.T) {
+		assert.Equal(t, "length 0", describeBase64Value(""))
+		assert.Equal(t, "length 8", describeBase64Value("aGVsbG8="))
+
+		// url-safe alphabet: '-' and '_' are illegal in std encoding
+		assert.Equal(t, "length 4, valid as url-safe base64", describeBase64Value("-_-_"))
+
+		// plaintext that merely contains a hyphen is not url-safe base64,
+		// and saying so would point at the wrong culprit
+		assert.Equal(
+			t,
+			"length 18, not padded to a multiple of 4",
+			describeBase64Value("super-secret-value"),
+		)
+
+		// unpadded values are not a multiple of 4 characters long
+		assert.Equal(t, "length 7, not padded to a multiple of 4", describeBase64Value("aGVsbG8"))
+
+		assert.Equal(
+			t,
+			"length 9, not padded to a multiple of 4, contains whitespace",
+			describeBase64Value("aGVs bG8="),
+		)
+
+		// \r and \n are ignored by the decoder, so they are not padding problems
+		assert.Equal(t, "length 9, contains whitespace", describeBase64Value("aGVsbG8=\n"))
+
+		assert.Equal(
+			t,
+			"length 6, not padded to a multiple of 4",
+			describeBase64Value("aGV-b8"),
+		)
+	})
+
+	t.Run("does not include the value itself", func(t *testing.T) {
+		assert.NotContains(t, describeBase64Value("super-secret-value"), "secret")
+	})
+}
+
+func Test__ValidateEncoding(t *testing.T) {
+	validValue := base64.StdEncoding.EncodeToString([]byte("VALUE"))
+
+	t.Run("no error when everything decodes", func(t *testing.T) {
+		request := JobRequest{
+			EnvVars: []EnvVar{{Name: "A", Value: validValue}},
+			Files:   []File{{Path: "a/b/c", Content: validValue}},
+		}
+
+		assert.NoError(t, request.ValidateEncoding())
+	})
+
+	t.Run("no error when there is nothing to decode", func(t *testing.T) {
+		assert.NoError(t, (&JobRequest{}).ValidateEncoding())
+	})
+
+	t.Run("collects every offender", func(t *testing.T) {
+		request := JobRequest{
+			EnvVars: []EnvVar{
+				{Name: "GOOD", Value: validValue},
+				{Name: "BAD_ONE", Value: "abc$def"},
+				{Name: "BAD_TWO", Value: "-_-_"},
+			},
+			Files: []File{{Path: "d/e/f", Content: "abc$def"}},
+		}
+
+		err := request.ValidateEncoding()
+		assert.ErrorContains(t, err, "error decoding 'BAD_ONE'")
+		assert.ErrorContains(t, err, "error decoding 'BAD_TWO'")
+		assert.ErrorContains(t, err, "error decoding 'd/e/f'")
+		assert.NotContains(t, err.Error(), "GOOD")
+	})
+
+	t.Run("every offender stays individually reachable", func(t *testing.T) {
+		request := JobRequest{
+			EnvVars: []EnvVar{
+				{Name: "BAD_ONE", Value: "abc$def"},
+				{Name: "BAD_TWO", Value: "-_-_"},
+			},
+			Files: []File{{Path: "d/e/f", Content: "abc$def"}},
+		}
+
+		err := request.ValidateEncoding()
+		joined, ok := err.(interface{ Unwrap() []error })
+		assert.True(t, ok, "expected the errors to be joined, not flattened")
+		assert.Len(t, joined.Unwrap(), 3)
+
+		// the underlying base64 error survives the wrapping
+		var corrupt base64.CorruptInputError
+		assert.True(t, errors.As(err, &corrupt))
+	})
+
+	t.Run("env vars are reported before files", func(t *testing.T) {
+		request := JobRequest{
+			EnvVars: []EnvVar{{Name: "BAD_ENV_VAR", Value: "abc$def"}},
+			Files:   []File{{Path: "bad/file", Content: "abc$def"}},
+		}
+
+		err := request.ValidateEncoding()
+		assert.Error(t, err)
+		assert.Less(
+			t,
+			strings.Index(err.Error(), "BAD_ENV_VAR"),
+			strings.Index(err.Error(), "bad/file"),
+		)
+	})
+
+	// SSH public keys are a known gap: a bad key is fatal on the docker-compose
+	// executor, but it is not validated here and PublicKey.Decode carries no name.
+	t.Run("container env vars and image pull credentials are out of scope", func(t *testing.T) {
+		request := JobRequest{
+			SSHPublicKeys: []PublicKey{"abc$def"},
+			Compose: Compose{
+				Containers: []Container{
+					{Name: "main", EnvVars: []EnvVar{{Name: "CONTAINER_VAR", Value: "abc$def"}}},
+				},
+				ImagePullCredentials: []ImagePullCredentials{
+					{EnvVars: []EnvVar{{Name: "CREDENTIAL_VAR", Value: "abc$def"}}},
+				},
+			},
+		}
+
+		assert.NoError(t, request.ValidateEncoding())
+	})
+}

pkg/executors/authorized_keys.go

@@ -36,8 +36,8 @@ func InjectEntriesToAuthorizedKeys(keys []api.PublicKey) error {
 		return err
 	}
 
-	for _, key := range keys {
-		authorizedKeysEntry, err := key.Decode()
+	for index, key := range keys {
+		authorizedKeysEntry, err := key.DecodeAt(index)
 		if err != nil {
 			_ = authorizedKeys.Close()
 			return err