[GEOT-7557] Provide the ability for downstream applications (e.g. GeoServer) to append additional querystring parameters e.g. AuthKey to every request made by the HTTPClient

[petersmythe]

https://osgeo-org.atlassian.net/browse/GEOT-7557

Supports github.com/geoserver/geoserver/pull/7552

Background: There is a GeoServer option to configure a WMTS data store with HTTP Headers (as well as basic username/password), however the HTTP Headers option is not available for WMS data stores (only username/password).

In addition, specifically for AuthKeys/API keys, some WMS/WMTS servers are not aware of authentication mechanisms that they sit behind. They then return Capabilities documents that do not include the authentication mechanism appended to the published URLs. Clients that properly parse the Capabilities documents (QGIS, GeoServer) find that they can securely request the Capabilities documents however break on the subsequent Map/Tile requests.

I propose to add an AuthKey option to both WMS and WMTS data stores.

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).
• Avoid Java 9+ split packages.
• 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).
• There is an issue in GeoTools Jira (except for changes not visible to end users).
• Commit message(s) must be in the form [GEOT-XYZW] Title of the Jira ticket.
• Bug fixes and small new features are presented as a single commit.
• The commit targets a single objective (if multiple focuses cannot be avoided, each one is in its own commit, and has a separate ticket describing it).

[petersmythe]

Downstream integration build with GeoServer fails with:

/home/runner/geoserver/src/main/src/test/java/org/geoserver/test/http/MockHttpClient.java:[26,8] org.geoserver.test.http.MockHttpClient is not abstract and does not override abstract method setAuthKey(java.lang.String) in org.geotools.http.HTTPClient

[jodygarnett]

This is a geotools change so it will need a GeoTools ticket please.

[aaime]

@roarbra do you have time to look at this change? Seems up your alley.

[roarbra]

This was strange. Having a AuthKey that are appended to the query string of the url is not a common way of handling authentication. Rather the opposite. It's not recommended to have secrets in the url.

Having said that. I like the idea of having a way to add query parameters to the url from within Geoserver, but it shouldn't be named authKey and it should be open for more than just a single value. Like for instance a Map<String, Object> parameters.

[petersmythe]

This was strange. Having a AuthKey that are appended to the query string of the url is not a common way of handling authentication. Rather the opposite. It's not recommended to have secrets in the url.

Having said that. I like the idea of having a way to add query parameters to the url from within Geoserver, but it shouldn't be named authKey and it should be open for more than just a single value. Like for instance a Map<String, Object> parameters.

I agree, an AuthKey/API key in the querystring is not ideal (certainly AWS API Gateway refuses to implement it for that reason), however, it is still an extremely popular way to provide REST authentication, including our own GeoServer extension called AuthKey. I think the HTTP headers option is far better but thought while I was busy with fixing WMS stores geoserver/geoserver#7552 to include an API key option. Should I rather undo the work?

I currently can't see the benefit of making it more generic, but I understand what you're asking for. Should I spend the extra time to support a Map<String, Object> additionalQuerystringParams rather, and then the GeoServer panel can still be called AuthKey/API Key?

[mprins]

Having a AuthKey that are appended to the query string of the url is not a common way of handling authentication

Actually there are quite a few providers that have an "API user key" in the url as a query param, eg. Google, Thunderforest, Bing to name a few, just like there are many that actually provide a unique url with a token eg. ttps://s.map5.nl/map/bbpp.6488ab/service?layer=map5topo_simple&style=default&tilematrixset=geonovum_grid&Service=WMTS&Request=GetTile&Version=1.0.0&Format=image%2Fjpeg&TileMatrix=11&TileCol=967&TileRow=1032

[roarbra]

Having a AuthKey that are appended to the query string of the url is not a common way of handling authentication

Actually there are quite a few providers that have an "API user key" in the url as a query param, eg. Google, Thunderforest, Bing to name a few, just like there are many that actually provide a unique url with a token eg. ttps://s.map5.nl/map/bbpp.6488ab/service?layer=map5topo_simple&style=default&tilematrixset=geonovum_grid&Service=WMTS&Request=GetTile&Version=1.0.0&Format=image%2Fjpeg&TileMatrix=11&TileCol=967&TileRow=1032

Yes, you're right. That is a unique key providing a user specific url. That has nothing to do with authentication. The problem with the PR was that it were introducing a property called "authKey" that have nothing to do with a general HTTPClient. Rather it is specific for a very special situation.

[aaime]

@petersmythe the API here could be a Map, and the GUI in GeoServer could be a simple text area that accepts a property file syntax. With the right Wicket converter/model you can turn that into the expected map object.

[petersmythe]

@petersmythe the API here could be a Map, and the GUI in GeoServer could be a simple text area that accepts a property file syntax. With the right Wicket converter/model you can turn that into the expected map object.

Understood, I will make the GeoTools API more generic and keep the GeoServer implementation more specific - just a single AuthKey text field is required, I believe.

[roarbra]

I currently can't see the benefit of making it more generic, but I understand what you're asking for. Should I spend the extra time to support a Map<String, Object> additionalQuerystringParams rather, and then the GeoServer panel can still be called AuthKey/API Key?

I don't know why you needed to have such a long name.

If you would like to see the reason to make it more generic, you could have a look at GEOT-7097.

[petersmythe]

Would you mind taking another look over the PR @roarbra ?

[roarbra]

Would you mind taking another look over the PR @roarbra ?

That looks a lot better. Thanks!

[petersmythe]

GeoServer still successfully builds with the PR at this time:

[petersmythe]

Merging with geoserver/geoserver#7552

[aaime]

The backport to 31.x failed:

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

stderr

error: could not apply 5282fc86a2... Add AuthKey by appending to URL in HTTPClient
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

stdout

[backport-4727-to-31.x 70bb412cc6] To support HTTP Headers see github.com/geoserver/geoserver/pull/7552
 Author: Peter Smythe <peter@afrigis.co.za>
 Date: Wed Apr 17 12:57:08 2024 +0200
 1 file changed, 21 insertions(+)
Auto-merging modules/library/http/pom.xml
CONFLICT (content): Merge conflict in modules/library/http/pom.xml
Auto-merging modules/plugin/http-commons/pom.xml
CONFLICT (content): Merge conflict in modules/plugin/http-commons/pom.xml
Auto-merging modules/plugin/mongodb/pom.xml
CONFLICT (content): Merge conflict in modules/plugin/mongodb/pom.xml

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-31.x 31.x
# Navigate to the new working tree
cd .worktrees/backport-31.x
# Create a new branch
git switch --create backport-4727-to-31.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 2c64415c672901095533b5c75fe25a8e7bcbc196,5282fc86a283dac457aa0ee9ba9e8ed4c9c4d93b,e75b49195d58f635b1bb0a9788956934a00aaee8,8c9e05cc9dbb273a049b473dc3a7a3fec4bfccde,cbcd07ce43e7d2caa109579685edf3c273508890,c2e8460f88fd72d59af4ea56d1376ee97e34fef0,bfab9b24983eb6f6207581c548b5589dc5a2c5f4,9cac6a53965d2cb69780222849bbcafec3aee201,da90c1663aa0de0ed0622b5b041866c772bf214c,20993c164f6d46e6e495cd956207ce199a4696b6,f22ce7a77a9d47c49721cf5fa450e57e404862fe,711c099db0ed9fbdb67adba7113d63e1cace7910,2ad536e9990454870e7bde15386b7fe2424f20a9,8dd3aa97f3d77237eeb1b87f2f1bcdde2276bf53,0f8d61aed046585763591a66619e968b0de2826f,6fdafbd7e9b5eb3ff7510be19c07718bed4f4551,5b00f2ef7c4000ce86083f6877933963a86593cb,a754bea7af2a2856b3601db6ff4c67e4673d07f2,741510a74bf1660f8e21fee3a86ccc08e22069f0,d1807c6a4f9406fbb9bb414c18db14e2f93c00e7,16fd45686aa23eb7a3e9c70d348b1381d39ad683,a29aabc35cef1610ed095bf0a20ae9ccc9b749cd,5a04c3a3bd328e8c3d25cc6afde012449105ecac,d79060463c0649659439eafeefa6c83633cb7d3c,7ef354bf7f00c59297ba74b1672a1ab279a64603,97321be859c58b1e323153c89ee38bf99b39620c,7ad1c8177df106e826f843b4c3dc49ae9d1e8c07,40ea5b296e5904939a2c0a3a2f8d2259d40691b8,f7560f897e42757028223e2d951c8cc2f35156ec,0f22d69735f97201e6132650f50e1c2be31b5934
# Push it to GitHub
git push --set-upstream origin backport-4727-to-31.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-31.x

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