-
Notifications
You must be signed in to change notification settings - Fork 32
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
recommend auth code flow using signed requests #226
Conversation
My apologies, I created #233, before seeing this PR. Possible to use #233 instead? |
Co-authored-by: Jesús Peña García-Oliva <jesus.penagarcia-oliva@telefonica.com>
I would like to clarify why the OIDC specification was chosen over RFC 9101. It is important because, although they are very similar, they have small but significant differences. OIDC:
From my point of view, it is more interesting to follow RFC 9101, as it would ignore parameters that the client did not have to send and could potentially change how an authorization server processes the request. |
I would like to propose to append the following point regarding the 8< ------------------------ |
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.
Added proposed changes, including references
Pending topics:
Could we please resume and close these topics as it's under the Spring25 scope. |
Referring to the Breaking it down: Point b) is read as "OP's Issuer Identifier URL" is used as a prefix. |
Referring to OIDC, Client Authentication, private_key_jwt, the following statement is present: If this signed request object (for /authorize) |
RFC9101 states: "The value of aud should be the value of the authorization server (AS) issuer..." in that sense. So again it sounds reasonable to suggest that the aud field should be the issuer of the authorisation server, taking into account both standards statements. |
I changed the text regarding "aud".
|
@AxelNennker |
For easier review: The current text is this. Can this be merged? I think it resolves all review comments and suggestions. Signed Authentication Requests It is RECOMMENDED that signed authentication requests be used, as specified by OIDC. The same key MAY be used for signing the authentication request as is used for client authentication. It is RECOMMENDED that the value of the aud field of the signed authentication request is the URL of the Authorization Endpoint. The authorization server MAY accept different values of the aud field e.g. the issuer field of its OpenID Provider Metadata. The authorization server MUST check the value of the aud field and reject signed authentication requests if the value of the aud is not associated with the authorization server. Note: Care must be taken in a multi-tenant environment that a signed authentication request for one tenant is not accepted at another tenant endpoint. Note: For security reasons it is recommended that the API consumer never includes a sub field in the signed request object, because otherwise the signed request object might be used for client authenticaton. |
I have not received feedback from your side on the comment regarding why the OIDC specification is preferred over RFC9101 when handling parameters outside the request object parameter. |
I still don't understand why we change the And in the case of CIBA, the profile doesn't even mention the |
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.
LGTM, based on @shilpa-padgaonkar corrections.
@AxelNennker, Thanks.
@jpengar
In RFC 8725 JSON Web Token Best Current Practices, section 3.12, Use Mutually Exclusive Validation Rules for Different Kinds of JWTs, the following is indicated:
The main security concern to address, is to prevent having a signed request object being used as a
By specifying the /authorize endpoint as the |
Co-authored-by: Shilpa Padgaonkar <77152136+shilpa-padgaonkar@users.noreply.github.com>
I think now all comments and suggestions are addressed. Please review again. |
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.
LGTM
I'd like to see the conflicts resolved before approving |
merge conflicts have been resolved, please review and consider approving. |
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.
LGTM
I believe this PR (#226) was merged prematurely, as key concerns raised during the discussion remain unresolved. Specifically:
Since these points were still under discussion and the proposed text was not agreed upon by all participants, I recommend reverting this PR until a consensus is reached and the final text is approved. @AxelNennker @mhfoo @garciasolero @sebdewet @eric-murray @shilpa-padgaonkar |
I reverted the merge which created #249 |
@garciasolero wrote:
Sorry, @garciasolero, I thought this was addressed. I think that 9101 and OIDC say the same thing regarding parameters existing both in the parameters and in the request object. For compatibility reasons OAuth2 parameters that are required are also required in OIDC. But if parameters are also in the request object then those are the only ones that are relevant. You write: "From my point of view, it is more interesting to follow RFC 9101, as it would ignore parameters that the client did not have to send and could potentially change how an authorization server processes the request." Maybe an some examples: (with line wraps within values for display purposes only) Valid example. All required parameters are in the request AND the request object.
Invalid OIDC example because required parameters are missing although they provided in the request object
This PR does not change the OIDC standard, it just RECOMMENDS signed request objects. |
Regarding aud: CIBA writes:
We recommend: This PR writes:
This PR does what RFC9101 recommends, that is, the aud value is the URL of the endpoint the request is send to. While allowing other values to be used as long as the Authorization Server validates that the used value for "aud" is itself. For interoperability reasons CIBA and OIDC are quite lenient regarding aud values. The responsibility in both flows lies with the authorization server. The profile recommends what rfc9101 recommends. I thought that the above was the agreement here. Sorry, if I misinterpreted that. |
@AxelNennker As was mentioned by @garciasolero here, the recommendations apply depending on the type of assertion, client authentication assertion (client_assertion) or signed request object assertion. The CAMARA profile specifies "This document RECOMMENDS that for OIDC Authorisation Code Flow and OAuth2 Client Credentials Grant the audience SHOULD be the URL of the Authorisation Server's Token Endpoint. This document RECOMMENDS that for OIDC CIBA the audience SHOULD be the Backchannel Authentication Endpoint", but this is for client authentication. Also, for the Auth code flow, note that there is no client authentication in the /authorize endpoint (only in /token). And in the CAMARA profile document, in the case of CIBA, there is not even a mention of the aud parameter for the signed authentication request. So it relies on what the CIBA standard says, which is The Audience claim MUST contain the value of the Issuer Identifier for the OP, which identifies the Authorization Server as an intended audience.". So, we would be recommending different things to Auth code flow and CIBA. |
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
This PR recommends using signed OIDC authoritzation code flow requests.
Which issue(s) this PR fixes:
Fixes #205