Add support for PKCE and AuthProc filters

[tft7000]

Added support for PKCE and AuthProc filters.

Reasons:

• PKCE is mandatory with some providers.
• AuthProc filters helps me to normalize the attributes between different SPs.

PKCE was implemented via the underlying library. For that I had to increase the minimum version for league/oauth2-client to 2.7. I think this should be fine, as there seems not to be any dependency differences between 2.6 and 2.7.

AuthProc filters were implemented in a similar way as the OIDC module is doing it. So it is separated from the regular filters and only basic operations are supported (e.g. attribute modification).

I tried to be as less invasive as possible, so the features both needs a defined strategy class in the config to get activated.

• [x] Tests added for this feature
• [x] Documentation added for this feature
• [x] Tests pass
• [x] composer run validate passes

[pradtke]

Thanks @tft7000 . I'll try to review the PKCE part this week.

For the AuthProc filters I would prefer to use an approach that is compatible with authproc filters that need to do user interaction. For example, reusing ProcessingChain, allows a Filter to interact with a user and then resume processing. Additionally SSP 2.1 adds a feature that allows a condition to be defined on filters that control if they will be executed or not. if ($filter->checkPrecondition($state) === true) { By replicating only parts of AuthProc processing chain, the behavior of an authproc filter will differ in non-obvious ways between running on a SAML SP, a SAML IdP, and on authoauth. This same issue is in the main OIDC module and the CAS server module (and probably others), and I'd rather we figure out how to solve it. Maybe something we can discuss with others on the SSP slack channel?

[tft7000]

Thank you, @pradtke, for your feedback.

I understand your concerns regarding the AuthProc support. I'm open to removing it for now, and we can consider reintroducing it with a more robust implementation once SSP 2.1 is released. However, I propose retaining the corresponding Strategy Interface and its associated calls. This would enable users of the library, like me, to implement functionalities such as attribute renaming without duplicating the entire code. We can then expand the interface in the future as required, with additional entry points from the code base if needed.

Regarding the authproc with user interaction, I'm not fully familiar with how SSP handles certain things, like managing states. So, I might not be the best fit to implement it the way you're thinking, I guess I would need some guidance.

[pradtke]

@tft7000 Thanks for your patience as I find time to look through this. I think let's start with getting the PKCE changes reviewed and incorporated.

What other ways to store the PKCE code challenge do you anticipate?

I can't really think of a scenario beyond storing it in the session, and am wondering if we can reduce the amount of code to just supporting 'pkceMethod' => 'S256', and plain as the configuration options.

I see some pkceMethod logic as part of OpenIDConnectProvider.php constructor. FYI, SSP has some simpler syntax for doing the validation. e.g $this->pkceMethod = $optionsConfig->getOptionalValueValidate('pkceMethod', [self::PKCE_METHOD_S256, self::PKCE_METHOD_PLAIN], null); will validate the value is one of the appropriate values.

The constructor for OpenIDConnectProvider.php is odd for other reasons. It seems like if an option is provided and the field is not-private than the AbstractProvider will set it, otherwise it seems to error.

(Please don't make any code changes in response to this, I just want to get your thoughts on the need for the PkceStrategyInterface aspect vs always storing it in the session.)

[tft7000]

@pradtke Thank you for your review and your thoughts.

What other ways to store the PKCE code challenge do you anticipate?

I can't really think of a scenario beyond storing it in the session, and am wondering if we can reduce the amount of code to just supporting 'pkceMethod' => 'S256', and plain as the configuration options.

I think you are right: in the sense of 'YAGNI', it is not necessary to implement a strategy pattern for storing the PKCE code challenge. If we have a superior way to store it (e.g. using states instead of the session directly?), we can exchange the code. Better testability is probably also not an issue.

FYI, SSP has some simpler syntax for doing the validation. e.g $this->pkceMethod = $optionsConfig->getOptionalValueValidate('pkceMethod', [self::PKCE_METHOD_S256, self::PKCE_METHOD_PLAIN], null); will validate the value is one of the appropriate values.

cool, thanks.

I see some pkceMethod logic as part of OpenIDConnectProvider.php constructor. [..] The constructor for OpenIDConnectProvider.php is odd for other reasons. It seems like if an option is provided and the field is not-private than the AbstractProvider will set it, otherwise it seems to error.

that is because of the underlying code:

• there are two providers: AbstractProvider and GenericProvider.
• The GenericProvider provides support for the pkceMethod option and returns its value with a getter. While the AbstractProvider only provides a getter that returns null.
• OpenIDConnectProvider extends AbstractProvider and therefore has no support for the pkceMethod option.
• I added this functionality in the same way as the GenericProvider did.
• Note: I did not check if the OpenIDConnectProvider could extend GenericProvider instead of AbstractProvider.

(Please don't make any code changes in response to this, I just want to get your thoughts on the need for the PkceStrategyInterface aspect vs always storing it in the session.)

Based on your feedback I would suggest that I incorporate the following changes to the PR:

AuthProc part

• remove the 'AuthProc' support (Strategy, initialization in OAuth2, locators)
• add a protected method afterPostFinalStep to OAuth2 that will be called from OAuth2::postFinalStep
• keep the added parent::postFinalStep() calls in the different Auth/Sources that extend OAuth2
• Q: how can I access the authsource config from afterPostFinalStep when I want to implement e.g. a dynamic filter there?

PKCE part

• remove the strategy files, only keep PkceSessionStrategy.
• replace the validation in OpenIDConnectProvider with the one from SSP
• if pkceMethod is set, set $this->pkceStrategy to a new instance of PkceSessionStrategy
• Q: is there really no dependency injection in SSP? it feels kind of weird to instantiate a fixed service inside a class.

What do you think?

[pradtke]

@tft7000 For PKCE part, that sounds good. Do you mind doing that in a new PR? I still need some more time to look + think through the AuthProc steps, so I think hold off on any authproc changes. If separating is a pain , then I'm going to try to get to the authproc side review next week. Thanks again for your patience.

Q: is there really no dependency injection in SSP? it feels kind of weird to instantiate a fixed service inside a class.

Yeah, it is such a pain. I've seen a lot things attempted - I tried doing stuff with locater traits, I've seen people expand the constructor to take in optional dependencies , and I've also seen some that take a list of class names and then instantiate those when needed. Part of what makes dependencies difficult to handle is that authproc filters may get serialized under certain user interactions, so SSP has recommended not creating new objects in the constructor but waiting till something runs. A lot has changed with SSP 2 so some these things may have improved.

[tft7000]

@pradtke : I added the PR #87 that just adds support for PKCE.

[pradtke]

@tft7000 fyi: I started experimenting with using SSP ProcessingChain for running authproc filters. I'm doing that in #88 . I still need to test it with an authproc filter that does user interaction, but so far it seems promising.

[pradtke]

PKCE and authproc features were split into separate PRs, adjusted a bit and merged in #87 and #88 . Closing this PR.