Support for openid connect SSO and Bearer Tokens #6494

davidblasby · 2022-08-09T17:41:22Z

This PR adds support for OAuth2 OpenId Connect Single Sign-on and also support for OIDC Bearer tokens.

This has been tested with Azure AD and with KeyCloak (backed by Azure AD). See the documentation changes in geonetwork/doc#201.

Support for "normal" OIDC login. See documentation for details.
Support for "Bearer Tokens". See documentation for details. This allow the use of the OIDC Access Token to be used by API calls from Desktop or Web Services. Simple attach the token to the header of your requests.

.../org/fao/geonet/kernel/security/openidconnect/GeonetworkOAuth2LoginAuthenticationFilter.java

+            try {
+                URI redirectUri = new URI(redirectURL);
+                if (redirectUri != null && !redirectUri.isAbsolute()) {
+                    response.sendRedirect(redirectUri.toString());


web/src/main/webapp/WEB-INF/config-security/openid-configuration-jose.json

web/src/main/webapp/WEB-INF/config-security/openid-configuration-azure.json

...main/webapp/WEB-INF/config-security/config-security-openidconnectbearer-overrides.properties

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/OIDCConfiguration.java

.../java/org/fao/geonet/kernel/security/openidconnect/GeonetworkClientRegistrationProvider.java

.../org/fao/geonet/kernel/security/openidconnect/GeonetworkOAuth2LoginAuthenticationFilter.java

.../java/org/fao/geonet/kernel/security/openidconnect/GeonetworkClientRegistrationProvider.java

.../src/main/java/org/fao/geonet/kernel/security/openidconnect/GeonetworkOidcLogoutHandler.java

josegar74 · 2022-09-09T13:06:06Z

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/SimpleOidcUser.java

+            username = org.apache.commons.lang.StringUtils.left(username, 256); //first max 256 chars
+        }
+
+        if (username != null && username.length() > 0) {


Change to !org.apache.commons.lang.StringUtils.isBlank(username)?

changed (new commit)

changed - new commit.

josegar74 · 2022-09-09T13:06:30Z

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/SimpleOidcUser.java

+            username = org.apache.commons.lang.StringUtils.left(username, 256); //first max 256 chars
+        }
+
+        if (username != null && username.length() > 0) {


Change to !org.apache.commons.lang.StringUtils.isBlank(username)?

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/SimpleOidcUser.java

josegar74 · 2022-09-09T13:10:52Z

.../main/java/org/fao/geonet/kernel/security/openidconnect/bearer/MSGraphUserRolesResolver.java

+ * <p>
+ * NOTE: to be consistent with the rest of Azure, we use the Groups OID (guid) NOT its name.
+ */
+public class MSGraphUserRolesResolver implements UserRolesResolver {


This class seems not using the internet proxy configuration if a proxy server is configured, to review.

hmmm - I don't think the internal spring library is using the proxy either.

I don't think I can support that right now - and there's insufficient examples in the code base.

will not change.

@josegar74 - no action on this. Not sure what to do...

...in/java/org/fao/geonet/kernel/security/openidconnect/bearer/SubjectAccessTokenValidator.java

ianwallen · 2022-09-09T13:29:37Z

@davidblasby
I have tried to make this work on my localhost and I can get it to authenticate with keycloak however it always returns 401. I don't have time to fully debug the issue at this time but here is my configuration - maybe you will see something simple that I missed.

This is the configuration for the environment variables in intellij.

And this is an example of the resource access that exists in my keycloak token (This is how it was setup when I was using keycloak drivers)

  "resource_access": {
    "catalogue": {
      "roles": [
        "Administrator",
        "a:Reviewer",
        "b:Editor",
        "a:Editor",
        "b:Reviewer"
      ]
    }
  },

believe the issue may be related to the roles I have assigned?
I don't understand the role assignment in the new openidconnect plugin.

Documentation says to use the following.

#. On the `Roles` tab, create some roles: Administrator, Editor, Reviewer, RegisteredGuest

I understand how RegisteredGuest and Administrator work as those roles are not dependent on the groups that a users is assigned to. But Editor and Reviewer are dependent on the groups that the user is assigned to so I don't understand what it means to be an "Editor" in this context. Generally geonetwork would use the highest role for the groups. i.e. if a user was a editor on group a and a reviewer on group b then it would identify the user as a reviewer. And if the user was an editor on group a and a registeredUser on group b then they would be identified as editor.
But with the new logic, if a user is an Editor on group a and the roles tab identifies the user as a Reviewer then will the system show the Reviewer options and fail because the user is not really a reviewer for any group?

...main/webapp/WEB-INF/config-security/config-security-openidconnectbearer-overrides.properties

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/OIDCConfiguration.java

davidblasby · 2022-09-09T17:12:29Z

@ianwallen - RE: you get 401s

This should work with the "a:Editor" - in the same way as the old keycloak provider (I reused the code there). I haven't done any extensive testing of it, however. You should still be able to login and shouldn't be getting 401s.

I could be giving the incorrect permissions, but I give the user the "normal" permission (like RegisteredUser and Editor) and then give them the group permissions. Most of that is in the OIDCRoleProcessor.

Perhaps we can share a screen and debug next week?

Perhaps @josegar74 can confirm, but I think there's two sets of permissions. The "generic" permissions, and then group-specific permissions. Jose - can you explain how to the permissions system works for group (and non-group) permissions?

.../src/main/java/org/fao/geonet/kernel/security/openidconnect/GeonetworkOidcLogoutHandler.java

davidblasby · 2022-09-20T18:34:51Z

@ianwallen - added the OPENIDCONNECT_SERVERMETADATA_CONFIG_URL environment variable (to download the config JSON from idp server on GN startup) and updated doc.

I this this just needs a review by @josegar74 (see tags above).

ianwallen · 2022-09-25T11:44:33Z

@davidblasby , I was able to finally get the authentication to work here are the issues that I have encountered.

you have the following in the documentation.

#null/empty -> use all in server
openidconnect_Configuration.scopes=${OPENIDCONNECT_SCOPES:#{''}}
If I don't change the setting, I get the 401 error. I had to change this to "openid profile email" for it to work in my configuration - I have not attempted different combinations. But I'm assuming that if I only have openid then it may not be able to get the profile information.

The other issue I had was that the role assignments are based on the IdToken instead of the AccessToken. My keycloak configuration did not have the roles in the IdToken and based on the following https://auth0.com/blog/id-token-access-token-what-is-the-difference/

it seems like the role should be in the accessToken (default in keycloak) because it is used to determine if the user has access to something. I was able to configure keycloak to include the role in the idToken but it is not added in the idToken by default - which tells me that it probably should not be using the roles from the idToken and it should check the accessToken instead.

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/OidcUser2GeonetworkUser.java

davidblasby · 2022-09-26T00:23:45Z

@ianwallen

Hi, I don't think that roles should be in the Access Token. In fact, you shouldn't ever parse the access token - it doesn't have to be a JWT. It's only required to be readable/usable by the server that produced them.
NOTE: the Bearer token does assume that the parse the access token as a JWT, but just for validation purposes.

Azure AD doesn't put roles in the access token - you can request that it puts them in the ID Token (see my documentation - you have to turn this on inside Azure). Since, for Bearer-token-only access (which is an Azure-generated AccessToken), I use the MSGraphRolesResolver (graph.microsoft.com) to get roles.

I also included UserInfoAccessTokenRolesResolver, which allows you to get roles from the userinfo endpoint - which is how I use bearer tokens from keycloak.

There's also code there for getting roles from an ID token.

I should probably default the scopes (OPENIDCONNECT_SCOPES) to "openid profile email".

davidblasby · 2022-09-27T22:32:51Z

I updated so the default scopes are "openid email profile" (norma), and "openid email profile offline_access" (for bearer tokens).

davidblasby · 2022-09-27T22:33:06Z

@ianwallen @josegar74 - I think this is ready to merge???

josegar74 · 2022-09-29T05:20:10Z

@davidblasby

Perhaps @josegar74 can confirm, but I think there's two sets of permissions. The "generic" permissions, and then group-specific permissions. Jose - can you explain how to the permissions system works for group (and non-group) permissions?

Administrator permission is global (not assigned per group)
Other permissions are applied per group. GeoNetwork doesn't support to be for example Editor in all groups.

ianwallen · 2022-10-01T22:38:15Z

@ianwallen

Hi, I don't think that roles should be in the Access Token. In fact, you shouldn't ever parse the access token - it doesn't have to be a JWT. It's only required to be readable/usable by the server that produced them. NOTE: the Bearer token does assume that the parse the access token as a JWT, but just for validation purposes.

I'm not an expert? I tried to find information on best practices for group token and I only found the following.
https://devforum.okta.com/t/best-practices-groups-claim-in-id-token-or-access-token/6532

Azure AD doesn't put roles in the access token - you can request that it puts them in the ID Token (see my documentation - you have to turn this on inside Azure). Since, for Bearer-token-only access (which is an Azure-generated AccessToken), I use the MSGraphRolesResolver (graph.microsoft.com) to get roles.

I also included UserInfoAccessTokenRolesResolver, which allows you to get roles from the userinfo endpoint - which is how I use bearer tokens from keycloak.

I don't understand why you seem to indicate AD doesn't put the roles in the access token. To my knowledge it does not put it in any token unless configured as shown here
https://docs.cribl.io/stream/3.3/usecase-azure-ad/#configure-token-and-claims. It can be added to ID token, Access token or SAML.

I have not used the role token from Azure so I may not be understanding your configuration.

We currently use keycloak, and I would have expected that I could simply changed geonetwork configuration from the keycloak drivers to the this openidconnect driver without making any changes to the keycloak configuration however that was not the case. If you are correct that the ID token should be used, then does this mean that keycloak configuration has a bug because it uses the access token?

It is odd that the keycloak drivers defaults to using the access token but the oauth driver default to using the id token. I'm not sure if there should be a configuration parameter for both keycloak and openidconnect drivers indicating where the group token comes from so that it is more clear otherwise I think this can get confusing.

davidblasby · 2022-10-06T18:38:53Z

I'm not an expert? I tried to find information on best practices for group token and I only found the following. https://devforum.okta.com/t/best-practices-groups-claim-in-id-token-or-access-token/6532

From that group;

If your application calls your own API endpoints using the access token, then it’s best to have the groups claim inside the access token, so that the endpoints will check the user’s permissions prior to displaying the response.
If your application check the user’s permissions prior to creating the local session, then it’s best to have the groups claim inside the ID token so that the session is created successfully and user is granted proper permissions.

We're using the permissions in a local session, so we should be using the ID token.

I tried looking at those instructions (azure ad) for putting the groups in the access token - I tried a long time ago (many many times) and couldnt get it to work, and I tried today and wasn't able to get then in the access token. Perhaps it only works for oauth2 requests (which don't have an ID token) or for saml requests of some type. Most of the online documentation for azure ad oidc follows what I put in the GN documentation for setting up Azure AD - modify the manifest to get it to put tokens in the ID token.

I think the keycloak driver is specialized to talk to keycloak and does whats easiest for keycloak. If you look at what the keycloak driver is doing, it basically doing an openid request to keycloak. I expect its putting the groups in the access token so it easier to work with with oauth2 (i.e. not oidc - and only has an access token). Since its controlling both the server and client, it can do anything it wants.

I could modify it so that it could grab roles from the access token - but that's probably non-trivial todo. And it would make sense to allow use of the other providers (userinfo, MS Graph) as well.

I can put a big note in the documentation saying that it should be using the ID token for roles. However, i think its pretty explicit (it one of the first things mentioned). The Azure AD and Keycloak sections both explain how to setup so it puts the roles in the id token.

ianwallen · 2022-10-08T11:51:04Z

I could modify it so that it could grab roles from the access token - but that's probably non-trivial todo. And it would make sense to allow use of the other providers (userinfo, MS Graph) as well.

Thank you for looking into it some more. I agree that this may be extra work and I think we can submit a separate PR in the future to make this configurable.

I will do one final test on my local system with the new changes and if the user profile changes work then I will approve your PR.

ianwallen

I identified a few typos that should be easy to fix.

I also noticed some inconsistencies with the naming where it is using both camel case and _ separator. Generally I would expect it to only use one.

i.e. openidconnect_Configuration expecting OIDCConfiguration, openIDConnectConfiguration, oidc_configuration,...

Here are a couple others examples that are inconsistent but there are lots in the code.
openidconnect_UserService
openidconnect_DefaultAuthorizationCodeTokenResponseClient
openidconnect_bearer_tokenvalidator
openidconnect_bearer_rolesprovider

These should all be easy to change if required.

I'm not sure if naming standards are enforced in this project so if @josegar74 wants to approved the PR with these naming issues then I will approve as well

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/OIDCRoleProcessor.java

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/bearer/JwtDecoderFactory.java

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/OIDCConfiguration.java

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/OIDCRoleProcessor.java

davidblasby · 2022-10-11T17:06:45Z

@ianwallen - I changed the name of all those beans so they don't have "_" in the name.

@josegar74 - I will send you a private message for connecting to the geocat keycloak and Azure AD for testing.

davidblasby · 2022-10-11T17:07:05Z

@ianwallen - I think, once jose has a chance to test, we are ready to merge!

ianwallen

@ianwallen - I think, once jose has a chance to test, we are ready to merge!

I good with the latest changes so I will approve - thank you

davidblasby · 2022-10-18T00:18:44Z

Minor change for group - its was using the wrong isGroupUpdate()

davidblasby · 2022-10-24T21:43:55Z

Added a new environment variable OPENIDCONNECT_LOGSENSITIVE_INFO for logging information. This is pretty much required when setting up so you can trouble shoot.

See docs.

* missed file in #6494 * jose feedback Co-authored-by: Jose García <josegar74@gmail.com> --------- Co-authored-by: david.blasby <david.blasby@geocat.net> Co-authored-by: Jose García <josegar74@gmail.com>

david.blasby added 14 commits June 1, 2022 19:00

WIP: OAUTH2 Open ID connect security plugin

6d674e4

added missing files

04c4ae2

reformat code

937203f

fix test cases due to default config changes

0171d09

allow env var to have the openid server's json metadata text

dcfe538

add support for limiting scope

c185185

fixes from jose's review

73d1928

removed some unused configuration options

f8c8fc4

added comments, trivial changes

1547d2f

WIP: bearer tokens

c662c78

more comments, code reformat, added missing pom.xml update

67d235a

update tests for refactoring

196a0a5

bearer token support and validation

e38daa7

use constants for claim names

b87b6f9

davidblasby mentioned this pull request Aug 9, 2022

OAuth2 OpenId Connect Documentation geonetwork/doc#201

Merged

github-advanced-security bot found potential problems Aug 9, 2022

View reviewed changes

ianwallen reviewed Aug 12, 2022

View reviewed changes

web/src/main/webapp/WEB-INF/config-security/openid-configuration-jose.json Outdated Show resolved Hide resolved

ianwallen reviewed Aug 12, 2022

View reviewed changes

web/src/main/webapp/WEB-INF/config-security/openid-configuration-azure.json Outdated Show resolved Hide resolved

remove accidently committed files

da9088b

ianwallen reviewed Sep 9, 2022

View reviewed changes

...main/webapp/WEB-INF/config-security/config-security-openidconnectbearer-overrides.properties Show resolved Hide resolved

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/OIDCConfiguration.java Outdated Show resolved Hide resolved

josegar74 reviewed Sep 9, 2022

View reviewed changes

ianwallen reviewed Sep 9, 2022

View reviewed changes

...main/webapp/WEB-INF/config-security/config-security-openidconnectbearer-overrides.properties Outdated Show resolved Hide resolved

add OPENIDCONNECT_LOGINTYPE

735b658

ianwallen reviewed Sep 9, 2022

View reviewed changes

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/OIDCConfiguration.java Show resolved Hide resolved

changes from jose and ian review

950c97a

ianwallen reviewed Sep 14, 2022

View reviewed changes

.../src/main/java/org/fao/geonet/kernel/security/openidconnect/GeonetworkOidcLogoutHandler.java Outdated Show resolved Hide resolved

feedback from ian - URL + change in logging

3932c57

ianwallen reviewed Sep 25, 2022

View reviewed changes

core/src/main/java/org/fao/geonet/kernel/security/openidconnect/OidcUser2GeonetworkUser.java Show resolved Hide resolved

change default for scopes

78dc9b1

jose comments - looks like some changes were lost

d1d07f1

change group/profile update rules (ian feedback)

41cb415

ianwallen reviewed Oct 8, 2022

View reviewed changes

minor typos and remove '_' from bean names

8c1cda0

ianwallen approved these changes Oct 14, 2022

View reviewed changes

fix - wasn't using the isGroupUpdate correctly

e965a49

add support for logging information about the OIDC login

33e0285

josegar74 approved these changes Nov 9, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into openid

93f09ab

josegar74 merged commit 37bd851 into geonetwork:main Nov 9, 2022

josegar74 mentioned this pull request Dec 12, 2022

OAuth2 OpenId Connect Documentation geonetwork/doc#219

Closed

davidblasby pushed a commit to davidblasby/core-geonetwork that referenced this pull request Jun 9, 2023

missed file in geonetwork#6494

8758e3b

davidblasby mentioned this pull request Jun 9, 2023

Fix error in OpenID connector / add missing class #7150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for openid connect SSO and Bearer Tokens #6494

Support for openid connect SSO and Bearer Tokens #6494

davidblasby commented Aug 9, 2022 •

edited

Loading

josegar74 Sep 9, 2022

davidblasby Sep 28, 2022

davidblasby Sep 28, 2022

josegar74 Sep 9, 2022

davidblasby Sep 9, 2022

josegar74 Sep 9, 2022

davidblasby Sep 9, 2022

davidblasby Sep 28, 2022

ianwallen commented Sep 9, 2022

davidblasby commented Sep 9, 2022 •

edited

Loading

davidblasby commented Sep 20, 2022

ianwallen commented Sep 25, 2022

davidblasby commented Sep 26, 2022

davidblasby commented Sep 27, 2022

davidblasby commented Sep 27, 2022

josegar74 commented Sep 29, 2022

ianwallen commented Oct 1, 2022

davidblasby commented Oct 6, 2022

ianwallen commented Oct 8, 2022

ianwallen left a comment

davidblasby commented Oct 11, 2022

davidblasby commented Oct 11, 2022

ianwallen left a comment

davidblasby commented Oct 18, 2022

davidblasby commented Oct 24, 2022

Support for openid connect SSO and Bearer Tokens #6494

Support for openid connect SSO and Bearer Tokens #6494

Conversation

davidblasby commented Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianwallen commented Sep 9, 2022

davidblasby commented Sep 9, 2022 • edited Loading

davidblasby commented Sep 20, 2022

ianwallen commented Sep 25, 2022

davidblasby commented Sep 26, 2022

davidblasby commented Sep 27, 2022

davidblasby commented Sep 27, 2022

josegar74 commented Sep 29, 2022

ianwallen commented Oct 1, 2022

davidblasby commented Oct 6, 2022

ianwallen commented Oct 8, 2022

ianwallen left a comment

Choose a reason for hiding this comment

davidblasby commented Oct 11, 2022

davidblasby commented Oct 11, 2022

ianwallen left a comment

Choose a reason for hiding this comment

davidblasby commented Oct 18, 2022

davidblasby commented Oct 24, 2022

davidblasby commented Aug 9, 2022 •

edited

Loading

davidblasby commented Sep 9, 2022 •

edited

Loading