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

docs: correct auth jwt role requirements #27384

Merged
merged 10 commits into from
Jun 12, 2024

Conversation

fairclothjm
Copy link
Contributor

@fairclothjm fairclothjm commented Jun 6, 2024

Description

This PR fixes the documentation to clearly state the Auth jwt plugin's behavior. For jwt roles, the bound_audiences parameter is required if the JWT's aud claim is present and must match at least one of the aud claims on the token.

There are a few issues at play here:

  • the Auth JWT docs used to clearly mark the expectations. However, this PR incorrectly changed the API doc for bound_audiences.
  • In 1.16.3, the plugin fixes a bug and security issue where the aud claim on the token is not checked if it is a string.

References

From https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.3

The "aud" (audience) claim identifies the recipients that the JWT is
intended for. Each principal intended to process the JWT MUST
identify itself with a value in the audience claim. If the principal
processing the claim does not identify itself with a value in the
"aud" claim when this claim is present, then the JWT MUST be
rejected.

Relates #27343
Relates #25791

@fairclothjm fairclothjm requested a review from a team June 6, 2024 16:00
@fairclothjm fairclothjm requested a review from a team as a code owner June 6, 2024 16:00
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jun 6, 2024
Copy link

github-actions bot commented Jun 6, 2024

CI Results:
All Go tests succeeded! ✅

Copy link

github-actions bot commented Jun 6, 2024

Build Results:
All builds succeeded! ✅

Copy link
Contributor

@thyton thyton left a comment

Choose a reason for hiding this comment

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

LGTM. Appreciate you taking this on!

I have one minor comment around clarifying the expected behavior after 1.16.3.


JWT auth roles of type "jwt" require the `bound_audiences` claim to match at
least one of the JWT's `aud` claims. Prior to 1.16.3, the JWT auth method would
ignore token `aud` claims that were not a list of strings.
Copy link
Contributor

@thyton thyton Jun 6, 2024

Choose a reason for hiding this comment

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

Suggested change
ignore token `aud` claims that were not a list of strings.
ignore token `aud` claims that were not a list of strings. Starting from 1.16.3,
it would ignore token `aud` claims that are not a single audience string or a list of strings.

I think we should state what changed from 1.16.3. My wording might be confusing here.

Copy link
Contributor Author

@fairclothjm fairclothjm Jun 6, 2024

Choose a reason for hiding this comment

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

I think the previous sentence makes that clear. Maybe we can get the education team's input on this? That said, I think your wording should say something like

Starting from 1.16.3, token aud claims can be a single string or a list of strings and they must match at least one of the role's bound_audiences.

But then maybe the next paragraph should be reworked?

cc @schavis

Copy link
Contributor

@thyton thyton Jun 7, 2024

Choose a reason for hiding this comment

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

I think saying this could be misleading.

Starting from 1.16.3, token aud claims can be a single string or a list of strings

The value type of aud in JWT claims follows the specification of JWT, which changed to allow string type independently prior to 1.16.3. I believe users had been sending login requests with single string aud. We didn't include that type into our invalidation logic until 1.16.3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, how about I change the wording of the paragraph to this:

JWT auth roles of type "jwt" require the bound_audiences claim to match at
least one of the JWT's aud claims. The JWT's aud claim can be a single string
or a list of strings as per RFC 7519 Section 4.1.3. Prior to 1.16.3, the JWT auth method
would ignore token aud claims that were not a list of strings.

Copy link
Contributor

@thyton thyton left a comment

Choose a reason for hiding this comment

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

Looks great!

website/content/docs/auth/jwt/index.mdx Outdated Show resolved Hide resolved
website/content/docs/auth/jwt/index.mdx Outdated Show resolved Hide resolved
website/content/docs/auth/jwt/index.mdx Outdated Show resolved Hide resolved
website/content/docs/upgrading/upgrade-to-1.16.x.mdx Outdated Show resolved Hide resolved
@fairclothjm
Copy link
Contributor Author

@schavis I reverted the change to the website/content/docs/upgrading/upgrade-to-1.16.x.mdx file to match the current formatting of the rest of the file. I checked the Vercel preview with the Warning tag and it didn't look correct. It makes it look like the Auth JWT info is part of the LDAP section above it but that isn' the intent. See the attached screenshot:

Screenshot 2024-06-11 at 9 50 03 AM

@fairclothjm
Copy link
Contributor Author

This PR will need some updates in light of some recent decisions around the code changes. Converting to draft until I get this reworked.

@fairclothjm fairclothjm marked this pull request as draft June 11, 2024 15:48
@fairclothjm fairclothjm marked this pull request as ready for review June 12, 2024 15:28
Copy link
Contributor

@thyton thyton left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed pr/no-changelog pr/no-milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants