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

Expose metrics to non-admins #3344

Merged
merged 2 commits into from Dec 22, 2016

Conversation

Projects
None yet
5 participants
@erasche
Copy link
Member

commented Dec 20, 2016

Fixes #3252 and #2954. Has commits from #3340 so that should be merged first.

@erasche erasche changed the title Expose metrics Expose metrics to non-admins Dec 20, 2016

@erasche erasche force-pushed the erasche:expose-metrics branch from fc7507d to 83d9ddd Dec 20, 2016

@erasche

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2016

Rebased to pull in lint fix.

#expose_job_metrics = False

# This option allows users to see the built command line.
#expose_job_command_line = False

This comment has been minimized.

Copy link
@jmchilton

jmchilton Dec 20, 2016

Member

I feel like we should reuse expose_dataset_path for expose_job_command_line and we could rename expose_job_metrics to expose_potentially_sensitive_job_metrics so we could one day default to exposing the core metrics for instance - I feel like this one should just be displayed to everyone regardless of setting up extra things.

This comment has been minimized.

Copy link
@erasche

erasche Dec 20, 2016

Author Member

@peterjc you requested the granularity. Thoughts?

@galaxybot galaxybot added this to the 17.01 milestone Dec 20, 2016

@peterjc

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2016

This is pretty much exactly what I was picturing implementing, so +1.

Regarding John's suggestion, showing the command line implicitly gives away the (input and output) dataset paths, so there is some logic in using a single setting for both.

@erasche

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2016

Okie dokie. Simplified.

@bgruening

This comment has been minimized.

Copy link
Member

commented Dec 21, 2016

@erasche @peterjc is this just the path which we care about? Why do we not replace the path with a placeholder for non-admins? Every job should know the job_working_directory so we can simply .replace('job_working_directory', '[hidden path]'). If this is still to insecure because of databases and data from loc files we could replace all path with a regex?

@erasche

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2016

Devil's advocate, why do we hide paths at all? What real, actionable intelligence could a malicious cluster user (i.e. worst case scenario) obtain from knowing that some files exist in some paths?

@jmchilton

This comment has been minimized.

Copy link
Member

commented Dec 22, 2016

@erasche The default deployment should reveal as little information as necessary about the configuration of the deployment. This is a fairly traditional approach to securing web applications. Certainly the attacks I can imagine in your described situation are contrived - but if we succeed in pushing Galaxy into more and more sensitive areas of the field there is going to be continued pushes to lock down information exposure. We've already received some in the past.

@bgruening This would be incomplete unless the regex was so expansive that the result would be quite confusing. I prefer the current approach.

I'm +1 now that these changes have been made - can everyone agree this is at least a step forward?

@erasche

This comment has been minimized.

Copy link
Member Author

commented Dec 22, 2016

@jmchilton fair enough. (xref OWASP). I just like to push back on security requirements. This one has always felt like a bit of security through obscurity, but I can certainly recognise there are organisations who are obsessed with passing their automated vulnerability reports.

@peterjc

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2016

Thanks all - this is a useful step forward and does not expose anything more by default.

@jmchilton jmchilton merged commit cac828e into galaxyproject:dev Dec 22, 2016

4 checks passed

api test Build finished. 244 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 132 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Copy link
Member

commented Dec 22, 2016

Thanks for the input all! Thanks for the implementation @erasche - very nice work!

@erasche erasche deleted the erasche:expose-metrics branch Aug 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.