[GEOS-11330] OAuth2 kid verification should be optional #7470

afabiani · 2024-03-11T12:20:02Z

References: https://osgeo-org.atlassian.net/browse/GEOS-11330

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).
The REST API docs have been updated (when changing configuration objects or the REST controllers).
There is an issue in the GeoServer Jira (except for changes that do not affect administrators or end users in any way).
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).

aaime · 2024-03-18T10:34:32Z

...d-connect-core/src/main/java/org/geoserver/security/oauth2/ValidatingOAuth2RestTemplate.java

-        if (jwkUri != null) this.store = new JwkTokenStore(jwkUri);
+        if (jwkUri != null) {
+            this.store = new JwkTokenStore(jwkUri);
+        }


Unnecessary change?

This is actually the fix, the KID verification is optional

aaime · 2024-03-18T10:39:09Z

...d-connect-core/src/test/java/org/geoserver/security/oauth2/OpenIdConnectIntegrationTest.java

@@ -78,7 +78,7 @@ public static void beforeClass() throws Exception {
                                        .withStatus(200)
                                        .withHeader(
                                                "Content-Type", MediaType.APPLICATION_JSON_VALUE)
-                                        .withBodyFile("jkws.json")));
+                                        .withBodyFile("jwks.json")));


Corrects the name here but the test resource modified is still named jkws.json? Is it used?

The resource has been renamed as well and added a test case.

aaime · 2024-03-18T10:39:26Z

...d-connect-core/src/main/java/org/geoserver/security/oauth2/ValidatingOAuth2RestTemplate.java

+                try {
+                    store.readAuthentication(idToken);
+                } catch (InvalidTokenException e) {
+                    LOGGER.warning("Failed to validate ID token: " + e.getMessage());
+                }


What if the validation failed for something other than the kid verification? This seems to be ignoring all other sorts of validity issues?

This is actually the fix, the KID verification is optional

Is KID verification the only possible reason to get an InvalidTokenException? Or will we ignore also verification failures that should be considered?

Most likely this is the reason, other reasons might be related to the impossibility to decrypt due to cypher algorythm. In any case the verification is optional for the protocol.
In order to intercept the specific one what we can do is to inspect the exception message probably even if, generally speaking, that shouldn't be a blocking error at all.

What about having an option on the GUI in order to enforce the velidation or not instead?

I agree that sounds like a safer approach (let the admin decide whether to verify or not)

…make this available by the Admin

...-web/src/test/java/org/geoserver/web/security/oauth2/OpenIdConnectAuthProviderPanelTest.java

…OS-11330

aaime · 2024-03-27T13:42:10Z

Thanks!

aaime · 2024-03-27T13:42:30Z

The backport to 2.24.x failed:

The process '/usr/bin/git' failed with exit code 128

stderr

error: commit 4d89ccee81959cbc375b5578d3758de2ab13e28a is a merge but no -m option was given.
fatal: cherry-pick failed

stdout

[backport-7470-to-2.24.x 6acd9dcdc1] [GEOS-11330] kid verification should be optional
 Author: afabiani <alessio.fabiani@geosolutionsgroup.com>
 Date: Mon Mar 11 13:19:27 2024 +0100
 3 files changed, 25 insertions(+), 3 deletions(-)
[backport-7470-to-2.24.x 878b7baa4a]  - Introducing the Enforce Token Validation panel option in order to make this available by the Admin
 Author: afabiani <alessio.fabiani@geosolutionsgroup.com>
 Date: Tue Mar 19 19:03:32 2024 +0100
 9 files changed, 70 insertions(+), 4 deletions(-)

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.24.x 2.24.x
# Navigate to the new working tree
cd .worktrees/backport-2.24.x
# Create a new branch
git switch --create backport-7470-to-2.24.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick ecb4184a5065be3bec7be8cbc043f3f5f37b7b4f,d985ca9ba08c43780ad270faee4584b42085d279,4d89ccee81959cbc375b5578d3758de2ab13e28a,91ded7c5685e2154a53b25e5b7bdc1c0871107d4
# Push it to GitHub
git push --set-upstream origin backport-7470-to-2.24.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.24.x

Then, create a pull request where the base branch is 2.24.x and the compare/head branch is backport-7470-to-2.24.x.

aaime · 2024-03-27T13:42:32Z

The backport to 2.25.x failed:

The process '/usr/bin/git' failed with exit code 128

stderr

error: commit 4d89ccee81959cbc375b5578d3758de2ab13e28a is a merge but no -m option was given.
fatal: cherry-pick failed

stdout

[backport-7470-to-2.25.x 61fc418c05] [GEOS-11330] kid verification should be optional
 Author: afabiani <alessio.fabiani@geosolutionsgroup.com>
 Date: Mon Mar 11 13:19:27 2024 +0100
 3 files changed, 25 insertions(+), 3 deletions(-)
[backport-7470-to-2.25.x 85c26b44da]  - Introducing the Enforce Token Validation panel option in order to make this available by the Admin
 Author: afabiani <alessio.fabiani@geosolutionsgroup.com>
 Date: Tue Mar 19 19:03:32 2024 +0100
 9 files changed, 70 insertions(+), 4 deletions(-)

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.25.x 2.25.x
# Navigate to the new working tree
cd .worktrees/backport-2.25.x
# Create a new branch
git switch --create backport-7470-to-2.25.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick ecb4184a5065be3bec7be8cbc043f3f5f37b7b4f,d985ca9ba08c43780ad270faee4584b42085d279,4d89ccee81959cbc375b5578d3758de2ab13e28a,91ded7c5685e2154a53b25e5b7bdc1c0871107d4
# Push it to GitHub
git push --set-upstream origin backport-7470-to-2.25.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.25.x

Then, create a pull request where the base branch is 2.25.x and the compare/head branch is backport-7470-to-2.25.x.

* [GEOS-11330] kid verification should be optional * - Introducing the Enforce Token Validation panel option in order to make this available by the Admin * - Fixing OpenIdConnectAuthProviderPanelTest

[GEOS-11330] kid verification should be optional

ecb4184

afabiani self-assigned this Mar 11, 2024

afabiani changed the title ~~[GEOS-11330] kid verification should be optional~~ [GEOS-11330] OAuth2 kid verification should be optional Mar 14, 2024

aaime reviewed Mar 18, 2024

View reviewed changes

- Introducing the Enforce Token Validation panel option in order to …

d985ca9

…make this available by the Admin

afabiani requested a review from aaime March 19, 2024 18:03

aaime reviewed Mar 25, 2024

View reviewed changes

...-web/src/test/java/org/geoserver/web/security/oauth2/OpenIdConnectAuthProviderPanelTest.java Show resolved Hide resolved

afabiani added 2 commits March 27, 2024 13:07

Merge branch 'main' of https://github.com/geoserver/geoserver into GE…

4d89cce

…OS-11330

- Fixing OpenIdConnectAuthProviderPanelTest

91ded7c

aaime added backport 2.24.x Instructs the bot to create a 2.24.x backport PR on merge backport 2.25.x Instructs the bot to create a 2.25.x backport PR on merge labels Mar 27, 2024

aaime merged commit c7c2dd7 into main Mar 27, 2024
13 checks passed

aaime deleted the GEOS-11330 branch March 27, 2024 13:42

aaime added the failed backport label Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GEOS-11330] OAuth2 kid verification should be optional #7470

[GEOS-11330] OAuth2 kid verification should be optional #7470

afabiani commented Mar 11, 2024 •

edited by pull-request-badge bot

Loading

aaime Mar 18, 2024

afabiani Mar 18, 2024

aaime Mar 18, 2024

afabiani Mar 18, 2024

aaime Mar 18, 2024

afabiani Mar 18, 2024

aaime Mar 18, 2024

afabiani Mar 18, 2024

aaime Mar 18, 2024

afabiani Mar 19, 2024

aaime commented Mar 27, 2024

aaime commented Mar 27, 2024

aaime commented Mar 27, 2024

[GEOS-11330] OAuth2 kid verification should be optional #7470

[GEOS-11330] OAuth2 kid verification should be optional #7470

Conversation

afabiani commented Mar 11, 2024 • edited by pull-request-badge bot Loading

Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaime commented Mar 27, 2024

aaime commented Mar 27, 2024

aaime commented Mar 27, 2024

afabiani commented Mar 11, 2024 •

edited by pull-request-badge bot

Loading