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

secure config for thredds #99

Merged
merged 7 commits into from Nov 12, 2020

Conversation

fmigneault
Copy link
Collaborator

No description provided.

@crim-jenkins-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@tlvu
Copy link
Collaborator

tlvu commented Nov 10, 2020

LGTM. Can you add brief description of the new component to README https://github.com/bird-house/birdhouse-deploy/blob/master/birdhouse/optional-components/README.md (follow existing format) and also add to

#export EXTRA_CONF_DIRS="./optional-components/canarie-api-full-monitoring
# ./optional-components/emu
# ./optional-components/testthredds
# ./optional-components/generic_bird
# ./optional-components/all-public-access
# ./components/scheduler
# ./components/monitoring
# /path/to/private-config-repo"

permissions:
# this can be combined with 'all-public-access'
# it only ensures that it is set if not also employed
- service: thredds
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the README, make clear to user this will make all of Thredds publicly readable, except those 2 secure folders. Once enabled, to disable, user have to login to Magpie and perform manual changes, diactivating the component is not enough. Similar to https://github.com/bird-house/birdhouse-deploy/tree/master/birdhouse/optional-components#give-public-access-to-all-resources-for-testing-purposes

Copy link
Collaborator

Choose a reason for hiding this comment

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

On second though, do you even need this section? Just let user decide what they want to expose themselves. You just want to ensure those 2 folders are secured.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Acutally, coming back on my previous comment.
Yes, since the user can decide or not to add both the public-access + secure, no need to add the public one again here.

Copy link
Collaborator

@tlvu tlvu left a comment

Choose a reason for hiding this comment

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

I think it safer to not force public Thredds content on the user. So yeah, change that and update README and env.local also, thanks.

I guess we will have to test this together with the magpie upgrade? This can not be tested alone.

@fmigneault
Copy link
Collaborator Author

@tlvu
The feature makes sense only when combined with public at an higher level, otherwise the directories/files are blocked by default and there is not much point to add the explicit permission on the "secure" directories.

@dbyrns
Copy link
Collaborator

dbyrns commented Nov 10, 2020

Maybe there is no point into adding permissions for thredds after all. Because, even if it would make sense to add permission to the secure directory, from the birdhouse-deploy repo perspective, we should not even know that this folder exists. For wps permissions it can make sense because the API is known, but otherwise it is a per-node config that really should only exist in the private config.

@fmigneault
Copy link
Collaborator Author

Maybe there is no point into adding permissions for thredds after all. Because, even if it would make sense to add permission to the secure directory, from the birdhouse-deploy repo perspective, we should not even know that this folder exists. For wps permissions it can make sense because the API is known, but otherwise it is a per-node config that really should only exist in the private config.

Indeed, most of these kind on configs should be only on private server side.
I see that entry more for a documentation example.

The listed directories are the only ones I could find that made sense to protect.
They are generated via https://github.com/bird-house/birdhouse-deploy/blob/master/birdhouse/scripts/bootstrap-testdata, so should be present on instances deployed for testing minimally.

@fmigneault fmigneault requested a review from tlvu November 10, 2020 21:10
@tlvu
Copy link
Collaborator

tlvu commented Nov 10, 2020

Maybe there is no point into adding permissions for thredds after all. Because, even if it would make sense to add permission to the secure directory, from the birdhouse-deploy repo perspective, we should not even know that this folder exists. For wps permissions it can make sense because the API is known, but otherwise it is a per-node config that really should only exist in the private config.

I think DavidB have a point here. Thredds permissions will be different for each organization, can not have a one size fit all config like this.

You should just move the 2 directives about /birdhouse/testdata/secure into optional-components/all-public-access in the other bump magpie PR #102 so it can be tested together and just abandon this PR.

@fmigneault
Copy link
Collaborator Author

I feel adding something that explicitly removes access into a config named all-public-access is more counter-intuitive.
Note the config is still optional, so it doesn't have to be included if the organization deems it unnecessary.

@fmigneault fmigneault changed the base branch from bump_twitcher_to_magpie-3.1.0 to bump_magpie_and_twitcher_to_3.2.0 November 10, 2020 23:44
@fmigneault fmigneault changed the base branch from bump_magpie_and_twitcher_to_3.2.0 to master November 10, 2020 23:45
@fmigneault fmigneault changed the base branch from master to bump_magpie_and_twitcher_to_3.2.0 November 10, 2020 23:50
@tlvu
Copy link
Collaborator

tlvu commented Nov 11, 2020

I feel adding something that explicitly removes access into a config named all-public-access is more counter-intuitive.

In the README https://github.com/bird-house/birdhouse-deploy/tree/master/birdhouse/optional-components#give-public-access-to-all-resources-for-testing-purposes we clearly state all-public-access is for testing.

And we will only revoke access for /birdhouse/testdata/secure, not everything. In all-public-access we actually open up everything, like the original state of this PR.

Note the config is still optional, so it doesn't have to be included if the organization deems it unnecessary.

Agreed but why try to block /secure when we do not even know if the org will have it or not. As for blocking /birdhouse/testdata/secure it's really for testing so might as well move it to all-public-access which is meant for testing.

Like @dbyrns, I think a secure-thredds component similar to this one does have it's use but it will belong to each org private config repo, not in this generic repo.

@fmigneault
Copy link
Collaborator Author

I personally prefer to preserve the granularity of the two files, even if the configs are only for testing.
This way, if I want to boot a test instance that is fully open, I can do so, and if I want to boot another one fully open except the secure directory, I can also do it.

I agree with the root /secure. It is not needed here.

@tlvu
Copy link
Collaborator

tlvu commented Nov 11, 2020

I personally prefer to preserve the granularity of the two files, even if the configs are only for testing.
This way, if I want to boot a test instance that is fully open, I can do so, and if I want to boot another one fully open except the secure directory, I can also do it.

OK I see your point.

Can you then please add to the optional-components README https://github.com/bird-house/birdhouse-deploy/tree/master/birdhouse/optional-components#give-public-access-to-all-resources-for-testing-purposes for your new secure-thredds component, following the same pattern as all-public-access and mention in the primary README https://github.com/bird-house/birdhouse-deploy/tree/master/birdhouse#optional-prepare-instance-to-run-automated-end-to-end-test-suite that user also need to enable the secure-thredds for the Jenkins test suite.

Copy link
Collaborator

@tlvu tlvu left a comment

Choose a reason for hiding this comment

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

Almost there :D

birdhouse/README.md Outdated Show resolved Hide resolved
birdhouse/optional-components/README.md Outdated Show resolved Hide resolved
birdhouse/optional-components/README.md Outdated Show resolved Hide resolved
birdhouse/optional-components/README.md Show resolved Hide resolved
birdhouse/components/README.rst Show resolved Hide resolved
@fmigneault fmigneault requested a review from tlvu November 11, 2020 21:05
Copy link
Collaborator

@tlvu tlvu left a comment

Choose a reason for hiding this comment

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

+1, merge when ready.

@fmigneault fmigneault merged commit b08f972 into bump_magpie_and_twitcher_to_3.2.0 Nov 12, 2020
@fmigneault fmigneault deleted the test-magpie-auth branch November 12, 2020 02:41
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.

None yet

4 participants