New OAuth2 middlewares with token persistence

[ItalyPaleAle]

Fixes #2635
Replaces #2963

Currently the OAuth 2 middleware suffers from scalability issues as reported in #2635. Tokens obtained from the authorization server are stored in-memory in the Dapr runtime (a session ID is stored in a cookie on the client), so they do not persist if the runtime is restarted, and make it impossible to scale the application horizontally.

The most common solution to this problem is to store the tokens in the clients, as self-contained tokens such as JWTs; because tokens are sensitive, they are encrypted (using encrypted JWTs).

Two components

This PR rewrites the OAuth2 middleware and exposes 2 new components (which are mostly based on the same underlying implementation):

• middleware.http.oauth2.cookie (aliased to middleware.http.oauth2 for backwards-compatibility)
  • Just like today's component, stores the JWT in a cookie that is passed to the client.
  • After successfully authenticating, users are automatically redirected to the endpoint they were visiting at the beginning
• middleware.http.oauth2.header:
  • Requires the token (issued by Dapr and not by the IdP!) to be present in the Authorization header
  • When users authenticate successfully, the client sees a response with a JSON body containing a single key Authorization and the value of the Authorization header that clients must pass

Why 2 components?

The reason why we have two flavors of the component is due to the issue described in #2963. TLDR: tokens returned by Azure AD can be too large to be stored in a cookie.

However, using headers is not a perfect 1:1 replacement:

• Cookies can set in a 302 response while the client is redirected to the original endpoint that was invoked. We cannot do that with headers
• Cookies are handled automatically by browsers and other clients, while headers must always be handled manually

Token encryption key

Because cookies now store the token encrypted, users need to provide an encryption key via the new metadata property tokenEncryptionKey:

• This property is required.
• For backwards-compatibility, in Dapr 1.12 this property is optional and, if missing, a key will be randomly-generated by the runtime. When this happens, the key is ephemeral so tokens do not survive a restart of the Dapr runtime and horizontal scaling is not supported - this is pretty much what happens in Dapr 1.11 today! I have opted for allowing this behavior in Dapr 1.12 to offer a smoother transition period (even if the component is in alpha stage).
• Technically speaking, tokenEncryptionKey is not the actual encryption key used: the Dapr runtime deterministically derives both a signing key and an encryption key from that

Future work

• When Allow (middleware) components to define internal actors dapr#6538 is implemented, we can use actors (if enabled) to store tokens when they're too large for a cookie. In this case, if the cookie is small enough, it's sent to the client as-is. Otherwise, Dapr stores the token in an actor's state, and then sends a session ID as cookie to the client; this is done transparently only for tokens that are too large to be included in cookies.
• Add certification tests so the component(s) can be made stable. See Certification test for middleware.http.oauth2 #2621 .
  • Note that because this PR is essentially a complete rewrite, the components cannot be made stable until Dapr 1.13 at the earliest, even with certification tests
• The middleware middleware.http.oauth2clientcredentials has the same issues with scalability as the OAuth2, and needs to be updated as well (and perhaps we can re-use the shared implementation of this middleware).

[artursouza]

I remember the discussing being around keeping the existing component but having a flag instead to change the behavior. Any reason for the change of course?

[ItalyPaleAle]

I remember the discussing being around keeping the existing component but having a flag instead to change the behavior. Any reason for the change of course?

I have sent a message on Discord this morning saying that I thought it made more sense to have 2 components since the behavior is very different: one redirects, one doesn't; one works with browsers (because of cookies), the other one doesn't. I asked if anyone had objections and didn't get any.

PS: the existing component is still around and in 1.12 existing configurations will work without changes (however startign in 1.13 setting tokenEncryptionKey will be required).

[yaron2]

I have sent a message on Discord this morning saying that I thought it made more sense to have 2 components since the behavior is very different: one redirects, one doesn't; one works with browsers (because of cookies), the other one doesn't. I asked if anyone had objections and didn't get any.

Going forward I advise to not equate 5 hours of no responses on Discord with an explicit agreement from everyone.

one works with browsers (because of cookies), the other one doesn't

That is not accurate and does not justify a new codebase with the additional maintenance overhead that it brings. The existing component does work with browsers and cookies, but as you pointed out recent changes in Azure AD breaks this scenario for that given provider.

one redirects, one doesn't

This change in behavior can easily be captured in the existing component without requiring an entirely new one.

[artursouza]

Can we keep the previous decision? It seems overkill to duplicate the component in this case. Even when we did duplicate it, like in etcd, the code is mostly shared, just the param at registration is different.

[ItalyPaleAle]

Can we keep the previous decision? It seems overkill to duplicate the component in this case. Even when we did duplicate it, like in etcd, the code is mostly shared, just the param at registration is different.

Please review the code. Just like with etcd, the code is almost entirely shared. I am the first one that doesn't want to duplicate code :)

There just happens to be 2 folders, rather than 2 constructors in the same folder like for etcd, because I cannot put 2 metadata.yaml in the same folder

[ItalyPaleAle]

Closing in favor of #2967