nmcontrol
nmcontrol copied to clipboard
namecoin
Support REST API as backend.
Allow to use a REST API server as backend for NMControl. Supports both HTTP and HTTPS connections, and allows to optionally specify the expected TLS certificate. This makes it safe to use with a remote host.
Note, though, that I'm not a TLS nor Python expert. So I can't guarantee that the code is safe (but I expect it to be, review welcome).
See https://github.com/namecoin/nmcontrol/issues/48.
Can someone check on what TLS settings end up being used here, in particular the TLS version, cipher suite, and SNI? (If no one is able, I'll get around to it once my computer maintenance is under control and I have my data back.)
As far as I've seen, the cipher suites (and possibly more) can be configured on the server using namecoin.conf. But we could also add more specific configuration to the Python side to enforce particular versions / ciphers. Patches welcome, I don't feel like a TLS expert to judge what we want here. (Of course I know of some of the commonly stated things, but I don't want to base actual code off that and pretend I would know for sure it is safe.)
I will try to review this over the weekend. If I can't then we can merge as long as it's marked as experimental and not for end users.
@domob1812 Can you post a sample config file line for the fingerprint verification? I'm not quite sure how it's supposed to be specified.
So, I'm looking at the code, and it's very messy due to use of "urllib" directly. I would really prefer to use the "requests" library which makes this kind of thing much simpler. Requests also can verify TLS certs with a single extra argument (although it wants the cert file rather than a fingerprint). I can possibly write the requests code if @domob1812 is too busy.
I've mostly ported to requests; is there a public REST server available to test with? The one @domob1812 linked on the forum ( http://chain.huntercoin.org:8336/rest/name/d%2fdomob.json ) isn't loading for me. (Both HTTP and HTTPS would be nice to test against.)
Without having tested it, the example config line should be something like https://localhost:8336;sha256=abcdef. Note that I think it is a good idea to verify against a fingerprint, and not a full cert. Seems easier to me. But apart from that, if you think that the code is shorter with that other library, feel free to change it.
Okay, so it seems that using requests to validate by fingerprint only allows SHA1 fingerprints, not SHA256 (which I don't think is acceptable). I've just filed a bug report, so hopefully they'll deal with that soon. In the meantime we can use @domob1812 's code (once the TLS settings are confirmed valid) and replace it once requests supports SHA256. @domob1812 is there a public REST server (both HTTP and HTTPS) that I can test with?
I just did some testing, looks like this code uses some ciphersuites which are weak and/or don't have forward secrecy. I'm looking into how easy this will be to fix. Also, @domob1812 's example config line was missing a forward slash between the port number and the semicolon, in case anyone else wants to test this.
Okay, so to properly fix the TLS settings, we need Python 2.7.9 or Python 3.4. Unfortunately 2.7.9 isn't yet in most Linux distros (Fedora 21 has 2.7.8). Also, I believe that the current code from @domob1812 supports insecure SSL/TLS versions such as SSLv3 (I couldn't easily test this).
I am against recommending to end users a system that is known to be insecure, but it's not clear to me how fast this will be properly fixable.
I don't think most end users will need to contact a non-localhost REST server. If they do, they can tunnel over SSH or something like that, bypassing the need for secure TLS. So I think I'm okay with merging insecure TLS code, on the condition that it is clearly labeled to end users as insecure and nonrecommended, and we fix it once we have access to newer Python code. Upgrading to Python3 (as planned) will also fix this (Fedora 21 has Python 3.4.1).
Thoughts on what to do here?
My ticket regarding SHA-256 fingerprint support in the dependency that requests uses has been fixed -- that was fast. So once that makes its way into a release, we should switch to using requests.
I've not made any efforts to select ciphers / suites, but from what I read it should be enough to add some more config lines. Not sure about the requirement for new Python versions, though. I've no idea how that requests library works, so if you think that the fix makes its way into stable distros fast enough, feel free to submit a competing pull request so I can see how much easier the code is.
Regarding API server: If the one at chain.huntercoin.org does not work (not sure, maybe I disabled the config), I have none usable at the moment. Sorry.
I figure I will put this here instead of commenting on Jeremy's pull request, because this is where all the discussion seems to be.
I'd just like to note that Debian Jessie, which will be released as the new stable release of Debian soon and then is set to be the stable release for approximately another couple of years, has version 2.7.8 of Python 2. Since Debian Jessie is frozen, new upstream versions of packages can't get into Debian Jessie. So there is pretty much zero chance of Debian Jessie shipping with 2.7.9 or greater.
So if you want to retain support across the stable releases of all the popular distros, you need to either go with Python 3 (which I mentioned at the last irc meeting that I had been working on Python 3 support, I should upload what I have of that), wait a couple of years for when distros support sufficient versions of Python 2 to merge Jeremy's code, or just not merge the code.
I think going with Python 3 is the best option.
Just for reference, the ciphersuites can be fixed in older Pythons. However, enforcing usage of TLS 1.1 or 1.2 rather than 1.0 is not supported by Pythons earlier than 2.7.9. See https://docs.python.org/2.7/library/ssl.html
Hey guys. So I've got TLS 1.2 enforcement working (I think) on older Pythons. Turns out that the ciphersuite selection can be rigged so that it intentionally breaks on previous TLS versions (which is what we want). Learn something new every day in this job, I do.
There however is a big issue that I neglected to mention. As far as I can tell, @domob1812 's code is validating the TLS fingerprint after the request is sent (is this correct as far as you know?). This means that anyone can do a MITM and see the names being requested (although this will trigger a TLS error in NMControl and they won't be able to make NMControl receive false data).
So, the question is, which is preferable: using @domob1812 's code (with my fixes), where impersonation requires breaking SHA256 but surveillance is trivially easy, or using requests (without the latest change that isn't in a release), where impersonation and surveillance both require breaking SHA1? My opinion is that the latter is preferable. I can also make the requests code so that if the user supplies an exact cert, then that will be matched instead (so people who are afraid of SHA1 can do that until the latest code is in distros).
Thoughts?
Okay, good news. I was able to get everything working using requests. What works: TLS 1.2; secure ciphers; SNI; SHA256 fingerprint verification before sending data. And this is using Python 2.7.8, so it should work most places. The only issue is that a few packages need to be loaded via pip (such as PyOpenSSL).
I will post my code asap, but I'd like to test it on a Namecore instance first (I've been testing on SSLLabs's server).
/me does victory lap
Testing my requests-based code on Namecoin Core is currently blocked by https://github.com/domob1812/namecore/issues/6 , since I cannot build Namecoin Core until that's fixed.
Okay, see https://github.com/namecoin/nmcontrol/pull/58 for my rewrite using python-requests.
OK, I looked into this. My thoughts: 1.) From Python 2.7.8/2.7.9 and 3.3 / 3.4 there has been quite an improvement in TLS support 2.) If Debian is stuck with an old Python version it is bad but not our problem. 3.) In general I would prefer standard modules, especially security related ones. 4.) Maybe we should move the https lookup with fingerprint check into a separate module, might be useful in other applications 5.) Improvements for the version using standard modules would be: break on earlier versions, restrict TLS versions, restrict ciphers, check fingerprint without leaking info (does it currently allow self signed certs?) 6.) Instead of manually checking the fingerprint maybe it would be easier to add the cert? 7.) @JeremyRand The requests version does not work with TLS? 8.) Maybe we should start with a very simple version and improve it later on
@phelixnmc :
- With the built-in SSL module that is correct. With PyOpenSSL the Python version makes no difference. We're already going to depend on PyOpenSSL for the MITMProxy code.
- A lot of people use distros with slightly outdated packages. In some cases this is not voluntary, e.g. security-oriented OS's such as Whonix use Debian. I am strongly against abandoning those users. I believe @josephbisch is against abandoning them as well.
- The question should not be whether they are standard but rather whether they have received a lot of review. The standard modules all qualify under that criterion, but so do PyOpenSSL and Requests. Both PyOpenSSL and Requests are very widely used and recommended -- search StackOverflow and you will find that the near-unanimous consensus is that you should use Requests instead of the standard module. Requests is far more Pythonic than the standard module.
- I'm unaware of any other things we're planning to do in NMControl that require such functionality; is this suggestion aimed at a specific use case? In any event I have no objection to doing so once it's used in more than one thing... until then it seems premature.
- I am not sure I understand what you're saying here, can you rephrase?
- Checking the fingerprint is trivial with PyOpenSSL/Requests (it's just a short callback function). Using a cert file is much less usable.
- The requests version I posted may work with TLS on Namecoin Core. The Namecoin Core build I'm using doesn't seem to work properly in TLS mode, so I couldn't test it on Namecoin Core. When I tested the TLS code on a public TLS web server, all of the TLS-specific code worked properly. If there are problems, they are likely trivial to fix.
- I believe the Requests version is mergeable (except for the typo that Daniel reported, which I will fix shortly). I support incremental progress; what I don't support is investing effort into a codebase that will need to be completely rewritten to make progress later. Therefore I think the Requests version is a better choice.
Three side notes:
- End users will be installing this via a package manager or bundle installer. Dependencies can therefore be automatically included by default. It's not an inconvenience to end users to use PyOpenSSL/Requests.
- The Requests code also will probably be faster, because it reuses the HTTP session to the Namecoin Core instance. This is especially the case for HTTPS and/or Tor-based connections, though it also matters for clearnet HTTP. In addition, the Requests code supports multiple sessions based on an identity string (which will be hooked into Tor's stream isolation feature). Doing this with the standard libs is substantially more complicated if it's possible at all.
- The Requests code can be simplified by moving the fingerprint config file setting to its own setting rather than it being part of the URL. I haven't touched the config file code in NMControl yet, so I didn't mess with it... but that would eliminate all the URL parsing and reconstruction code that I left verbatim from @domob1812 's code. So, the current state of my pull request is not representative of the maximum simplicity achievable using Requests. I'll happily work on that later, shouldn't be too hard.
EDIT: Also I'm totally fine with making it show a friendly error in case Requests isn't installed. I already did that for the TLS modules, I should have done it for Requests too but I forgot. I can fix that too.
1.) OK 2.) A weak argument imho. Can't they upgrade manually? 3.) Yes. I am aware that requests is very popular and I think it is completely legit to use it. These SO infos are mostly from before the python SSL update though. ("pythonic" made me smile in regard to this codebase) 4.) Moving the httpsrequest with fingerprint check into it's own module would be cleaner and also allow external software to use it. E.g. other applications that need to query an API server. I am quite certain this is an improvement. 5.) This was a todo list for httpsrequest code based on 2.7.9 standard modules. I forgot SNI. 6.) OK 7.) Great. 8.) Yes. SN1.) Yes. SN2.) OK SN3.) ACK splitting these might be a good idea.
Requests has quite some advantages and I don't see any real disadvantages so I am fine with using it. IMHO there should be a separate queryapiserver module, though.
Users can't always upgrade Python 3 to a newer version than that provided by their distro easily. For example, python3.4-minimal (which is a dependency of python3 in Debian Jessie and Sid) requires libc6 2.15 or greater for the amd64 architecture.
Debian Wheezy has libc6 2.13. Trying to upgrade libc6 is almost guaranteed to break your system, since there are so many packages that depend on libc6. So it is at best non-trivial to port Python 3.4 to Debian Wheezy. At worst, it is completely impossible. Since Debian Jessie will hopefully be released soon, this particular example isn't really a concern, but I haven't analyized the entire dependency chain for Python 3.4, or looked at distros other than Debian, so it is possible there are similar issues.
I have set up the experimental API server with https. You can query it like this to experiment: https://github.com/phelixnmc/nmcontrol/blob/rest-simple/lib/nmcapiclient.py https://github.com/phelixnmc/nmcontrol/tree/rest-simple
@phelixbtc Only the callback function at https://github.com/namecoin/nmcontrol/pull/58/files#diff-f0642a76a70a05557239a272a3cd10c6R18 and the try clause at https://github.com/namecoin/nmcontrol/pull/58/files#diff-f0642a76a70a05557239a272a3cd10c6R41 are relevant to the secure HTTPS with fingerprint validation. I can break this out into a separate file if you like, I just don't think it's much code.
Anyway, I'll fix the issue @domob1812 reported in my requests version, then I'll ask for a merge, and once merged I'll address the other minor issues you bring up.
The more I look into this the more I get the impression that both the standard Python https functions as well as the requests code is not meant to be used with fingerprints. Maybe we should leave it out for now - people can still add certificates to their system ca store I think.
Requests seems prone to complication on Windows (especially so with the fingerprint check), maybe we should use builtin functions where available and fall back to requests on systems where up to date libs are not available.