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

Question regarding the upcoming IAM behaviour change on 27th July #6736

Closed
ct-dh opened this issue Jul 3, 2020 · 16 comments
Closed

Question regarding the upcoming IAM behaviour change on 27th July #6736

ct-dh opened this issue Jul 3, 2020 · 16 comments
Assignees
Labels

Comments

@ct-dh
Copy link

ct-dh commented Jul 3, 2020

Hi, the release notes for Cloud IAM show that on July 27th 2020 the change to prefix deleted service accounts in IAM policies with 'deleted:' will be attempted again. It was enabled in December 2019 and caused a lot of issues because a lot of things (including this provider) were not ready to handle the behavior change and was rolled back.

My question is, are the maintainers of this provider aware of this upcoming change and are they confident things will not break in the provider? I know there was a PR merged in Dec that made it into version 3.3.0 and 2.20.1 that was supposed to stop this from causing bad requests.

Can anyone advise what the expected behaviour will be in the following scenarios after the change goes live (because AFAICT there is no way we can test the behaviour ourselves prior to the 27th July)?

  • there is a deleted service account binding for role X and deleted user A, I add a new binding via google_project_iam_member for the same role X for user B. I would hope this would work without issue.
  • there is a deleted service account binding for role X and deleted user A, I add a new binding via google_project_iam_member for user a recreated user A (i.e different internal ID). What happens? Does the provider remove the entry for the deleted account and rebind to the new user? Will it error?
@edwardmedia edwardmedia self-assigned this Jul 3, 2020
@edwardmedia
Copy link
Contributor

@ct-dh Can you be more specific to the release notes & PRs you mentioned above? Links and numbers would be greatly helpful. Thanks

@ct-dh
Copy link
Author

ct-dh commented Jul 6, 2020

@ct-dh
Copy link
Author

ct-dh commented Jul 13, 2020

Any update on this by the way, the July 27th date is getting close if we think this will require any remediation/workarounds.

@slevenick
Copy link
Collaborator

Hey @ct-dh I worked on the initial PRs around the deleted IAM behavior back in December.

"there is a deleted service account binding for role X and deleted user A, I add a new binding via google_project_iam_member for the same role X for user B. I would hope this would work without issue."

This should work without issue. The current implementation of iam_member will only add the specified member to the binding, leaving the rest of the members for that binding untouched. We consider deleted:user:userA different from user:userA so there should be no problem if they both exist on the same policy.

"there is a deleted service account binding for role X and deleted user A, I add a new binding via google_project_iam_member for user a recreated user A (i.e different internal ID). What happens? Does the provider remove the entry for the deleted account and rebind to the new user? Will it error?"

In this case the provider will not remove the deleted account, but it will rebind to the new user. This should not cause any errors because these are different accounts with different ids, and when using iam_member all other members for a given role will be preserved.

In general the provider considers bindings for deleted users to be unique from bindings for undeleted users. For example, if you have a terraform iam_member for a user A, role X, then manually delete user A, on the next run terraform will not identify the existing binding for deleted:user:userA as matching the iam_member, and attempt to recreate it.

Does this clarify the provider behavior?

@ct-dh
Copy link
Author

ct-dh commented Jul 14, 2020

Hi @slevenick thanks for the detailed response.

I think I understood everything apart from the last point where I can't quite tell if you meant:

  1. For example, if you have a terraform iam_member for a user A, role X, then manually delete user A, on the next run terraform will not identify the existing binding for deleted:user:userA as matching the iam_member, so it will attempt to recreate it (and presumably succeed barring any other unrelated issues).
    or
  2. For example, if you have a terraform iam_member for a user A, role X, then manually delete user A, on the next run terraform will not identify the existing binding for deleted:user:userA as matching the iam_member, and will not attempt to recreate it.

I think it you meant option 1 but if you could clarify the above that would be great, but this sounds like it shouldn't cause us any impact.

For the next question, to make it easier to write, v1 is referring to the exisiting IAM API behaviour (i.e. no deleted: prefixes on members) and v2 referring to the new version that will be released on the 27th July. My only other concern was that it seemed when this was being rolled out in a phased fashion over 4 days in Decemember that terraform may read the current policy from the v2 API, but when attempting to update the policy it might hit the v1 API which would complain about the deleted: prefix in the policy. I see this is slated for a phased rollout again so wondering if it is possible this could happen? I understand this may not have happened in December and it could just be a bad theory that we came up with based on a superficial understanding of what was actually happening but any thoughts would be welcome.

@slevenick
Copy link
Collaborator

@ct-dh For your first question option 1 should be correct. Terraform attempts to fix drift, so as long as the iam_member still exists in the terraform config, it should be recreated once terraform sees that it does not exist.

For your second question I would expect that the API would be able to handle receiving requests with deleted: prefixes to the "v1 API". I don't have much info on how this will work, but I do not expect it to be an issue

@edwardmedia
Copy link
Contributor

@ct-dh let's close this question as it is July 27th today. Please feel free to reopen it if you see a need to continue the question. Thanks

@ct-dh
Copy link
Author

ct-dh commented Jul 28, 2020

Just in-case anyone else looks at this, the IAM release notes say this change is being pushed back to the end of August 2020:

July 20, 2020
We are delaying the upcoming changes for deleted members that are bound to a role. These changes will take effect starting on August 31, 2020.

https://cloud.google.com/iam/docs/release-notes#July_20_2020

@mancaus
Copy link
Contributor

mancaus commented Aug 12, 2020

I have a further question on this after seeing the latest update on the rollout of this feature, sent to me via email. That outlines a period during which the changes will be rolled out, and states that

... during the propagation period...

  • Submitting an IAM policy that contains both deleted and active versions of the account (i.e. with the same email address) will result in an error.

Can you clarify how this will impact role assignments made with the *_iam_member resources, e.g. google_project_iam_member? In particular, if during the "propagation period" I:

  1. Create a service account with email address x@y
  2. Use the provider to assign service account x@y to a role via google_project_iam_member
  3. Delete service account x@y
  4. Create a new service account x@y
  5. Re-run step 2

Will the expected result be:

  1. No change is applied, as the state matches, based on the members having the same email address.
  2. The old binding is removed and the new binding added, as they are for the same member (based on email address), but the state does not match (based on internal ID).
  3. An error, as the provider sees this as a new member being assigned (based on internal ID), but the resultant policy with the new binding added results in a mix of deleted and non-deleted bindings for the same email address.

FYI, I am a colleague of @ct-dh.

@ct-dh
Copy link
Author

ct-dh commented Aug 12, 2020

Attempted to re-open due to @mancaus question but don't seem to have that ability. @edwardmedia / @slevenick could this be re-opened?

@slevenick slevenick reopened this Aug 12, 2020
@slevenick
Copy link
Collaborator

@mancaus the expected result will be #3, but with the caveat that the error will be on the API-side not the provider side. This means the API will not accept the request to set the policy with a new and deleted member with the same name.

The way to resolve this error will be to remove the deleted member on the policy, either manually or by using the _iam_policy resource.

This is to prevent granting the permissions of an old user to a new user by mistake

@mancaus
Copy link
Contributor

mancaus commented Aug 13, 2020

Thanks for confirming @slevenick.

For us this will be quite difficult to manage as we have no way of testing the changes we will be required to make in advance of the change.

For others I expect they will come across failures without having understood the impact.

This could be a fairly large set of impacted users, as I would expect most use those IAM resources that allow a part of policy to be modified (i.e. by role or member). And, many will have stale bindings caused by managing service accounts separately from their bindings.

I think the API behaviour makes sense, as in the API the policy is set as a single resource, so ensuring the consistency of membership should be the responsibility of the API user.

In the provider however this is not the case, as the provider's resources target and update just a part of the policy. Asking users to reconcile these bindings "out of band", especially requiring logic that cannot be performed within Terraform itself, IMO places a very large burden on them.

Regardless, thank you for confirming the expected behaviour.

@slevenick
Copy link
Collaborator

@mancaus thanks for the perspective

The problem is that the concept of deleted: members is new, and attempting to prevent users from assigning permissions meant for an old, deleted account to a new account with the same name. From my point of view we have two options:

  1. If a user specifies a member in any terraform IAM resource that matches a deleted: member on the same policy, delete any references to the deleted: member
  2. Do nothing, allowing a user specifying a recently deleted and recreated IAM member via a iam_member resource to run into API-level errors

If we go back to the example from #6736 (comment) the problem comes when the IAM principal (service account in this situation) is deleted without deleting the associated permissions. Is this flow of deleting a service account out of band, but expecting a new service account with the same name to be granted the same permissions via terraform normal for you? Can you elaborate on why this is the case rather than managing the service account and its permissions via the same tool?

If the service account and associated permissions are managed with terraform it will have its permissions deleted prior to its deletion, which should prevent any issues.

What worries me about option 1 is that the terraform resources for iam_member and iam_binding would have impact on the IAM policy that would not be shown in the plan, and are likely not expected

@mancaus
Copy link
Contributor

mancaus commented Aug 17, 2020

I agree there's no clear "right" path to mitigating the impact.

I like the idea of removing deleted members from a policy when a non-deleted member with matching email is specified in a resource. I think the plan / state could show that the internal ID of the member (which is keyed on email) has changed, which forces the recreation. I think that would work in all cases when using member bindings, and some or most when using role bindings. The case of a role binding with a member that clashes with a different role binding is could be problematic. That could be seen as a corner case, but it would be quite a frustrating one.

Re managing service accounts as part of the same tf deployment /state as bindings: There are two very valid use cases, and I expect quite common ones, where they're managed separately. Both apply to us in different scenarios:

  • Our Terraform deploys loosely coupled components that have largely independent life-cycles. It's common for a component to create a service account, and for a second component to create bindings for that service account to allow access to resources within the second component. I won't elaborate the reasons for loosely coupled components, but it is a given for us that we cannot manage each tenant as "one big deployment".
  • In many configurations some service accounts are privileged entities, and must be created separately from application infrastructure. This is particularly the case when used in combination with Shared VPC and org level , organisation level firewall rules providing access to sensitive endpoints will rely on centrally governed service accounts.

Hope this context is useful.

@slevenick
Copy link
Collaborator

#7278 has some information on this. Closing in favor of that issue

@ghost
Copy link

ghost commented Oct 16, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked as resolved and limited conversation to collaborators Oct 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants