From 350151569ab681fbee7a8f3f65a93891a8cb4f9f Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 11 Oct 2023 12:03:35 -0400 Subject: [PATCH 1/4] auth: add client-only ACL The RPC handlers expect to see `nil` ACL objects whenever ACLs are disabled. By using `nil` as a sentinel value, we have the risk of nil pointer exceptions and improper handling of `nil` when returned from our various auth methods that can lead to privilege escalation bugs. This is the third in a series to eliminate the use of `nil` ACLs as a sentinel value for when ACLs are disabled. This patch involves creating a new "virtual" ACL object for checking permissions on client operations and a matching `AuthenticateClientOnly` method for client-only RPCs that can produce that ACL. Unlike the server ACLs PR, this also includes a special case for "legacy" client RPCs where the client was not previously sending the secret as it should (leaning on mTLS only). Those client RPCs were fixed in Nomad 1.6.0, but it'll take a while before we can guarantee they'll be present during upgrades. Ref: https://github.com/hashicorp/nomad-enterprise/pull/1218 Ref: https://github.com/hashicorp/nomad/pull/18703 Ref: https://github.com/hashicorp/nomad/pull/18715 Ref: https://github.com/hashicorp/nomad/pull/16799 --- .semgrep/rpc_endpoint.yml | 29 +++- acl/acl.go | 44 +++++- acl/acl_test.go | 3 + acl/virtual.go | 12 ++ nomad/acl.go | 12 +- nomad/alloc_endpoint.go | 17 +-- nomad/alloc_endpoint_test.go | 10 +- nomad/auth/auth.go | 113 ++++++++++++-- nomad/auth/auth_test.go | 159 ++++++++++++++++++++ nomad/csi_endpoint.go | 11 +- nomad/node_endpoint.go | 37 +++-- nomad/node_endpoint_test.go | 7 +- nomad/operator_endpoint_test.go | 16 +- nomad/rpc_test.go | 2 +- nomad/service_registration_endpoint.go | 80 +++------- nomad/service_registration_endpoint_test.go | 6 +- 16 files changed, 419 insertions(+), 139 deletions(-) diff --git a/.semgrep/rpc_endpoint.yml b/.semgrep/rpc_endpoint.yml index 5fc8f22ebbd6..c4ad201d9f3e 100644 --- a/.semgrep/rpc_endpoint.yml +++ b/.semgrep/rpc_endpoint.yml @@ -40,17 +40,38 @@ rules: } ... - # Pattern used by endpoints that are used by both ACLs and Clients. - # These endpoints will always have a ctx passed to Authenticate + + # Pattern used by endpoints that are used only for client-to-server. + # Authorization can be done after forwarding, but must check the + # AllowClientOp policy - pattern-not-inside: | - authErr := $A.$B.Authenticate($A.ctx, args) + aclObj, err := $A.$B.AuthenticateClientOnly($A.ctx, args) + ... + if done, err := $A.$B.forward($METHOD, ...); done { + return err + } + ... + if !aclObj.AllowClientOp() { + return structs.ErrPermissionDenied + } + ... + + + # Pattern used by endpoints that are used only for client-to-server. + # Authorization can be done after forwarding, but must check the + # AllowClientOp policy. This should not be added to any new endpoints. + - pattern-not-inside: | + aclObj, err := $A.$B.AuthenticateClientOnlyLegacy($A.ctx, args) ... if done, err := $A.$B.forward($METHOD, ...); done { return err } ... - ... := $A.$B.ResolveClientOrACL(...) + if !aclObj.AllowClientOp() { + return structs.ErrPermissionDenied + } ... + # Pattern used by ACL endpoints that need to interact with the token directly - pattern-not-inside: | authErr := $A.$B.Authenticate($A.ctx, args) diff --git a/acl/acl.go b/acl/acl.go index a791dc576c1a..44c807b1c755 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -78,6 +78,7 @@ type ACL struct { // The attributes below detail a virtual policy that we never expose // directly to the end user. + client string server string isLeader bool } @@ -301,6 +302,7 @@ func NewACL(management bool, policies []*Policy) (*ACL, error) { acl.variables = svTxn.Commit() acl.wildcardVariables = wsvTxn.Commit() + acl.client = PolicyDeny acl.server = PolicyDeny acl.isLeader = false @@ -332,6 +334,11 @@ func (a *ACL) AllowNamespaceOperation(ns string, op string) bool { return true } + // Clients need to be able to read their namespaced objects + if a.client != PolicyDeny { + return true + } + // If using the all namespaces wildcard, allow if any namespace allows the // operation. if ns == AllNamespacesSentinel && a.anyNamespaceAllowsOp(op) { @@ -731,6 +738,9 @@ func (a *ACL) AllowNodeRead() bool { return true case a.node == PolicyRead: return true + case a.client == PolicyRead, + a.client == PolicyWrite: + return true case a.server == PolicyRead, a.server == PolicyWrite: return true @@ -813,6 +823,9 @@ func (a *ACL) AllowPluginRead() bool { return true case a.management: return true + case a.client == PolicyRead, + a.client == PolicyWrite: + return true case a.plugin == PolicyRead: return true default: @@ -828,6 +841,9 @@ func (a *ACL) AllowPluginList() bool { return true case a.management: return true + case a.client == PolicyRead, + a.client == PolicyWrite: + return true case a.plugin == PolicyList: return true case a.plugin == PolicyRead: @@ -837,6 +853,15 @@ func (a *ACL) AllowPluginList() bool { } } +func (a *ACL) AllowServiceRegistrationReadList(ns string, isWorkload bool) bool { + if a == nil { + // ACL is nil only if ACLs are disabled + // TODO(tgross): return false when there are no nil ACLs + return true + } + return isWorkload || a.AllowNsOp(ns, NamespaceCapabilityReadJob) +} + // AllowServerOp checks if server-only operations are allowed func (a *ACL) AllowServerOp() bool { if a == nil { @@ -847,6 +872,15 @@ func (a *ACL) AllowServerOp() bool { return a.server != PolicyDeny || a.isLeader } +func (a *ACL) AllowClientOp() bool { + if a == nil { + // ACL is nil only if ACLs are disabled + // TODO(tgross): return false when there are no nil ACLs + return true + } + return a.client != PolicyDeny +} + // IsManagement checks if this represents a management token func (a *ACL) IsManagement() bool { return a.management @@ -856,14 +890,18 @@ func (a *ACL) IsManagement() bool { // a list of operations. Returns true (allowed) if acls are disabled or if // *any* capabilities match. func NamespaceValidator(ops ...string) func(*ACL, string) bool { - return func(acl *ACL, ns string) bool { + return func(a *ACL, ns string) bool { // Always allow if ACLs are disabled. - if acl == nil { + if a == nil { + return true + } + // Clients need to be able to read namespaced objects + if a.client != PolicyDeny { return true } for _, op := range ops { - if acl.AllowNamespaceOperation(ns, op) { + if a.AllowNamespaceOperation(ns, op) { // An operation is allowed, return true return true } diff --git a/acl/acl_test.go b/acl/acl_test.go index 260daa10067b..a0332f6559c2 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -100,6 +100,7 @@ func TestACLManagement(t *testing.T) { must.True(t, acl.AllowQuotaRead()) must.True(t, acl.AllowQuotaWrite()) must.True(t, acl.AllowServerOp()) + must.True(t, acl.AllowClientOp()) } func TestACLMerge(t *testing.T) { @@ -143,6 +144,7 @@ func TestACLMerge(t *testing.T) { must.True(t, acl.AllowQuotaRead()) must.True(t, acl.AllowQuotaWrite()) must.False(t, acl.AllowServerOp()) + must.False(t, acl.AllowClientOp()) // Merge read + blank p3, err := Parse("") @@ -178,6 +180,7 @@ func TestACLMerge(t *testing.T) { must.True(t, acl.AllowQuotaRead()) must.False(t, acl.AllowQuotaWrite()) must.False(t, acl.AllowServerOp()) + must.False(t, acl.AllowClientOp()) // Merge read + deny p4, err := Parse(denyAll) diff --git a/acl/virtual.go b/acl/virtual.go index e9fd58813a69..fa136a3cf799 100644 --- a/acl/virtual.go +++ b/acl/virtual.go @@ -3,8 +3,20 @@ package acl +var ClientACL = initClientACL() var ServerACL = initServerACL() +func initClientACL() *ACL { + aclObj, err := NewACL(false, []*Policy{}) + if err != nil { + panic(err) + } + aclObj.client = PolicyWrite + aclObj.agent = PolicyRead + aclObj.server = PolicyRead + return aclObj +} + func initServerACL() *ACL { aclObj, err := NewACL(false, []*Policy{}) if err != nil { diff --git a/nomad/acl.go b/nomad/acl.go index 22cb7c65e692..39157d3e3f1b 100644 --- a/nomad/acl.go +++ b/nomad/acl.go @@ -16,6 +16,14 @@ func (srv *Server) AuthenticateServerOnly(ctx *RPCContext, args structs.RequestW return srv.auth.AuthenticateServerOnly(ctx, args) } +func (srv *Server) AuthenticateClientOnly(ctx *RPCContext, args structs.RequestWithIdentity) (*acl.ACL, error) { + return srv.auth.AuthenticateClientOnly(ctx, args) +} + +func (srv *Server) AuthenticateClientOnlyLegacy(ctx *RPCContext, args structs.RequestWithIdentity) (*acl.ACL, error) { + return srv.auth.AuthenticateClientOnlyLegacy(ctx, args) +} + func (srv *Server) ResolveACL(args structs.RequestWithIdentity) (*acl.ACL, error) { return srv.auth.ResolveACL(args) } @@ -28,10 +36,6 @@ func (srv *Server) ResolveToken(secretID string) (*acl.ACL, error) { return srv.auth.ResolveToken(secretID) } -func (srv *Server) ResolveClientOrACL(args structs.RequestWithIdentity) (*acl.ACL, error) { - return srv.auth.ResolveClientOrACL(args) -} - func (srv *Server) ResolvePoliciesForClaims(claims *structs.IdentityClaims) ([]*structs.ACLPolicy, error) { return srv.auth.ResolvePoliciesForClaims(claims) } diff --git a/nomad/alloc_endpoint.go b/nomad/alloc_endpoint.go index ee0d3d52703b..bdd1f5801e68 100644 --- a/nomad/alloc_endpoint.go +++ b/nomad/alloc_endpoint.go @@ -155,7 +155,7 @@ func (a *Alloc) GetAlloc(args *structs.AllocSpecificRequest, // Check namespace read-job permissions before performing blocking query. allowNsOp := acl.NamespaceValidator(acl.NamespaceCapabilityReadJob) - aclObj, err := a.srv.ResolveClientOrACL(args) + aclObj, err := a.srv.ResolveACL(args) if err != nil { return err } @@ -200,21 +200,20 @@ func (a *Alloc) GetAlloc(args *structs.AllocSpecificRequest, func (a *Alloc) GetAllocs(args *structs.AllocsGetRequest, reply *structs.AllocsGetResponse) error { - authErr := a.srv.Authenticate(a.ctx, args) - - // Ensure the connection was initiated by a client if TLS is used. - err := validateTLSCertificateLevel(a.srv, a.ctx, tlsCertificateLevelClient) + aclObj, err := a.srv.AuthenticateClientOnly(a.ctx, args) + a.srv.MeasureRPCRate("alloc", structs.RateMetricWrite, args) if err != nil { - return err + return structs.ErrPermissionDenied } + if done, err := a.srv.forward("Alloc.GetAllocs", args, args, reply); done { return err } - a.srv.MeasureRPCRate("alloc", structs.RateMetricList, args) - if authErr != nil { + defer metrics.MeasureSince([]string{"nomad", "alloc", "get_allocs"}, time.Now()) + + if !aclObj.AllowClientOp() { return structs.ErrPermissionDenied } - defer metrics.MeasureSince([]string{"nomad", "alloc", "get_allocs"}, time.Now()) allocs := make([]*structs.Allocation, len(args.AllocIDs)) diff --git a/nomad/alloc_endpoint_test.go b/nomad/alloc_endpoint_test.go index e5d6d99405e6..dbbdda87f16c 100644 --- a/nomad/alloc_endpoint_test.go +++ b/nomad/alloc_endpoint_test.go @@ -948,11 +948,15 @@ func TestAllocEndpoint_GetAllocs(t *testing.T) { t.Fatalf("err: %v", err) } + node := mock.Node() + state.UpsertNode(structs.MsgTypeTestSetup, 1001, node) + // Lookup the allocs get := &structs.AllocsGetRequest{ AllocIDs: []string{alloc.ID, alloc2.ID}, QueryOptions: structs.QueryOptions{ - Region: "global", + Region: "global", + AuthToken: node.SecretID, }, } var resp structs.AllocsGetResponse @@ -986,6 +990,9 @@ func TestAllocEndpoint_GetAllocs_Blocking(t *testing.T) { codec := rpcClient(t, s1) testutil.WaitForLeader(t, s1.RPC) + node := mock.Node() + state.UpsertNode(structs.MsgTypeTestSetup, 50, node) + // Create the allocs alloc1 := mock.Alloc() alloc2 := mock.Alloc() @@ -1014,6 +1021,7 @@ func TestAllocEndpoint_GetAllocs_Blocking(t *testing.T) { QueryOptions: structs.QueryOptions{ Region: "global", MinQueryIndex: 150, + AuthToken: node.SecretID, }, } var resp structs.AllocsGetResponse diff --git a/nomad/auth/auth.go b/nomad/auth/auth.go index 69756839a8d9..7602086c5005 100644 --- a/nomad/auth/auth.go +++ b/nomad/auth/auth.go @@ -249,6 +249,103 @@ func (s *Authenticator) AuthenticateServerOnly(ctx RPCContext, args structs.Requ return acl.ServerACL, nil } +// AuthenticateClientOnly returns an ACL object for use *only* with internal +// RPCs originating from clients (including those forwarded). This should never +// be used for RPCs that serve HTTP endpoints to avoid confused deputy attacks +// by making a request to a client that's forwarded. It should also not be used +// with Node.Register, which should use AuthenticateClientTOFU +// +// The returned ACL object is always a acl.ClientACL but in the future this +// could be extended to allow clients access only to their own pool and +// associated namespaces, etc. +func (s *Authenticator) AuthenticateClientOnly(ctx RPCContext, args structs.RequestWithIdentity) (*acl.ACL, error) { + + remoteIP, err := ctx.GetRemoteIP() // capture for metrics + if err != nil { + s.logger.Error("could not determine remote address", "error", err) + } + + identity := &structs.AuthenticatedIdentity{RemoteIP: remoteIP} + defer args.SetIdentity(identity) // always set the identity, even on errors + + if s.tlsEnabled && !ctx.IsStatic() { + tlsCert := ctx.Certificate() + if tlsCert == nil { + return nil, errors.New("missing certificate information") + } + + // set on the identity whether or not its valid for server RPC, so we + // can capture it for metrics + identity.TLSName = tlsCert.Subject.CommonName + + expected := fmt.Sprintf("client.%s.nomad", s.region) + _, err := validateCertificateForName(tlsCert, expected) + if err != nil { + expected := fmt.Sprintf("server.%s.nomad", s.region) + _, err := validateCertificateForName(tlsCert, expected) + if err != nil { + return nil, err + } + } + } + + secretID := args.GetAuthToken() + if secretID == "" { + return nil, structs.ErrPermissionDenied + } + + // Otherwise, see if the secret ID belongs to a node. We should + // reach this point only on first connection. + node, err := s.getState().NodeBySecretID(nil, secretID) + if err != nil { + // this is a go-memdb error; shouldn't happen + return nil, fmt.Errorf("could not resolve node secret: %w", err) + } + if node == nil { + return nil, structs.ErrPermissionDenied + } + identity.ClientID = node.ID + return acl.ClientACL, nil +} + +// AuthenticateClientOnlyLegacy is a version of AuthenticateClientOnly that's +// used by a few older RPCs that did not properly enforce node secrets. +// COMPAT(1.8.0): In Nomad 1.6.0 we starting sending those node secrets, so we +// can remove this in Nomad 1.8.0. +func (s *Authenticator) AuthenticateClientOnlyLegacy(ctx RPCContext, args structs.RequestWithIdentity) (*acl.ACL, error) { + + remoteIP, err := ctx.GetRemoteIP() // capture for metrics + if err != nil { + s.logger.Error("could not determine remote address", "error", err) + } + + identity := &structs.AuthenticatedIdentity{RemoteIP: remoteIP} + defer args.SetIdentity(identity) // always set the identity, even on errors + + if s.tlsEnabled && !ctx.IsStatic() { + tlsCert := ctx.Certificate() + if tlsCert == nil { + return nil, errors.New("missing certificate information") + } + + // set on the identity whether or not its valid for server RPC, so we + // can capture it for metrics + identity.TLSName = tlsCert.Subject.CommonName + + expected := fmt.Sprintf("client.%s.nomad", s.region) + _, err := validateCertificateForName(tlsCert, expected) + if err != nil { + expected := fmt.Sprintf("server.%s.nomad", s.region) + _, err := validateCertificateForName(tlsCert, expected) + if err != nil { + return nil, err + } + } + } + + return acl.ClientACL, nil +} + // validateCertificateForName returns true if the certificate is valid // for the given domain name. func validateCertificateForName(cert *x509.Certificate, expectedName string) (bool, error) { @@ -285,22 +382,6 @@ func (s *Authenticator) ResolveACLForToken(aclToken *structs.ACLToken) (*acl.ACL return resolveACLFromToken(snap, s.aclCache, aclToken) } -// ResolveClientOrACL resolves an ACL if the identity has a token or claim, and -// falls back to verifying the client ID if one has been set -func (s *Authenticator) ResolveClientOrACL(args structs.RequestWithIdentity) (*acl.ACL, error) { - identity := args.GetIdentity() - if !s.aclsEnabled || identity == nil || identity.ClientID != "" { - return nil, nil - } - aclObj, err := s.ResolveACL(args) - if err != nil { - return nil, err - } - - // Returns either the users aclObj, or nil if ACLs are disabled. - return aclObj, nil -} - // ResolveToken is used to translate an ACL Token Secret ID into // an ACL object, nil if ACLs are disabled, or an error. func (s *Authenticator) ResolveToken(secretID string) (*acl.ACL, error) { diff --git a/nomad/auth/auth_test.go b/nomad/auth/auth_test.go index 8184c449eba3..bcac38976fe7 100644 --- a/nomad/auth/auth_test.go +++ b/nomad/auth/auth_test.go @@ -414,6 +414,165 @@ func TestAuthenticateServerOnly(t *testing.T) { } +func TestAuthenticateClientOnly(t *testing.T) { + ci.Parallel(t) + + testAuthenticator := func(t *testing.T, store *state.StateStore, + hasACLs, hasTLS bool) *Authenticator { + leaderACL := uuid.Generate() + + return NewAuthenticator(&AuthenticatorConfig{ + StateFn: func() *state.StateStore { return store }, + Logger: testlog.HCLogger(t), + GetLeaderACLFn: func() string { return leaderACL }, + AclsEnabled: hasACLs, + TLSEnabled: hasTLS, + Region: "global", + Encrypter: nil, + }) + } + + testCases := []struct { + name string + testFn func(*testing.T, *state.StateStore, *structs.Node) + }{ + { + name: "no mTLS or ACLs but no node secret", + testFn: func(t *testing.T, store *state.StateStore, node *structs.Node) { + ctx := newTestContext(t, noTLSCtx, "192.168.1.1") + args := &structs.GenericRequest{} + args.AuthToken = "" + + auth := testAuthenticator(t, store, false, false) + + aclObj, err := auth.AuthenticateClientOnly(ctx, args) + must.ErrorIs(t, err, structs.ErrPermissionDenied) + must.Eq(t, ":192.168.1.1", args.GetIdentity().String()) + must.Nil(t, aclObj) + }, + }, + { + name: "no mTLS or ACLs but with node secret", + testFn: func(t *testing.T, store *state.StateStore, node *structs.Node) { + ctx := newTestContext(t, noTLSCtx, "192.168.1.1") + args := &structs.GenericRequest{} + args.AuthToken = node.SecretID + + auth := testAuthenticator(t, store, false, false) + + aclObj, err := auth.AuthenticateClientOnly(ctx, args) + must.NoError(t, err) + must.NotNil(t, aclObj) + must.Eq(t, "client:"+node.ID, args.GetIdentity().String()) + }, + }, + { + name: "no mTLS but with ACLs", + testFn: func(t *testing.T, store *state.StateStore, node *structs.Node) { + ctx := newTestContext(t, noTLSCtx, "192.168.1.1") + args := &structs.GenericRequest{} + args.AuthToken = node.SecretID + + auth := testAuthenticator(t, store, true, false) + + aclObj, err := auth.AuthenticateClientOnly(ctx, args) + must.NoError(t, err) + must.NotNil(t, aclObj) + must.Eq(t, "client:"+node.ID, args.GetIdentity().String()) + must.True(t, aclObj.AllowClientOp()) + }, + }, + { + name: "no mTLS but with ACLs and bad secret", + testFn: func(t *testing.T, store *state.StateStore, node *structs.Node) { + ctx := newTestContext(t, noTLSCtx, "192.168.1.1") + args := &structs.GenericRequest{} + args.AuthToken = uuid.Generate() + + auth := testAuthenticator(t, store, true, false) + + aclObj, err := auth.AuthenticateClientOnly(ctx, args) + must.ErrorIs(t, err, structs.ErrPermissionDenied) + must.Eq(t, ":192.168.1.1", args.GetIdentity().String()) + must.Nil(t, aclObj) + }, + }, + { + name: "with mTLS and ACLs but CLI cert", + testFn: func(t *testing.T, store *state.StateStore, node *structs.Node) { + ctx := newTestContext(t, "cli.global.nomad", "192.168.1.1") + args := &structs.GenericRequest{} + + auth := testAuthenticator(t, store, true, true) + + aclObj, err := auth.AuthenticateClientOnly(ctx, args) + must.EqError(t, err, + "invalid certificate, server.global.nomad not in cli.global.nomad") + must.Eq(t, "cli.global.nomad:192.168.1.1", args.GetIdentity().String()) + must.Nil(t, aclObj) + }, + }, + { + name: "with mTLS and ACLs with server cert but bad token", + testFn: func(t *testing.T, store *state.StateStore, node *structs.Node) { + ctx := newTestContext(t, "server.global.nomad", "192.168.1.1") + args := &structs.GenericRequest{} + args.AuthToken = uuid.Generate() + + auth := testAuthenticator(t, store, true, true) + + aclObj, err := auth.AuthenticateClientOnly(ctx, args) + must.ErrorIs(t, err, structs.ErrPermissionDenied) + must.Eq(t, "server.global.nomad:192.168.1.1", args.GetIdentity().String()) + must.Nil(t, aclObj) + }, + }, + { + name: "with mTLS and ACLs with server cert and valid token", + testFn: func(t *testing.T, store *state.StateStore, node *structs.Node) { + ctx := newTestContext(t, "server.global.nomad", "192.168.1.1") + args := &structs.GenericRequest{} + args.AuthToken = node.SecretID + + auth := testAuthenticator(t, store, true, true) + + aclObj, err := auth.AuthenticateClientOnly(ctx, args) + must.NoError(t, err) + + must.Eq(t, "client:"+node.ID, args.GetIdentity().String()) + must.NotNil(t, aclObj) + must.True(t, aclObj.AllowClientOp()) + }, + }, + { + name: "with mTLS and ACLs with client cert", + testFn: func(t *testing.T, store *state.StateStore, node *structs.Node) { + ctx := newTestContext(t, "client.global.nomad", "192.168.1.1") + args := &structs.GenericRequest{} + args.AuthToken = node.SecretID + + auth := testAuthenticator(t, store, true, true) + + aclObj, err := auth.AuthenticateClientOnly(ctx, args) + must.NoError(t, err) + must.Eq(t, "client:"+node.ID, args.GetIdentity().String()) + must.NotNil(t, aclObj) + must.True(t, aclObj.AllowClientOp()) + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + node := mock.Node() + store := testStateStore(t) + store.UpsertNode(structs.MsgTypeTestSetup, 100, node) + tc.testFn(t, store, node) + }) + } + +} + func TestResolveACLToken(t *testing.T) { ci.Parallel(t) diff --git a/nomad/csi_endpoint.go b/nomad/csi_endpoint.go index 3ffb41e0e252..8d32efd155a7 100644 --- a/nomad/csi_endpoint.go +++ b/nomad/csi_endpoint.go @@ -189,7 +189,7 @@ func (v *CSIVolume) Get(args *structs.CSIVolumeGetRequest, reply *structs.CSIVol allowCSIAccess := acl.NamespaceValidator(acl.NamespaceCapabilityCSIReadVolume, acl.NamespaceCapabilityCSIMountVolume, acl.NamespaceCapabilityReadJob) - aclObj, err := v.srv.ResolveClientOrACL(args) + aclObj, err := v.srv.ResolveACL(args) if err != nil { return err } @@ -452,14 +452,13 @@ func (v *CSIVolume) Claim(args *structs.CSIVolumeClaimRequest, reply *structs.CS return structs.ErrPermissionDenied } + defer metrics.MeasureSince([]string{"nomad", "volume", "claim"}, time.Now()) + allowVolume := acl.NamespaceValidator(acl.NamespaceCapabilityCSIMountVolume) - aclObj, err := v.srv.ResolveClientOrACL(args) + aclObj, err := v.srv.ResolveACL(args) if err != nil { return err } - - defer metrics.MeasureSince([]string{"nomad", "volume", "claim"}, time.Now()) - if !allowVolume(aclObj, args.RequestNamespace()) || !aclObj.AllowPluginRead() { return structs.ErrPermissionDenied } @@ -694,7 +693,7 @@ func (v *CSIVolume) Unpublish(args *structs.CSIVolumeUnpublishRequest, reply *st defer metrics.MeasureSince([]string{"nomad", "volume", "unpublish"}, time.Now()) allowVolume := acl.NamespaceValidator(acl.NamespaceCapabilityCSIMountVolume) - aclObj, err := v.srv.ResolveClientOrACL(args) + aclObj, err := v.srv.ResolveACL(args) if err != nil { return err } diff --git a/nomad/node_endpoint.go b/nomad/node_endpoint.go index 824da2c93ff7..485be68c237a 100644 --- a/nomad/node_endpoint.go +++ b/nomad/node_endpoint.go @@ -1021,7 +1021,7 @@ func (n *Node) GetNode(args *structs.NodeSpecificRequest, defer metrics.MeasureSince([]string{"nomad", "client", "get_node"}, time.Now()) // Check node read permissions - aclObj, err := n.srv.ResolveClientOrACL(args) + aclObj, err := n.srv.ResolveACL(args) if err != nil { return err } @@ -1317,23 +1317,21 @@ func (n *Node) GetClientAllocs(args *structs.NodeSpecificRequest, // - The node status is down or disconnected. Clients must call the // UpdateStatus method to update its status in the server. func (n *Node) UpdateAlloc(args *structs.AllocUpdateRequest, reply *structs.GenericResponse) error { - - authErr := n.srv.Authenticate(n.ctx, args) - - // Ensure the connection was initiated by another client if TLS is used. - err := validateTLSCertificateLevel(n.srv, n.ctx, tlsCertificateLevelClient) + // COMPAT(1.9.0): move to AuthenticateClientOnly + aclObj, err := n.srv.AuthenticateClientOnlyLegacy(n.ctx, args) + n.srv.MeasureRPCRate("node", structs.RateMetricWrite, args) if err != nil { - return err + return structs.ErrPermissionDenied } + if done, err := n.srv.forward("Node.UpdateAlloc", args, args, reply); done { return err } - n.srv.MeasureRPCRate("node", structs.RateMetricWrite, args) - if authErr != nil { - return structs.ErrPermissionDenied - } defer metrics.MeasureSince([]string{"nomad", "client", "update_alloc"}, time.Now()) + if !aclObj.AllowClientOp() { + return structs.ErrPermissionDenied + } // Ensure at least a single alloc if len(args.Alloc) == 0 { @@ -2253,22 +2251,21 @@ func taskUsesConnect(task *structs.Task) bool { } func (n *Node) EmitEvents(args *structs.EmitNodeEventsRequest, reply *structs.EmitNodeEventsResponse) error { - - authErr := n.srv.Authenticate(n.ctx, args) - - // Ensure the connection was initiated by another client if TLS is used. - err := validateTLSCertificateLevel(n.srv, n.ctx, tlsCertificateLevelClient) + // COMPAT(1.9.0): move to AuthenticateClientOnly + aclObj, err := n.srv.AuthenticateClientOnlyLegacy(n.ctx, args) + n.srv.MeasureRPCRate("node", structs.RateMetricWrite, args) if err != nil { - return err + return structs.ErrPermissionDenied } + if done, err := n.srv.forward("Node.EmitEvents", args, args, reply); done { return err } - n.srv.MeasureRPCRate("node", structs.RateMetricWrite, args) - if authErr != nil { + defer metrics.MeasureSince([]string{"nomad", "client", "emit_events"}, time.Now()) + + if !aclObj.AllowClientOp() { return structs.ErrPermissionDenied } - defer metrics.MeasureSince([]string{"nomad", "client", "emit_events"}, time.Now()) if len(args.NodeEvents) == 0 { return fmt.Errorf("no node events given") diff --git a/nomad/node_endpoint_test.go b/nomad/node_endpoint_test.go index c67fa2806728..c9b273efd417 100644 --- a/nomad/node_endpoint_test.go +++ b/nomad/node_endpoint_test.go @@ -4514,8 +4514,11 @@ func TestClientEndpoint_UpdateAlloc_Evals_ByTrigger(t *testing.T) { } updateReq := &structs.AllocUpdateRequest{ - Alloc: []*structs.Allocation{clientAlloc}, - WriteRequest: structs.WriteRequest{Region: "global"}, + Alloc: []*structs.Allocation{clientAlloc}, + WriteRequest: structs.WriteRequest{ + Region: "global", + AuthToken: node.SecretID, + }, } var nodeAllocResp structs.NodeAllocsResponse diff --git a/nomad/operator_endpoint_test.go b/nomad/operator_endpoint_test.go index 0608d8794100..02d5c932f6b8 100644 --- a/nomad/operator_endpoint_test.go +++ b/nomad/operator_endpoint_test.go @@ -487,7 +487,7 @@ func TestOperator_TransferLeadershipToServerAddress_ACL(t *testing.T) { // Create ACL token invalidToken := mock.CreatePolicyAndToken(t, state, 1001, "test-invalid", mock.NodePolicy(acl.PolicyWrite)) - arg := structs.RaftPeerRequest{ + arg := &structs.RaftPeerRequest{ RaftIDAddress: structs.RaftIDAddress{Address: addr}, WriteRequest: structs.WriteRequest{Region: s1.config.Region}, } @@ -496,7 +496,7 @@ func TestOperator_TransferLeadershipToServerAddress_ACL(t *testing.T) { t.Run("no-token", func(t *testing.T) { // Try with no token and expect permission denied - err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", &arg, &reply) + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", arg, &reply) must.Error(t, err) must.ErrorIs(t, err, rpcPermDeniedErr) }) @@ -504,7 +504,7 @@ func TestOperator_TransferLeadershipToServerAddress_ACL(t *testing.T) { t.Run("invalid-token", func(t *testing.T) { // Try with an invalid token and expect permission denied arg.AuthToken = invalidToken.SecretID - err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", &arg, &reply) + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", arg, &reply) must.Error(t, err) must.ErrorIs(t, err, rpcPermDeniedErr) }) @@ -512,7 +512,7 @@ func TestOperator_TransferLeadershipToServerAddress_ACL(t *testing.T) { t.Run("good-token", func(t *testing.T) { // Try with a management token arg.AuthToken = tc.token.SecretID - err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", &arg, &reply) + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", arg, &reply) must.NoError(t, err) // Is the expected leader the new one? @@ -543,7 +543,7 @@ func TestOperator_TransferLeadershipToServerID_ACL(t *testing.T) { // Create ACL token invalidToken := mock.CreatePolicyAndToken(t, state, 1001, "test-invalid", mock.NodePolicy(acl.PolicyWrite)) - arg := structs.RaftPeerRequest{ + arg := &structs.RaftPeerRequest{ RaftIDAddress: structs.RaftIDAddress{ ID: tgtID, }, @@ -554,7 +554,7 @@ func TestOperator_TransferLeadershipToServerID_ACL(t *testing.T) { t.Run("no-token", func(t *testing.T) { // Try with no token and expect permission denied - err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", &arg, &reply) + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", arg, &reply) must.Error(t, err) must.ErrorIs(t, err, rpcPermDeniedErr) }) @@ -562,7 +562,7 @@ func TestOperator_TransferLeadershipToServerID_ACL(t *testing.T) { t.Run("invalid-token", func(t *testing.T) { // Try with an invalid token and expect permission denied arg.AuthToken = invalidToken.SecretID - err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", &arg, &reply) + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", arg, &reply) must.Error(t, err) must.ErrorIs(t, err, rpcPermDeniedErr) }) @@ -570,7 +570,7 @@ func TestOperator_TransferLeadershipToServerID_ACL(t *testing.T) { t.Run("good-token", func(t *testing.T) { // Try with a management token arg.AuthToken = tc.token.SecretID - err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", &arg, &reply) + err := msgpackrpc.CallWithCodec(codec, "Operator.TransferLeadershipToPeer", arg, &reply) must.NoError(t, err) // Is the expected leader the new one? diff --git a/nomad/rpc_test.go b/nomad/rpc_test.go index 4ab9717865a2..a33c69b698df 100644 --- a/nomad/rpc_test.go +++ b/nomad/rpc_test.go @@ -1257,7 +1257,7 @@ func TestRPC_TLS_Enforcement_RPC(t *testing.T) { name: "other region server/clients only rpc", cn: "server.other.nomad", rpcs: localClientsOnlyRPCs, - expectErr: "(certificate|broken pipe)", + expectErr: "(Permission denied|broken pipe)", }, // Other region client. { diff --git a/nomad/service_registration_endpoint.go b/nomad/service_registration_endpoint.go index 68bbf54d564c..b5c9ac6cf86a 100644 --- a/nomad/service_registration_endpoint.go +++ b/nomad/service_registration_endpoint.go @@ -40,20 +40,20 @@ func (s *ServiceRegistration) Upsert( args *structs.ServiceRegistrationUpsertRequest, reply *structs.ServiceRegistrationUpsertResponse) error { - authErr := s.srv.Authenticate(s.ctx, args) - - // Ensure the connection was initiated by a client if TLS is used. - if err := validateTLSCertificateLevel(s.srv, s.ctx, tlsCertificateLevelClient); err != nil { - return err + aclObj, err := s.srv.AuthenticateClientOnly(s.ctx, args) + s.srv.MeasureRPCRate("service_registration", structs.RateMetricWrite, args) + if err != nil { + return structs.ErrPermissionDenied } + if done, err := s.srv.forward(structs.ServiceRegistrationUpsertRPCMethod, args, args, reply); done { return err } - s.srv.MeasureRPCRate("service_registration", structs.RateMetricWrite, args) - if authErr != nil { + defer metrics.MeasureSince([]string{"nomad", "service_registration", "upsert"}, time.Now()) + + if !aclObj.AllowClientOp() { return structs.ErrPermissionDenied } - defer metrics.MeasureSince([]string{"nomad", "service_registration", "upsert"}, time.Now()) // Nomad service registrations can only be used once all servers, in the // local region, have been upgraded to 1.3.0 or greater. @@ -62,17 +62,6 @@ func (s *ServiceRegistration) Upsert( minNomadServiceRegistrationVersion) } - // This endpoint is only callable by nodes in the cluster. Therefore, - // perform a node lookup using the secret ID to confirm the caller is a - // known node. - node, err := s.srv.fsm.State().NodeBySecretID(nil, args.AuthToken) - if err != nil { - return err - } - if node == nil { - return structs.ErrTokenNotFound - } - // Use a multierror, so we can capture all validation errors and pass this // back so fixing in a single swoop. var mErr multierror.Error @@ -124,41 +113,10 @@ func (s *ServiceRegistration) DeleteByID( minNomadServiceRegistrationVersion) } - // Perform the ACL token resolution. - aclObj, err := s.srv.ResolveACL(args) - - switch err { - case nil: - // If ACLs are enabled, ensure the caller has the submit-job namespace - // capability. - if aclObj != nil { - hasSubmitJob := aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) - if !hasSubmitJob { - return structs.ErrPermissionDenied - } - } - default: - // This endpoint is generally called by Nomad nodes, so we want to - // perform this check, unless the token resolution gave us a terminal - // error. - if err != structs.ErrTokenNotFound { - return err - } - - // Attempt to lookup AuthToken as a Node.SecretID and return any error - // wrapped along with the original. - node, stateErr := s.srv.fsm.State().NodeBySecretID(nil, args.AuthToken) - if stateErr != nil { - var mErr multierror.Error - mErr.Errors = append(mErr.Errors, err, stateErr) - return mErr.ErrorOrNil() - } - - // At this point, we do not have a valid ACL token, nor are we being - // called, or able to confirm via the state store, by a node. - if node == nil { - return structs.ErrTokenNotFound - } + if aclObj, err := s.srv.ResolveACL(args); err != nil { + return structs.ErrPermissionDenied + } else if !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilitySubmitJob) { + return structs.ErrPermissionDenied } // Update via Raft. @@ -216,12 +174,12 @@ func (s *ServiceRegistration) List( return s.listAllServiceRegistrations(args, reply) } - aclObj, err := s.srv.ResolveClientOrACL(args) + aclObj, err := s.srv.ResolveACL(args) if err != nil { - return structs.ErrPermissionDenied + return err } - if args.GetIdentity().Claims == nil && - !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + if aclObj != nil && !aclObj.AllowServiceRegistrationReadList(args.RequestNamespace(), + args.GetIdentity().Claims != nil) { return structs.ErrPermissionDenied } @@ -381,12 +339,12 @@ func (s *ServiceRegistration) GetService( } defer metrics.MeasureSince([]string{"nomad", "service_registration", "get_service"}, time.Now()) - aclObj, err := s.srv.ResolveClientOrACL(args) + aclObj, err := s.srv.ResolveACL(args) if err != nil { return structs.ErrPermissionDenied } - if args.GetIdentity().Claims == nil && - !aclObj.AllowNsOp(args.RequestNamespace(), acl.NamespaceCapabilityReadJob) { + if aclObj != nil && !aclObj.AllowServiceRegistrationReadList(args.RequestNamespace(), + args.GetIdentity().Claims != nil) { return structs.ErrPermissionDenied } diff --git a/nomad/service_registration_endpoint_test.go b/nomad/service_registration_endpoint_test.go index 094f27f07bad..3fbe01aaa3d9 100644 --- a/nomad/service_registration_endpoint_test.go +++ b/nomad/service_registration_endpoint_test.go @@ -52,8 +52,7 @@ func TestServiceRegistration_Upsert(t *testing.T) { var serviceRegResp structs.ServiceRegistrationUpsertResponse err := msgpackrpc.CallWithCodec( codec, structs.ServiceRegistrationUpsertRPCMethod, serviceRegReq, &serviceRegResp) - require.Error(t, err) - require.Contains(t, err.Error(), "node lookup by SecretID failed") + must.EqError(t, err, structs.ErrPermissionDenied.Error()) // Generate a node and retry the upsert. node := mock.Node() @@ -135,8 +134,7 @@ func TestServiceRegistration_Upsert(t *testing.T) { var serviceRegResp structs.ServiceRegistrationUpsertResponse err := msgpackrpc.CallWithCodec( codec, structs.ServiceRegistrationUpsertRPCMethod, serviceRegReq, &serviceRegResp) - require.Error(t, err) - require.Contains(t, err.Error(), "node lookup by SecretID failed") + must.EqError(t, err, structs.ErrPermissionDenied.Error()) // Generate a node and retry the upsert. node := mock.Node() From 9701aca48f2650e453325f9a048922317e2ba455 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Thu, 12 Oct 2023 10:37:22 -0400 Subject: [PATCH 2/4] remove useless nil check --- nomad/service_registration_endpoint.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nomad/service_registration_endpoint.go b/nomad/service_registration_endpoint.go index b5c9ac6cf86a..57402cb4a513 100644 --- a/nomad/service_registration_endpoint.go +++ b/nomad/service_registration_endpoint.go @@ -178,7 +178,7 @@ func (s *ServiceRegistration) List( if err != nil { return err } - if aclObj != nil && !aclObj.AllowServiceRegistrationReadList(args.RequestNamespace(), + if !aclObj.AllowServiceRegistrationReadList(args.RequestNamespace(), args.GetIdentity().Claims != nil) { return structs.ErrPermissionDenied } @@ -343,7 +343,7 @@ func (s *ServiceRegistration) GetService( if err != nil { return structs.ErrPermissionDenied } - if aclObj != nil && !aclObj.AllowServiceRegistrationReadList(args.RequestNamespace(), + if !aclObj.AllowServiceRegistrationReadList(args.RequestNamespace(), args.GetIdentity().Claims != nil) { return structs.ErrPermissionDenied } From b07a32213525a0a4a72e5a5f03a1d45362972d58 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Thu, 12 Oct 2023 10:42:35 -0400 Subject: [PATCH 3/4] update cert validation to take a slice, clean up dead code --- nomad/alloc_endpoint.go | 17 +++++------ nomad/auth/auth.go | 66 ++++++++++++++++++++--------------------- nomad/auth/auth_test.go | 4 +-- nomad/util.go | 62 -------------------------------------- 4 files changed, 42 insertions(+), 107 deletions(-) diff --git a/nomad/alloc_endpoint.go b/nomad/alloc_endpoint.go index bdd1f5801e68..8a83229fd9dd 100644 --- a/nomad/alloc_endpoint.go +++ b/nomad/alloc_endpoint.go @@ -451,22 +451,21 @@ func (a *Alloc) GetServiceRegistrations( // This is an internal-only RPC and not exposed via the HTTP API. func (a *Alloc) SignIdentities(args *structs.AllocIdentitiesRequest, reply *structs.AllocIdentitiesResponse) error { - authErr := a.srv.Authenticate(a.ctx, args) - - // Ensure the connection was initiated by a client if TLS is used. - if err := validateTLSCertificateLevel(a.srv, a.ctx, tlsCertificateLevelClient); err != nil { - return err + aclObj, err := a.srv.AuthenticateClientOnly(a.ctx, args) + a.srv.MeasureRPCRate("alloc", structs.RateMetricRead, args) + if err != nil { + return structs.ErrPermissionDenied } + if done, err := a.srv.forward("Alloc.SignIdentities", args, args, reply); done { return err } - a.srv.MeasureRPCRate("alloc", structs.RateMetricRead, args) - if authErr != nil { + defer metrics.MeasureSince([]string{"nomad", "alloc", "sign_identities"}, time.Now()) + + if !aclObj.AllowClientOp() { return structs.ErrPermissionDenied } - defer metrics.MeasureSince([]string{"nomad", "alloc", "sign_identities"}, time.Now()) - if len(args.Identities) == 0 { // Client bug. Fail loudly instead of letting clients waste time with // noops. diff --git a/nomad/auth/auth.go b/nomad/auth/auth.go index 7602086c5005..696a23fe707b 100644 --- a/nomad/auth/auth.go +++ b/nomad/auth/auth.go @@ -17,6 +17,7 @@ import ( "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/nomad/state" "github.com/hashicorp/nomad/nomad/structs" + "golang.org/x/exp/slices" ) // aclCacheSize is the number of ACL objects to keep cached. ACLs have a parsing @@ -46,6 +47,9 @@ type Authenticator struct { getLeaderACL LeaderACLGetter region string + validServerCertNames []string + validClientCertNames []string + // aclCache is used to maintain the parsed ACL objects aclCache *structs.ACLCache[*acl.ACL] @@ -66,14 +70,19 @@ type AuthenticatorConfig struct { func NewAuthenticator(cfg *AuthenticatorConfig) *Authenticator { return &Authenticator{ - aclsEnabled: cfg.AclsEnabled, - tlsEnabled: cfg.TLSEnabled, - logger: cfg.Logger.With("auth"), - getState: cfg.StateFn, - getLeaderACL: cfg.GetLeaderACLFn, - region: cfg.Region, - aclCache: structs.NewACLCache[*acl.ACL](aclCacheSize), - encrypter: cfg.Encrypter, + aclsEnabled: cfg.AclsEnabled, + tlsEnabled: cfg.TLSEnabled, + logger: cfg.Logger.With("auth"), + getState: cfg.StateFn, + getLeaderACL: cfg.GetLeaderACLFn, + region: cfg.Region, + aclCache: structs.NewACLCache[*acl.ACL](aclCacheSize), + encrypter: cfg.Encrypter, + validServerCertNames: []string{"server." + cfg.Region + ".nomad"}, + validClientCertNames: []string{ + "client." + cfg.Region + ".nomad", + "server." + cfg.Region + ".nomad", + }, } } @@ -232,9 +241,7 @@ func (s *Authenticator) AuthenticateServerOnly(ctx RPCContext, args structs.Requ // set on the identity whether or not its valid for server RPC, so we // can capture it for metrics identity.TLSName = tlsCert.Subject.CommonName - - expected := "server." + s.region + ".nomad" - _, err := validateCertificateForName(tlsCert, expected) + _, err := validateCertificateForNames(tlsCert, s.validServerCertNames) if err != nil { return nil, err } @@ -277,15 +284,9 @@ func (s *Authenticator) AuthenticateClientOnly(ctx RPCContext, args structs.Requ // set on the identity whether or not its valid for server RPC, so we // can capture it for metrics identity.TLSName = tlsCert.Subject.CommonName - - expected := fmt.Sprintf("client.%s.nomad", s.region) - _, err := validateCertificateForName(tlsCert, expected) + _, err := validateCertificateForNames(tlsCert, s.validClientCertNames) if err != nil { - expected := fmt.Sprintf("server.%s.nomad", s.region) - _, err := validateCertificateForName(tlsCert, expected) - if err != nil { - return nil, err - } + return nil, err } } @@ -331,38 +332,35 @@ func (s *Authenticator) AuthenticateClientOnlyLegacy(ctx RPCContext, args struct // set on the identity whether or not its valid for server RPC, so we // can capture it for metrics identity.TLSName = tlsCert.Subject.CommonName - - expected := fmt.Sprintf("client.%s.nomad", s.region) - _, err := validateCertificateForName(tlsCert, expected) + _, err := validateCertificateForNames(tlsCert, s.validClientCertNames) if err != nil { - expected := fmt.Sprintf("server.%s.nomad", s.region) - _, err := validateCertificateForName(tlsCert, expected) - if err != nil { - return nil, err - } + return nil, err } } return acl.ClientACL, nil } -// validateCertificateForName returns true if the certificate is valid -// for the given domain name. -func validateCertificateForName(cert *x509.Certificate, expectedName string) (bool, error) { +// validateCertificateForNames returns true if the certificate is valid for any +// of the given domain names. +func validateCertificateForNames(cert *x509.Certificate, expectedNames []string) (bool, error) { if cert == nil { return false, nil } validNames := []string{cert.Subject.CommonName} validNames = append(validNames, cert.DNSNames...) - for _, valid := range validNames { - if expectedName == valid { + + for _, expectedName := range expectedNames { + if slices.Contains(validNames, expectedName) { return true, nil } } - return false, fmt.Errorf("invalid certificate, %s not in %s", - expectedName, strings.Join(validNames, ",")) + return false, fmt.Errorf("invalid certificate: %s not in expected %s", + strings.Join(validNames, ", "), + strings.Join(expectedNames, ", ")) + } // ResolveACLForToken resolves an ACL from a token only. It should be used only diff --git a/nomad/auth/auth_test.go b/nomad/auth/auth_test.go index bcac38976fe7..376d2f39a459 100644 --- a/nomad/auth/auth_test.go +++ b/nomad/auth/auth_test.go @@ -383,7 +383,7 @@ func TestAuthenticateServerOnly(t *testing.T) { aclObj, err := auth.AuthenticateServerOnly(ctx, args) must.EqError(t, err, - "invalid certificate, server.global.nomad not in client.global.nomad") + "invalid certificate: client.global.nomad not in expected server.global.nomad") must.Eq(t, "client.global.nomad:192.168.1.1", args.GetIdentity().String()) must.Nil(t, aclObj) }, @@ -507,7 +507,7 @@ func TestAuthenticateClientOnly(t *testing.T) { aclObj, err := auth.AuthenticateClientOnly(ctx, args) must.EqError(t, err, - "invalid certificate, server.global.nomad not in cli.global.nomad") + "invalid certificate: cli.global.nomad not in expected client.global.nomad, server.global.nomad") must.Eq(t, "cli.global.nomad:192.168.1.1", args.GetIdentity().String()) must.Nil(t, aclObj) }, diff --git a/nomad/util.go b/nomad/util.go index 6effadb98abc..bc99bd757d14 100644 --- a/nomad/util.go +++ b/nomad/util.go @@ -282,65 +282,3 @@ func getAlloc(state AllocGetter, allocID string) (*structs.Allocation, error) { return alloc, nil } - -// tlsCertificateLevel represents a role level for mTLS certificates. -type tlsCertificateLevel int8 - -const ( - tlsCertificateLevelServer tlsCertificateLevel = iota - tlsCertificateLevelClient -) - -// validateTLSCertificateLevel checks if the provided RPC connection was -// initiated with a certificate that matches the given TLS role level. -// -// - tlsCertificateLevelServer requires a server certificate. -// - tlsCertificateLevelServer requires a client or server certificate. -func validateTLSCertificateLevel(srv *Server, ctx *RPCContext, lvl tlsCertificateLevel) error { - switch lvl { - case tlsCertificateLevelClient: - err := validateLocalClientTLSCertificate(srv, ctx) - if err != nil { - return validateLocalServerTLSCertificate(srv, ctx) - } - return nil - case tlsCertificateLevelServer: - return validateLocalServerTLSCertificate(srv, ctx) - } - - return fmt.Errorf("invalid TLS certificate level %v", lvl) -} - -// validateLocalClientTLSCertificate checks if the provided RPC connection was -// initiated by a client in the same region as the target server. -func validateLocalClientTLSCertificate(srv *Server, ctx *RPCContext) error { - expected := fmt.Sprintf("client.%s.nomad", srv.Region()) - - err := validateTLSCertificate(srv, ctx, expected) - if err != nil { - return fmt.Errorf("invalid client connection in region %s: %v", srv.Region(), err) - } - return nil -} - -// validateLocalServerTLSCertificate checks if the provided RPC connection was -// initiated by a server in the same region as the target server. -func validateLocalServerTLSCertificate(srv *Server, ctx *RPCContext) error { - expected := fmt.Sprintf("server.%s.nomad", srv.Region()) - - err := validateTLSCertificate(srv, ctx, expected) - if err != nil { - return fmt.Errorf("invalid server connection in region %s: %v", srv.Region(), err) - } - return nil -} - -// validateTLSCertificate checks if the RPC connection mTLS certificates are -// valid for the given name. -func validateTLSCertificate(srv *Server, ctx *RPCContext, name string) error { - if srv.config.TLSConfig == nil || !srv.config.TLSConfig.VerifyServerHostname { - return nil - } - - return ctx.ValidateCertificateForName(name) -} From 262324ca52aca6543f397167bf1e11ba424fe521 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Thu, 12 Oct 2023 11:11:14 -0400 Subject: [PATCH 4/4] fixup test setup for signing requests which must be authenticated --- client/widmgr/widmgr_int_test.go | 2 +- command/agent/event_endpoint_test.go | 2 +- command/agent/plugins_test.go | 2 +- command/agent/testagent.go | 2 +- command/operator_debug_test.go | 2 +- command/operator_scheduler_set_config_test.go | 6 +++--- command/testing_test.go | 4 ++-- command/var_lock_test.go | 4 ++-- nomad/alloc_endpoint_test.go | 8 ++++++++ 9 files changed, 20 insertions(+), 12 deletions(-) diff --git a/client/widmgr/widmgr_int_test.go b/client/widmgr/widmgr_int_test.go index b5e70a8aef94..e2392d1cf2b9 100644 --- a/client/widmgr/widmgr_int_test.go +++ b/client/widmgr/widmgr_int_test.go @@ -30,7 +30,7 @@ func TestWIDMgr(t *testing.T) { t.Cleanup(ta.Shutdown) mgr := widmgr.NewSigner(widmgr.SignerConfig{ - NodeSecret: uuid.Generate(), // not checked when ACLs disabled + NodeSecret: ta.Client().Node().SecretID, Region: "global", RPC: ta, }) diff --git a/command/agent/event_endpoint_test.go b/command/agent/event_endpoint_test.go index 88ac9986cbcf..66415b5f46a8 100644 --- a/command/agent/event_endpoint_test.go +++ b/command/agent/event_endpoint_test.go @@ -212,7 +212,7 @@ func TestHTTP_Alloc_Port_Response(t *testing.T) { ci.Parallel(t) httpTest(t, nil, func(srv *TestAgent) { - client := srv.Client() + client := srv.APIClient() defer srv.Shutdown() defer client.Close() diff --git a/command/agent/plugins_test.go b/command/agent/plugins_test.go index de9e5b9cabc5..09d015408253 100644 --- a/command/agent/plugins_test.go +++ b/command/agent/plugins_test.go @@ -31,6 +31,6 @@ func testServer(t *testing.T, runClient bool, cb func(*Config)) (*TestAgent, *ap }) t.Cleanup(a.Shutdown) - c := a.Client() + c := a.APIClient() return a, c, a.HTTPAddr() } diff --git a/command/agent/testagent.go b/command/agent/testagent.go index 2cdfb34d8646..9f378b737c6d 100644 --- a/command/agent/testagent.go +++ b/command/agent/testagent.go @@ -309,7 +309,7 @@ func (a *TestAgent) HTTPAddr() string { return proto + a.Server.Addr } -func (a *TestAgent) Client() *api.Client { +func (a *TestAgent) APIClient() *api.Client { conf := api.DefaultConfig() conf.Address = a.HTTPAddr() c, err := api.NewClient(conf) diff --git a/command/operator_debug_test.go b/command/operator_debug_test.go index 4d612187d95e..026169c01be5 100644 --- a/command/operator_debug_test.go +++ b/command/operator_debug_test.go @@ -898,7 +898,7 @@ func testServerWithoutLeader(t *testing.T, runClient bool, cb func(*agent.Config }) t.Cleanup(func() { a.Shutdown() }) - c := a.Client() + c := a.APIClient() return a, c, a.HTTPAddr() } diff --git a/command/operator_scheduler_set_config_test.go b/command/operator_scheduler_set_config_test.go index c6d4c70c935f..4ea0ace79471 100644 --- a/command/operator_scheduler_set_config_test.go +++ b/command/operator_scheduler_set_config_test.go @@ -22,7 +22,7 @@ func TestOperatorSchedulerSetConfig_Run(t *testing.T) { ui := cli.NewMockUi() c := &OperatorSchedulerSetConfig{Meta: Meta{Ui: ui}} - bootstrappedConfig, _, err := srv.Client().Operator().SchedulerGetConfiguration(nil) + bootstrappedConfig, _, err := srv.APIClient().Operator().SchedulerGetConfiguration(nil) require.NoError(t, err) require.NotEmpty(t, bootstrappedConfig.SchedulerConfig) @@ -34,7 +34,7 @@ func TestOperatorSchedulerSetConfig_Run(t *testing.T) { // Read the configuration again and test that nothing has changed which // ensures our empty flags are working correctly. - nonModifiedConfig, _, err := srv.Client().Operator().SchedulerGetConfiguration(nil) + nonModifiedConfig, _, err := srv.APIClient().Operator().SchedulerGetConfiguration(nil) require.NoError(t, err) schedulerConfigEquals(t, bootstrappedConfig.SchedulerConfig, nonModifiedConfig.SchedulerConfig) @@ -56,7 +56,7 @@ func TestOperatorSchedulerSetConfig_Run(t *testing.T) { s := ui.OutputWriter.String() require.Contains(t, s, "Scheduler configuration updated!") - modifiedConfig, _, err := srv.Client().Operator().SchedulerGetConfiguration(nil) + modifiedConfig, _, err := srv.APIClient().Operator().SchedulerGetConfiguration(nil) require.NoError(t, err) schedulerConfigEquals(t, &api.SchedulerConfiguration{ SchedulerAlgorithm: "spread", diff --git a/command/testing_test.go b/command/testing_test.go index 8a1326d671ac..66f70e58f317 100644 --- a/command/testing_test.go +++ b/command/testing_test.go @@ -31,7 +31,7 @@ func testServer(t *testing.T, runClient bool, cb func(*agent.Config)) (*agent.Te }) t.Cleanup(a.Shutdown) - c := a.Client() + c := a.APIClient() return a, c, a.HTTPAddr() } @@ -46,7 +46,7 @@ func testClient(t *testing.T, name string, cb func(*agent.Config)) (*agent.TestA }) t.Cleanup(a.Shutdown) - c := a.Client() + c := a.APIClient() t.Logf("Waiting for client %s to join server(s) %s", name, a.GetConfig().Client.Servers) testutil.WaitForClient(t, a.Agent.RPC, a.Agent.Client().NodeID(), a.Agent.Client().Region()) diff --git a/command/var_lock_test.go b/command/var_lock_test.go index b18ac377c009..4df597a8cde8 100644 --- a/command/var_lock_test.go +++ b/command/var_lock_test.go @@ -101,7 +101,7 @@ func TestVarLockCommand_Good(t *testing.T) { code := cmd.Run([]string{"-address=" + url, "test/var/shell", "touch ", filePath}) require.Equal(t, 0, code, "expected exit 0, got: %d; %v", code, ui.ErrorWriter.String()) - sv, _, err := srv.Client().Variables().Peek("test/var/shell", nil) + sv, _, err := srv.APIClient().Variables().Peek("test/var/shell", nil) must.NoError(t, err) must.NotNil(t, sv) @@ -135,7 +135,7 @@ func TestVarLockCommand_Good_NoShell(t *testing.T) { code := cmd.Run([]string{"-address=" + url, "-shell=false", "test/var/noShell", "touch", filePath}) require.Zero(t, 0, code) - sv, _, err := srv.Client().Variables().Peek("test/var/noShell", nil) + sv, _, err := srv.APIClient().Variables().Peek("test/var/noShell", nil) must.NoError(t, err) must.NotNil(t, sv) diff --git a/nomad/alloc_endpoint_test.go b/nomad/alloc_endpoint_test.go index dbbdda87f16c..de988d0b0ee2 100644 --- a/nomad/alloc_endpoint_test.go +++ b/nomad/alloc_endpoint_test.go @@ -1692,11 +1692,15 @@ func TestAlloc_SignIdentities_Bad(t *testing.T) { codec := rpcClient(t, s1) testutil.WaitForLeader(t, s1.RPC) + node := mock.Node() + must.NoError(t, s1.fsm.State().UpsertNode(structs.MsgTypeTestSetup, 100, node)) + req := &structs.AllocIdentitiesRequest{ QueryOptions: structs.QueryOptions{ Region: "global", Namespace: structs.DefaultNamespace, AllowStale: true, + AuthToken: node.SecretID, }, } var resp structs.AllocIdentitiesResponse @@ -1779,6 +1783,9 @@ func TestAlloc_SignIdentities_Blocking(t *testing.T) { testutil.WaitForLeader(t, s1.RPC) state := s1.fsm.State() + node := mock.Node() + must.NoError(t, s1.fsm.State().UpsertNode(structs.MsgTypeTestSetup, 100, node)) + // Create the alloc we're going to query for, but don't insert it yet. This // simulates querying a slow follower or a restoring server. alloc := mock.Alloc() @@ -1819,6 +1826,7 @@ func TestAlloc_SignIdentities_Blocking(t *testing.T) { AllowStale: true, MinQueryIndex: 1999, MaxQueryTime: 10 * time.Second, + AuthToken: node.SecretID, }, } var resp structs.AllocIdentitiesResponse