[GEOS-10646] OAuth access tokens #6167

davidblasby · 2022-09-08T21:15:10Z

This PR is improving the support for Bearer Tokens. See the updated GS OpenID documentation for setup and usage.

Issue: https://osgeo-org.atlassian.net/browse/GEOS-10646

Summary: attach an Access Token to your HTTP requests - useful for automated (i.e. desktop/remote web) access of the REST api.

There was already some partial support for Bearer tokens. This PR improves it.

There is now better token validation (see GS doc + OIDC specification). This improves security.
Since you cannot use ID Tokens with attached access tokens, I added two new ways to get user groups:
a) from the "userinfo" endpoint (recommended for KeyCloak)
b) from the MS Graph API (only for MS Azure AD)
There is a configuration on the OIDC web page to turn on/off bearer tokens
I added some minor validation to the OIDC web page
Added a few test cases
Updated documentation for Attached Bearer Tokens
Tested with Keycloak and Azure AD
I attach the "userinfo" results to the incomming request (in the same manner as the ID/Access tokens + other spring stuff)
I attach a flag to the incomming request indicating if the authentication is BEARER or USER (in the same manner as the ID/Access tokens + other spring stuff)

Checklist

I have read the contribution guidelines.
I have sent a Contribution Licence Agreement (not required for small changes, e.g., fixing typos in documentation).
First PR targets the main branch (backports managed later; ignore for branch specific issues).
All the build checks are green (see automated QA checks).

For core and extension modules:

New unit tests have been added covering the changes.
Documentation has been updated (if change is visible to end users).
[n/a] The REST API docs have been updated (when changing configuration objects or the REST controllers).
[GEOS-10646] There is an issue in the GeoServer Jira (except for changes that do not affect administrators or end users in any way).
[squash on merge] Commit message(s) must be in the form [GEOS-XYZWV] Title of the Jira ticket.
Bug fixes and small new features are presented as a single commit.
Each commit has a single objective (if there are multiple commits, each has a separate JIRA ticket describing its goal).

...2/oauth2-core/src/main/java/org/geoserver/security/oauth2/GeoServerAccessTokenConverter.java

doc/en/user/source/community/oauth2/index.rst

...h2-openid-connect-core/src/main/java/org/geoserver/security/oauth2/MSGraphRolesResolver.java

...nect-core/src/main/java/org/geoserver/security/oauth2/OpenIdConnectAuthenticationFilter.java

jodygarnett · 2022-09-09T16:33:39Z

...nect-core/src/main/java/org/geoserver/security/oauth2/OpenIdConnectAuthenticationFilter.java

+        //            LOGGER.log(Level.FINE, "ID token: " + (String)
+        // request.getAttribute(ID_TOKEN_VALUE));
+        //        } catch (Exception e) {
+        //        }


Thanks for the comments; this is the rare case where keeping commented out code in the codebase may be worthwhile.

One trick I do is to do an:

private static boolean DEBUGGING = false; if (DEBUGGING) { // some stuff that would otherwise be commented out }

May have to do a PMD ignore to get that passed QA checks but at least the debugging code will still be compiled.

...ct-core/src/main/java/org/geoserver/security/oauth2/OpenIdConnectAuthenticationProvider.java

jodygarnett · 2022-09-09T16:36:04Z

...nid-connect-core/src/main/java/org/geoserver/security/oauth2/bearer/MultiTokenValidator.java

+import org.geoserver.security.oauth2.OpenIdConnectFilterConfig;
+
+/**
+ * This is a simple token validator that runs a list of TokenValidators. This doesn't do any


Suggested change

* This is a simple token validator that runs a list of TokenValidators. This doesn't do any

* This is a token validator that runs a list of TokenValidators. This doesn't do any

jodygarnett

I appreciate the TokenValidator interface and classes. I would like one of the other teams using OAuth to review (but I understand they may not be interested in bearer tokens at this time).

jodygarnett · 2022-09-13T20:06:23Z

Update from today's meeting:

Ask @taba90 or @afabiani to review due to number of files changes
Double check tests have been run locally, since some tests already extract credentials

taba90

Overall looks good, tests passed running them locally. I've done some tests and it works fine.
I've added some minor comments.

taba90 · 2022-09-15T15:35:38Z

...h2-openid-connect-core/src/main/java/org/geoserver/security/oauth2/MSGraphRolesResolver.java

+        try {
+            memberOfEndpoint = new URL("https://graph.microsoft.com/v1.0/me/memberOf");
+        } catch (MalformedURLException e) {
+            e.printStackTrace();


Suggested change

e.printStackTrace();

e.printStackTrace();

please remove printStackTrace();

made it log (shouldn't throw unless someone makes a type on the above line)

taba90 · 2022-09-15T15:43:35Z

...nect-core/src/main/java/org/geoserver/security/oauth2/OpenIdConnectAuthenticationFilter.java

+
+        List<GeoServerRole> result = new ArrayList<>();
+        if (o instanceof String) {
+            result.add(new GeoServerRole((String) o));
+        } else if (o instanceof List) {
+            ((List) o).stream().forEach(v -> result.add(new GeoServerRole((String) v)));
+        } else {
+            LOGGER.log(
+                    Level.FINE,
+                    "Did not find "
+                            + rolesAttributePath
+                            + " in the token, returning an empty role list");
+        }
+        if (!result.isEmpty()) {
+            enrichWithRoleCalculator(result);
+        }
+        return result;
+    }


this piece of code seems equal to the one in the getRolesFromToken method. Maybe extracting common parts in a separate method would be better.

taba90 · 2022-09-15T15:45:36Z

...nect-core/src/main/java/org/geoserver/security/oauth2/OpenIdConnectAuthenticationFilter.java

+
+        String rolesAttributePath =
+                ((OpenIdConnectFilterConfig) this.filterConfig).getTokenRolesClaim();
+        Object o = JsonPath.read(userinfoMap, rolesAttributePath);


I would catch this since if the attribute is not present this will throw exception blocking the user to be authenticated ( it will not throw an IOException catched in the OAuth2Filter superclass). It should be fine to just reuse the extracFromJSON static method above.

I created another extractFromJSON(), based on Map (instead of a JSON string).

taba90 · 2022-09-16T07:35:25Z

...enid-connect-core/src/main/java/org/geoserver/security/oauth2/OpenIdConnectFilterConfig.java

@@ -19,11 +19,14 @@ public class OpenIdConnectFilterConfig extends GeoServerOAuth2FilterConfig {
    String tokenRolesClaim;
    String responseMode;
    boolean sendClientSecret = false;
+    boolean allowBearerTokens = false;


Since bearer authentication was already working although without validation of the token, maybe this flag could be kept as true by default.

I changed it to true. I was a bit worried leaving bearer tokens as the default.

davidblasby · 2022-09-19T23:32:32Z

@taba90 - thanks for the review!

…ole source

Co-authored-by: Jody Garnett <jody.garnett@gmail.com>

…nect-core/src/main/java/org/geoserver/security/oauth2/MSGraphRolesResolver.java Co-authored-by: Jody Garnett <jody.garnett@gmail.com>

…nect-core/src/main/java/org/geoserver/security/oauth2/OpenIdConnectAuthenticationFilter.java Co-authored-by: Jody Garnett <jody.garnett@gmail.com>

…nect-core/src/main/java/org/geoserver/security/oauth2/OpenIdConnectAuthenticationProvider.java Co-authored-by: Jody Garnett <jody.garnett@gmail.com>

davidblasby · 2022-09-20T16:18:35Z

FYI - rebased due to test case failure (not to do with this pr)

davidblasby · 2022-09-20T20:05:17Z

@taba90 - ready to merge?

taba90

looks good to me, merging...

davidblasby requested a review from jodygarnett September 8, 2022 21:17

jodygarnett reviewed Sep 8, 2022

View reviewed changes

...2/oauth2-core/src/main/java/org/geoserver/security/oauth2/GeoServerAccessTokenConverter.java Show resolved Hide resolved

jodygarnett changed the title ~~Accesstokens~~ [GEOS-10646] OAuth access tokens Sep 8, 2022