-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
acl: remove ResolveTokenToIdentity #12166
Changes from all commits
a5e8af7
edca8d6
e134e43
7a6e03c
d363cc0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:deprecation | ||
acl: The `consul.acl.ResolveTokenToIdentity` metric is no longer reported. The values that were previous reported as part of this metric will now be part of the `consul.acl.ResolveToken` metric. | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,10 +34,6 @@ var ACLSummaries = []prometheus.SummaryDefinition{ | |
Name: []string{"acl", "ResolveToken"}, | ||
Help: "This measures the time it takes to resolve an ACL token.", | ||
}, | ||
{ | ||
Name: []string{"acl", "ResolveTokenToIdentity"}, | ||
Help: "This measures the time it takes to resolve an ACL token to an Identity.", | ||
}, | ||
} | ||
|
||
// These must be kept in sync with the constants in command/agent/acl.go. | ||
|
@@ -1115,31 +1111,17 @@ func (r *ACLResolver) ResolveTokenToIdentityAndAuthorizer(token string) (structs | |
return identity, acl.NewChainedAuthorizer(chain), nil | ||
} | ||
|
||
// TODO: rename to AccessorIDFromToken. This method is only used to retrieve the | ||
// ACLIdentity.ID, so we don't need to return a full ACLIdentity. We could | ||
// return a much smaller type (instad of just a string) to allow for changes | ||
// in the future. | ||
func (r *ACLResolver) ResolveTokenToIdentity(token string) (structs.ACLIdentity, error) { | ||
if !r.ACLsEnabled() { | ||
return nil, nil | ||
} | ||
|
||
if acl.RootAuthorizer(token) != nil { | ||
return nil, acl.ErrRootDenied | ||
} | ||
|
||
// handle the anonymous token | ||
if token == "" { | ||
token = anonymousToken | ||
} | ||
type ACLResolveResult struct { | ||
acl.Authorizer | ||
// TODO: likely we can reduce this interface | ||
structs.ACLIdentity | ||
} | ||
|
||
if ident, _, ok := r.resolveLocallyManagedToken(token); ok { | ||
return ident, nil | ||
func (a ACLResolveResult) AccessorID() string { | ||
if a.ACLIdentity == nil { | ||
return "" | ||
} | ||
|
||
defer metrics.MeasureSince([]string{"acl", "ResolveTokenToIdentity"}, time.Now()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just noticed I am removing this metric. I'm going to push one more commit that documents that this metric is being removed. I'll add a changelog as well. |
||
|
||
return r.resolveIdentityFromToken(token) | ||
return a.ACLIdentity.ID() | ||
} | ||
|
||
func (r *ACLResolver) ACLsEnabled() bool { | ||
|
@@ -1158,10 +1140,10 @@ func (r *ACLResolver) ACLsEnabled() bool { | |
return true | ||
} | ||
|
||
func (r *ACLResolver) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (acl.Authorizer, error) { | ||
func (r *ACLResolver) ResolveTokenAndDefaultMeta(token string, entMeta *structs.EnterpriseMeta, authzContext *acl.AuthorizerContext) (ACLResolveResult, error) { | ||
identity, authz, err := r.ResolveTokenToIdentityAndAuthorizer(token) | ||
if err != nil { | ||
return nil, err | ||
return ACLResolveResult{}, err | ||
} | ||
|
||
if entMeta == nil { | ||
|
@@ -1179,7 +1161,7 @@ func (r *ACLResolver) ResolveTokenAndDefaultMeta(token string, entMeta *structs. | |
// Use the meta to fill in the ACL authorization context | ||
entMeta.FillAuthzContext(authzContext) | ||
|
||
return authz, err | ||
return ACLResolveResult{Authorizer: authz, ACLIdentity: identity}, err | ||
} | ||
|
||
// aclFilter is used to filter results from our state store based on ACL rules | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -159,13 +159,12 @@ func (s *Server) ResolveRoleFromID(roleID string) (bool, *structs.ACLRole, error | |
return s.InPrimaryDatacenter() || index > 0, role, acl.ErrNotFound | ||
} | ||
|
||
// TODO: remove | ||
func (s *Server) ResolveToken(token string) (acl.Authorizer, error) { | ||
Comment on lines
+162
to
163
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is done in #12167 |
||
_, authz, err := s.ACLResolver.ResolveTokenToIdentityAndAuthorizer(token) | ||
return authz, err | ||
} | ||
|
||
// TODO: Client has an identical implementation, remove duplication | ||
|
||
func (s *Server) filterACL(token string, subj interface{}) error { | ||
return filterACL(s.ACLResolver, token, subj) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,6 +100,7 @@ func (s *Intention) Apply(args *structs.IntentionRequest, reply *string) error { | |
} | ||
|
||
// Get the ACL token for the request for the checks below. | ||
// TODO: use ResolveTokenAndDefaultMeta | ||
identity, authz, err := s.srv.ACLResolver.ResolveTokenToIdentityAndAuthorizer(args.Token) | ||
if err != nil { | ||
return err | ||
|
@@ -432,7 +433,8 @@ func (s *Intention) Get(args *structs.IntentionQueryRequest, reply *structs.Inde | |
|
||
// Get the ACL token for the request for the checks below. | ||
var entMeta structs.EnterpriseMeta | ||
if _, err := s.srv.ResolveTokenAndDefaultMeta(args.Token, &entMeta, nil); err != nil { | ||
authz, err := s.srv.ResolveTokenAndDefaultMeta(args.Token, &entMeta, nil) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
|
@@ -479,13 +481,11 @@ func (s *Intention) Get(args *structs.IntentionQueryRequest, reply *structs.Inde | |
reply.Intentions = structs.Intentions{ixn} | ||
|
||
// Filter | ||
if err := s.srv.filterACL(args.Token, reply); err != nil { | ||
return err | ||
} | ||
s.srv.filterACLWithAuthorizer(authz, reply) | ||
|
||
// If ACLs prevented any responses, error | ||
if len(reply.Intentions) == 0 { | ||
accessorID := s.aclAccessorID(args.Token) | ||
accessorID := authz.AccessorID() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is great, love that this avoids re-resolving the token |
||
// todo(kit) Migrate intention access denial logging over to audit logging when we implement it | ||
s.logger.Warn("Request to get intention denied due to ACLs", "intention", args.IntentionID, "accessorID", accessorID) | ||
return acl.ErrPermissionDenied | ||
|
@@ -618,7 +618,7 @@ func (s *Intention) Match(args *structs.IntentionQueryRequest, reply *structs.In | |
for _, entry := range args.Match.Entries { | ||
entry.FillAuthzContext(&authzContext) | ||
if prefix := entry.Name; prefix != "" && authz.IntentionRead(prefix, &authzContext) != acl.Allow { | ||
accessorID := s.aclAccessorID(args.Token) | ||
accessorID := authz.AccessorID() | ||
// todo(kit) Migrate intention access denial logging over to audit logging when we implement it | ||
s.logger.Warn("Operation on intention prefix denied due to ACLs", "prefix", prefix, "accessorID", accessorID) | ||
return acl.ErrPermissionDenied | ||
|
@@ -708,7 +708,7 @@ func (s *Intention) Check(args *structs.IntentionQueryRequest, reply *structs.In | |
var authzContext acl.AuthorizerContext | ||
query.FillAuthzContext(&authzContext) | ||
if authz.ServiceRead(prefix, &authzContext) != acl.Allow { | ||
accessorID := s.aclAccessorID(args.Token) | ||
accessorID := authz.AccessorID() | ||
// todo(kit) Migrate intention access denial logging over to audit logging when we implement it | ||
s.logger.Warn("test on intention denied due to ACLs", "prefix", prefix, "accessorID", accessorID) | ||
return acl.ErrPermissionDenied | ||
|
@@ -760,24 +760,6 @@ func (s *Intention) Check(args *structs.IntentionQueryRequest, reply *structs.In | |
return nil | ||
} | ||
|
||
// aclAccessorID is used to convert an ACLToken's secretID to its accessorID for non- | ||
// critical purposes, such as logging. Therefore we interpret all errors as empty-string | ||
// so we can safely log it without handling non-critical errors at the usage site. | ||
func (s *Intention) aclAccessorID(secretID string) string { | ||
_, ident, err := s.srv.ResolveIdentityFromToken(secretID) | ||
if acl.IsErrNotFound(err) { | ||
return "" | ||
} | ||
if err != nil { | ||
s.logger.Debug("non-critical error resolving acl token accessor for logging", "error", err) | ||
return "" | ||
} | ||
if ident == nil { | ||
return "" | ||
} | ||
return ident.ID() | ||
} | ||
|
||
func (s *Intention) validateEnterpriseIntention(ixn *structs.Intention) error { | ||
if err := s.srv.validateEnterpriseIntentionPartition(ixn.SourcePartition); err != nil { | ||
return fmt.Errorf("Invalid source partition %q: %v", ixn.SourcePartition, err) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concern here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this method being called in two places:
Agent.filterMembers
, andAgent.sendCoordinate
.Agent.sendCoordinate
is similar to the other case inagent/local
. It's only done when we receive a permission denied error, and we will be included the accessorID in that error soon, so we should be able to remove that call entirely.The second call
Agent.filterMembers
already has anacl.Authorizer
. I missed this on the first pass! I'll push a commit which removes this call, and usesauthz.AccessorID()
to get the value. (edit: Done in the latest commit)