Skip to content
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

Specific list of approvers #48

Open
rickihastings opened this issue Mar 7, 2023 · 11 comments
Open

Specific list of approvers #48

rickihastings opened this issue Mar 7, 2023 · 11 comments
Labels
enhancement New feature or request

Comments

@rickihastings
Copy link

Hello, we are exploring using this where I work and we have some concerns about allowing peers to approve each others requests.

Is there any appetite for allowing a configurable list of approvers instead? I appreciate this isn't an issue more of a feature request.

I'm happy to try and contribute this in my spare time if you think this is something that would be accepted as a PR.

@jpassing jpassing added the enhancement New feature or request label Mar 8, 2023
@jpassing
Copy link
Collaborator

jpassing commented Mar 8, 2023

I think it's an interesting idea and I'm sure there's a use case for it. What I'm unsure about is how the configuration might look like: I suppose one option would be to specify the (group or list of) approvers in the IAM condition itself somehow. I think that would work, but as the number of eligible role bindings grow, it might become quite difficult though to keep track of and manage who can approve which roles.

Another option would be to maintain the configuration (who can approve which role on which resource) in some sort of configuration database that's separate from IAM. But having such a database is something I've been seeking to avoid as it would be difficult to secure and could create a possible avenue for privilege escalations.

I'm sure there are other options I haven't thought of yet... if you have some ideas, please let us know.

@rickihastings
Copy link
Author

This is something I was trying to figure out yesterday when I suggested this, but wasn't particularly sure of the best way. There's two things I can think of:

  1. Building on what you suggested, if Bob can escalate up to Project IAM Admin and has the MPA IAM condition applied to his user, however only Alice and John can approve, could they they have the Project IAM Admin with a new condition applied: has({}.multiPartyAccessApprover). This way it doesn't need to be embedded within the role, it only needs to be embedded in the approvers. It could still get messy to manage on large orgs though.
  2. The other option I was thinking of was some sort of YAML or JSON configuration object, however I don't particularly like the idea of loading configurations in from files as deploying it becomes more difficult.

@jpassing
Copy link
Collaborator

Sorry for not replying earlier.

If I understand your comment correctly, then you're suggesting two things:

  1. Project IAM Admins (or really, anybody with resourcemanager.projects.setIamPolicy permissions on a project) should be allowed to approve any actviation request in that project, because they can modify the IAM policy anyway.

    This would apply to users that have been granted the Project IAM Admin role either permanently or with a has({}.jitAccessConstraint) constraint.

  2. Introduce a has({}.multiPartyAccessApprover) condition that:

    • Entitles a user to approve requests for this role, but
    • Doesn't allow the user to request access to that role for themselves

    So in some sense, a role binding with a has({}.multiPartyAccessApprover) condition only gives you half the power of a role binding with a has({}.multiPartyApprovalConstraint) condition.

    In combination with (1), that means: By granting a user Project IAM Admin with a has({}.multiPartyAccessApprover) condition, you allow them to approve any any actviation requests for that project without allowing them to request any access for themselves.

That's actually very smart.

I am a bit worried about the extra complexity introduced by having a third kind of condition, and what that means for managing IAM policies. So I wonder if the following might be sufficient:

  1. Users that have been granted the Project IAM Admin role with a has({}.multiPartyApprovalConstraint) condition...

    • can approve any requests in that project, for any role
    • always show up in the list of available peers (in that project)
  2. If you grant two users the Project IAM Admin role with a has({}.multiPartyApprovalConstraint) condition, they can still approve each other access. That's different from what you proposed -- but it avoids the extra condition.

Wdyt?

@rickihastings
Copy link
Author

No problem.

I have to admit thinking about this in more depth, and taking a look at how our projects are setup, I am concerned that this could really complicate how permissions are managed and it might appear quite messy in the IAM console.

What you're suggesting is a good middle ground, this feels a lot cleaner and much more scalable. This will work well for us aswell.

@jpassing
Copy link
Collaborator

Great. I still have to double-check whether this approach will work properly with the Policy Analyzer API, but I'd be happy to move ahead with this.

@RickB-2840
Copy link

RickB-2840 commented Mar 27, 2023

Let's not overlook the scenario where IAM admin is done entirely via service accounts, where no users of the project have the IAM Admin role. In that scenario, making only the IAM admin users the approvers leaves us with no approvers. I see the complications of naming groups and users in the policy conditions, and I'd prefer not to go there. Perhaps a conditional policy could offer one or more of the following approver options:

  1. self authorization (no approvers),
  2. peer authorization (someone else identified via the binding can approve) and
  3. specified role authorization (people who have specified roles in other binding(s) are the approvers)

A conditional binding could specify 2) or 3) or both. If neither is specified, case 1) applies.

In case 3, what goes into the conditional binding is the role name(s) of one or more other bindings (not the conditional one they are approvers of). The roles could be predefined or custom roles, and the resources they are attached to would not be specified, i.e. they would be roles from whatever resource policies are inherited by the targeted resource or the project it's in.

The advantage of identifying approvers by referring to other role bindings is that users and groups are stored in bindings just as they are for everything else, making that more manageable overall.

@jpassing
Copy link
Collaborator

Let's not overlook the scenario where IAM admin is done entirely via service accounts, where no users of the project have the IAM Admin role.

That's a good point. However, I'm a bit wary of introducing dependencies between roles or imposing some hierarchy among them as that might create subtle paths to escalate privileges and could make it difficult to reason about who has access to which resources.

I think Project IAM Admin (or more precisely, the resourcemanager.projects.setIamPolicy permission) is special in that regard as somebody who has been unconditionally granted that role can freely modify the IAM policy anyway. Allowing Project IAM Admins to approve requests for other roles therefore seems to be in line with the general access model.

@duxbuse
Copy link

duxbuse commented Jun 5, 2023

Yeah I guess the only comment is that we would love to set Product owners/managers as approvers, but would not want them to actually have any access.

@rajat2911
Copy link

Hello @jpassing @duxbuse @duxbuse @RickB-2840 @proppy Can anyone help me where to set the peer approvers so that its visible on JIT application . Do we have any steps to do it

@jpassing
Copy link
Collaborator

@rajat2911, I assume you're asking how that works currently?

This doc has more details:

Multi-party approval is handled on a peer-to-peer basis::
If two users are granted an MPA-eligible role binding for, say roles/compute.admin on project-1, then they can approve each other’s requests.

So there's no explicit step to "set peer approvers" -- if you grant the same role to multiple users, and add the has({}.multiPartyApprovalConstraint) condition to each role binding, then these users can approve each other's request for this specific role.

This thread here is about an alternative way to designate approvers that's not implemented yet.

@duxbuse
Copy link

duxbuse commented Jun 21, 2023

Had a thought, would be interesting to be able to give someone iam.admin which would then allow them to be an approver.
But add the constraint that they cant call any methods on the iam.googleapis.com This would functionally make them only an approver without giving away any real permissions.

And depending on where that role is given will depend on what they are able to approve.

@jpassing It might also be worthwhile to have a config flag to allow peers to approve or onlyiam.admins for the case where you want to force a set of approvers. eg. my boss must approve, my teammates cant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants