Skip to content

Commit

Permalink
metrics: Add remaining server RPC rate metrics (#15901)
Browse files Browse the repository at this point in the history
  • Loading branch information
@tgross
tgross committed Jan 27, 2023
1 parent 1e84a53 commit e53b591
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 47 deletions.
55 changes: 31 additions & 24 deletions .semgrep/rpc_endpoint.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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'
Expand All @@ -100,4 +107,4 @@ rules:
severity: "WARNING"
paths:
include:
- "*_endpoint.go"
- "nomad/*_endpoint.go"
8 changes: 4 additions & 4 deletions client/alloc_endpoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions command/agent/agent_endpoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion command/agent/alloc_endpoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion command/agent/stats_endpoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
45 changes: 30 additions & 15 deletions nomad/acl_endpoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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())

Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down

0 comments on commit e53b591

Please sign in to comment.