-
Notifications
You must be signed in to change notification settings - Fork 547
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 resources for Vault Identity Tokens #488
Conversation
- Add `vault_identity_oidc_key` - Add `vault_identity_oidc_role` resource - Add `vault_identity_oidc` resource - Add `vault_identity_oidc_key_allowed_client_id`
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.
Generally speaking, this is looking very good! Just a couple questions here and there.
Hi @tyrannosaurus-becks. I will answer several of your questions in a comment since they are mostly related. Client IDsThe steps to use an Identity Token Role involve:
As you can see that there is a circular dependency here. You cannot know the The example code below and also written in the documentation should demonstrate what I mean: resource "vault_identity_oidc_key" "key" {
name = "key"
algorithm = "RS256"
}
resource "vault_identity_oidc_role" "role" {
name = "role"
key = vault_identity_oidc_key.key.name
}
resource "vault_identity_oidc_key_allowed_client_id" "role" {
key_name = vault_identity_oidc_key.key.name
allowed_client_id = vault_identity_oidc_role.role.client_id
} MutexThe issue now is that we cannot have both vaultMutexKV.Lock(path)
defer vaultMutexKV.Unlock(path) where the This pattern is first used in the AWS Provider for resources like The AWS Provider also declares a global provider KV Mutex which is what I did here. I intend the key of the global KV mutex to be the Vault Path for resources if they need to use it. Hope this answers your questions. |
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.
Thank you @lawliet89 for this PR and for the explanations!
This looks great to me. I'm going to hold on merging it in case my colleague Jim wants to also provide input, since he also self-assigned reviewing this PR. However, barring any blockers, I will make sure to get this merged so it can go out with the next release that I'm aiming to cut by the end of the week.
Hi @lawliet89. I'm reviewing your PR and everything is looking good so far. One not-obvious thing about the relationship between roles and keys is that you can reference a non-existent named key when you make a role. Of course it will fail if used, but it won't fail at config time. So you should be able to:
That might simplify some logic. |
@kalafut I see what you mean. But I think there is a use case for the I will add another example and explanation to the documentation to make the two workflows clearer. (See ec58619) |
if rs.Type != "vault_identity_oidc_key" { | ||
continue | ||
} | ||
secret, err := client.Logical().Read(identityEntityIDPath(rs.Primary.ID)) |
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.
Do you instead want something like: resp, err := identityOidcKeyApiRead(rs.Primary.Attributes["name"], client)
?
Not that when looking at this I discovered that Vault is incorrectly returning a 400 for key not found instead of 404. I've just fixed that, but in the interim you may need the subsequent check to be something like:
if err != nil && !strings.Contains(err.Error(), "no named key found") {
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've fixed this and added the check in identityOidcKeyApiRead
(commit 34517b6).
@lawliet89 Yeah I can see that use, and actually I'd misinterpreted things the first time around and thought the |
@lawliet89 Key deletion will be blocked if there are roles referencing the key. For this provider I see a couple of issues:
|
@lawliet89 By the way, I think this is good as-is if we hit our release/merge window, and the latest comments can be follow-on improvements if need be. Thanks! |
To account for hashicorp/vault#7267 which will be released with Vault 1.2.2
Fixed the error formatting for that.
I don't think this can be done from the provider. Terraform will delete things in reverse dependency order. So the user will have to implicitly or explicitly have the role depend on the key for the destroy to work in one single operation. |
I'm going to go ahead and merge this because I see that the linked PR was taken off this branch, so its diff will be smaller when this is merged. Going to merge this then go take a look. |
Add resources for Vault Identity Tokens
vault_identity_oidc_key
vault_identity_oidc_role
resourcevault_identity_oidc
resourcevault_identity_oidc_key_allowed_client_id
The name for the last resource might be a bit too long... 😅