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

Adding OAuth 2.1 specifications #1971

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

csfreak92
Copy link
Collaborator

Update 0x51-V51-OAuth2.md based on recommendations from reviewers to reflect OAuth 2.1 specifications and comments from reviewers.

This Pull Request relates to issue #996 by taking into consideration the initial wave of recommendations.

Update 0x51-V51-OAuth2.md based on recommendations from reviewers to reflect OAuth 2.1 specifications and comments from reviewers.
@@ -14,15 +14,17 @@ There are various different personas in the OAuth process, described in more det
| **51.1.4** | [ADDED] Verify that refresh tokens are sender-constrained or use refresh token rotation to prevent token replay attacks. Refresh token rotation prevents usage in the event of a compromised refresh token. Sender-constrained refresh tokens cryptographically binds the refresh token to a particular Client. | ✓ | ✓ | ✓ |
| **51.1.5** | [ADDED] Verify that if a Client sends a valid PKCE "code_challenge" parameter in the authorization request, the Authorization Server enforces the correct usage of "code_verifier" at the token endpoint. | ✓ | ✓ | ✓ |
| **51.1.6** | [ADDED] Verify that the Resource Owner password credentials grant is not used or configured by the Authorization Server. This grant type insecurely exposes the credentials of the Resource Owner to the client, increasing the attack surface of the application. | ✓ | ✓ | ✓ |
| **51.1.7** | [ADDED] Verify that the Authorization Server does not expose URLs that forward the user's browser to arbitrary URIs obtained from a query parameter ("open redirectors") which can enable exfiltration of authorization codes and access tokens. Redirect URIs must be compared using exact string-matching. | ✓ | ✓ | ✓ |

Choose a reason for hiding this comment

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

"Redirect URIs must be compared using exact string-matching.": Should this not be a separate requirement ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean the second sentence to be separated and create another requirement than them both being in one requirement?

Choose a reason for hiding this comment

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

@csfreak92: Yes. This looks like quite unrelated things to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I read it, the first part is "do not have an open-redirect vulnerability" on the authorization server (covered by 5.1.5):

The second part is for the string-match method, end would be covered by the outcome from #1965 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah so we should just drop the open-redirect in this requirement and retain the string-match method. Gotcha, I can fix that, but let's wait for the result of that thread's comment. :)

@elarlang
Copy link
Collaborator

Hi, I wrote my comment in #1965

In short, let's have an agreement on proposed requirement text before we do PR. I close this for now.

@elarlang elarlang closed this May 24, 2024
@elarlang elarlang reopened this May 24, 2024
@elarlang
Copy link
Collaborator

Note, I reopened it at the moment. First I thought it was only about the one redirect_uri requirement, but there were more changes in.

@csfreak92
Copy link
Collaborator Author

Hi Elar, noted.

@@ -82,7 +84,7 @@ Restricting token privileges ensures a Client is granted the proper access to a

### Resource Owner Password Credentials Grant

Aside from this grant type can leak credentials in more places than just the Authorization Server, adapting the Resource Owner password credentials grant to two-factor authentication, authentication with cryptographic credentials (e.g. WebCrypto, WebAuthn) and authentication processes that require multiple steps can be hard or impossible.
Aside from this grant type can leak credentials in more places than just the Authorization Server, adapting the Resource Owner password credentials grant to two-factor authentication, authentication with cryptographic credentials (e.g. WebCrypto, WebAuthn) and authentication processes that require multiple steps can be hard or impossible. This grant type is not recommended in general due to security concerns. Instead, use the authorization code grant with PKCE. This grant type is omitted from OAuth 2.1 specification.
Copy link

@randomstuff randomstuff Jun 22, 2024

Choose a reason for hiding this comment

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

Instead, use the authorization code grant with PKCE. This grant type is omitted from OAuth 2.1 specification.

Is it me or might this be interpreted as "authorization code grant with PKCE is omitted from OAuth 2.1"?

Choose a reason for hiding this comment

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

Maybe swap the two sentences?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @randomstuff, great catch! Now that I read it a few times, it makes sense to swap the positioning of sentences. I'll update this PR as well for this.

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

3 participants