From 881a4cfaffe81620ea41d6783c9914b3b8112510 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Fri, 27 Jan 2023 08:29:53 -0500 Subject: [PATCH] metrics: Add remaining server RPC rate metrics (#15901) --- .semgrep/rpc_endpoint.yml | 55 ++++++++++++++++------------ client/alloc_endpoint_test.go | 8 ++-- command/agent/agent_endpoint_test.go | 4 +- command/agent/alloc_endpoint_test.go | 2 +- command/agent/stats_endpoint_test.go | 2 +- nomad/acl_endpoint.go | 45 +++++++++++++++-------- 6 files changed, 69 insertions(+), 47 deletions(-) diff --git a/.semgrep/rpc_endpoint.yml b/.semgrep/rpc_endpoint.yml index 6fe60bc29..45ffe0025 100644 --- a/.semgrep/rpc_endpoint.yml +++ b/.semgrep/rpc_endpoint.yml @@ -6,78 +6,84 @@ rules: if done, err := $A.$B.forward($METHOD, ...); done { return err } + # Pattern used by typical endpoints that take an auth token or workload + # identity. Some of these endpoints have no context for Authenticate - pattern-not-inside: | + authErr := $A.$B.Authenticate(...) + ... if done, err := $A.$B.forward($METHOD, ...); done { return err } ... - ... := $X.$Y.ResolveToken(...) + ... := $A.$B.ResolveACL(...) ... + # Pattern used by endpoints that are used by both ACLs and Clients. + # These endpoints will always have a ctx passed to Authenticate - pattern-not-inside: | + authErr := $A.$B.Authenticate($A.ctx, args) + ... if done, err := $A.$B.forward($METHOD, ...); done { return err } ... - ... := $U.requestACLToken(...) + ... := $A.$B.ResolveClientOrACL(...) ... + # Pattern used by ACL endpoints that need to interact with the token directly - pattern-not-inside: | + authErr := $A.$B.Authenticate($A.ctx, args) + ... if done, err := $A.$B.forward($METHOD, ...); done { return err } ... - ... := $T.NamespaceValidator(...) + ... := args.GetIdentity().GetACLToken() ... # Pattern used by endpoints called exclusively between agents # (server -> server or client -> server) - pattern-not-inside: | + authErr := $A.$B.Authenticate($A.ctx, args) ... ... := validateTLSCertificateLevel(...) ... if done, err := $A.$B.forward($METHOD, ...); done { return err } - # Pattern used by endpoints that support both normal ACLs and - # workload identity + # Pattern used by endpoints that support both normal ACLs and workload + # identity but break authentication and authorization up + # TODO: currently this is just for Variables and should be removed once + # https://github.com/hashicorp/nomad/issues/15875 is complete. - pattern-not-inside: | + authErr := $A.$B.Authenticate($A.ctx, args) + ... if done, err := $A.$B.forward($METHOD, ...); done { return err } ... ... := $T.handleMixedAuthEndpoint(...) ... - # Pattern used by endpoints that support both normal ACLs and + # Second pattern used by endpoints that support both normal ACLs and # workload identity but break authentication and authorization up + # TODO: currently this is just for Variables and should be removed once + # https://github.com/hashicorp/nomad/issues/15875 is complete. - pattern-not-inside: | + authErr := $A.$B.Authenticate($A.ctx, args) + ... if done, err := $A.$B.forward($METHOD, ...); done { return err } ... - ... := $T.authorize(...) + ... := svePreApply($A, args, args.Var) ... # Pattern used by some Node endpoints. - pattern-not-inside: | + authErr := $A.$B.Authenticate($A.ctx, args) + ... if done, err := $A.$B.forward($METHOD, ...); done { return err } ... return $A.deregister(...) ... - # Pattern used by Authenticate method. - # TODO: add authorization steps as well. - - pattern-not-inside: | - authErr := $A.$B.Authenticate($A.ctx, args) - ... - if authErr != nil { - return $C - } - ... - - pattern-not-inside: | - authErr := $A.$B.Authenticate(nil, args) - ... - if authErr != nil { - return $C - } - ... - metavariable-pattern: metavariable: $METHOD patterns: @@ -86,6 +92,7 @@ rules: - pattern-not: '"ACL.ResolveToken"' - pattern-not: '"ACL.UpsertOneTimeToken"' - pattern-not: '"ACL.ExchangeOneTimeToken"' + - pattern-not: '"ACL.WhoAmI"' - pattern-not: 'structs.ACLListAuthMethodsRPCMethod' - pattern-not: 'structs.ACLOIDCAuthURLRPCMethod' - pattern-not: 'structs.ACLOIDCCompleteAuthRPCMethod' @@ -100,4 +107,4 @@ rules: severity: "WARNING" paths: include: - - "*_endpoint.go" + - "nomad/*_endpoint.go" diff --git a/client/alloc_endpoint_test.go b/client/alloc_endpoint_test.go index f3c6e3e2b..99a818bef 100644 --- a/client/alloc_endpoint_test.go +++ b/client/alloc_endpoint_test.go @@ -136,7 +136,7 @@ func TestAllocations_Restart_ACL(t *testing.T) { var resp nstructs.GenericResponse err := client.ClientRPC("Allocations.Restart", &req, &resp) require.NotNil(err) - require.EqualError(err, nstructs.ErrPermissionDenied.Error()) + require.ErrorContains(err, nstructs.ErrPermissionDenied.Error()) } // Try request with an invalid token and expect failure @@ -323,7 +323,7 @@ func TestAllocations_GarbageCollect_ACL(t *testing.T) { var resp nstructs.GenericResponse err := client.ClientRPC("Allocations.GarbageCollect", &req, &resp) require.NotNil(err) - require.EqualError(err, nstructs.ErrPermissionDenied.Error()) + require.ErrorContains(err, nstructs.ErrPermissionDenied.Error()) } // Try request with an invalid token and expect failure @@ -420,7 +420,7 @@ func TestAllocations_Signal_ACL(t *testing.T) { var resp nstructs.GenericResponse err := client.ClientRPC("Allocations.Signal", &req, &resp) require.NotNil(err) - require.EqualError(err, nstructs.ErrPermissionDenied.Error()) + require.ErrorContains(err, nstructs.ErrPermissionDenied.Error()) } // Try request with an invalid token and expect failure @@ -526,7 +526,7 @@ func TestAllocations_Stats_ACL(t *testing.T) { var resp cstructs.AllocStatsResponse err := client.ClientRPC("Allocations.Stats", &req, &resp) require.NotNil(err) - require.EqualError(err, nstructs.ErrPermissionDenied.Error()) + require.ErrorContains(err, nstructs.ErrPermissionDenied.Error()) } // Try request with an invalid token and expect failure diff --git a/command/agent/agent_endpoint_test.go b/command/agent/agent_endpoint_test.go index 0572b84ac..ab9bce414 100644 --- a/command/agent/agent_endpoint_test.go +++ b/command/agent/agent_endpoint_test.go @@ -806,7 +806,7 @@ func TestHTTP_AgentSetServers_ACL(t *testing.T) { respW := httptest.NewRecorder() _, err := s.Server.AgentServersRequest(respW, req) require.NotNil(err) - require.Equal(err.Error(), structs.ErrPermissionDenied.Error()) + require.ErrorContains(err, structs.ErrPermissionDenied.Error()) } // Try request with an invalid token and expect failure @@ -860,7 +860,7 @@ func TestHTTP_AgentListServers_ACL(t *testing.T) { respW := httptest.NewRecorder() _, err := s.Server.AgentServersRequest(respW, req) require.NotNil(err) - require.Equal(err.Error(), structs.ErrPermissionDenied.Error()) + require.ErrorContains(err, structs.ErrPermissionDenied.Error()) } // Try request with an invalid token and expect failure diff --git a/command/agent/alloc_endpoint_test.go b/command/agent/alloc_endpoint_test.go index f85cf296f..73222355c 100644 --- a/command/agent/alloc_endpoint_test.go +++ b/command/agent/alloc_endpoint_test.go @@ -1032,7 +1032,7 @@ func TestHTTP_AllocAllGC_ACL(t *testing.T) { respW := httptest.NewRecorder() _, err := s.Server.ClientGCRequest(respW, req) require.NotNil(err) - require.Equal(err.Error(), structs.ErrPermissionDenied.Error()) + require.ErrorContains(err, structs.ErrPermissionDenied.Error()) } // Try request with an invalid token and expect failure diff --git a/command/agent/stats_endpoint_test.go b/command/agent/stats_endpoint_test.go index ad8c6d550..6778ef58d 100644 --- a/command/agent/stats_endpoint_test.go +++ b/command/agent/stats_endpoint_test.go @@ -90,7 +90,7 @@ func TestClientStatsRequest_ACL(t *testing.T) { respW := httptest.NewRecorder() _, err := s.Server.ClientStatsRequest(respW, req) assert.NotNil(err) - assert.Equal(err.Error(), structs.ErrPermissionDenied.Error()) + assert.ErrorContains(err, structs.ErrPermissionDenied.Error()) } // Try request with an invalid token and expect failure diff --git a/nomad/acl_endpoint.go b/nomad/acl_endpoint.go index 27c87248a..15a9a7c39 100644 --- a/nomad/acl_endpoint.go +++ b/nomad/acl_endpoint.go @@ -353,20 +353,22 @@ func (a *ACL) GetPolicies(args *structs.ACLPolicySetRequest, reply *structs.ACLP if !a.srv.config.ACLEnabled { return aclDisabled } + authErr := a.srv.Authenticate(a.ctx, args) if done, err := a.srv.forward("ACL.GetPolicies", args, args, reply); done { return err } + a.srv.MeasureRPCRate("acl", structs.RateMetricList, args) + if authErr != nil { + return structs.ErrPermissionDenied + } defer metrics.MeasureSince([]string{"nomad", "acl", "get_policies"}, time.Now()) // For client typed tokens, allow them to query any policies associated with that token. // This is used by clients which are resolving the policies to enforce. Any associated // policies need to be fetched so that the client can determine what to allow. - token, err := a.requestACLToken(args.AuthToken) - if err != nil { - return err - } + token := args.GetIdentity().GetACLToken() if token == nil { - return structs.ErrTokenNotFound + return structs.ErrPermissionDenied } // Generate a set of policy names. This is initially generated from the @@ -423,9 +425,14 @@ func (a *ACL) Bootstrap(args *structs.ACLTokenBootstrapRequest, reply *structs.A args.Region = a.srv.config.AuthoritativeRegion providedTokenID := args.BootstrapSecret + authErr := a.srv.Authenticate(a.ctx, args) if done, err := a.srv.forward("ACL.Bootstrap", args, args, reply); done { return err } + a.srv.MeasureRPCRate("acl", structs.RateMetricWrite, args) + if authErr != nil { + return structs.ErrPermissionDenied + } defer metrics.MeasureSince([]string{"nomad", "acl", "bootstrap"}, time.Now()) // Always ignore the reset index from the arguments @@ -1135,10 +1142,15 @@ func (a *ACL) ExchangeOneTimeToken(args *structs.OneTimeTokenExchangeRequest, re // called only by garbage collection func (a *ACL) ExpireOneTimeTokens(args *structs.OneTimeTokenExpireRequest, reply *structs.GenericResponse) error { + authErr := a.srv.Authenticate(a.ctx, args) if done, err := a.srv.forward( "ACL.ExpireOneTimeTokens", args, args, reply); done { return err } + a.srv.MeasureRPCRate("acl", structs.RateMetricWrite, args) + if authErr != nil { + return structs.ErrPermissionDenied + } defer metrics.MeasureSince( []string{"nomad", "acl", "expire_one_time_tokens"}, time.Now()) @@ -1148,7 +1160,7 @@ func (a *ACL) ExpireOneTimeTokens(args *structs.OneTimeTokenExpireRequest, reply // Check management level permissions if a.srv.config.ACLEnabled { - if acl, err := a.srv.ResolveToken(args.AuthToken); err != nil { + if acl, err := a.srv.ResolveACL(args); err != nil { return err } else if acl == nil || !acl.IsManagement() { return structs.ErrPermissionDenied @@ -1480,19 +1492,20 @@ func (a *ACL) GetRolesByID(args *structs.ACLRolesByIDRequest, reply *structs.ACL if !a.srv.config.ACLEnabled { return aclDisabled } - + authErr := a.srv.Authenticate(a.ctx, args) if done, err := a.srv.forward(structs.ACLGetRolesByIDRPCMethod, args, args, reply); done { return err } + a.srv.MeasureRPCRate("acl", structs.RateMetricList, args) + if authErr != nil { + return structs.ErrPermissionDenied + } defer metrics.MeasureSince([]string{"nomad", "acl", "get_roles_id"}, time.Now()) // For client typed tokens, allow them to query any roles associated with // that token. This is used by Nomad agents in client mode which are // resolving the roles to enforce. - token, err := a.requestACLToken(args.AuthToken) - if err != nil { - return err - } + token := args.GetIdentity().GetACLToken() if token == nil { return structs.ErrTokenNotFound } @@ -2041,17 +2054,19 @@ func (a *ACL) GetAuthMethods( if !a.srv.config.ACLEnabled { return aclDisabled } + authErr := a.srv.Authenticate(a.ctx, args) if done, err := a.srv.forward( structs.ACLGetAuthMethodsRPCMethod, args, args, reply); done { return err } + a.srv.MeasureRPCRate("acl", structs.RateMetricList, args) + if authErr != nil { + return structs.ErrPermissionDenied + } defer metrics.MeasureSince([]string{"nomad", "acl", "get_auth_methods"}, time.Now()) // allow only management token holders to query this endpoint - token, err := a.requestACLToken(args.AuthToken) - if err != nil { - return err - } + token := args.GetIdentity().GetACLToken() if token == nil { return structs.ErrTokenNotFound }