From b07f567831136ee093bcc6b38fa7b07ba613d7da Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 19 Jul 2022 16:17:34 -0400 Subject: [PATCH] secure vars: rename automatically accessible vars path for jobs (#13848) Tasks are automatically granted access to variables on a path that matches their workload identity, with a well-known prefix. Change the prefix to `nomad/jobs` to allow for future prefixes like `nomad/volumes` or `nomad/plugins`. Reserve the prefix by emitting errors during validation. --- nomad/secure_variables_endpoint.go | 2 +- nomad/secure_variables_endpoint_test.go | 32 ++++++++++++------------- nomad/structs/secure_variables.go | 13 ++++++++++ nomad/structs/secure_variables_test.go | 32 +++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 17 deletions(-) diff --git a/nomad/secure_variables_endpoint.go b/nomad/secure_variables_endpoint.go index 39cdd3580..dd30655a8 100644 --- a/nomad/secure_variables_endpoint.go +++ b/nomad/secure_variables_endpoint.go @@ -515,7 +515,7 @@ func (sv *SecureVariables) authValidatePrefix(claims *structs.IdentityClaims, ns } parts := strings.Split(pathOrPrefix, "/") - expect := []string{"jobs", alloc.Job.ID, alloc.TaskGroup, claims.TaskName} + expect := []string{"nomad", "jobs", alloc.Job.ID, alloc.TaskGroup, claims.TaskName} if len(parts) > len(expect) { return structs.ErrPermissionDenied } diff --git a/nomad/secure_variables_endpoint_test.go b/nomad/secure_variables_endpoint_test.go index 2649b0d36..70db95f7b 100644 --- a/nomad/secure_variables_endpoint_test.go +++ b/nomad/secure_variables_endpoint_test.go @@ -66,7 +66,7 @@ func TestSecureVariablesEndpoint_auth(t *testing.T) { policy.Name = fmt.Sprintf("_:%s/%s/%s", ns, jobID, alloc1.TaskGroup) policy.Rules = `namespace "nondefault-namespace" { secure_variables { - path "jobs/*" { capabilities = ["read"] } + path "nomad/jobs/*" { capabilities = ["read"] } path "other/path" { capabilities = ["read"] } }}` policy.SetHash() @@ -81,7 +81,7 @@ func TestSecureVariablesEndpoint_auth(t *testing.T) { t.Run("terminal alloc should be denied", func(t *testing.T) { err = srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint( structs.QueryOptions{AuthToken: idToken, Namespace: ns}, "n/a", - fmt.Sprintf("jobs/%s/web/web", jobID)) + fmt.Sprintf("nomad/jobs/%s/web/web", jobID)) require.EqualError(t, err, structs.ErrPermissionDenied.Error()) }) @@ -93,7 +93,7 @@ func TestSecureVariablesEndpoint_auth(t *testing.T) { t.Run("wrong namespace should be denied", func(t *testing.T) { err = srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint( structs.QueryOptions{AuthToken: idToken, Namespace: structs.DefaultNamespace}, "n/a", - fmt.Sprintf("jobs/%s/web/web", jobID)) + fmt.Sprintf("nomad/jobs/%s/web/web", jobID)) require.EqualError(t, err, structs.ErrPermissionDenied.Error()) }) @@ -108,28 +108,28 @@ func TestSecureVariablesEndpoint_auth(t *testing.T) { name: "valid claim for path with task secret", token: idToken, cap: "n/a", - path: fmt.Sprintf("jobs/%s/web/web", jobID), + path: fmt.Sprintf("nomad/jobs/%s/web/web", jobID), expectedErr: nil, }, { name: "valid claim for path with group secret", token: idToken, cap: "n/a", - path: fmt.Sprintf("jobs/%s/web", jobID), + path: fmt.Sprintf("nomad/jobs/%s/web", jobID), expectedErr: nil, }, { name: "valid claim for path with job secret", token: idToken, cap: "n/a", - path: fmt.Sprintf("jobs/%s", jobID), + path: fmt.Sprintf("nomad/jobs/%s", jobID), expectedErr: nil, }, { name: "valid claim for path with namespace secret", token: idToken, cap: "n/a", - path: "jobs", + path: "nomad/jobs", expectedErr: nil, }, { @@ -157,62 +157,62 @@ func TestSecureVariablesEndpoint_auth(t *testing.T) { name: "valid claim with no permissions denied by path", token: noPermissionsToken, cap: "n/a", - path: fmt.Sprintf("jobs/%s/w", jobID), + path: fmt.Sprintf("nomad/jobs/%s/w", jobID), expectedErr: structs.ErrPermissionDenied, }, { name: "valid claim with no permissions allowed by namespace", token: noPermissionsToken, cap: "n/a", - path: "jobs", + path: "nomad/jobs", expectedErr: nil, }, { name: "valid claim with no permissions denied by capability", token: noPermissionsToken, cap: acl.PolicyRead, - path: fmt.Sprintf("jobs/%s/w", jobID), + path: fmt.Sprintf("nomad/jobs/%s/w", jobID), expectedErr: structs.ErrPermissionDenied, }, { name: "extra trailing slash is denied", token: idToken, cap: "n/a", - path: fmt.Sprintf("jobs/%s/web/", jobID), + path: fmt.Sprintf("nomad/jobs/%s/web/", jobID), expectedErr: structs.ErrPermissionDenied, }, { name: "invalid prefix is denied", token: idToken, cap: "n/a", - path: fmt.Sprintf("jobs/%s/w", jobID), + path: fmt.Sprintf("nomad/jobs/%s/w", jobID), expectedErr: structs.ErrPermissionDenied, }, { name: "missing auth token is denied", cap: "n/a", - path: fmt.Sprintf("jobs/%s/web/web", jobID), + path: fmt.Sprintf("nomad/jobs/%s/web/web", jobID), expectedErr: structs.ErrPermissionDenied, }, { name: "invalid signature is denied", token: invalidIDToken, cap: "n/a", - path: fmt.Sprintf("jobs/%s/web/web", jobID), + path: fmt.Sprintf("nomad/jobs/%s/web/web", jobID), expectedErr: structs.ErrPermissionDenied, }, { name: "acl token read policy is allowed to list", token: aclToken.SecretID, cap: acl.PolicyList, - path: fmt.Sprintf("jobs/%s/web/web", jobID), + path: fmt.Sprintf("nomad/jobs/%s/web/web", jobID), expectedErr: nil, }, { name: "acl token read policy is not allowed to write", token: aclToken.SecretID, cap: acl.PolicyWrite, - path: fmt.Sprintf("jobs/%s/web/web", jobID), + path: fmt.Sprintf("nomad/jobs/%s/web/web", jobID), expectedErr: structs.ErrPermissionDenied, }, } diff --git a/nomad/structs/secure_variables.go b/nomad/structs/secure_variables.go index 25e36d638..c4e677105 100644 --- a/nomad/structs/secure_variables.go +++ b/nomad/structs/secure_variables.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "reflect" + "strings" "time" // note: this is aliased so that it's more noticeable if someone @@ -148,6 +149,18 @@ func (sv SecureVariableData) Copy() SecureVariableData { } func (sv SecureVariableDecrypted) Validate() error { + + if len(sv.Path) == 0 { + return fmt.Errorf("variable requires path") + } + parts := strings.Split(sv.Path, "/") + switch { + case len(parts) == 1 && parts[0] == "nomad": + return fmt.Errorf("\"nomad\" is a reserved top-level directory path, but you may write variables to \"nomad/jobs\" or below") + case len(parts) >= 2 && parts[0] == "nomad" && parts[1] != "jobs": + return fmt.Errorf("only paths at \"nomad/jobs\" or below are valid paths under the top-level \"nomad\" directory") + } + if len(sv.Items) == 0 { return errors.New("empty variables are invalid") } diff --git a/nomad/structs/secure_variables_test.go b/nomad/structs/secure_variables_test.go index 02a1da2a9..448e9c575 100644 --- a/nomad/structs/secure_variables_test.go +++ b/nomad/structs/secure_variables_test.go @@ -31,3 +31,35 @@ func TestStructs_SecureVariableDecrypted_Copy(t *testing.T) { sv2.Items["new"] = "new" require.False(t, sv.Equals(sv2), "sv and sv2 should not be equal") } + +func TestStructs_SecureVariableDecrypted_Validate(t *testing.T) { + ci.Parallel(t) + + sv := SecureVariableDecrypted{ + SecureVariableMetadata: SecureVariableMetadata{Namespace: "a"}, + Items: SecureVariableItems{"foo": "bar"}, + } + + testCases := []struct { + path string + ok bool + }{ + {path: ""}, + {path: "nomad"}, + {path: "nomad/other"}, + {path: "a/b/c", ok: true}, + {path: "nomad/jobs", ok: true}, + {path: "nomadjobs", ok: true}, + {path: "nomad/jobs/whatever", ok: true}, + } + for _, tc := range testCases { + tc := tc + sv.Path = tc.path + err := sv.Validate() + if tc.ok { + require.NoError(t, err, "should not get error for: %s", tc.path) + } else { + require.Error(t, err, "should get error for: %s", tc.path) + } + } +}