Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

oauth improvements #9217

Merged
merged 5 commits into from
Dec 17, 2023
Merged

oauth improvements #9217

merged 5 commits into from
Dec 17, 2023

Conversation

EdouardVanbelle
Copy link
Contributor

@EdouardVanbelle EdouardVanbelle commented Nov 16, 2023

  • Add oauth_config_uri to parse the .well-known/openid-configuration and discover oauth_*_uri
  • Add oauth_logout_uri support of Support for OpenID Connect RP-Initiated Logout #9109 (security improvement) (credit: @Westie)
  • Add support of OAUTHBEARER
  • Fix: missing config oauth_provider_name in rcmail_oauth's constructor
  • Refactor: move display to the rcmail_oauth class and use loginform_content hook

@EdouardVanbelle
Copy link
Contributor Author

EdouardVanbelle commented Nov 16, 2023

Dear team I am proposing you this patch to improve the OIDC use, I am currently using it for my personal use on the 1.6.5 version

I would like to thanks also @Westie for his work (I included it in this PR)

Let me know if you prefer non squashed PR

Kind regards
Edouard

@EdouardVanbelle
Copy link
Contributor Author

EdouardVanbelle commented Nov 16, 2023

#9109 is satisfied

#9110 not yet satisfied here

@azmeuk
Copy link

azmeuk commented Nov 18, 2023

Thank you very much for this patch! This would also fix #8201.

@azmeuk
Copy link

azmeuk commented Nov 18, 2023

I have tested this PR. Discovery works well, a thousand thanks for this!

As the specification recommend to pass the id_token as an id_token_hint parameter in the IdP end_session_endpoint, the refresh_access_token method is automatically called at logout since the id_token is absent from the session. I find this very strange to actually need to refresh a token just to logout, and consequently invalid the newly created token a few seconds after.

It was discussed a few years ago in #8214, at the time the maintainers preferred not to keep the id_token in the session. @thomascube and @alecpl would you reconsider this? I don't know if that was for security reasons, but nowadays with RFC9068 and people using JWT access tokens containing more or less the same payload than the id_token, I don't know if this is still relevant.

@EdouardVanbelle
Copy link
Contributor Author

As we have the discovery, I took opportunity to fetch jwks (certificates) to verify JWT signatures, it is important for backchannel call to trust foreign calls

A future PR will come, for manual setup it will be config option oauth_jwks_uri

@azmeuk
Copy link

azmeuk commented Nov 18, 2023

If the refresh_access_token returns false (because of wrong credentials or IdP misconfiguration for instance, or expired refresh token), then $_SESSION['oauth_token']['id_token'] is empty.

If refresh_access_token fails for whatever reason, $this->logout_redirect_url anyways and id_token_hint cannot be filled, perhaps the logout_hint parameter would be useful in that situation? However I am not sure which value it should have. The obvious one is the email because it is one thing we are sure that roundcube has, but this may not be the value expected by the IdP. Does roundcube knows about sub at this time? Maybe this should be a configuration option so the roundcube logout_hint can fit the IdP expectations?

What do you think?

@EdouardVanbelle
Copy link
Contributor Author

@azmeuk I will check your remarks, currently the refresh_token is saved which is more sensitive than the id_token
I consider in a such case that we could store id_token too via the encrypt() method

@azmeuk
Copy link

azmeuk commented Nov 18, 2023

One last thing. I noticed id_token was always null at logout, even after calling refresh_access_token. After having a look at the refresh_access_token method, I cannot see anything that would save the id_token in $_SESSION, unlike request_access_token (and it gets removed in the end of request_access_token anyways).

Looking closer at the OIDC spec, it seems the refresh token request should not return an id_token anyways:

Upon successful validation of the Refresh Token, the response body is the Token Response of Section 3.1.3.3Successful Token Response except that it might not contain an id_token.

Thus, to be able to fill id_token_hint parameter for the IdP logout request, it seems that keeping the id_token somewhere is actually mandatory.

@EdouardVanbelle
Copy link
Contributor Author

EdouardVanbelle commented Nov 19, 2023

here is some improvements:

  • add cache to Discovery and JWKS
  • add debug, activated vi òauth_debug` = true
  • improve refresh token
  • fetch sub (will be used later)
  • fix leak of access_token
  • use firebase/php-jwt JWT livrary and check signature if oauth_jwks_uri present
  • provide id_token_hint and client_id on OpenID Connect RP-Initiated Logout
  • refresh access_token only if necessary on OpenID Connect RP-Initiated Logout

@azmeuk if you test you can activate the debug option

@azmeuk
Copy link

azmeuk commented Nov 22, 2023

Hi @EdouardVanbelle, I will try to test this soon.
The PR has become quite large. I wonder if splitting it up would not make the whole review process easier for maintainers? 🤷

@EdouardVanbelle
Copy link
Contributor Author

EdouardVanbelle commented Nov 22, 2023

Hello @azmeuk , I pushed the version with the backchannel logout, you can add in your IDP a callback to /index.php/oauth/backchannel to test the functionality

regarding the pull request, I need to check with @alecpl
I don't really know how to plug a such page which requires no session, and the way I did it may be an ugly hack

due to sensitivity of this patch it could be time to add tests before and after this patch, for that I need to learn how to code in php :)

Right now I considered that it is overkill to redevelop a full JWT parser, so I used firebase/php-jwt which breaks current compatibility with php 7.3

@EdouardVanbelle EdouardVanbelle force-pushed the master branch 3 times, most recently from 24b4adf to 723c89c Compare November 24, 2023 21:10
@EdouardVanbelle
Copy link
Contributor Author

Hello @alecpl let me know if you need to split the pull request or rebase/squash it to make it more easier to review
I will appreciate any suggestion on how to catch a call for the backchannel url rather than the hack I did

@EdouardVanbelle EdouardVanbelle force-pushed the master branch 2 times, most recently from 3bbe9b1 to 0da86ca Compare November 26, 2023 16:55
  * Add `oauth_config_uri` to parse the .well-known/openid-configuration and discover `oauth_*_uri`
  * Add `oauth_logout_uri` and support of OIDC RP-initiated Logout with ìd_token_hint & client_id (credit: @Westie)
  * Add `oauth_issuer` to check JWT
  * Add `jwks_uri` used to fetch JWKS certificates
  * Add support of OIDC Back-Channel Logout (url: <roundcube>/index.php/login/backchannel) (security improvement)
  * Add support of OAUTHBEARER
  * Fix: missing config `oauth_provider_name` in constructor
  * Improvement: move display to the rcmail_oauth class and use `loginform_content` hook
  * Add `oauth_debug` to trace OIDC/Oauth events (target: <default log path>/oauth.log )
  * Improvement: align access_token expiration with refresh frequency
  * Improvement: store refresh_token expiration
  * Fix leak of access_token
  * Improvement: add unit tests
@Neustradamus
Copy link

To follow

@EdouardVanbelle
Copy link
Contributor Author

Hello @alecpl you have the next coming changes on another branch (once this one is approved)
https://github.com/EdouardVanbelle/roundcubemail/compare/master...EdouardVanbelle:roundcubemail:feat/hooks-and-oauth?expand=1

  • choose the most appropriate auth mechanism for Sieve, IMAP and SMTP (will require latest libraries versions)
  • simplify action/login/oauth in order to use hooks

@alecpl
Copy link
Member

alecpl commented Dec 13, 2023

@EdouardVanbelle So, the last part here that is not clear is that one with 'min_refresh_interval'.

Since you already know this code better than me, could you take a look at #9244 for feedback, please.

@alecpl
Copy link
Member

alecpl commented Dec 16, 2023

Please, fix the code style issues. We deployed code style checking in CI.

@EdouardVanbelle
Copy link
Contributor Author

@alecpl here it is !

@alecpl alecpl merged commit 588a879 into roundcube:master Dec 17, 2023
@alecpl alecpl added this to the 1.7-beta milestone Dec 17, 2023
@alecpl
Copy link
Member

alecpl commented Dec 17, 2023

@EdouardVanbelle Thank you for this work and I'm waiting for more!

@EdouardVanbelle
Copy link
Contributor Author

Thank you @alecpl !
I prepare the next merge request
This also close this request: #9110

@Neustradamus
Copy link

@EdouardVanbelle: Good job!

@oculos
Copy link

oculos commented Jun 8, 2024

@EdouardVanbelle Could you share a bit how to configure logout for keycloak? I've updated Roundcube, and attempted to use the new configs, but I can't seem to log off. I noticed now that, with Keycloak, oauth_config_uri doesn't seem to work - I disable the token_uri just to test, and it didn't seem to have fetched the right uri from the discovery endpoint.

@EdouardVanbelle
Copy link
Contributor Author

EdouardVanbelle commented Jun 10, 2024

Hello @oculos , here is my configuration:

tip concerning your oauth_config_uri: if you open this url in your browser you should have a json containing all your OIDC config

Roundcube config:

(on my side I have enabled the debugging)

$config['oauth_debug'] = true;
$config['oauth_provider'] = 'generic';
$config['oauth_provider_name'] = 'Vanbelle';
$config['oauth_client_id'] = 'YOUR-CLIENT-ID';
$config['oauth_client_secret'] = '#######';
$config['oauth_config_uri'] = 'https://YOUR-KEYCLOAK-DOMAIN/realms/YOUR-REALM/.well-known/openid-configuration';
$config['oauth_verify_peer'] = true;
$config['oauth_scope'] = "email openid";
$config['oauth_auth_parameters'] = [];
$config['oauth_identity_fields'] = ['email'];
// setup to true once config ok to remove user/pass prompt and redirect to oauth login
$config['oauth_login_redirect'] = false;
$config['oauth_cache'] = 'db';
$config['oauth_cache_ttl'] = '1d';

Keycloak config:

General Settings
    Client ID: YOUR-CLIENT-ID
    Root URL : https://YOUR-ROUNDCUBE-DOMAIN/
    Home URL : https://YOUR-ROUNDCUBE-DOMAIN/
    Valid redirect URIs : https://YOUR-ROUNDCUBE-DOMAIN/index.php/login/oauth
    Valid post logout redirect URIs : https://YOUR-ROUNDCUBE-DOMAIN/?_task=logout
    Web origins : YOUR-ROUNDCUBE-DOMAIN
    Admin URL : https://YOUR-ROUNDCUBE-DOMAIN/

Capability config
    Client authentication : On
    Authorization : Off
    Authentication flow
        [X] Standard flow
        [ ] Direct access grants  <------ BEWARE: use it only if you need your MTA as a proxy in user/password gateway, usefull for a transition, I do not recommand it.
        [ ] Implicit flow
        [X] Service accounts roles
        [ ] OAuth 2.0 Device Authorization Grant
        [ ] OIDC CIBA Grant

Logout settings
    Front channel logout : Off
    Backchannel logout URL : https://YOUR-ROUNDCUBE-DOMAIN/index.php/login/backchannel
    Backchannel logout session required : On
    Backchannel logout revoke offline sessions : Off

Debugging backchannel

To try backchannel logout: If you are on your Keycloak account in user:
https://YOUR-KEYCLOAK-DOMAIN/realms/YOUR-REALM/account/#/ > logout

on your roundcube's log directory, if you enabled debug on config (set oauth_debug to true), you should have:

$ tail -f oauth.log
[10-Jun-2024 17:30:34 +0000]: <0mb25nud> DEBUG: [ip=<IP FROM KEYCLOAK SERVER> sub=- ses=-] backchannel: logout event received, schedule a revocation for token's sub: <ID Of THE SUB>
[10-Jun-2024 17:30:39 +0000]: <6m4s24lp> DEBUG: [ip=<IP OF THE CUSTOMER (next refresh)> sub=<ID Of THE SUB> ses=6da52dcb-855b-4776-a813-6552f1275f2e] abort, token for sub <ID OF THE SUB> has been revoked

Debugging postlogout

once logged in Roundcube, just logout, it should propagate to Keycloak the logout request:

$ tail -f oauth.log
[10-Jun-2024 17:38:41 +0000]: <oetqnpfm> DEBUG: [ip=<IP OF THE CUSTOMER>  sub=<ID Of THE SUB> ses=fa2a21b9-5f69-43c5-829a-a2bf1c22c1b7] creating logout call: https://YOUR-KEYCLOAK-DOMAIN/realms/YOUR-REALM/protocol/openid-connect/logout?post_logout_redirect_uri=https%3A%2F%2FYOUR-ROUNDCUBE-DOMAIN%2F%3F_task%3Dlogout&client_id=YOUR-CLIENT-ID&id_token_hint=YOUR-TOKEN

and your session on keycloak should be kicked

@oculos
Copy link

oculos commented Jun 10, 2024

Thanks a lot @EdouardVanbelle
What is weird here is that I don't think roundcube is indeed picking up the configurations.
As I said, having only the oauth_config_uri doesn't work, even if the link is correct (I clicked on it from the terminal and it does show all the links - it's just that Roundcube doesn't pick them.

Not even the debug is working:

-rw-r--r--  1 www-data www-data 677K Jun 10 12:03 errors.log
-rw-r--r--  1 www-data www-data    0 Jun  8 19:00 oauth.log

I created the oauth.log file because it wasn't created at all. and it's still empty despite debug is on. So something is not working as it should.
For the record, I'm running RC 1.6.7.

I see that when I click logout, the url displays this:

https://mail.med-lo.eu/webmail/?_task=logout&_token=BZgxxxxxxxx

But clicking on login again sends me right back.

@EdouardVanbelle
Copy link
Contributor Author

EdouardVanbelle commented Jun 10, 2024

oauth_config_uri works only if you enabled the cache, which means ensuring:

$config['oauth_cache'] = 'db';
$config['oauth_cache_ttl'] = '1d';

could you confirm you enabled it ?

if config uri does not work, you should have the reason in your errors.log (errors are kept in default errors.log)
something like:
Error fetching {$config_uri} : ...

@oculos
Copy link

oculos commented Jun 11, 2024

This is my configuration:

$config['oauth_provider'] = 'generic';
$config['oauth_provider_name'] = 'mydomain SSO';
$config['oauth_client_id'] = 'roundcube';
$config['oauth_client_secret'] = 'somesecret';
$config['oauth_auth_uri'] = 'https://auth.mydomain.eu/realms/mydomain/protocol/openid-connect/auth';
$config['oauth_token_uri'] = 'https://auth.mydomain.eu/realms/mydomain/protocol/openid-connect/token';
$config['oauth_identity_uri'] = 'https://auth.mydomain.eu/realms/mydomain/protocol/openid-connect/userinfo';
$config['oauth_scope'] = 'Roundcube_email openid';
$config['oauth_config_uri'] = 'https://auth.mydomain.eu/realms/mydomain/.well-known/openid-configuration';
$config['oauth_logout_uri'] = 'https://auth.mydomain.eu/realms/mydomain/protocol/openid-connect/logout';
$config['oauth_cache'] = 'db';
$config['oauth_debug'] = true;
$config['oauth_verify_peer'] = true;
$config['oauth_cache_ttl'] = '8h';
$config['oauth_identity_fields'] = ['postfixMailAddress'];
$config['oauth_login_redirect'] = true;

If I comment out $config['oauth_token_uri'] = 'https://auth.mydomain.eu/realms/mydomain/protocol/openid-connect/token';, I don't get pass the Roundcube login window with the login button.

Absolutely no new errors on errors.log when doing so.

@EdouardVanbelle
Copy link
Contributor Author

@oculos even if you comment out oauth_token_uriand provided the oauth_config_uri the discover() method should grab it and explain any error in errors.log

Could you let met know which commit you are working on ? so I could check to reproduce
I just need the hash:

git rev-parse --short HEAD

(feature is right now only available on master's branch which is still in development )

@oculos
Copy link

oculos commented Jun 13, 2024

Ohhh now I see. I mistakenly thought this was already added to the package that is publicly available (1.6.7). You see, I don’t install from the repo, I just download the package from Roundcube’s website.
so sorry for wasting your time @EdouardVanbelle

@EdouardVanbelle
Copy link
Contributor Author

No worries @oculos, at least I provided a sample of use ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants