feat: [#628] PAR implementation #660

vivshankar · 2022-03-26T08:13:41Z

This PR implements RFC9126 - Pushed Authorization Request.

It introduces the following:

New PushedAuthorizeEndpointHandlers
Request and response writer handlers
New PARStorage interface
New functions for PAR lifecycle added to OAuth2Provider interface and corresponding implementation in Fosite
New default handler for PAR called PushedAuthorizeHandler that complies with the PushedAuthorizeEndpointHandler interface

Checklist

I have read the contributing guidelines
and signed the CLA.
I have referenced an issue containing the design document if my change
introduces a new feature.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added necessary documentation within the code base (if
appropriate).

Further comments

N/A

…st object

aeneasr · 2022-04-11T22:00:33Z

Just fyi, currently on vacation and not able to review this most likely in the next 2-3 weeks!

aeneasr

Thank you very much, I have done a preliminary review of the PR. I still need to read the spec very carefully to see what we can and can't set as part of PAR.

What would be immensely helpful is if you could add code comments to the sections that specify things like "this value MUST be set" or similar. That way, we always know what values are set and why according to the spec.

I'm also not 100% sure how far we need to go with validation here? Do we validate e.g. redirect URLs as part of PAR, or do we validate them when we actually execute the PAR request? Or what about response types? I saw that the PAR handler checks e.g. scope and audience, but it's missing (unless I overlooked it) further validation steps.

Finally, it's not quite clear to me how an exemplary HTTP handler would look like - maybe you could add such a handler to ory/fosite-example?

Thank you so much!

aeneasr · 2022-04-14T19:26:21Z

handler/par/flow_pushed_authorize.go

+	defaultPARKeyLength = 32
+)
+
+var b64 = base64.URLEncoding.WithPadding(base64.NoPadding)


Is this up to us, or is it specified in a spec?

I took a look at the spec and nothing specifically references the format of the reference value, it only references entropy requirements. Here is the specific section that discusses it (removed an irrelevant paragraph):

2.2. Successful Response:

If the verification is successful, the server MUST generate a request
URI and provide it in the response with a "201" HTTP status code.
The following parameters are included as top-level members in the
message body of the HTTP response using the "application/json" media
type as defined by [RFC8259].

request_uri
The request URI corresponding to the authorization request posted.
This URI is a single-use reference to the respective request data
in the subsequent authorization request. The way the
authorization process obtains the authorization request data is at
the discretion of the authorization server and is out of scope of
this specification. There is no need to make the authorization
request data available to other parties via this URI.

The format of the "request_uri" value is at the discretion of the
authorization server, but it MUST contain some part generated using a
cryptographically strong pseudorandom algorithm such that it is
computationally infeasible to predict or guess a valid value (see
Section 10.10 of [RFC6749] for specifics). The authorization server
MAY construct the "request_uri" value using the form
"urn:ietf:params:oauth:request_uri:<reference-value>" with
"<reference-value>" as the random part of the URI that references the
respective authorization request data.

10.10. Credentials-Guessing Attacks:

The authorization server MUST prevent attackers from guessing access
tokens, authorization codes, refresh tokens, resource owner
passwords, and client credentials.

The probability of an attacker guessing generated tokens (and other
credentials not intended for handling by end-users) MUST be less than
or equal to 2^(-128) and SHOULD be less than or equal to 2^(-160).

The authorization server MUST utilize other means to protect
credentials intended for end-user usage.

Thus the use of github.com/ory/fosite/token/hmac.RandomBytes(32) is correct and exceeds the entropy requirements at 2^(-256). Though I suspect you're asking about the encoding.

It's just a state type value. I don't really see a need to make this configurable as long as it meets the spec requirements, which it does.

I agree, what I wanted to know also was whether the vocabulary of B64 without padding (I think it's [a-zA-Z0-9+/]+) matches the one defined in the RFC

The charset isn't dictated. It just requires the entropy for which I use the random number generator as @james-d-elliott indicated.

aeneasr · 2022-04-14T19:27:10Z

handler/par/flow_pushed_authorize.go

+func (c *PushedAuthorizeHandler) HandlePushedAuthorizeEndpointRequest(ctx context.Context, ar fosite.AuthorizeRequester, resp fosite.PushedAuthorizeResponder) error {
+	storage, ok := c.Storage.(fosite.PARStorage)
+	if !ok {
+		return errorsx.WithStack(fosite.ErrServerError.WithHint("Invalid storage type"))


Could we return a better error here - such as OAuth2 storage provider does not support Pushed Authorization Requests?

Yes, we can do that. The expectation would be to never hit this state anyway. This would be a major misconfiguration, where the service is writing a HTTP handler calling into the PAR handlers (NewPushAuthorizationRequest etc.) without actually implementing the storage interface. This was really more of a safety net.

Makes sense! Good error messages can help tremendously when debugging, so even if this a coding error I think it just gives that extra 1% better developer experience :)

aeneasr · 2022-04-14T19:29:56Z

handler/par/flow_pushed_authorize.go

+	return nil
+}
+
+func (c *PushedAuthorizeHandler) secureChecker() func(*url.URL) bool {


I would feel more comfortable with a signature of secureChecker(*url.URL) bool to avoid mistakes where the inner function is not properly being invoked because the caller messed up.

Ok let me take a look. I think I copied this from somewhere else in Fosite.

authorize_request_handler.go

aeneasr · 2022-04-14T19:32:29Z

authorize_request_handler.go

+	// Reject the request if the "request_uri" authorization request
+	// parameter is provided.
+	if requestURI, _ := claims["request_uri"].(string); isPARRequest && requestURI != "" {
+		return errorsx.WithStack(ErrInvalidRequestObject.WithHint("The request must not contain 'request_uri'."))


Clarify the error message a bit:

Pushed Authorization Requests can not contain the 'request_uri' parameter.

Ok, I can make this change.

aeneasr · 2022-04-14T19:34:54Z

authorize_request_handler.go

+	request.RedirectURI = parRequest.GetRedirectURI()
+	request.ResponseTypes = parRequest.GetResponseTypes()
+	request.State = parRequest.GetState()
+	request.ResponseMode = parRequest.GetResponseMode()


Could you point to where these values are also set so that we ensure we did not forget setting some important ones? Could you please also point to the RFC section (and maybe add them as a comment) that state which parameters can be set as part of PAR?

Link to RFC section

A client sends the parameters that comprise an authorization request directly to the PAR endpoint. A typical parameter set might include: client_id, response_type, redirect_uri, scope, state, code_challenge, and code_challenge_method as shown in the example below. However, the pushed authorization request can be composed of any of the parameters applicable for use at the authorization endpoint, including those defined in [[RFC6749](https://www.rfc-editor.org/rfc/rfc9126.html#RFC6749)] as well as all applicable extensions. The request_uri authorization request parameter is one exception, and it MUST NOT be provided.

These are set in the call to NewAuthorizeRequest from within the NewPushedAuthorizationRequest.

aeneasr · 2022-04-14T19:35:33Z

authorize_request_handler.go

+
+	// validate the clients match
+	if clientID != request.GetClient().GetID() {
+		return false, errorsx.WithStack(ErrInvalidRequest.WithHint("The 'client_id' must match the pushed authorization request."))


The 'client_id' must match the one sent in the pushed authorization request.

aeneasr · 2022-04-14T19:36:25Z

authorize_request_handler.go

+			// No need to continue
+			return request, nil
+		} else if f.EnforcePushedAuthorization {
+			return request, errorsx.WithStack(ErrInvalidRequest.WithHint("Pushed authorization request is enforced."))


Pushed Authorization Requests are enforced but no such request was sent.

fosite.go

handler/par/flow_pushed_authorize.go

aeneasr · 2022-05-06T08:51:24Z

@vivshankar just wanted to check in and see if you still have appetite to get this merged :)

vivshankar · 2022-05-06T09:39:15Z

@aeneasr Yes. Just back from travel. I will look at this late next week. I responded to some of your comments. If you are happy with the ones where I have answered the question, perhaps you could mark those resolved. Please leave the ones that need changes from me unresolved, so I remember to fix those.

aeneasr · 2022-05-06T09:51:09Z

Thank you for the update @vivshankar - glad to have you back! I will look at the comments as soon as possible

vivshankar · 2022-05-06T12:21:25Z

@aeneasr I have addressed the review comments (guess it didn't need to wait till next week :-) )

Here is an example of how a HTTP handler could be written. This is similar or same as the authorize HTTP handler func.

authorizeRequest, err := oauth2Provider.NewPushedAuthorizeRequest(ctx, r)
if err != nil {
  oauth2Provider.WritePushedAuthorizeError(rw, authorizeRequest, err)
  return
}

// Next we create a response
resp, err := oauth2Provider.NewPushedAuthorizeResponse(ctx, authorizeRequest, session)
if err != nil {
  oauth2Provider.WritePushedAuthorizeError(rw, authorizeRequest, err)
  return
}

oauth2Provider.WritePushedAuthorizeResponse(rw, authorizeRequest, resp)

aeneasr

Thank you! Sorry for the long review times :(

Can you please add an integration tests for this feature? Since we're adding a completely new flow and endpoint it is required to ensure the code works as intended.

You can find an example here: https://github.com/ory/fosite/blob/master/integration/authorize_code_grant_test.go

aeneasr · 2022-05-24T08:30:09Z

And also add an example to: github.com/ory/fosite-example :)

vivshankar · 2022-05-28T04:04:05Z

@aeneasr Not entirely clear what's failing with the linter. I have completed the changes as requested. I will add to fosite-example in a couple of days.

aeneasr · 2022-05-31T15:38:47Z

The URL got defunked, I'll fix it on master quickly

…st object

vivshankar · 2022-06-14T12:08:00Z

@aeneasr Will you be able to review the latest changes and we can close this item? I am happy to contribute the new config provider for PAR as well.

…o vivshankar-628-par

vivshankar · 2022-06-25T07:14:13Z

@aeneasr I have now made the changes based on the Configurator. Can you please review?

aeneasr

Thank you! I have some comments left but overall this looks quite good!

pushed_authorize_request_handler.go

authorize_request_handler_oidc_request_test.go

aeneasr · 2022-07-04T10:34:30Z

authorize_request_handler.go

+	clientID := r.Form.Get("client_id")
+
+	storage, ok := f.Store.(PARStorage)
+	if !ok {


We check on multiple occasions that the config or store implement PAR. We could use a single error var PARNotImplemented = ... and reuse that error instead of writing new errors for every check

pushed_authorize_request_handler.go

integration/pushed_authorize_code_grant_test.go

…g error messages

mchristophe · 2022-07-13T11:39:02Z

Hello everyone! Thanks for this PR and bringing this topic. This is exactly what I was needing. I am using Ory Hydra (which is great btw, thanks for the hard work!) and I was missing the PAR notion. Hope we will all be able to test it soon on our implementation of Hydra.

mchristophe · 2022-07-14T12:55:58Z

I have another question regarding this implementation.
There are currently 2 topics regarding the parameters sent to the /authorize endpoint and some concerns regarding how it works today, especially within OpenBanking world where clients are asked to sign confidential transactions. There are ongoing discussions about how we should rethink the scope notion to be more granular, making them more dynamic.
While waiting for a proper answer from IETF, 2 topics regarding the subject:

The OAuth 2.0 Authorization Framework: JWT-Secured Authorization Request (JAR)
RFC 9101: instead of sending the parameters, goal would be to provide a request object (JWT signed) sent to the AS, or sent as reference (to a place reachable by the AS)
OAuth 2.0 Rich Authorization Requests: DRAFT new parameter authorization_details that is used to carry fine-grained authorization data in OAuth messages

The latest one would be a perfect fit for the PAR integration, while the 1st one would be a quick solution to manage custom parameters as secured as possible within financial sector.

My question is: could we join forces to proactively work on integrating those features from IETF ? I know the second one is in draft status, but we could already maybe analyse how it would fit into fosite ?

Thanks for your feedback :)

vivshankar · 2022-07-14T13:37:23Z

@mchristophe I can take a crack because we seem to be on the same journey.

JAR: Fosite already supports signed request objects. So you are covered there. That format is also supported with the PAR implementation, so a client can send the signed request object as part of the PAR payload. I have my own extensions here that enforces the request object to contain all parameters other than client credentials, which is a requirement for FAPI 1.0 Advanced Final (if that is a concern for you).
Rich authorization request is something I have been paying close attention to and is a major advancement to capture fine-grained consents. In my personal opinion, the authorization_details make perfect sense for expressive consent statements, while scopes continue to be good enough for course grained consent statements. It is however still a draft, last I checked, and ory (understandably) waits for it to become a RFC. This is the same reason DPoP hasn't been implemented even though DPoP is the only available solution to enforce sender-constrained access tokens for SPAs.

mchristophe · 2022-07-15T06:26:42Z

@vivshankar : Question to finally clear my mind on that. When something is possible on Fosite (the JAR in this matter), does it mean that it is available on Hydra as well ? I know Hydra is an implementation of Fosite, so I guess the answer is yes. But the question popped up in my head while reading the discussion here.
Regarding your second point, I asked the authors of RAR draft why they were not using scopes instead of types within the json object definition (authorization_details). I have the feeling that doing so would assure backward compatibility w/ old oAuth implementation w/o too much effort. In the meantime, I guess I could already use that kind of new parameter within the request object and do my own check

vivshankar · 2022-07-15T06:31:37Z

@mchristophe I expect so but since I don't work in ory or on Hydra, I will defer to @aeneasr to answer that.

aeneasr · 2022-07-19T11:52:55Z

Thank you @vivshankar - great answer!

@vivshankar : Question to finally clear my mind on that. When something is possible on Fosite (the JAR in this matter), does it mean that it is available on Hydra as well ? I know Hydra is an implementation of Fosite, so I guess the answer is yes. But the question popped up in my head while reading the discussion here.

Most of the times fixes land automatically in Ory Hydra. Here this is however not the case, as it requires implementation of additional endpoints and business logic. That in turn requires e2e tests, API specs, docs, ...

From maintainer side, this topic is far back in the list of priorities for Ory Hydra to support as we're focusing on v2 and releasing ory hydra to ory cloud

aeneasr

Thank you for your perseverance! This was not an easy change to get merged and with other priorities coming in between I was not able to be as responsive as I wish I could have been. That's the price once you go professional, you don't have as much time any more to be on GitHub :/

A job well done - looking forward to see this in prod!

mchristophe · 2022-10-11T06:57:53Z

Hello @aeneasr , thanks for your job! Indeed a very nice feature nicely handled. I did not understand your answer to my last question: does this mean PAR will be part of the next version of Hydra ? Sorry for asking this again, but I want to clear that out :). Thanks

maricavor · 2024-09-02T18:23:44Z

Hello, we are using Hydra version 2.2.0, but no PAR flow there, which version will support PAR flow?

feat: [ory#628] PAR implementation

f9e075b

vivshankar requested a review from aeneasr as a code owner March 26, 2022 08:13

vivshankar added 3 commits March 26, 2022 16:16

feat: Added the PAR handler and fixed a lint error

a4ca02e

fix: [ory#628] Validate that request_uri is not included in the reque…

bc679eb

…st object

fix: [ory#628] Error code should be invalid_request_object

0b255d6

aeneasr requested changes Apr 14, 2022

View reviewed changes

vivshankar added 2 commits May 6, 2022 20:09

Merge branch 'master' of github.com:ory/fosite into vivshankar-628-par

2966302

fix: [ory#628] Fixed messages and added test

17b394c

aeneasr self-requested a review May 18, 2022 12:26

aeneasr self-assigned this May 18, 2022

aeneasr requested changes May 20, 2022

View reviewed changes

test: [ory#628] Added PAR integration test

c1a97c0

doc: Added reference to the RFC

0d0fdcd

vivshankar added 7 commits May 31, 2022 18:25

feat: [ory#628] PAR implementation

1c6d3b8

feat: Added the PAR handler and fixed a lint error

e308949

fix: [ory#628] Validate that request_uri is not included in the reque…

fa21602

…st object

fix: [ory#628] Error code should be invalid_request_object

c6cfc66

fix: [ory#628] Fixed messages and added test

ec00439

test: [ory#628] Added PAR integration test

96fe9d6

doc: Added reference to the RFC

7e7319a

aeneasr force-pushed the vivshankar-628-par branch from 0d0fdcd to 7e7319a Compare May 31, 2022 16:25

vivshankar added 3 commits June 25, 2022 11:19

Merge branch 'vivshankar-628-par' of github.com:vivshankar/fosite int…

962698a

…o vivshankar-628-par

fix: [ory#628] Changes to adopt the Configurator interface pattern

de4888a

fix: Minor error message change

ef94374

aeneasr requested changes Jul 4, 2022

View reviewed changes

aeneasr and others added 3 commits July 11, 2022 11:24

chore: code review

850ff16

fix: [ory#628] Add more error checking in tests and standardize confi…

9bfe344

…g error messages

fix: [ory#628] Add more error checking in tests and standardize confi…

5a68b42

…g error messages

aeneasr added 2 commits July 19, 2022 13:58

chore: remove empty method check

49729fe

chore: improve error message

da3dab6

aeneasr approved these changes Jul 19, 2022

View reviewed changes

aeneasr merged commit 3de78db into ory:master Jul 19, 2022

feat: [#628] PAR implementation #660

feat: [#628] PAR implementation #660

Conversation

vivshankar commented Mar 26, 2022

Related Issue or Design Document

Checklist

Further comments

aeneasr commented Apr 11, 2022

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-d-elliott Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented May 6, 2022

vivshankar commented May 6, 2022 • edited Loading

aeneasr commented May 6, 2022

vivshankar commented May 6, 2022

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr commented May 24, 2022

vivshankar commented May 28, 2022

aeneasr commented May 31, 2022

vivshankar commented Jun 14, 2022

vivshankar commented Jun 25, 2022

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchristophe commented Jul 13, 2022

mchristophe commented Jul 14, 2022 • edited Loading

vivshankar commented Jul 14, 2022

mchristophe commented Jul 15, 2022

vivshankar commented Jul 15, 2022

aeneasr commented Jul 19, 2022

aeneasr left a comment

Choose a reason for hiding this comment

mchristophe commented Oct 11, 2022

maricavor commented Sep 2, 2024

james-d-elliott Apr 15, 2022 •

edited

Loading

vivshankar commented May 6, 2022 •

edited

Loading

mchristophe commented Jul 14, 2022 •

edited

Loading