This is a recommended openid-connect parameter [1] and OAuth2 integrations seem to require it [2], but I'm not sure if it should be enforced. Basically because I don't know all the IdPs around.



[1] https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest
[2] https://www.oauth.com/oauth2-servers/accessing-data/authorization-request/

Created attachment 151019 [details] [review]
Bug 33675: Add CSRF protection to OAuth/OIDC authentication

This patch makes the OAuth/OIDC client pass a `state` parameter with a
CSRF protection token, to be validated back when the flow returns to
Koha.

Ideally, the Mojolicious::Plugin::OAuth2 library should deal with this
implicitly, probably making use of JWT. But as of now, this is the best
way to implement it.

To test:
1. Have a working SSO solution (ktd --sso)
2. Click to login using SSO
=> SUCCESS: Notice a 'state' parameter on the URL, looks like a random
thing
3. When you login, no error is reported

(In reply to Tomás Cohen Arazi from comment #1)
> This is a recommended openid-connect parameter [1] and OAuth2 integrations
> seem to require it [2], but I'm not sure if it should be enforced. Basically
> because I don't know all the IdPs around.

It looks like OAuth2 also only recommends it: https://www.rfc-editor.org/rfc/rfc6749#section-4.1.1

However, the specs do say that the "state" parameter is required in the Authorization Response if it was included in the Authorization Request: https://www.rfc-editor.org/rfc/rfc6749#section-4.1.2

I suspect that most IdPs should support "state" if they want to be spec compliant, although I suppose there's no guarantee. I've certainly dealt with 1 non-compliant IdP in the past, although that was nearly 10 years ago now.

If we are worried, I think we could make using "state" optional in terms of whether or not to send it, but... I think it should be all right.

Hmm I got an error the first time I tried it and then when I clicked the "Login" button a second time it worked. The error might be residue from previous work I suppose. Going to see if I can reproduce it...

(In reply to David Cook from comment #4)
> Hmm I got an error the first time I tried it and then when I clicked the
> "Login" button a second time it worked. The error might be residue from
> previous work I suppose. Going to see if I can reproduce it...

Can't reproduce so yeah probably not a problem.

Created attachment 151036 [details] [review]
Bug 33675: Add CSRF protection to OAuth/OIDC authentication

This patch makes the OAuth/OIDC client pass a `state` parameter with a
CSRF protection token, to be validated back when the flow returns to
Koha.

Ideally, the Mojolicious::Plugin::OAuth2 library should deal with this
implicitly, probably making use of JWT. But as of now, this is the best
way to implement it.

To test:
1. Have a working SSO solution (ktd --sso)
2. Click to login using SSO
=> SUCCESS: Notice a 'state' parameter on the URL, looks like a random
thing
3. When you login, no error is reported

Signed-off-by: David Cook <dcook@prosentient.com.au>

First time trying this, and I failed to test.

I am getting

"""
test
We are sorry...

Invalid parameter: redirect_uri
"""
After I clicked "login with test"
The generated URL is
http://sso:8082/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=kohaoidc&redirect_uri=http%3A%2F%2Fkohadev-intra.myDNSname.org%3A8081%2Fapi%2Fv1%2Foauth%2Flogin%2Ftest%2Fstaff&scope=openid+email&state=873f7839ab38384730c0635c3b260564297de884%2Ca443349be07f4216b95089525be35eeb96b74452%2C1683794690

I guess I made an error in the config somewhere but I have double-checked everything :-/

(In reply to Jonathan Druart from comment #7)
> First time trying this, and I failed to test.
> 
> I am getting
> 
> """
> test
> We are sorry...
> 
> Invalid parameter: redirect_uri
> """
> After I clicked "login with test"
> The generated URL is
> http://sso:8082/auth/realms/test/protocol/openid-connect/
> auth?response_type=code&client_id=kohaoidc&redirect_uri=http%3A%2F%2Fkohadev-
> intra.myDNSname.
> org%3A8081%2Fapi%2Fv1%2Foauth%2Flogin%2Ftest%2Fstaff&scope=openid+email&state
> =873f7839ab38384730c0635c3b260564297de884%2Ca443349be07f4216b95089525be35eeb9
> 6b74452%2C1683794690
> 
> I guess I made an error in the config somewhere but I have double-checked
> everything :-/

The wiki suggests using localhost:port/* but your URL is using kohadev.myDNSname... because it picks it from the baseURL prefs

Could we add a test to cover this change?

Created attachment 151436 [details] [review]
Bug 33675: Add CSRF protection to OAuth/OIDC authentication

This patch makes the OAuth/OIDC client pass a `state` parameter with a
CSRF protection token, to be validated back when the flow returns to
Koha.

Ideally, the Mojolicious::Plugin::OAuth2 library should deal with this
implicitly, probably making use of JWT. But as of now, this is the best
way to implement it.

To test:
1. Have a working SSO solution (ktd --sso)
2. Click to login using SSO
=> SUCCESS: Notice a 'state' parameter on the URL, looks like a random
thing
3. When you login, no error is reported

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Pushed to master for 23.05.

Nice work everyone, thanks!

Nice work everyone!

Pushed to stable for 22.11.x

No tests => regression later!
bug 36219.

(In reply to Jonathan Druart from comment #13)
> No tests => regression later!
> bug 36219.

:-(