Geoserver: protect web interface and ows routes behind magpie/twitcher #348
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
component/geoserver
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1682/Result : failure BIRDHOUSE_DEPLOY_BRANCH : geoserver-behind-twitcher DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-154.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1199/NOTEBOOK TEST RESULTS |
fmigneault
requested changes
Jun 19, 2023
There was a problem hiding this comment.
I don't think this is the right approach.
Although this looks to work, it is very hackish and does not handle all use cases.
For example, the WPS endpoint can be accessed in multiple ways:
https://pavics.ouranos.ca/geoserver/ows?request=GetCapabilities&service=wps
https://pavics.ouranos.ca/geoserver/wps?request=GetCapabilities
Same for all other W*[X]*S variants:
https://pavics.ouranos.ca/geoserver/wms?request=GetCapabilities
https://pavics.ouranos.ca/geoserver/ows?request=GetCapabilities&service=wms
This does not include also the upcoming OGC API endpoints:
/geoserver/ogc/features/...
/geoserver/ogc/maps/...
etc.
Using different services causes a lot of permission duplication across the endpoints, which will cause access errors. Instead the Magpie implementation should be updated to handle the /web/ endpoint accordingly:
Is this a change you already have proposed for Magpie or should I add an issue for it? |
|
What if we flipped the nginx conf for now: That way we can accommodate the Then when magpie catches up we can put it back under one magpie service definition |
You can add it. It was not already planned. It should be fairly easy to extend the existing class using |
This can potentially completely open GeoServer if the admin adds the To properly configure it for web-only access, they need to actually create a It is possible to do workarounds, but I think this causes more confusion than fixing the missing implementation on the Magpie side. |
tlvu
reviewed
Jun 29, 2023
| @@ -1,5 +1,16 @@ | |||
| location /geoserver/ { | |||
| proxy_pass http://${PAVICS_FQDN}:8087; | |||
| proxy_pass http://${PAVICS_FQDN}${TWITCHER_PROTECTED_PATH}/geoserver-web/; | |||
There was a problem hiding this comment.
Would it be easy to have a toogle to disable this? Meaning a toogle to restore the original proxy config to not go through Twitcher?
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1877/Result : failure BIRDHOUSE_DEPLOY_BRANCH : geoserver-behind-twitcher DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-35.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1230/NOTEBOOK TEST RESULTS |
|
Maggie https://github.com/Ouranosinc/Magpie/releases/tag/3.35.0 is released with the relevant functionality. |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2058/Result : failure BIRDHOUSE_DEPLOY_BRANCH : geoserver-behind-twitcher DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-216.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1305/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2059/Result : failure BIRDHOUSE_DEPLOY_BRANCH : geoserver-behind-twitcher DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-216.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1306/NOTEBOOK TEST RESULTS |
fmigneault
reviewed
Sep 14, 2023
Comment on lines
3
to
4
| resource: / | ||
| type: route |
There was a problem hiding this comment.
I think this is not necessary, or might cause a problem. At this / level, the type is the service itself, not a nested route.
There was a problem hiding this comment.
No problem. I don't think I understood how to set this properly with the new geoserver. Do you mind pointing me to the documentation or an example please
There was a problem hiding this comment.
The resource field is needed mostly when applying a permission on a child resource under the service. Since the permission is applied directly on the service such that it applies for all its children, the resource shouldn't be needed.
The config might be completely valid, I just don't remember if Magpie code handled the / correctly to resolve as the service. If it does, it's good to leave the config as is.
CHANGES.md
Outdated
| ## Changes | ||
| - Geoserver: protect web interface and ows routes behind magpie/twitcher | ||
|
|
||
| Updates Magpie version to 3.35.0 in order to take advantage of the updated Geoserver Service. |
There was a problem hiding this comment.
Use the tag link [3.35.0](https://github.com/Ouranosinc/Magpie/tree/3.35.0)
CHANGES.md
Outdated
Comment on lines
19
to
23
| - Geoserver: protect web interface and ows routes behind magpie/twitcher | ||
|
|
||
| Updates Magpie version to 3.35.0 in order to take advantage of the updated Geoserver Service. | ||
|
|
||
| See https://github.com/bird-house/birdhouse-deploy/issues/333 for details. |
There was a problem hiding this comment.
Should be a bit more verbose about the "potentially breaking" changes:
geoserverwmsMagpie service deprecated (could need manual permission updates to be transferred togeoserverif used by some external configuration)geoserverhaving new secured access by default instead of using https://github.com/bird-house/birdhouse-deploy/tree/master/birdhouse/optional-components/test-geoserver-secured-access- breaks instances that assumed fully-open access to
/geoserverendpoint and its OWS requests - makes
optional-components/test-geoserver-secured-accessdeprecated andgeoserver-securedendpoint and service deprecated as well
- breaks instances that assumed fully-open access to
- warning about
all-public-accessthat will now apply additional permissions for geoserver as well
(somewhat of an edge-case, but someone that was usinggeoserver-sercured+all-public-accesstechnically had a "more secured" geoserver that what these changes provide, see https://github.com/bird-house/birdhouse-deploy/blob/master/birdhouse/optional-components/test-geoserver-secured-access/config/magpie/permissions.cfg.template).
Since the new permission set allows "writable" or "destructive" operations on layers, this could be an issue.
There was a problem hiding this comment.
Ok I can add some of these as warnings.
makes optional-components/test-geoserver-secured-access deprecated and geoserver-secured endpoint and service deprecated as well
If these are now deprecated, I could move them to the deprecated-components directory as part of this PR. What would you suggest @fmigneault
There was a problem hiding this comment.
Yes, that would be good. Thank you.
Comment on lines
44
to
47
| - service: geoserver | ||
| permission: describeprocess | ||
| group: anonymous | ||
| action: create |
There was a problem hiding this comment.
This is not relevant if wps: false. Same as other comment.
Comment on lines
28
to
31
| - service: geoserver | ||
| permission: execute | ||
| group: anonymous | ||
| action: create |
There was a problem hiding this comment.
This is not relevant if wps: false.
I don't think it would break, so can be left there with a comment regarding wps option for reference.
It should update the
If this is not what happens, I would need further logs to investigate.
That is correct. Magpie will not merge services together. This must be done manually. Steps should be (pseudo SQL, don't use directly):
It could be formalized in a specific script. |
|
Here's the relevant section of the Magpie logs: The URL for the geoserver (type = wfs) is updated but the actual "type" is not updated. Since Magpie thinks that it has processed the changes for this provider, it doesn't create a new service with type = geoserver or update to current service to type = geoserver. I think that we're missing a line like: |
|
@mishaschwartz |
Ok cool. Should we update that on the Magpie side then? This PR can wait until Magpie has been updated |
fmigneault
added a commit
to Ouranosinc/Magpie
that referenced
this pull request
Oct 3, 2023
|
https://github.com/Ouranosinc/Magpie/tree/3.36.0 pushed. The docker image should be ready in ~10min. |
|
@fmigneault I've bumped the magpie version so that the service type will be updated nicely. With this update, we still need to manually do steps 2 and 3 from here first right? #348 (comment) |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2151/Result : failure BIRDHOUSE_DEPLOY_BRANCH : geoserver-behind-twitcher DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-118.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1363/NOTEBOOK TEST RESULTS |
yes |
|
@tlvu Are you ok with this PR? I'd like to get it merged soon if there's no outstanding changes that need to be made |
tlvu
approved these changes
Oct 30, 2023
2 tasks
|
If you have permissions in magpie set for the |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2202/Result : failure BIRDHOUSE_DEPLOY_BRANCH : geoserver-behind-twitcher DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-104.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1390/NOTEBOOK TEST RESULTS |
fmigneault
added a commit
that referenced
this pull request
Nov 3, 2023
…ions (#397) ## Overview The `type: route` is invalid in that case, because the permission is applied on the service itself (not a child `route` resource). The type should be `service`, but can be omitted when no `resource` is specified, as in the case for all other permissions. ## Changes **Non-breaking changes** - Remove erroneous `type: route` for a Magpie permission set directly on the service. **Breaking changes** - n/a ## Related Issue / Discussion - Introduced by #348
tlvu
reviewed
Apr 12, 2024
| ## Changes | ||
| - Geoserver: protect web interface and ows routes behind magpie/twitcher | ||
|
|
||
| Updates Magpie version to [3.35.0](https://github.com/Ouranosinc/Magpie/tree/3.35.0) in order to take advantage of |
There was a problem hiding this comment.
Woops, the actual Magpie in this PR is 3.36 and not 3.35 !
|
I test deploy this against our Magpie production DB, got the following warning, just want to ensure these are normal and expected: |
|
@tlvu |
tlvu
added a commit
to Ouranosinc/PAVICS-e2e-workflow-tests
that referenced
this pull request
Apr 15, 2024
# Overview Add a new text box `CONFIG_OVERRIDE_SCRIPT_URL` so we can specify a URL to a script that can be sourced to alter any configs before starting the test run. A sample script is provided to filter the notebooks for testing GeoServer only. This greatly improve the turn over time by avoiding to run unrelated notebooks. Was useful to test this PR bird-house/birdhouse-deploy#348. This custom script can be coming from any URL and change other configs than just the list of notebooks to run. Default is no custom script URL, same as the current behavior. @fmigneault excellent question: I'm curious to better understand the use case to see if it is the right (and only) solution. The current (CRIM) birdhouse-deploy CI sources the `env.local` file, starts a test instance with it, and then forwards the env values to the children Jenkins job for E2E workflow tests. In other words, if a variable needs to be overridden for tests, it can be set directly in it, or a custom component can be defined and listed at the end of `EXTRA_CONF_DIRS`. There is also a `EXTRA_TEST_ENV_VAR` variable for yet even more additional variables to pass between the test instance creation and the E2E workflow tests. This is notably useful for manual test trigger or quick tweaks against some existing branch/config combination. Is there no way to use those approaches or accomplish similar behavior in case another CI (or local test) are used? More specifically, is there a need for yet another method to override variables? @tlvu answer: 2 reasons: 1. The existing way via `env.local` only works for CRIM pipeline where each PAVICS deployment is meant to run only one test run, then the deployment is discarded. My test servers are meant for multiple tests configs so I'd rather not have to change my `env.local` each time and having to restart the stack, wait for all the compoments to be ready, then launch Jenkins, it's too slow. Furthermore, if I want to test against the production instance, I won't be able to touch the `env.local`. 1. The existing way can only impact "input" configs, not the "resulting" configs. The list of notebooks to run is not part of the input, it is calculated so there is no other way to modify it than to intercept it the way I did. I had thought of exposing a big text box to manually specify the list of notebooks but that is way too cumbersome. Filtering it programmatically is way more flexible and repeatable across multiples runs. Talking about flexibility, this "hook" can do way more than just filtering the list of notebooks. Any extra oneoff pre-processing steps for some edge-case scenario just before launching `py.test` can be done there. Those are edge-cases processing so can not be committed as "regular" steps. @fmigneault Just to be clear, this new way to customize the config is not meant to replace the existing way. It just gives us new possibilities not possible before. It is 100% backward-compatible with the existing way so there are no changes required on CRIM pipeline side. If whatever you are doing already works, keep it, no need to change anything.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Geoserver: protect web interface and ows routes behind magpie/twitcher
Updates Magpie version to 3.35.0 in order to take advantage of updated Geoserver Service.
The
geoserverwmsMagpie service is now deprecated. If a deployment is currently using this service, it is highly recommended that the permissions are transferred from the deprecatedgeoserverwmsservice to thegeoserverservice.The
/geoserverendpoint is now protected by default. If a deployment currently assumes open access to Geoserver and would like to keep the same permissions after upgrading to this version, please update the permissions for thegeoserverservice in Magpie to allow theanonymousgroup access.A
Magpieservice namedgeoserverwith typewfsexists already and must be manually deleted before the newMagpieservice created here can take effect.The
optional-components/all-public-accesscomponent provides full access to thegeoserverservice for theanonymousgroup in Magpie. Please note that this includes some permissions that will allow anonymous users to perform destructive operations. Because of this, please remember that enabling theoptional-components/all-public-accesscomponent is not recommended in a production environment.Introduces the
GEOSERVER_SKIP_AUTHenvironment variable. If set toTrue, then requests to the geoserver endpoint will not be authorized through twitcher/magpie at all. This is not recommended at all. However, it will slightly improve performance when accessing geoserver endpoints.Changes
Non-breaking changes
In order to provide public access to geoserver by default now, the
all-public-accessoptional component must be enabledBreaking changes
The current
wfsMagpie service namedgeoservermust be deleted before the change here can take effect.Related Issue / Discussion
Additional Information