proposal(op): new server interface to replace storage

[muhlemmer]

This issue proposes a new interface Server in the op package. It's purpose is to obsolete the current Storage interface and Provider type over time.

Motivation

Community feedback

From the community (and at zitadel we feel the same pain):

• Lots op type assertions: https://github.com/zitadel/oidc/discussions/377. Due to the various storage implementation
• The need to customize certain parts, where usually the storage is the limitation.
  • https://github.com/zitadel/oidc/discussions/409
  • https://github.com/zitadel/oidc/issues/393
  • https://github.com/zitadel/oidc/issues/324
  • https://github.com/zitadel/oidc/issues/325
  • https://github.com/zitadel/oidc/issues/326
  • https://github.com/zitadel/oidc/issues/384

Maintainability

In the current design there is a mix of business logic and the implementation of the OpenID connect / Oauth2 standards. Over time this resulted in overly complex code and duplication of code which is hard to understand and maintain.

Some examples

https://github.com/zitadel/oidc/blob/daf82a5e041cb1a2279d7e6f8f56d64dd58b4371/pkg/op/token_code.go#L64-L103 https://github.com/zitadel/oidc/blob/daf82a5e041cb1a2279d7e6f8f56d64dd58b4371/pkg/op/token_refresh.go#L89-L129

Performance

Due to the business logic in OIDC and its reliance on the Storage interface. We found that there are multiple calls to certain methods. For example, we found for the token endpoint there are cases where GetUserByID is called 3 times. If we would sperate the business logic from implementation we could optimized this by a single call. Basically, business logic shouldn't be scope of the OIDC framework.

Proposed interface

It will be the scope of this framework to:

• Route requests to their methods;
  • Parse request data into a type that represents the model from the standards;
  • Validate required fields in the request;
  • Provide utility functions that helps implementations build the response;

It will be the scope of the implementer

• Handle the request data to obtain or modify state in their storage / database;
• Implement the business logic that is needed to build the response;

// Server describes the interface that needs to be implemented to serve
// OpenID Connect and Oauth2 standard requests.
//
// Methods are called after the HTTP route is resolved and
// the request body is parsed into the Request's Data field.
// When a method is called, it can be assumed that required fields,
// as described in their relevant standard, are validated already.
// The Response Data field may be of any type to allow flexibility
// to extend responses with custom fields. There are however requirements
// in the standards regarding the response models. Where applicable
// the method documentation gives a recommended type which can be used
// directly or extended upon.
type Server interface {
    // Health should return a status of "ok" once the Server is listining.
    // The recommended Response Data type is [Status].
    Health(context.Context, *Request[struct{}]) (*Response, error)

    // Ready should return a status of "ok" once all dependecies,
    // such as database storage are ready.
    // An error can be returned to explain what is not ready.
    // The recommended Response Data type is [Status].
    Ready(context.Context, *Request[struct{}]) (*Response, error)

    // Discovery return the OpenID Provider Configuration Information for this server.
    // https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig
    // The recommended Response Data type is [oidc.DiscoveryConfiguration].
    Discovery(context.Context, *Request[struct{}]) (*Response, error)

    // Authorize initiates the authorization flow and redirects to a login page.
    // See the various https://openid.net/specs/openid-connect-core-1_0.html
    // authorize endpoint sections (one for each type of flow).
    Authorize(context.Context, *Request[oidc.AuthRequest]) (*Redirect, error)

    // AuthorizeCallback? Do we still need it?

    // DeviceAuthorization initiates the device authorization flow.
    // https://datatracker.ietf.org/doc/html/rfc8628#section-3.1
    // The recommended Response Data type is [oidc.DeviceAuthorizationResponse].
    DeviceAuthorization(context.Context, *Request[oidc.DeviceAuthorizationRequest]) (*Response, error)

    // VerifyClient is called on most oauth/token handlers to authenticate,
    // using either a secret (POST, Basic) or assertion (JWT).
    // If no secrets are provided, the client must be public.
    // This method is called before each method that takes a
    // [ClientRequest] argument.
    VerifyClient(context.Context, *Request[ClientCredentials]) (Client, error)

    // CodeExchange returns Tokens after an authorization code
    // is obtained in a succesfull Authorize flow.
    // It is called by the Token endpoint handler when
    // grant_type has the value authorization_code
    // https://openid.net/specs/openid-connect-core-1_0.html#TokenEndpoint
    // The recommended Response Data type is [oidc.AccessTokenResponse].
    CodeExchange(context.Context, *ClientRequest[oidc.AccessTokenRequest]) (*Response, error)

    // RefreshToken returns new Tokens after verifying a Refresh token.
    // It is called by the Token endpoint handler when
    // grant_type has the value refresh_token
    // https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokens
    // The recommended Response Data type is [oidc.AccessTokenResponse].
    RefreshToken(context.Context, *ClientRequest[oidc.RefreshTokenRequest]) (*Response, error)

    // JWTProfile handles the OAuth 2.0 JWT Profile Authorization Grant
    // It is called by the Token endpoint handler when
    // grant_type has the value urn:ietf:params:oauth:grant-type:jwt-bearer
    // https://datatracker.ietf.org/doc/html/rfc7523#section-2.1
    // The recommended Response Data type is [oidc.AccessTokenResponse].
    JWTProfile(context.Context, *Request[oidc.JWTProfileGrantRequest]) (*Response, error)

    // TokenExchange handles the OAuth 2.0 token exchange grant
    // It is called by the Token endpoint handler when
    // grant_type has the value urn:ietf:params:oauth:grant-type:token-exchange
    // https://datatracker.ietf.org/doc/html/rfc8693
    // The recommended Response Data type is [oidc.AccessTokenResponse].
    TokenExchange(context.Context, *ClientRequest[oidc.TokenExchangeRequest]) (*Response, error)

    // ClientCredentialsExchange handles the OAuth 2.0 client credentials grant
    // It is called by the Token endpoint handler when
    // grant_type has the value client_credentials
    // https://datatracker.ietf.org/doc/html/rfc6749#section-4.4
    // The recommended Response Data type is [oidc.AccessTokenResponse].
    ClientCredentialsExchange(context.Context, *ClientRequest[oidc.ClientCredentialsRequest]) (*Response, error)

    // DeviceToken handles the OAuth 2.0 Device Authorization Grant
    // It is called by the Token endpoint handler when
    // grant_type has the value urn:ietf:params:oauth:grant-type:device_code.
    // It is typically called in a polling fashion and appropiate errors
    // should be returned to signal authorization_pending or access_denied etc.
    // https://datatracker.ietf.org/doc/html/rfc8628#section-3.4,
    // https://datatracker.ietf.org/doc/html/rfc8628#section-3.5.
    // The recommended Response Data type is [oidc.AccessTokenResponse].
    DeviceToken(context.Context, *ClientRequest[oidc.DeviceAccessTokenRequest]) (*Response, error)

    // Introspect handles the OAuth 2.0 Token Introspection endpoint.
    // https://datatracker.ietf.org/doc/html/rfc7662
    // The recommended Response Data type is [oidc.IntrospectionResponse].
    Introspect(context.Context, *Request[oidc.IntrospectionRequest]) (*Response, error)

    // UserInfo handles the UserInfo endpoint and returns Claims about the authenticated End-User.
    // https://openid.net/specs/openid-connect-core-1_0.html#UserInfo
    // The recommended Response Data type is [oidc.UserInfo].
    UserInfo(context.Context, *Request[oidc.UserInfoRequest]) (*Response, error)

    // Revocation handles token revocation using an access or refresh token.
    // https://datatracker.ietf.org/doc/html/rfc7009
    // There are no response requirements. Data may remain empty.
    Revocation(context.Context, *Request[oidc.RevocationRequest]) (*Response, error)

    // EndSession handles the OpenID Connect RP-Initiated Logout.
    // https://openid.net/specs/openid-connect-rpinitiated-1_0.html
    // There are no response requirements. Data may remain empty.
    EndSession(context.Context, *Request[oidc.EndSessionRequest]) (*Response, error)

    // Keys serves the JWK set which the client can use verify signatures from the op.
    // https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata `jwks_uri` key.
    // The recommended Response Data type is [jose.JSOMWebKeySet].
    Keys(context.Context, *Request[struct{}]) (*Response, error)

    // mustImpl forces implementations to embed the UnimplementedServer for forward
    // compatibilty with the interface.
    mustImpl()
}

// Request contains the [http.Request] informational fields
// and parsed Data from the request body (POST) or URL parameters (GET).
// Data can be assumed to be validated according to the applicable
// standard for the specific endpoints.
type Request[T any] struct {
    Method   string
    URL      *url.URL
    Header   http.Header
    Form     url.Values
    PostForm url.Values
    Data     *T
}

// ClientRequest is a Request with a verified client attached to it.
// Methods the recieve this argument may assume the client was authenticated,
// or verified to be a public client.
type ClientRequest[T any] struct {
    *Request[T]
    Client Client
}

type Response struct {
    // Header map will be merged with the
    // header on the [http.ResponseWriter].
    Header http.Header

    // Data will be JSON marshaled to
    // the response body.
    // We allow any type, so that implementations
    // can extend the standard types as they wish.
    // However, each method will recommend which
    // (base) type to use as model, in order to
    // be complaint with the standards.
    Data any
}

// Redirect is a special response type which will
// initiate a [http.StatusFound] redirect.
// The Params field will be encoded and set to the
// URL's RawQuery field before building the URL.
//
// If the RawQuery contains values that need to persist,
// the implementation should parse them into Params and
// add request specific values after.
type Redirect struct {
    // Header map will be merged with the
    // header on the [http.ResponseWriter].
    Header http.Header

    URL    url.URL
    Params url.Values
}

Questions

There are still some doubts in the above design I would like to clarify:

1. AuthorizeCallback is an endpoint which is not part of the standard, but required for Zitadel's login mechanism. Should it be part of this interface? (I believe it shouldn't, nut I'm open to other opinions)
2. Allowing any in the Response object, is that the right way to go? In my original concept I used the concrete types defined in the oidc package, but figured that I would prevent flexibility if implementations need to return more. I've played with the idea of providing an Extra map field for that purpose, but that would lead into messy code trying to merge and marshal the struct and map types. In the end json.Marshal takes an interface type anyway. On the flip side is that now it is not clear what response type is required from the method signature alone. One would always need to read the comments in the interface comments.

Alternative response definitions

```go type Response[T any] struct { // Header map will be merged with the // header on the [http.ResponseWriter]. Header http.Header // Data will be JSON marshaled to // the response body. Data T Extra map[string]any } ... CodeExchange(context.Context, *ClientRequest[oidc.AccessTokenRequest]) (*Response[*oidc.AccessTokenResponse], error) ... ```

Forward compatibility

What we've learned so far is that a central interface like Storage, needs to grows over time to suit different needs. We ended up with many optional interfaces and type assertions throughout the framework. Inspired much by how the gRPC ecosystem solves this,we want to provide and UnimplementedServer implementation. The UnimplementedServer will implement all methods but will return an error describing that the particular endpoint isn't implemented on that server.

Implementations will need to embed the UnimplementedServer in their struct type, so that we can keep adding methods without breaking those implementations. This is enforced by the mustImpl() un-exported method.

How we match that with the Endpoints configuration and discovery still needs to be determined.

Unimplemented server

```go type UnimplementedServer struct{} // UnimplementedStatusCode is the statuscode returned for methods // that are not yet implemented. // Note that this means methods in the sense of the Go interface, // and not http methods covered by "501 Not Implemented". var UnimplementedStatusCode = http.StatusNotFound func unimplementedError[T any](r *Request[T]) StatusError { err := oidc.ErrServerError().WithDescription(fmt.Sprintf("%s not implemented on this server", r.URL.Path)) return StatusError{ parent: err, statusCode: UnimplementedStatusCode, } } func (UnimplementedServer) mustImpl() {} func (UnimplementedServer) Health(_ context.Context, r *Request[struct{}]) (*Response, error) { return nil, unimplementedError(r) } func (UnimplementedServer) Ready(_ context.Context, r *Request[struct{}]) (*Response, error) { return nil, unimplementedError(r) } func (UnimplementedServer) Discovery(_ context.Context, r *Request[struct{}]) (*Response, error) { return nil, unimplementedError(r) } func (UnimplementedServer) Authorize(_ context.Context, r *Request[oidc.AuthRequest]) (*Redirect, error) { return nil, unimplementedError(r) } func (UnimplementedServer) DeviceAuthorization(_ context.Context, r *Request[oidc.DeviceAuthorizationRequest]) (*Response, error) { return nil, unimplementedError(r) } func (UnimplementedServer) VerifyClient(_ context.Context, r *Request[ClientCredentials]) (Client, error) { return nil, unimplementedError(r) } func (UnimplementedServer) CodeExchange(_ context.Context, r *ClientRequest[oidc.AccessTokenRequest]) (*Response, error) { return nil, unimplementedError(r.Request) } func (UnimplementedServer) RefreshToken(_ context.Context, r *ClientRequest[oidc.RefreshTokenRequest]) (*Response, error) { return nil, unimplementedError(r.Request) } func (UnimplementedServer) JWTProfile(_ context.Context, r *Request[oidc.JWTProfileGrantRequest]) (*Response, error) { return nil, unimplementedError(r) } func (UnimplementedServer) TokenExchange(_ context.Context, r *ClientRequest[oidc.TokenExchangeRequest]) (*Response, error) { return nil, unimplementedError(r.Request) } func (UnimplementedServer) ClientCredentialsExchange(_ context.Context, r *ClientRequest[oidc.ClientCredentialsRequest]) (*Response, error) { return nil, unimplementedError(r.Request) } func (UnimplementedServer) DeviceToken(_ context.Context, r *ClientRequest[oidc.DeviceAccessTokenRequest]) (*Response, error) { return nil, unimplementedError(r.Request) } func (UnimplementedServer) Introspect(_ context.Context, r *Request[oidc.IntrospectionRequest]) (*Response, error) { return nil, unimplementedError(r) } func (UnimplementedServer) UserInfo(_ context.Context, r *Request[oidc.UserInfoRequest]) (*Response, error) { return nil, unimplementedError(r) } func (UnimplementedServer) Revocation(_ context.Context, r *Request[oidc.RevocationRequest]) (*Response, error) { return nil, unimplementedError(r) } func (UnimplementedServer) EndSession(_ context.Context, r *Request[oidc.EndSessionRequest]) (*Response, error) { return nil, unimplementedError(r) } func (UnimplementedServer) Keys(_ context.Context, r *Request[struct{}]) (*Response, error) { return nil, unimplementedError(r) } ```

Backward compatibility

When we will release the new Server interface, we wil also bring an implementation that uses the current Storage and Provider underneath. That means implementations should be able to switch over without the direct need of refactoring their code base. There might be some small breaking changes in exported functions that are currently used inside the handlers, that's why we target v3.

There is already a small proof of concept for Code Exchange and Refresh Token that ties the new Server to the old Provider here:

Legacy Server

https://github.com/zitadel/oidc/blob/c340ed9ed587f8dd7b341acff464e3f92cf104a1/pkg/op/server_legacy.go#L9-L81

Planning

We intend to implement an experimental version of this interface into the V3 release. Current users of the Provider and Storage interfaces will not directly be affected by the addition and have a choice to use the old Provider, Provider wrapped in a LegacyServer or start implementing from scratch. Following inclusion, we will have to start re-writing most functions that that Storage arguments to accept valued arguments instead. Once that is completed we will be able to start deprecating the Provider and Storage types.

General Roadmap:

• pre-v3 (breaking changes):
  • Definition of Server interface, state experimental.
  • LegacyServer implementation.
• post-v3 (feature additions):
  • Adding configuration options based on current Provider.
• pre-v4:
  • Stabilization of the Server interface
  • Removal of direct Storage calls in exported functions, use values instead.
  • Deprecation of Storage.

After Storage has been deprecated, we will stop accepting new features for it. Actual removal will depend on community needs.

---

CC: @muir @livio-a @adlerhurst

[livio-a]

Hey @muhlemmer

Great write up and proposal.

I've checked out the branch and played around, but to your questions:

1. IMO the AuthorizeCallback is not necessary anymore. It was only used because of the way the library worked with the Storage interface. I guess we can just provide a helper function to be able to return the necessary response for Code Flow (code, state, ...), reps. Implicit Flow (tokens), where the latter is the same as used for the CodeExchange response.
2. This is a really good question and what i directly asked myself as well when i saw the proposed interface. I've played around with different ideas of generics, interfaces, ... TLDR: i don't have a better approach yet. Everything i've tried either is to constrained or mostly still does not really give the implementer clear information / instructions on what is returned.

I think the easiest solution to go forward is to have a discussion today afternoon, as proposed by you.

[muir]

What's the branch name?

[livio-a]

What's the branch name?

next-server-interface

[muhlemmer]

I've just pushed with some intermediate state / WIP on implementing the LegacyServer

[muhlemmer]

Some notes after meeting with @livio-a:

VerifyClient

We shortly discussed if VerifyClient should remain a method, because it is the only method that doesn't directly a handler. The alternative would be making it a utility function in which the implementation must pass the Client.

Discussed: Because of the possibility of client_assertion (JWT) the client ID might not be known until verification is complete. As such 2 utility functions would be required: To verify the client_assertion and one to verify all other methods. Both will need to be called in each method that requires a Client verification.

The conclusion is that we will keep it as a method which can be implemented once.

Response object

Type enforcement in the response vs flexibility. With the current proposal, the return object is not enforced in any way. The Go type system also doen't provide ways to say "type that embeds another type". There could be some ways to hack something in using private interface methods. But is this the right way?

The goal must be that the interface is clear enough to implement. Yet, we want to offer flexibilty.

No definitive conclusion was made, but we lean to keep the any type for flexibility. (I did an expiriment over the weekend which I will post shortly)

Errors and status codes

Currently we have the oidc.Error type which in no way enforces a certain http Status Code. Most of the status codes are created in the AuthRequestError and RequestError functions. Storage errors are not properly evaluated and even if there was an error that had to do with the storage itself, we would mostly return a Bad Request instead of a Internal Server Error.

In the proposal branch there is now:

type StatusError struct {
    parent     error
    statusCode int
}

Which would allow implementations to signal the proper signal code for an error case.

There is a concern that @livio-a raised that some standards specifically disallow certain status codes and that we should verify those standards before fully opening up StatusError capabilites.

Conclusion: check standards.

[muhlemmer]

For the response object typing we could use a private method interface which then enforces users (mostly) to embed certain base types. I made a small example using the oidc.DiscoveryConfiguration:

Define an interface with a private method, which only oidc can implement:

package oidc

type IsDiscovery interface {
    isDiscovery()
}

type DiscoveryConfiguration struct {
// ...
}

func (*DiscoveryConfiguration) isDiscovery() {}

Use the interface a type in the Response object:

package op

type Server interface {
        Discovery(context.Context, *Request[struct{}]) (*Response[oidc.IsDiscovery], error)
}

Implementation of an extended response:

package some_impl

type CustomDiscoveryConfig struct {
    *oidc.DiscoveryConfiguration
    Foo string `json:"foo"`
}

func (s *LegacyServer) Discovery(ctx context.Context, r *Request[none]) (*Response[oidc.IsDiscovery], error) {
    return NewResponse[oidc.IsDiscovery](
        &CustomDiscoveryConfig{
            DiscoveryConfiguration: CreateDiscoveryConfig(ctx, s.provider, s.provider.Storage()),
            Foo: "bar",
        },
    ), nil
}

[muhlemmer]

Also CC @lefelys , as he implemented the token exchange. As I figure now this also largely depends on custom storage. If you have time, we would love to hear you opinion on the proposed interface https://github.com/zitadel/oidc/issues/440#issue-1881779671.

[lefelys]

It will be the scope of this framework to:

• ...
• Validate required fields in the request;
• Provide utility functions that helps implementations build the response;

The beautiful concept of current approach is abstraction of OIDC standards from user via simple Storage interface that must be implemented. It doesn't always work as expected, and very often internals must be reviewed to understand details (lack of documentation, good thing Zitadel is there as a reference) - but this is what everybody would expect from framework and improving this approach IMO is the right way to go.

In this proposal all OIDC internals are exposed to the implementer, which harms security and developer experience. From storage interface it is clear that I need to have auth request, access/refresh tokens, schema and methods to store them and retrieve. With Server interface it becomes unclear.

Maybe we can have a hybrid? Most of current storage methods can stay as they are (CRUD for core entities), and with utility functions there can be a complete Server implementation like this:

func (s Server) CodeExchange(ctx context.Context, r *ClientRequest[oidc.AccessTokenRequest]) (*Response, error) {
    resp, err := utility.CodeExchange(ctx, r)
    // mutate resp here if needed
    return resp, err
}

This is a rough example. Like you mentioned if there is unimplemented server it can be used instead of utility as core implementation, and users can rewrite only methods they need to.

Better hooks and Interceptors also can be considered as an option.