Skip to content

NET-5590 - authorization: check for identity:write in CA certs, xds server, and getting envoy bootstrap params#19049

Merged
jmurret merged 2 commits intomainfrom
jm/NET-5590
Oct 3, 2023
Merged

NET-5590 - authorization: check for identity:write in CA certs, xds server, and getting envoy bootstrap params#19049
jmurret merged 2 commits intomainfrom
jm/NET-5590

Conversation

@jmurret
Copy link
Member

@jmurret jmurret commented Oct 3, 2023

Description

PR Discussion: https://github.com/hashicorp/consul-enterprise/pull/6760#discussion_r1319233169

TODO in code: https://github.com/hashicorp/consul/blob/main/agent/consul/leader_connect_ca.go#L1459

AuthorizeAndSignCertificate is called from two places and both resolve the authorizer from the token. So, we can utilize IdentityWriteAllowed which will pass in the authorizer based on the token to achieve Check for identity:write on the token when identity permissions are supported.

Acceptance Criteria:

  • check that we have identity:write on the token when the system signs a leaf certificate for the service or agent identified by the SPIFFE ID in the given CSR's SAN. It performs authorization.
  • enforce identity:write in xds server
  • enforce identity:write in getEnvoyBootstrapParams
  • when identity:write permission is not available on the token, return an Permission denied error.

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@jmurret jmurret added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-backport labels Oct 3, 2023
@jmurret jmurret requested a review from a team as a code owner October 3, 2023 21:02
@vercel
Copy link

vercel bot commented Oct 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
consul ⬜️ Ignored (Inspect) Visit Preview Oct 3, 2023 9:43pm
consul-ui-staging ⬜️ Ignored (Inspect) Visit Preview Oct 3, 2023 9:43pm

Copy link
Member

@zalimeni zalimeni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, AFAICT this is identical to previously reviewed Ent changes +/- one whitespace diff.

if err := allow.IdentityWriteAllowed(p.Identity.Name, resource.AuthorizerContext(p.Identity.Tenancy)); err != nil {
return err
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~ Looks like maybe discrepancy between Ent and this change due to whitespace, which may also be impacting the linter errors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. thanks. something got off with my commits and the script did not work, so I had to generate this by hand. I will fix. Thanks for pointing this out!

@jmurret jmurret enabled auto-merge (squash) October 3, 2023 21:43
@jmurret jmurret merged commit d67e5c6 into main Oct 3, 2023
@jmurret jmurret deleted the jm/NET-5590 branch October 3, 2023 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr/no-backport pr/no-changelog PR does not need a corresponding .changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants