Support for OAuth2 Strava

[sberyozkin]

Fixes #37839.

CC @gastaldi

It requires adding integration test for the new option, will look a bit later

[gastaldi]

LGTM, added a minor observation but I think it can be ignored

[github-actions]

🙈 The PR is closed and the preview is expired.

[sberyozkin]

Thanks George @gastaldi, sure, I'll simplify as you suggested.
I’ve just been thinking that this PR will need a bit more hardening work. The problem is, Strava does not force a specific redirect url, only a domain, and even though, if combined with other security mechanisms it can be safe, it is strongly recommended to enforce a unique redirect uri.

So, since Strava itself doesn't do it, Quarkus can do it itself. It has quarku.oidc.authentication.redirect-path, which we also register in Google, etc and all the providers enforce it. But in this case Quarkus should do an extra check itself if that property is set and then it will be tested.

Once this check is in, it will be safer to offer this integration. Not sure though when I'll complete it, 🙂, but happy enough it works in principle, will also check with Pedro.

Cheers

[sberyozkin]

Hi @gastaldi

Today I have added an integration test checking the client query authentication mode which is required by Strava, I had to fix a minor but not related to this enhancement (OidcProvider.SymmetricKeyResolver) - this is a test issue only - this key resolver is used to support all the well-known OAuth2 providers we already support - but none of the them require client id and secret POSTed to them as body form parameters, instead they are encoded in the Basic authentication, and once I updated the test to try POSTing them as body form parameters, the fact that OidcProvider.SymmetricKeyResolver does not check all the configuration properties where the client secret can be configured showed itself, so I fixed it.

Also, as promised earlier, I have added a hardening fix to enforce a precise redirect URI match when, after authenticating to Strava, the user is redirected back to Quarkus. I've updated the docs clarifying why it has to be done, linking to the OAuth2 best security practices draft spec. By the way - this is a typical issue when Google, and other providers will fail in case of the redirect_uri mismatch, see for example, https://developers.google.com/identity/protocols/oauth2/web-server#creatingclient, so with this hardening fix, the authentication will fail earlier in Quarkus, avoiding a remote call to Google/etc.

Now, I'm happy enough with this PR to go in, but I'll also keep it till early January in case yourself and Pedro would like to comment more.

Confirmed the latest code is working with Strava.

[sberyozkin]

@gastaldi @pedroigor This is the only hardening code that I've added to enforce a precise path match at the Quarkus level. For example, if the configured redirect path is /a/b/c but the current request attempting to complete the code flow is /a then it means, assuming the best and wide-spread practice that a registered application in my-provider has configured www.my-provider-app/a/b/c then this redirect may not be coming from that provider . We haven't had to deal it with it before because the providers will enforce such checks. For example, I linked to Google docs and it can easily be confirmed with all providers here except Strava that a mismatch will cause an eventual failure, but, it costs a remote call, and we rely on providers to do the right thing, so this simple check will make the initial check done early at the Quarkus level which is important for the Strava case.
This is also why I'm not considering this check changing the OIDC logic as such, it is only a hardening check

[sberyozkin]

I've found it difficult to test this specific check though as it is difficult to verify the test failed exactly because of this check, I've spent quite some time today on it

[sberyozkin]

@gastaldi Hi George, I've now added a test for this condition by modifying one of the existing CodeFlowTest integration tests, not perfect but is probably closest to what can be done without having to create a dedicated integration module and check logs. So I'm happy enough now.

[sberyozkin]

@gastaldi @pedroigor I've only updated the docs a bit, specifically, I've added a small section to the providers doc explaining how to deal with the configured redirect url now that Strava enforces it.

I'll hold on though with merging for another day or so and send a short update to the forum

[sberyozkin]

@gastaldi Tweaked the docs a bit more, resolved the conflict, will hold on with the merge for another day, thanks

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

[sberyozkin]

OK, let me merge now