Support Pushed Auth Request by RFC-9126 #768
Conversation
|
Thanks for your contribution @migregal we'll have a look at your PR in our next sprint. |
migregal
force-pushed
the
issue/10239
branch
7 times, most recently
from
9aa0e05 to
ec722cb
Compare
…itadel#759) Bumps the go_modules group with 1 update: [github.com/go-chi/chi/v5](https://github.com/go-chi/chi). Updates `github.com/go-chi/chi/v5` from 5.2.1 to 5.2.2 - [Release notes](https://github.com/go-chi/chi/releases) - [Changelog](https://github.com/go-chi/chi/blob/master/CHANGELOG.md) - [Commits](go-chi/chi@v5.2.1...v5.2.2) --- updated-dependencies: - dependency-name: github.com/go-chi/chi/v5 dependency-version: 5.2.2 dependency-type: direct:production dependency-group: go_modules ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
|
||
| // pushedAuthorizationRequestAuthExtention implements first step of PAR (RFC-9126) extention, | ||
| // providing request_uri for auth redirect. | ||
| func pushedAuthorizationRequestAuthExtension(url string, query url.Values) (string, error) { |
There was a problem hiding this comment.
Yeah, I know that's a little bit dirty and can be done better
Open to discussion on this logic because it seems like therre is no good way to wrap existing x/oauth2 implementation
Looking forward for golang/go#65956 - once it'll be accepted to stdlib, we'll be able to get rid of a mess introduced in current PR
muhlemmer
left a comment
muhlemmer
left a comment
There was a problem hiding this comment.
I just left some comments. I think the PR can be greatly simplified by not providing the Storage and Provider methods for Pushed auth requests.
After that's cleaned up I'll review the remainder.
| type PARStorage interface { | ||
| // StorePAR stores a new pushed authorization request in the database. | ||
| // RequestURI will be used by the user to complete the login flow and must be unique. | ||
| // | ||
| // Note that implementers of this interface must make sure that codes of | ||
| // expired authentication flows are purged, after some time. | ||
| StorePAR(ctx context.Context, requestURI string, request *oidc.AuthRequest, expires time.Time) error | ||
|
|
||
| // GetPARState returns the current state of the pushed authorization request flow in the database. | ||
| // Returned structure contains original auth request that should be used in auth API. | ||
| GetPARState(ctx context.Context, requestURI string) (*oidc.AuthRequest, error) | ||
| } |
There was a problem hiding this comment.
As this is for Zitadel, we are moving away from using the Storage interfaces. So I would propose dropping this additional / optional storage interface and define it for Server only: #440
There was a problem hiding this comment.
Hello there!
@muhlemmer, I'm not sure if this have to be done in current module architecture, actually
There is big issue with authorize function right here since it's using LegacyServer for backward compatibility. To provide PAR implementation, OAuth2.0 Provider have to handle Auth request in a very specific way, accepting new request_uri value instead base request.
For sure, we can start major refactoring, replacing LegacyServer usage in both new method and old authorize one, but it seems like a separate issue to me, that has to be done with LegacyServer removal later.
pkg/op/server_legacy.go
Outdated
| func (s *LegacyServer) PushedAuthorizationRequest( | ||
| ctx context.Context, r *Request[oidc.PARRequest], | ||
| ) (*Response, error) { | ||
| ctx, span := tracer.Start(ctx, "LegacyServer.PushedAuthorizationRequest") | ||
| defer span.End() | ||
|
|
||
| res, err := createPushedAuthorizationRequest(ctx, r.Data, s.provider) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return NewResponseWithStatus(http.StatusCreated, res), nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Does not need to be handled by LegacyServer, because it is a new feature, not one that needs to be kept as backward compatible.
There was a problem hiding this comment.
I think I'm missing smth right here.
What would the ideal solution look like for u? Just move this logic directly to werServer-attached function, without forcing Server interface to implement it?
It's just a little bit disorienting for me since u're proposing removing Storage interfaces and moving it's methods to the Server one. Look's like there still will be a need in LegacyServer patches since it has to implement Server interface
There was a problem hiding this comment.
@muhlemmer, can u share your thoughts on this, please?
There was a problem hiding this comment.
LegacyServer embeds the UnimplementedServer, from which it inherits all the methods needed to implement the interface. Missing methods in the LegacyServer will just return an unimplemented error.
There was a problem hiding this comment.
Just tried to implement feature this way and it seems like a big enough refactoring issue.
Due to direct usage of Legacy server in Auth flow, there is problem with separating logic - LegacyServer has to know about PAR storage and feature itself, to correctly impl Auth handler. Alternative way is to refactor Auth logic and move it to webServer as well, but this will break backward compatibility.
Waiting for your answer here, but I think, the best way to impl PAR is to reuse LegacyServer for know
There was a problem hiding this comment.
What do you mean by refactoring? Just don;t implement the method on LegacyServer and do not extend Storage. If anything it's less code.
muhlemmer
added
the
waiting
|
|
||
| // AuthRequest according to: | ||
| // https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest | ||
| type AuthRequest struct { |
There was a problem hiding this comment.
Adding omitempty for each field below is required to minimize stored values size since it's being stored via JSON-serialization. Can be optimized even better in future by adding protobuf or msgpack serialization in this scenario.
|
Please take note of #785 |
5 participants
Follows zitadel/zitadel#10239
Definition of Ready