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

identity: allow creating a role with a non-existent key #12251

Merged
merged 3 commits into from Aug 4, 2021

Conversation

fairclothjm
Copy link
Contributor

This PR maintains the current behavior in 1.8 by ensuring that a role can be created with a non-existent key. Any change to this behavior would be a breaking change for those dependent on it.

Background

#12151 fixed a bug that allowed a role's token_ttl to be longer than the verification_ttl of the key it references. However that PR introduced a bug that would cause the creation of a role with a non-existent key to fail with the error:

$ vault write identity/oidc/role/role1 key=key1 ttl=1m
Error writing data to identity/oidc/role/role1: Error making API request.

URL: PUT http://127.0.0.1:8200/v1/identity/oidc/role/role1
Code: 400. Errors:

* a role's token ttl cannot be longer than the verification_ttl of the key it references

Since the current plan is to ship #12151 in 1.8.1 then we need to make sure this PR is also shipped in 1.8.1 to maintain backward compatibility with existing behavior.

Additionally, #12208 is planned to be shipped in 1.9 and will enforce the key param and key existence on role creation.

@fairclothjm fairclothjm requested a review from a team August 4, 2021 17:15
@vercel vercel bot temporarily deployed to Preview – vault August 4, 2021 17:15 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook August 4, 2021 17:15 Inactive
@calvn
Copy link
Member

calvn commented Aug 4, 2021

Additionally, #12208 is planned to be shipped in 1.9 and will enforce the key param and key existence on role creation.

Since #12208 is going to disallow this behavior, is this a stopgap in the meantime to avoid raising errors?

@calvn calvn added this to the 1.8.1 milestone Aug 4, 2021
@fairclothjm
Copy link
Contributor Author

Since #12208 is going to disallow this behavior, is this a stopgap in the meantime to avoid raising errors?

@calvn Yes, that is correct. This is to ensure the current behavior until 1.9

@@ -0,0 +1,3 @@
```release-note:bug
identity: allow creating a role with a non-existent key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was always the case, up until #12151 was introduced right? Since this is a bug fix for a PR within 1.8.1 and results in no behavioral change from the user, I wonder if we can just pr/no-changelog this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, thanks

@calvn calvn merged commit 0260441 into main Aug 4, 2021
calvn added a commit that referenced this pull request Aug 4, 2021
calvn pushed a commit that referenced this pull request Aug 4, 2021
* identity: allow creating a role with a non-existent key

* remove whitespace

* add changelog
calvn added a commit that referenced this pull request Aug 4, 2021
…ey (#12251) (#12257)

* identity: allow creating a role with a non-existent key (#12251)

* identity: allow creating a role with a non-existent key

* remove whitespace

* add changelog

* changelog: remove 12251 entry (#12256)

Co-authored-by: John-Michael Faircloth <fairclothjm@users.noreply.github.com>
@fairclothjm fairclothjm deleted the oidc-token-ttl-verification-no-key branch August 4, 2021 18:54
jartek pushed a commit to jartek/vault that referenced this pull request Sep 11, 2021
)

* identity: allow creating a role with a non-existent key

* remove whitespace

* add changelog
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.

None yet

2 participants