-
Notifications
You must be signed in to change notification settings - Fork 170
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
Always do group expansion for CheckAccess subjects #3710
Conversation
/azp run e2e,ci |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run ci |
Azure Pipelines successfully started running 1 pipeline(s). |
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 agree with Weinongs comments but LGTM after that. I've re-run the E2E tests to see if that failure was just a flake. I had a look through the test that failed and it's not obvious to me that this change was related so 🤞
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.
LGTM!
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 too agree with Weinong. Once thats done, LGTM
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.
Not a blocker but a small suggestion. LGTM
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.
will approve once the magic word is updated.
96f6be1
to
e20b612
Compare
/azp run ci,e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
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.
LGTM
|
||
// GroupExpansion is the value to be used with ClaimName in SubjectAttributes | ||
// This value gives CheckAccess a hint that it needs to retrieve all the groups the principal belongs to | ||
// and then give the response based on all group entitlements. | ||
// | ||
// https://eng.ms/docs/microsoft-security/identity/auth/access-control-managed-identityacmi/azure-authz-data-plane/authz-dataplane-partner-wiki/remotepdp/checkaccess/samples/requestresponse | ||
GroupExpansion = `{"groups":"src1"}` |
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.
@nwnt Also, thanks a lot for the pointers to the docs as sometimes that kind of info is not easy to find. 🙂
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.
LGTM. Thanks. Just one remaining question from my side.
Which issue this PR addresses:
Fixes: ARO-9261
What this PR does / why we need it:
Refer to the IcM in JIRA, CheckAccess team informed that we need to send an additional field to give hints to CheckAccess that "group expansion" is needed.
Without group expansion, CheckAccess will not take into account what groups the principal belongs to. That is, if a service principal A is a member of group B, and group B allows writing to a VNet, calling CheckAccess without the group expansion field will return a response that A is not allowed to write to a VNet. With the group expansion hint, CheckAccess will return an allowed response, which is the behavior we want.
Test plan for issue:
I reproduced this by creating a Deny Assignment on a VNet resource from Azure Blueprint and made a group excluded from it. The testing app principal is then added to the group. After that I verified the following 2 scenarios:
Request (notice the lack of the new _Claim_Name field in Subject)
Response (Notice that some actions were reported as denied)
Request (Notice the additional field in Subject)
Response (all actions are allowed)
Is there any documentation that needs to be updated for this PR?
N/A - the code should be clear enough.
How do you know this will function as expected in production?
E2E tests should be enough.