serverlessworkflow / specification

Contains the official specification for the Serverless Workflow Domain Specific Language. It provides detailed guidelines and standards for defining, executing, and managing workflows in serverless environments, ensuring consistency and interoperability across implementations.
http://serverlessworkflow.io
Apache License 2.0
726 stars 146 forks source link

Add additional authority endpoints to OAUTH2 #912

Closed cdavernas closed 2 days ago

cdavernas commented 1 month ago

What would you like to be added: Add a new property to OAUTH2 authentication, used to define additional token endpoints in order to address https://github.com/serverlessworkflow/synapse/issues/342

Why is this needed: As explained by @jaliyaudagedara in https://github.com/serverlessworkflow/synapse/issues/342, this is a requirement for many librairies when facing authority mismatch

cdavernas commented 1 month ago

@ricardozanini @JBBianchi @matthias-pichler-warrify @fjtirado What do you guys think? Can I proceed with it and open a PR?

matthias-pichler commented 1 month ago

Is this in the case where the openId connect configuration lives on a different domain than e.g. the token endpoint?

cdavernas commented 1 month ago

Yes, exactly!

matthias-pichler commented 1 month ago

Ok got it. What would a proposed solution look like? Because I guess making authority an array would mess up the openId discovery right?

On a related note: since OpenID Connect and OAuth2 are separate specifications and many OAuth APIs don't have an OIDC config shouldn't there be a way to specify e.g the token uri directly? And from the fields client.id and client.secret I guess that only the client_credentials flow is supported: what about other flows? (e.g. the legacy password flow)

Would it make sense to either:

matthias-pichler commented 1 month ago

I am actually kind of struggling with the lacking support for oauth right now.

I think a solution should at least cover the following cases:

Open ID Connect

one can definitely specify a well-known uri to a open id config.

document: {}
use:
  authentications:
    exampleOIDC:
      oauth2:
        openIdConnect: https://example.com/.well-known/openid-configuration
        trustedAuthorities:
          - example.net
        grant: client-credentials
        client:
          id: workflow-runtime
          secret: "**********"
       # audience, scopes, etc

Question @cdavernas is the domain enough to whitelist additional endpoints?

Token endpoint

document: {}
use:
  authentications:
    exampleOIDC:
      oauth2:
        tokenEndpoint: https://example.com/oauth2/token
        grant: client-credentials
        client:
          id: workflow-runtime
          secret: "**********"
       # audience, scopes, etc

Open Questions

  1. Content-Type: RFC 6749 states that the access token request body should be application/x-www-form-urlencoded encoded, however JSON is also extremely common. Should this be configurable?
  2. Client Authentication: RFC 6749 states that the client_id and client_secret can be sent via Basic authentication (client_secret_basic) or in the request body (Section 2.3.1) (client_secret_post). Some APIs only support one way: should this be configurable? What about rarer ways of client authentication like client_secret_jwt and private_key_jwt?
  3. Grant types: Should we support other grant types as well? Specifically the password grant
fjtirado commented 1 month ago

@matthias-pichler-warrify I would say that by default, if nothing is specified, for 1. and 2. both should be suppoted. Meaning that, unless otherwise specified, the runtime should accept both x-www-form-urlencoded and application/json and the client_id and client_secret can both be on Basic header or in the request body. If we are going to configure something on the file, it should be to restrict it. Regarding grant types, I think the only additional one that we should support is the password grant.

matthias-pichler commented 1 month ago

Meaning that, unless otherwise specified, the runtime should accept both x-www-form-urlencoded and application/json and the client_id and client_secret can both be on Basic header or in the request body. If we are going to configure something on the file, it should be to restrict it.

Yeah the issue is that I don't think the runtime has a way to figure out which one is supported by any given endpoint. So I think the author has to specify which is desired. So my question is what should this config look like?

For example Amazon Cognito support both client_secret_basic and client_secret_post authentication but only application/x-www-form-urlencoded bodies.

Auth0 supports both application/x-www-form-urlencoded and application/json bodies but only client_secret_post authentication.

and there exists basically any combination imaginable out there

cdavernas commented 1 month ago

And from the fields client.id and client.secret I guess that only the client_credentials flow is supported: what about other flows? (e.g. the legacy password flow)

In my personal and professional use cases, I actually use both client_credentials and token_exchange, so I'd say those two, at the very least, should be supported.

or extend the oauth2 auth type to allow more flows and specification fields (like tokenUri) that would normally come from an openId config?

Good catch! Yes, we should probably update the oauth2 authentication, or as suggested, create a new openid one.

Question @cdavernas is the domain enough to whitelist additional endpoints?

I believe it should, yes, though I never had the need to whitelist, so the question would be better addressed to @jaliyaudagedara!

Content-Type: RFC 6749 states that the access token request body should be application/x-www-form-urlencoded encoded, however JSON is also extremely common. Should this be configurable?

I believe it should, yes.

Authorization: RFC 6749 states that the client_id and client_secret can be sent via Basic authentication or in the request body (Section 2.3.1). Some APIs only support one way: should this be configurable?

Yes, IMHO it should be configurable too!

Grant types: Should we support other grant types as well? Specifically the password grant

We probably should support the legacy password grant type, indeed. Others are useless in the workflow context, AFAIK (ex: implicit)

matthias-pichler commented 1 month ago

Content-Type: RFC 6749 states that the access token request body should be application/x-www-form-urlencoded encoded, however JSON is also extremely common. Should this be configurable?

I believe it should, yes.

Authorization: RFC 6749 states that the client_id and client_secret can be sent via Basic authentication or in the request body (Section 2.3.1). Some APIs only support one way: should this be configurable?

Yes, IMHO it should be configurable too!

Agreed. My only concern is that these are going to be a lot of options then 🙈 and also kind of hard/opinionated to choose a default

In my personal and professional use cases, I actually use both client_credentials and token_exchange, so I'd say those two, at the very least, should be supported.

I didn't even know that one 😅

fjtirado commented 1 month ago

Meaning that, unless otherwise specified, the runtime should accept both x-www-form-urlencoded and application/json and the client_id and client_secret can both be on Basic header or in the request body. If we are going to configure something on the file, it should be to restrict it.

Yeah the issue is that I don't think the runtime has a way to figure out which one is supported by any given endpoint. So I think the author has to specify which is desired. So my question is what should this config look like?

For example Amazon Cognito support both client_secret_basic and client_secret_post authentication but only application/x-www-form-urlencoded bodies.

Auth0 supports both application/x-www-form-urlencoded and application/json bodies but only client_secret_post authentication.

and there exists basically any combination imaginable out there

Ok, I forgot we are the client, not the server ;) They should be both configurable. The default for the token should be url-enconded. Which Im not sure which should be the default for client id, I would say Basic header, but it just a feeling.

cdavernas commented 1 month ago

To summarize, I believe we should add the following properties:

Name Type Required Description
tokenUri uri no Specifies the endpoint to use to generate tokens, when not using OIDC
authorities uri[] no List of the URI/DNS of all trusted authorities. Maybe this should actually replace the authority property instead, with a min length of 1.
request.encoding string no The encoding of the token request.
Supported values are application/x-www-form-urlencoded and application/json.
Defaults to application/x-www-form-urlencoded.
request.in string no Defines the destination of the request parameters.
Supported values are body and header.
Defaults to body.

These are just suggestions, of course. I'm convinced we can come up with a better wording!

cdavernas commented 1 month ago

Agreed. My only concern is that these are going to be a lot of options then 🙈

I don't think that's really a problem, is it? As long as we provide default values, we can ensure that most users will be able to declare OAUTH/OIDC auth kind of brievly.

kind of hard/opinionated to choose a default

As far as I know, the defaults are well defined, aren't they? My understanding is that the default encoding is application/x-www-form-urlencoded and that the default location of client credentials is body. At least, that's what I've always used, without exception.

I didn't even know that one 😅

It's an awesome one, though, I highly recommend you take a look at it! Amongst other things, it allows a client to act on behalf of a user, which is priceless, especially in the context of a workflow execution (ex: a user "starts" the workflow, then the runtime requests a new token to act on behalf of her)

matthias-pichler commented 1 month ago

As far as I know, the defaults are well defined, aren't they? My understanding is that the default encoding is application/x-www-form-urlencoded and that the default location of client credentials is body. At least, that's what I've always used, without exception.

From the OAuth2 RFC 6749 it seems like client_secret_basic is the default 😅 It's also the first listed in the OpenID docs (which does not list them in order because client_secret_post is before client_secret_jwt). However personally I also have seen more of client_secret_post.

For encoding form encoded seems to be the default.

But yeah defaults should be fine actually 👍

List of the URI/DNS of all trusted authorities. Maybe this should actually replace the authority property instead, with a min length of 1.

I think there should be one array listing allow listed domains/uris and a separate field specifying the openid config uri. Because otherwise it would look weird and be hard to determine where to find the config

request.encoding

something like this would make sense yeah 👍 I think maybe bodyEncoding, bodyFormat or bodyContentType would be more explicit but I also like the brevity of encoding or format

request.in

This should probably be called something else and also use the official names for client authentication from the OpenID docs

maybe clientAuthentication 😅 probably not authentication. What about request.credentials?

For other grants client.id and client.secret should probably become optional as well. Where should we put username and password for the password flow?

ricardozanini commented 2 weeks ago

@matthias-pichler-warrify @cdavernas great conversation here. Can we summarize again and move on with a PR? Personally, I'd go for a new openid type. I've seen in other implementation/projects that oauth2 and openid many times have distinct plugins/components to deal with each different use case. Just an example: https://apisix.apache.org/docs/apisix/plugins/openid-connect/

And I agree that we should stick with the names recognized by users.

cdavernas commented 2 weeks ago

@matthias-pichler-warrify Would you be so kind to summarize your key take-aways, so that I can address the issue in the best way?