-
Notifications
You must be signed in to change notification settings - Fork 431
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
Add principal type to role assignment create parameters #4625
Add principal type to role assignment create parameters #4625
Conversation
Hi @whites11. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
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
/assign @Jont828 @willie-yao
/cherrypick release-1.14
/cherrypick release-1.13
LGTM label has been added. Git tree hash: a5dc9adac08d18af234188d9019f38806d8058ed
|
@@ -352,6 +353,7 @@ func (m *MachineScope) RoleAssignmentSpecs(principalID *string) []azure.Resource | |||
Scope: m.SystemAssignedIdentityScope(), | |||
RoleDefinitionID: m.SystemAssignedIdentityDefinitionID(), | |||
PrincipalID: principalID, | |||
PrincipalType: armauthorization.PrincipalTypeServicePrincipal, |
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.
Instead of hardcoding PrincipalType: armauthorization.PrincipalTypeServicePrincipal
, would it be valuable to have the user specify PrincipalType
in the MachinePool spec?
Is it possible that PrincipleType
can take other values than PrincipalTypeServicePrincipal
?
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.
That's a great question. Thanks for reviewing @nawazkh!
I found these possible values for PrincipalType
const (
PrincipalTypeDevice = "Device"
PrincipalTypeForeignGroup = "ForeignGroup"
PrincipalTypeGroup = "Group"
PrincipalTypeServicePrincipal = "ServicePrincipal"
PrincipalTypeUser = "User"
)
and it seemed that the SP type would be associated with the PrincipalID
, but it's not entirely clear to me. @whites11 can you clarify?
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.
Sorry for the delay, I missed the notification.
So, this role assignment is used only for system assigned role for VMs/VMSSes.
In this scenario, as far as I understood and as far as my tests proved, the only meaningful value for the principal type is service principal
.
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.
Thanks for the explanation. I agree: none of the other values seemed relevant, and this is limited in scope.
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
LGTM label has been added. Git tree hash: a5dc9adac08d18af234188d9019f38806d8058ed
|
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mboersma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cherrypick release-1.14 |
@mboersma: once the present PR merges, I will cherry-pick it on top of release-1.14 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherrypick release-1.13 |
@mboersma: once the present PR merges, I will cherry-pick it on top of release-1.13 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@mboersma: new pull request created: #4663 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@mboersma: new pull request created: #4664 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
…-principal-type Add principal type to role assignment create parameters
…-principal-type Add principal type to role assignment create parameters
What type of PR is this?
/kind feature
What this PR does / why we need it:
Context: when using
identity: SystemAssigned
in an AzureMachineTemplate with role definition IDmyrole
.Accordingly to the CAPZ documentation: https://capz.sigs.k8s.io/topics/vm-identity#system-assigned-managed-identity the service principal used to run CAPZ controller manager requires
User Access Administrator
on the Subscription of Workload cluster.Based on customer feedback, we agree that this is a very wide permissions set assigned to the service principal at hand. The only restriction that can be achieved right now is to limit this access to Role Based Access Control Administrator with a condition on a role to be assigned, for example:
This however creates a security flaw, where, if the CAPZ service principal is leaked, the
myrole
role can be assigned to any resource type, including users.Desired behaviour: I would like to restrict the role assignment using a constraint like this one:
This would prevent assigning the
myrole
role to a User or Group while still allowing assignment to Service Principals.Problem: doing so without the changes introduced in this PR leads to a permission error:
This is because in the RoleAssignmentCreate API call, the principalType field is not assigned.
This PR addresses that problem by setting the PrincipalType to
ServicePrincipal
in the role assignment creation API call.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):None that I'm aware of.
Special notes for your reviewer:
TODOs:
Release note: