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

Interceptor security #386

Merged
merged 4 commits into from
Jul 16, 2020
Merged

Interceptor security #386

merged 4 commits into from
Jul 16, 2020

Conversation

weskamm
Copy link
Member

@weskamm weskamm commented Jul 15, 2020

This PR introduces enhanced and secured interceptor classes for WMS, WFS and WMTS.

With these changes, layer security from shogun is respected when calling layers through the geoserver.action endpoint, so users may e.g. only see / getmap a layer when they have the READ permission.

The geoserver.action endpoint is now capable of handling WMTS KVP and Restful requests. See below for example URLs.
In the long term, the exisiting wmts.action should be removed, as it does not handle permissions at all and just acts as a proxy.

Capabilities and DescribeFeatureType Documents for the different service types are cleaned to only contain allowed layers and internal geoserver URLs get replaced b the geoserver.action interface.

@terrestris/devs please review

Example URLs:

WMS
Getmap: http://localhost/shogun2-webapp/geoserver.action/SHOGUN?SERVICE=WMS&VERSION=1.1.1&REQUEST=GetMap&FORMAT=image%2Fpng&TRANSPARENT=true&LAYERS=SHOGUN%3ACOUNTRIES_LAKES&TILED=true&WIDTH=256&HEIGHT=256&SRS=EPSG%3A3857&STYLES=&BBOX=0%2C5009377.085%2C5009377.085%2C10018754.17
Featureinfo: http://localhost/shogun2-webapp/geoserver.action/SHOGUN?SERVICE=WMS&VERSION=1.1.1&REQUEST=GetFeatureInfo&FORMAT=image%2Fpng&TRANSPARENT=true&QUERY_LAYERS=SHOGUN%3ACOUNTRIES_LAKES&LAYERS=SHOGUN%3ACOUNTRIES_LAKES&TILED=true&INFO_FORMAT=application%2Fjson&FEATURE_COUNT=100&X=56&Y=170&WIDTH=256&HEIGHT=256&SRS=EPSG%3A3857&STYLES=&BBOX=0%2C5009377.085%2C5009377.085%2C10018754.17&_dc=1594708414426
GetLegend: http://localhost/shogun2-webapp/geoserver.action/SHOGUN?request=GetLegendGraphic&service=WMS&version=1.1.1&format=image%2Fpng&layer=SHOGUN%3ACOUNTRIES_LAKES&legend_options=forceLabels%3Aon
GetCapabilities: http://localhost/shogun2-webapp/geoserver.action/SHOGUN?SERVICE=WMS&VERSION=1.1.1&REQUEST=GetCapabilities

WFS
GetFeature: http://localhost/shogun2-webapp/geoserver.action/SHOGUN?SERVICE=WFS&VERSION=1.0.0&REQUEST=GetFeature&typename=SHOGUN:COUNTRIES_LAKES
DescribeFeatureType: http://localhost/shogun2-webapp/geoserver.action/SHOGUN?SERVICE=WFS&VERSION=1.0.0&REQUEST=DescribeFeatureType
                     http://localhost/shogun2-webapp/geoserver.action/SHOGUN?SERVICE=WFS&VERSION=1.0.0&REQUEST=DescribeFeatureType&typeName=SHOGUN:COUNTRIES_LAKES
GetCapabilities: http://localhost/shogun2-webapp/geoserver.action/SHOGUN?SERVICE=WFS&VERSION=1.0.0&REQUEST=GetCapabilities

WMTS:
GetTile : http://localhost/shogun2-webapp/geoserver.action/SHOGUN?layer=SHOGUN:Demographics_USA_Population_Density_1594311226978&style=&tilematrixset=EPSG%3A4326&Service=WMTS&Request=GetTile&Version=1.0.0&Format=image%2Fjpeg&TileMatrix=EPSG%3A4326%3A4&TileCol=5&TileRow=4
          http://localhost/shogun2-webapp/geoserver.action/SHOGUN/Demographics_USA_Population_Density_1594311226978/raster/EPSG:4326/EPSG:4326:4/4/5?format=image/png
Featureinfo: http://localhost/shogun2-webapp/geoserver.action/SHOGUN?VERSION=1.0.0&LAYER=SHOGUN:Demographics_USA_Population_Density_1594311226978&STYLE=&TILEMATRIX=EPSG:4326:1&TILEMATRIXSET=EPSG:4326&SERVICE=WMTS&FORMAT=image/jpeg&REQUEST=GetFeatureInfo&INFOFORMAT=text/html&TileCol=0&TileRow=0&I=10&J=172
             http://localhost/shogun2-webapp/geoserver.action/SHOGUN/Demographics_USA_Population_Density_1594311226978/raster/EPSG:4326/EPSG:4326:4/4/5/199/181?format=text/html
GetCapabilities : http://localhost/shogun2-webapp/geoserver.action/SHOGUN?SERVICE=WMTS&VERSION=1.0.0&REQUEST=GetCapabilities

@weskamm weskamm changed the title Interceptor Interceptor security Jul 15, 2020
@buehner
Copy link
Member

buehner commented Jul 15, 2020

Wow. Huge changeset and hard to review quickly for me as i currently dont have the time. But i like the idea/enhancements.

Just one important thought on this: This may have a heavy performance impact! Especially when accessing all/many layers, iterating them and then accessing all sources (hibernate lazy loading i think)...

Maybe we should have a sharp look and try to enhance such requests, measure the impact or tweak the SQL caching or similar. Just mentioning this as an idea/thought as i know that we already disabled the whole geoserver.action in a customer project due to it's performance impacts (in the customers case ~10 times slower than accessing the GeoServer directly)

@weskamm
Copy link
Member Author

weskamm commented Jul 15, 2020

I know that this is a performance penalty, but you need to decided between security and speed here.

It may of course be possible to save some information in memory about entity / permission for faster access. That would be an enhancement in a seperate PR.

One can still switch to the old interceptor implementations or disable them, but with this PR the interface will be secured per default for those layer types

Copy link
Member

@dnlkoch dnlkoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @weskamm!

I noted some questions you may want to address, but others should chime in as well. A lot of sensitive code it is…

public Response interceptGeoServerRequest(HttpServletRequest request, Optional<String> endpoint)
public Response interceptGeoServerRequest(
HttpServletRequest request,
HashMap<String, Optional<String>> optionals)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have the optionals typed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure here, do you want me to instantiate the hashmap with null values as class variable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of a simple class definition for optionals that defines the structure of the parameter.

…rvice/GeoServerInterceptorService.java

Co-authored-by: Daniel Koch <[email protected]>
@weskamm
Copy link
Member Author

weskamm commented Jul 16, 2020

Thanks for reviews. Merging now!

@weskamm weskamm merged commit 7e7a959 into master Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants