Support for credentials in form body for /token endpoint and make state parameter optional

csantos commented 1 year ago

Description

Added support for credentials in the form body (in addition to previous Authorization header support) to the /token endpoint. Removed requirement that the state parameter from the /authorize endpoint. These changes allow openidconnect.net and Apigee's integrated portal to work with Identity Facade without any additional work.

Added new EV-ClientCredentialsFromBody step to extract credentials from from body
Conditionally executing steps to either extract credentials from header or form body
Removed step that conditionally executed RF-InvalidRequest-Redirect if state parameter wasn't provided.

Housekeeping

(please check all that apply [x], do not edit the text)

[X] I have run all the tests locally and they all pass.
[X] I have followed the relevant style guide for my changes.

Full Repo Validation Required

(please check all that apply [x], do not edit the text)

[ ] PR requires full pipeline run (Run for changes only by default).

CC: @apigee-devrel-reviewers

OmidTahouri commented 1 year ago

Hi @csantos -- thanks for your contribution!

Although the state parameter is technically optional as per the OAuth specification, it is generally highly recommended and is therefore a hard dependancy for the Identity Facade pattern implemented here. The proxy leverages state to capture all the parameters on the /authorize request so that they can be carried over and extracted in the subsequent /token request. In a way, the state is used to maintain a session (or state) across requests. Without this, I can't see how the /token endpoint can be sure that the same client came from the corresponding /authorize endpoint where the client_id, redirect_uri, etc. were validated.

Take a look at the OA2-GetOriginalStateAttributes and OA2-GenerateAzCode-State2 policies. You can see how they're used in the proxy XML and sequence diagram.

I'll let @JoelGauci chime in here as well, to validate my thinking... I'm sure we can discuss this over a short call if it's easier.

Supporting credentials in the form parameters (instead of the Authorization header) sounds fair.

Thanks again!

csantos commented 1 year ago

Thanks for the quick response @OmidTahouri. Yep, totally agree that using the state parameter is to mitigate CSRF attacks, and that using it is a best practice. It is an optional param though, and the Integrated Portal does not pass it which hampers the ability to use the "Try this API" functionality (or any other client that doesn't pass this param) w/Identity Facade.

Identity Facade does use state2 for the proxy to IdP calls, making those calls more secure...so it seems it is more about whether to enforce that best practice and Identity Facade taking the stance that it is required?

Let me know if you'd like me to not include making state optional. I'll rollback that change and only have the added support for credentials in form body for /token endpoint. If so, did you want me to open up a separate pull request? That way I can clean up the history a bit and not see removal / putting back the state param requirement?

JoelGauci commented 1 year ago

According to me it is ok to make the state parameter optional, but I think we should in this case highly recommend and propose it by default in the proxy configuration. In this case, it is the responsibility of a customer to disable the use of the state parameter.

In the logic that is implemented, the state parameter (state1) is retrieved from the original request (/authorize) and stored into a "session" (state2).

state2 is used by Apigee to redirect the end-user with the identity provider (IdP) Then state1 is sent back to the client app at the end of the /callback processing.

As you mention Carlos, we can make it work without state1 and we will always implement the state2 used in the relation between Apigee and the IdP.

OmidTahouri commented 1 year ago

Thanks for clarifying, @JoelGauci

To summarise, it's imperative we maintain a state parameter between the proxy and the IdP, but not between the client and proxy.

csantos commented 1 year ago

@JoelGauci no problem. I've taken a stab at the update. Any feedback is welcomed.

@OmidTahouri thanks for bringing this up as a topic of discussion.

danistrebel commented 1 year ago

/gcbrun

apigee-devrel-bot commented 1 year ago

Pipeline Report

Pipeline	Result	Elapsed Wall Time
references/identity-facade	pass	371s
TOTAL PIPELINE	pass	371s

View details in Cloud Build (permission required)

Commit version: 8240a17

OmidTahouri commented 1 year ago

/gcbrun

apigee-devrel-bot commented 1 year ago

Pipeline Report

Pipeline	Result	Elapsed Wall Time
references/identity-facade	pass	369s
TOTAL PIPELINE	pass	369s

View details in Cloud Build (permission required)

Commit version: 6a7eadb

apigee / devrel