Bug 33675 - Add CSRF protection to OAuth/OIDC authentication
Summary: Add CSRF protection to OAuth/OIDC authentication
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL: https://www.oauth.com/oauth2-servers/...
Keywords:
Depends on: 33708
Blocks: 33782 34755 34163
  Show dependency treegraph
 
Reported: 2023-05-04 10:31 UTC by Nick Clemens
Modified: 2024-03-04 12:45 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This development adds support for the `state` parameter generation and delivery when contacting IdPs. This is an optional but recommended opaque value in the OAuth2/OIDC specs that helps prevent CSRF attacks, but is also a requirement on some Identity Provider solutions.
Version(s) released in:
23.05.00,22.11.07


Attachments
Bug 33675: Add CSRF protection to OAuth/OIDC authentication (2.45 KB, patch)
2023-05-10 16:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33675: Add CSRF protection to OAuth/OIDC authentication (2.50 KB, patch)
2023-05-10 23:39 UTC, David Cook
Details | Diff | Splinter Review
Bug 33675: Add CSRF protection to OAuth/OIDC authentication (2.55 KB, patch)
2023-05-18 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2023-05-04 10:31:18 UTC
Some Authentication solutions require a 'state' parameter to be passed during authentication to verify the sources on either end, it is supported by Mojolicious:
https://metacpan.org/pod/Mojolicious::Plugin::OAuth2

We should add this to Koha
Comment 1 Tomás Cohen Arazi 2023-05-10 13:42:25 UTC
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/
Comment 2 Tomás Cohen Arazi 2023-05-10 16:10:07 UTC
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
Comment 3 David Cook 2023-05-10 23:29:00 UTC
(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.
Comment 4 David Cook 2023-05-10 23:36:32 UTC
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...
Comment 5 David Cook 2023-05-10 23:38:50 UTC
(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.
Comment 6 David Cook 2023-05-10 23:39:51 UTC
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>
Comment 7 Jonathan Druart 2023-05-11 08:46:14 UTC
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 :-/
Comment 8 Tomás Cohen Arazi 2023-05-11 11:36:54 UTC
(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
Comment 9 Jonathan Druart 2023-05-16 12:36:12 UTC
Could we add a test to cover this change?
Comment 10 Nick Clemens 2023-05-18 18:29:30 UTC
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>
Comment 11 Tomás Cohen Arazi 2023-05-19 12:21:19 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 12 Matt Blenkinsop 2023-06-08 16:58:23 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 13 Jonathan Druart 2024-03-04 12:14:11 UTC
No tests => regression later!
bug 36219.
Comment 14 Tomás Cohen Arazi 2024-03-04 12:45:35 UTC
(In reply to Jonathan Druart from comment #13)
> No tests => regression later!
> bug 36219.

:-(