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

auth/aws: Fix error with empty bound_iam_principal_arn #3843

Merged
merged 2 commits into from
Jan 25, 2018

Conversation

joelthompson
Copy link
Contributor

In cases where there doesn't need to be a bound_iam_principal_arn, i.e.,
either auth_type is ec2 or there are other bindings with the iam
auth_type, but it is specified explicitly anyway, Vault tried to parse
it to resolve to internal unique IDs. This now checks to ensure that
bound_iam_principal_arn is non-empty before attempting to resolve it.

Fixes #3837

In cases where there doesn't need to be a bound_iam_principal_arn, i.e.,
either auth_type is ec2 or there are other bindings with the iam
auth_type, but it is specified explicitly anyway, Vault tried to parse
it to resolve to internal unique IDs. This now checks to ensure that
bound_iam_principal_arn is non-empty before attempting to resolve it.

Fixes hashicorp#3837
@jefferai
Copy link
Member

@joelthompson This change does mean that there's no way to clear out a bound_iam_principal_arn once it's set (other than deleting the role and re-creating it). I don't know if you'd want to or not -- if that's not a problem, then this LGTM.

@joelthompson
Copy link
Contributor Author

@jefferai -- I don't think that would be the case, maybe I'm missing something? But I do think it would be a problem if this change did mean that as what you called out should be a valid operation.

If bound_iam_principal_arn is explicitly passed in as empty, then line 498 would clear it out in the roleEntry. The if conditional on line 503 would then be false, so it would hit the else clause on line 511 and also set roleEntry.BoundIamPrincipalID = "".

Right?

@jefferai
Copy link
Member

This is what I get for looking at a diff without enough context. You're right.

@jefferai
Copy link
Member

LGTM, OK to merge on your end?

@joelthompson
Copy link
Contributor Author

Just realized I left in an extraneous newline, so a tiny commit to fix that. So, now, yep, thanks for the quick engagement here! :)

@jefferai jefferai added this to the 0.9.2 milestone Jan 25, 2018
@jefferai jefferai merged commit 5f4ee04 into hashicorp:master Jan 25, 2018
@joelthompson joelthompson deleted the issue_3837 branch January 25, 2018 04:08
@jefferai
Copy link
Member

You caught me at a specifically good time for very small fixes :-)

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

Successfully merging this pull request may close these issues.

Make AWS auth roles "roundtrippable" so we can manage them with terraform.
2 participants