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

Local network access restrictions #4289

Merged
merged 9 commits into from Sep 7, 2017

Conversation

Projects
None yet
3 participants
@erasche
Copy link
Member

commented Jul 7, 2017

Requires the administrator to add a whitelist exception if their users need to paste a URL from the private network address space. Handles round-robin DNS entries.

Happy for implementation feedback.

@erasche erasche added this to the 17.09 milestone Jul 7, 2017

@erasche erasche requested a review from jmchilton Jul 7, 2017

@erasche erasche changed the title Local network Local network access restrictions Jul 7, 2017

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

It looks like IPaddress didn't land up on the wheels server (https://wheels.galaxyproject.org/packages/) - the starforge PR was merged without copying the wheels over - also this should be easy to write an integration test for and that seems important because of the security implications.

How about I claim I will make these two changes in the next week and if I haven't done that by then - ping me again and I will merge as is? This is important and I don't want to slow it down too much. Is that fair @erasche?

@erasche

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

Fair @jmchilton, thank you!

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

So I sat down to actually write this and it occurred to me it needs to be local URL that needs to be unblocked - so my testing plan of using something like google.com with and without being in the list makes no sense. I thought maybe localhost:8080 since I know there some valid static content there at least during tests - but that fails in an unfortunate way with this PR:

>>> parsed_url = urlparse("http://127.0.0.1:8080/static/welcome.html")
>>> set( [info[4][0] for info in socket.getaddrinfo( parsed_url.netloc, 22 )] )
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
socket.gaierror: [Errno 8] nodename nor servname provided, or not known

So we can drop my requirement for a test - it is more non-trivial then I assumed at first thought. But we should fix the way localhost is handled and we make the resulting dataset RED instead of green I think.

I also realized we are creating an incompatibility in the upload.py and so all open upload jobs during the next upgrade would fail. There are some solutions to this - but they are all ugly. A few potential solutions:

  • Make it a named argument and preprocess the argument list in upload.py to allow it to be there or not.
  • Look at the argument in that position and see if it makes sense as an allow list (like if it isn't a file - it is probably the IP addresses.
  • Make a hard-break - copy upload.py to a new name and add it there, keeping the original upload.py in place for a year.
  • Just let the upload jobs fail.

Anyway - good luck packing - sorry for making this PR so complicated.

@erasche

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2017

  • gai error is bad, it should be pulling out 127.0.0.1 for localhost. Tests should work here, so that's a bug on my end.
  • named argument sounds easy enough. Just check if sys.argv[4] == '--iplist' or something and then do conditional behaviour based on that. The other options like keeping the original in place for a year is not desirable IMO.

No worries, these are good concerns, I did not consider some of these things. Thanks @jmchilton

@jmchilton

This comment has been minimized.

Copy link
Member

commented Aug 9, 2017

I'm hacking around on uploading today and it occurs to me we could maybe catch these before it gets to the job - maybe like in create_paramfile in upload_common.py - it seems to have all the data that is needed and we wouldn't need to modify the arguments to upload.py. You could also pass the data needed in there on a per-dataset basis in the dataset json for url uploads - this way the interface also isn't changing.

@erasche

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2017

@jmchilton sounds great. I'll do this first thing tomorrow! Thanks for the suggestion

@erasche erasche force-pushed the erasche:local-network branch 2 times, most recently from 43647c9 to 01ac018 Aug 21, 2017

@erasche

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2017

@jmchilton

  • I've rewritten due to pep8 changes
  • your comment led to me fixing a bug in the logic, the port was being returned as part of the URL. We now handle that (and cases like http auth credentials / ipv6) safely.
  • I made some changes to the UI in order to receive this error message and not just show "400 Bad request" which is incredibly useless. I'm not super sure I'm doing this in a nice way and would appreciate a second look from a UI person.
@jmchilton

This comment has been minimized.

Copy link
Member

commented Sep 5, 2017

@galaxybot test this

@jmchilton

This comment has been minimized.

Copy link
Member

commented Sep 5, 2017

Code nitpicks PR'd @ erasche#20.

I guess we should wait on @natefoo to resolve the comment in galaxyproject/starforge#133 before merging since the wheel still doesn't exist on wheels.galaxyproject.org.

@erasche

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2017

Works for me @jmchilton. I'll save rebasing this until then.

@natefoo

This comment has been minimized.

Copy link
Member

commented Sep 5, 2017

I guess we should wait on @natefoo to resolve the comment in galaxyproject/starforge#133

natefoo.wake()

erasche added some commits Aug 21, 2017

@erasche erasche force-pushed the erasche:local-network branch from a554dce to 4e2cd5f Sep 7, 2017

@erasche

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2017

rebased / client rebuilt.

@jmchilton jmchilton merged commit 2aeb459 into galaxyproject:dev Sep 7, 2017

0 of 6 checks passed

api test Test started.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
framework test Test started.
Details
integration test Test started.
Details
lgtm analysis: JavaScript Fetching Git Commits
Details
toolshed test Test started.
Details
@jmchilton

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

Thanks @erasche !

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Oct 6, 2017

Tests for blocking path pastes and intranet URLs for secondary files...
... during upload.

xref galaxyproject#4289 (blocking intranet URLs by default)
xref galaxyproject#4404 (implementing path pastes for normal uploads with the file:// prefix).
xref galaxyproject#4417 (initial tests for blocking file:// prefixes for non-admins added in galaxyproject#4404)

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Oct 6, 2017

Tests for blocking path pastes and intranet URLs for secondary files...
... during upload.

xref galaxyproject#4289 (blocking intranet URLs by default)
xref galaxyproject#4404 (implementing path pastes for normal uploads with the file:// prefix).
xref galaxyproject#4417 (initial tests for blocking file:// prefixes for non-admins added in galaxyproject#4404)

Rebased with a small fix thanks to the ever vigilant @nsoranzo.

nsoranzo added a commit to nsoranzo/galaxy that referenced this pull request Nov 14, 2017

nsoranzo added a commit to nsoranzo/galaxy that referenced this pull request Nov 14, 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.