Skip to content

wi: new endpoint for listing workload attached ACL policies #25588

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

Merged
merged 26 commits into from
May 19, 2025

Conversation

pkazmierczak
Copy link
Contributor

@pkazmierczak pkazmierczak commented Apr 2, 2025

This introduces a new HTTP endpoint (and an associated CLI command) for querying
ACL policies associated with a workload identity. It allows users that want
to learn about the ACL capabilities from within WI-tasks to know what sort of
policies are enabled.

Fixes #24663
Internal ref: https://hashicorp.atlassian.net/browse/NMD-423

(reviewers: this requires #25547 to work)

mismithhisler
mismithhisler previously approved these changes May 6, 2025
Copy link
Member

@mismithhisler mismithhisler left a comment

Choose a reason for hiding this comment

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

This is a really nice feature, and thanks @gulducat for quick local testing policy/job!

Comment on lines 198 to 202
// Resolve policies for workload identities
policyReply := structs.ACLPolicySetResponse{}
if err := s.agent.RPC("ACL.GetClaimPolicies", &policyArgs, &policyReply); err != nil {
return nil, err
}
Copy link
Member

@tgross tgross May 6, 2025

Choose a reason for hiding this comment

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

My objection to this API as it stands is that we're only presenting the ACL policies for Workload Identity claims but the API is /v1/acl/policy/self. Ignoring unifying the CLI output for the moment, either (a) this API should retrieve policies for any authenticated request or (b) it should be under a different route than /v1/acl/policy/self or (c) the RPC handler should be changed to cover both ACL tokens and WI claims. Unfortunately it looks like we shipped the RPC handler as-is and now it would be awkward to retrieve ACL token policies there. So I think (a) is my preferred option, but we'd need to split the API to send to different RPC handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4053a48 implements option (a), but then we end up with a bit of a mess, because GetClaimPolicies and ListPolicies RPCs return different types (maps or slices, respectively). I thought perhaps the key in the map is the WI name, but it's just policy name, so I'll follow-up with a change that just returns a list of ACLPolicyStub to get a unified output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah but then we'd lose some data that's useful for WI-attached policies if we only return ACLPolicyStub :/

// ACLPolicyListStub is used to for listing ACL policies
type ACLPolicyListStub struct {
	Name        string
	Description string
	Hash        []byte
	CreateIndex uint64
	ModifyIndex uint64
}

This doesn't contain JobACLs or Rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok @tgross, as discussed today, da46b31 unifies the output and adds JobACL field to the stub. I think this is an acceptable compromise.

Copy link
Contributor

@aimeeu aimeeu left a comment

Choose a reason for hiding this comment

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

Thanks for creating the docs content! I left a few style nits.

also adds JobACL field to the stub
aimeeu
aimeeu previously approved these changes May 19, 2025
Copy link
Contributor

@aimeeu aimeeu left a comment

Choose a reason for hiding this comment

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

Docs approved - thanks Piotr!

tgross
tgross previously approved these changes May 19, 2025
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

This is looking great @pkazmierczak. I've left a few minor items to clean up and then I think this will be ready to ship!

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM!

@pkazmierczak pkazmierczak merged commit cdc308a into main May 19, 2025
40 checks passed
@pkazmierczak pkazmierczak deleted the f-self-wi-lookup-policy branch May 19, 2025 17:54
pkazmierczak added a commit that referenced this pull request May 28, 2025
During #25547 and #25588 work, incorrect response codes from
/v1/acl/token/self were changed, but we did not make a note about this in the
upgrade guide.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.10.x backport to 1.10.x release line theme/docs Documentation issues and enhancements theme/workload-identity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow lookup of self ACL token when using workload identity
6 participants