#7635 closed enhancement (fixed)
notebook -- make it trivial for any user to restrict the notebook server to only listen on certain subdomain
Reported by: | was | Owned by: | was |
---|---|---|---|
Priority: | major | Milestone: | sage-4.3.1 |
Component: | notebook | Keywords: | |
Cc: | Merged in: | sagenb-0.4.8 | |
Authors: | William Stein | Reviewers: | Dan Drake |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Attachments (3)
Change History (18)
Changed 13 years ago by
Attachment: | sagenb_7635.patch added |
---|
comment:1 follow-up: 4 Changed 13 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 13 years ago by
Changed 13 years ago by
Attachment: | sagenb_7635-part2.patch added |
---|
add localhost to subnets by default, by request from dan drake.
comment:3 Changed 13 years ago by
Authors: | → William Stein |
---|---|
Reviewers: | → Dan Drake |
Status: | needs_review → needs_work |
The code here is simple, and ipaddr.py will be in Python 2.7 and is (I think) in Python 3.1, so we can drop that later. The only thing I'm worried about now is something that confused me when testing this -- you have to specify address=''
in addition to subnet=[...]
. That strikes me as confusing -- they seem to be saying the same thing.
Of course, if you read carefully and understand networking, they're not -- address
refers to a network interface. So perhaps we can change address
to interface
, but continue accepting address
for backwards compatibility? Something like this in the docstring:
- ``interface`` -- (default: 'localhost'), address of network interface to listen on; give '' to listen on all interfaces. You may use ``address`` here for backwards compatibility, but this is deprecated and will be removed in the future.
along with a warning issued when notebook()
gets an address=
keyword.
Or, we could have subnets
imply address=''
.
This will be a positive review once this small issue is sorted out.
comment:4 Changed 13 years ago by
Replying to was:
NOTE: The patch includes ipaddr.py, which I understand will be in the next (major?) Python release, at which point it could be removed from sagenb (though this could impact standalone use). Note that ipaddr.py is licensed PSF, so is OK to include. It's copyright is owned by Google, I think.
The license is actually Apache 2.0. We already have a bunch of stuff in Sage with that license, so including this shouldn't be a problem.
comment:5 Changed 13 years ago by
The license is actually Apache 2.0. We already have a bunch of stuff in Sage with that license, so including this shouldn't be a problem.
Especially since the code will be in Python soon enough. Also, there is a bunch of apache stuff in sagenb already. Note that apache is GPLv2 incompatible, by the way, but it is GPLv3 compatible (hence GPLv2+ compatible), and everything in sage is GPLv2+.
Dan -- regarding changing address=, would you be OK with that being a separate ticket. I like tickets to be as small as possible, if possible. I definitely agree with your suggested change. See #7639.
comment:6 Changed 13 years ago by
Status: | needs_work → needs_review |
---|
comment:7 Changed 13 years ago by
Status: | needs_review → positive_review |
---|
A separate ticket sounds good. Positive review.
comment:8 follow-up: 10 Changed 13 years ago by
Status: | positive_review → needs_work |
---|
Oops, I need to back up here...when I do address="whatever.foo"
, it blocks localhost, since it's coming from 127.0.1.1 -- not 127.0.0.1. I think the localhost stuff should be 127.0.0.0/8 (http://en.wikipedia.org/wiki/Localhost). I'll upload a micro-patch for this.
comment:9 Changed 13 years ago by
William, can you test my little patch? Try something like notebook(address='x.y.z', subnets=['blah'])
where x.y.z is the regular hostname of your computer and subnets is something reasonable with your external address. You should get denied access without my little patch and get access with it (presuming that your machine in this situation routes requests to the notebook via 127.0.1.1).
comment:10 follow-up: 11 Changed 13 years ago by
Status: | needs_work → needs_review |
---|
Replying to ddrake:
Oops, I need to back up here...when I do
address="whatever.foo"
, it blocks localhost, since it's coming from 127.0.1.1 -- not 127.0.0.1. I think the localhost stuff should be 127.0.0.0/8 (http://en.wikipedia.org/wiki/Localhost). I'll upload a micro-patch for this.
Wow! Your localhost is 127.0.1.1? What operating system is that on? For me on OS X and sage.math, localhost is 127.0.0.1. On Wikipedia it says "Localhost always translates to the loopback IP address 127.0.0.1 in IPv4." (same page you reference). That said, also according to wikipedia using 127.0.0.0/8 is safe. Your patch has 127.0.0.1/8 by the way.
I guess your micro patch doesn't quite work because of the line if '127.0.0.1' not in subnets:
right above the insert line.
Also, are you sure about your patch? You say: Try notebook(address='x.y.z', subnets=['blah'])
But as soon as you explicitly specify the address then Twisted only listens on that interface and ignores localhost or any other interface (localhost is just a network interface like any other).
So unless I'm missing something (I usually am!) we shouldn't apply your micro patch and should change this back to "positive review". What do you think?
comment:11 Changed 13 years ago by
Replying to was:
Wow! Your localhost is 127.0.1.1? What operating system is that on? For me on OS X and sage.math, localhost is 127.0.0.1. On Wikipedia it says "Localhost always translates to the loopback IP address 127.0.0.1 in IPv4."
I'm using Ubuntu 9.10. You're right that localhost always resolves to 127.0.0.1, but apparently connections over the loopback network interface can get assigned IP addresses other than that.
The source of all this confusion is that I've never actually given a real value to address
! I have only ever given address=''
to listen to everything, or not specified it at all to make it do localhost only. My computer is klee.kaist.ac.kr and I tried address='klee', so maybe I should go back and try some more.
Your patch has 127.0.0.1/8 by the way.
I noticed. I should change that, although the /8 bitmask means that the last three octets don't matter.
I guess your micro patch doesn't quite work because of the line
if '127.0.0.1' not in subnets:
right above the insert line.
I noticed that before, but it's not going to make a big difference -- if someone puts '127.0.0.1/8' or whatever into subnets, then the resulting list in memory would be something like
['127.0.0.0/8', something, '127.0.0.1/8']
which won't make any real difference with access control.
Also, are you sure about your patch? You say: Try
notebook(address='x.y.z', subnets=['blah'])
But as soon as you explicitly specify the address then Twisted only listens on that interface and ignores localhost or any other interface (localhost is just a network interface like any other).So unless I'm missing something (I usually am!) we shouldn't apply your micro patch and should change this back to "positive review". What do you think?
I think I'm going to play around with the address keyword some more until I understand how it works. Maybe the patch at #7639 isn't quite right; do we need the name of an interface (such as "eth0" or "lo") or an IP address to which that interface is bound? I'll play around a bit more and report back.
comment:12 Changed 13 years ago by
do we need the name of an interface (such as "eth0" or "lo") or an IP address to which that interface is bound? I'll play around a bit more and report back.
I think we need the *address* to which that interface is bound. That's why I called the parameter "address" before. However, in twisted it is called "interface", so your suggestion to change that name is definitely correct. See e.g., this diff there:
152 strport = 'tcp:%s:interface=%s'%(port, address) 161 strport = 'tcp:%s:interface=%s'%(port, interface)
Here strport gets passed on to twisted and has "interface=" in it.
William
Changed 13 years ago by
Attachment: | trac_7635_localhost_fix.patch added |
---|
use 127.0.0.0/8 instead of only 127.0.0.1
comment:13 Changed 13 years ago by
Status: | needs_review → positive_review |
---|
I've uploaded an improved version of the micro-patch.
Now I understand address
. If you do something like
notebook(address='123.456.1.2', subnets=['123.456.0.0/16'])
(which, yes, I know is silly), then nothing we do with subnets
will allow access via the loopback interface, since Twisted is simply not listening there. So getting denied access via loopback with the above is correct behavior. OTOH, with
notebook(address='', subnets=['123.456.0.0/16'])
I think it's more reasonable to put our own "127.0.0.0/8" into subnets
, since the user did implicitly ask for Twisted to listen on the loopback interface.
I see that Debian puts 127.0.1.1 into /etc/hosts. On my machine, that has the effect of having "localhost" resolve to 127.0.0.1, and "klee" resolve to 127.0.1.1. So we should definitely support the whole 127.*.*.* range.
Now that I understand the address/interface keyword, I'm putting your patches back to positive review. I also think we should allow 127.0.0.0/8; what do you think of my patch? Should the help text be modified to match the code?
comment:14 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | positive_review → closed |
Merged into sagenb-0.4.8.
comment:15 Changed 13 years ago by
Merged in: | → sagenb-0.4.8 |
---|
The attached patch implements the described capability. The help from the notebook? output is:
NOTE: The patch includes ipaddr.py, which I understand will be in the next (major?) Python release, at which point it could be removed from sagenb (though this could impact standalone use). Note that ipaddr.py is licensed PSF, so is OK to include. It's copyright is owned by Google, I think.