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

Merge OAuth chapter into master #1804

Merged
merged 17 commits into from
Mar 17, 2024
Merged

Merge OAuth chapter into master #1804

merged 17 commits into from
Mar 17, 2024

Conversation

tghosth
Copy link
Collaborator

@tghosth tghosth commented Dec 14, 2023

This Pull Request relates to issue #996.

For now, this is just to monitor the "mergability" of the changes. We won't actually merge this until it has finished the development process.

@tghosth
Copy link
Collaborator Author

tghosth commented Dec 14, 2023

Note some original comments were raised here:
#1494 (review)

@elarlang elarlang added the V51 Group issues related to OAuth label Dec 14, 2023
5.0/en/0x51-V51-OAuth2.md Show resolved Hide resolved
5.0/en/0x51-V51-OAuth2.md Outdated Show resolved Hide resolved
5.0/en/0x51-V51-OAuth2.md Outdated Show resolved Hide resolved
5.0/en/0x51-V51-OAuth2.md Outdated Show resolved Hide resolved
5.0/en/0x51-V51-OAuth2.md Outdated Show resolved Hide resolved
5.0/en/0x51-V51-OAuth2.md Outdated Show resolved Hide resolved
5.0/en/0x51-V51-OAuth2.md Outdated Show resolved Hide resolved
@tghosth tghosth mentioned this pull request Dec 14, 2023
@elarlang
Copy link
Collaborator

elarlang commented Dec 16, 2023

51.1.5 - I don't think we can/need to require it. It is recommended as one way to do that.
https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#name-authorization-code-grant
It is RECOMMENDED for AS to publish the element code_challenge_methods_supported in their AS metadata ([RFC8414]) containing the supported PKCE challenge methods (which can be used by the client to detect PKCE support). ASs MAY instead provide a deployment-specific way to ensure or determine PKCE support by the AS.

51.4.1 - We can not have requirements for the Resource Owner (end-user). Is a certain flow or grant is accepted/supported, it is the Authorization Server's responsibility to configure it for the OAuth Client. So it belongs to the "Authorization Server" section.

This is for a quick fix. But... as I opened a discussion in #996 (comment), maybe we should not list the grants/flows which are not allowed per requirement, but have one requirement to say "Use the PKCE flow and X only, which also means, y z w flows are not allowed."

@tghosth
Copy link
Collaborator Author

tghosth commented Feb 15, 2024

hi @csfreak92, are you still clear on the remaining feedback that needs to be actioned here. It would be great to get a first draft of this chapter merged into master.

@csfreak92
Copy link
Collaborator

Yes @tghosth, still working on it in my local copy first before pushing a commit. I will push a commit soon around late next week after this week's madness. :)

* Update 0x51-V51-OAuth2.md

Final edits for OAuth 2.0 protocol chapter

* Update 0x51-V51-OAuth2.md

Cleaning up trailing spaces

* Fixing lint errors

Changed tabs to spaces to show indentation on the terminology section.

* Various tweaks throughout the document

---------

Co-authored-by: Josh Grossman <[email protected]>
@tghosth
Copy link
Collaborator Author

tghosth commented Mar 6, 2024

@csfreak92 please do the following:

@jmanico
Copy link
Member

jmanico commented Mar 6, 2024

I also have one suggestion:

For...

+| 51.3.1 | [ADDED] Verify that Resource Servers are using mechanisms for sender-constraining access tokens to prevent token replay, such as Mutual TLS for OAuth 2.0 or OAuth Demonstration of Proof of Possession (DPoP). | ✓ | ✓ | ✓ |

... you are supposed to use the same client secret given to the client during registration. This implied it's arbitrary. My edit suggestion is:

+| 51.3.1 | [ADDED] "Verify Resource Servers implement sender-constrained access token mechanisms to mitigate token replay risks. This is achieved by mandating Mutual TLS for OAuth 2.0 or OAuth DPoP, using the client secret provided at client registration.". | ✓ | ✓ | ✓ |

@csfreak92
Copy link
Collaborator

... you are supposed to use the same client secret given to the client during registration. This implied it's arbitrary. My edit suggestion is:

+| 51.3.1 | [ADDED] "Verify Resource Servers implement sender-constrained access token mechanisms to mitigate token replay risks. This is achieved by mandating Mutual TLS for OAuth 2.0 or OAuth DPoP, using the client secret provided at client registration.". | ✓ | ✓ | ✓ |

Noted, @jmanico. Just as I mentioned in the other comment, I thought of explicitly calling it out as some people might not think it is implied already, but for brevity I agree with this assessment :)

@csfreak92
Copy link
Collaborator

This is for a quick fix. But... as I opened a discussion in #996 (comment), maybe we should not list the grants/flows which are not allowed per requirement, but have one requirement to say "Use the PKCE flow and X only, which also means, y z w flows are not allowed."

Oh sorry Elar, I thought I addressed this already. I will fix it in my new PR that I was asked to open.

* Finalizing OAuth 2.0 chapter

Ralph made the changes requested by the reviewers based on their comments/suggestions. Also removed the mapping subsection to clean it up.

* Update 0x51-V51-OAuth2.md

---------

Co-authored-by: Josh Grossman <[email protected]>
@tghosth tghosth marked this pull request as ready for review March 17, 2024 18:43
@tghosth tghosth enabled auto-merge (squash) March 17, 2024 18:45
@tghosth
Copy link
Collaborator Author

tghosth commented Mar 17, 2024

@elarlang @jmanico let's get this approved and then we can see what other comments we have on it

Copy link
Collaborator

@elarlang elarlang left a comment

Choose a reason for hiding this comment

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

Let's get it merged to have it in the spotlight to get more attention.

@tghosth tghosth merged commit bdac6d3 into master Mar 17, 2024
2 checks passed
@tghosth tghosth deleted the master-v51-oauth branch March 17, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V51 Group issues related to OAuth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants