-
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 - reloaded #251
Conversation
Regarding @jpengar 's comment #226 (comment)
The CIBA section in the profile does not mention "aud" but is does not need to because "aud" is defined in the client credentials section and all CIBA request must be authenticated using client credentials.
|
I may not have explained myself well enough, but I think you are missing the point I was trying to make with my comment in #226. Also, be careful not to mix up the client credentials grant, the client authentication methods and signed authentication requests. |
Let's take a step back and talk about this PR which says:
The Authorization Endpoint is the endpoint where the signed authentication request is sent to, right?
So, for this PR we are in option (b).
The intended interaction endpoint relevant for this PR is the authorization_endpoint and this PR "explicitely states" its intention by putting that URL into aud. Maybe you could write create a review and write a suggestion? I think this PR follows the recommendation of RFC9101. No, I am not mixing Client Authentication with Client Credentials Grant. I think there we are following the recommendation of RFC9101 and in this PR we do so as well. |
Well, when you read this sentence, it looks like a few things got mixed up.
First, you say "Client Credentials" section, which points to a section called "Client Authentication", so the naming thing could be misleading, as there is another section called "Client Credentials Flow", which is intended to describe definitions for Client Credentials Grant. Then you say that in CIBA there is no need to specify anything about aud because it is defined in the Client Credentials section (referring to the Client Authentication section). But those definitions refer to the definition of aud for the client_assertion, which is NOT the signed request object assertion. The same definitions do not necessarily apply to the signed request object unless the profile explicitly says so. And that will lead us to have same aud value for different assertions which is something that we want to avoid as described by @mhfoo here
So when in CIBA, the profile says nothing about the aud for the signed object request. A profile reader should just go to the CIBA standard as the profile does not define anything on top of it for signed request objects. And if you look at the CIBA standard, it says that the aud claim MUST contain the value of the Issuer Identifier for the OP, which identifies the Authorisation Server as the intended audience. Will we then define a different aud value for the signed request object in Auth Code Flow and CIBA? |
So are we going to ignore what RFC9101 says in section 4 "Request Object" in favour of section 10.3 "Explicit Endpoints"? You never say anything about that, because the Request Object section in RFC9101 says:
And will we also ignore OIDC Specification Section 6.1 "Passing a Request Object by Value" in favour of section 10.3 "Explicit Endpoints" of RFC9101? It says the following:
I could be wrong, of course, so let me know... |
RFC9101 seems not to be very consistent. I think that is not ignoring RFC9101 but following the stronger advice from the RFC9101 Security Considerations.
There the issuer could be for example https://issuer.example.org/ The advise from the RFC9101 security consideration advises to explicitly name the endpoint. Example RFC8114 metadata: In the Client Authentication section we follow that security advice. If all assertions, whether they are used for Client Credentials Grant or Signed Request Object used "issuer" from the server's metadata then a mixup is more likely. Therefore in Client Authentication we defined the explicit endpoint to be used. This PR does the same. I would not call that "ignoring". CAMARA deliberately RECOMMENDS the more secure option. |
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.
@AxelNennker @mhfoo @garciasolero @sebdewet @eric-murray @shilpa-padgaonkar Considering all the arguments and explanations, we at Telefónica will accept the existing text for the aud and sub claims. We are only proposing now one change to the current text to make us feel comfortable in approving the PR on our side. Change the reference to RFC9101 instead of OIDC.
Co-authored-by: Jesús Peña García-Oliva <jesus.penagarcia-oliva@telefonica.com>
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
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
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.
@mhfoo @garciasolero @sebdewet @eric-murray @shilpa-padgaonkar
Which issue(s) this PR fixes:
Fixes #205
additional context
new version of the merged #226 after reverting the merge