Namecoin Core REST support using Python-Requests#58
Namecoin Core REST support using Python-Requests#58JeremyRand wants to merge 8 commits intonamecoin:masterfrom
Conversation
|
I'll test it later today when I'm back home. |
There was a problem hiding this comment.
Nit pick: Please add a new line at the end.
|
It works with a tiny fix necessary - will add a line remark. TLS not tested. ACK from me with the fix. However, I would like to get also an ACK from phelix or someone else more involved in Python. I'm still not 100% convinced that we should be using a non-standard library even though the standard My experience was like this: Starting it up initially made everything fail (with not very helpful error messages). However, installing the requests library was easy with Debian's package system. Afterwards, it worked. So not a big deal, but still not as nice as using a standard library. |
lib/backendDataRest.py
Outdated
There was a problem hiding this comment.
This line did not work for me. I had to remove the () at the end, since result.json is a member variable and not function. With the change, it worked.
There was a problem hiding this comment.
@domob1812 with your change I get the following upon calling getIp4 via RPC:
Traceback (most recent call last):
File "/home/jeremy/Downloads/NMControl/nmcontrol/plugin/pluginRpc.py", line 170, in computeData
result = methodRpc(method, _params)
File "/home/jeremy/Downloads/NMControl/nmcontrol/lib/plugin.py", line 203, in _rpc
return func(_args)
File "/home/jeremy/Downloads/NMControl/nmcontrol/plugin/pluginDns.py", line 130, in getIp4
result = self._getRecordForRPC(domain, 'getIp4')
File "/home/jeremy/Downloads/NMControl/nmcontrol/plugin/pluginDns.py", line 125, in _getRecordForRPC
self._resolve(domain, recType, result)
File "/home/jeremy/Downloads/NMControl/nmcontrol/plugin/pluginDns.py", line 87, in _resolve
handler._resolve(domain, recType, result)
File "/home/jeremy/Downloads/NMControl/nmcontrol/plugin/pluginNamespaceDomain.py", line 53, in _resolve
nameData = app['plugins']['data'].getValueProcessed(name)
File "/home/jeremy/Downloads/NMControl/nmcontrol/plugin/pluginData.py", line 133, in getValueProcessed
data = self.getValue(name)
File "/home/jeremy/Downloads/NMControl/nmcontrol/plugin/pluginData.py", line 121, in getValue
data = self.getData(name)
File "/home/jeremy/Downloads/NMControl/nmcontrol/plugin/pluginData.py", line 107, in getData
if 'expired' in data and data['expired']:
TypeError: argument of type 'instancemethod' is not iterable
Without your change it works fine for me. Not sure why you're getting different results. FWIW the example code on the front page of http://www.python-requests.org/en/latest/ agrees with my results.
There was a problem hiding this comment.
I also do not know. Maybe json changed from a member variable to a member function with the version of requests? I'm running the one in Debian Wheezy, so already at least two years old.
There was a problem hiding this comment.
According to https://stackoverflow.com/questions/6386308/http-requests-and-json-parsing-in-python/17517598#17517598 , Wheezy is indeed on an old version of requests which has json as a variable. I will push a workaround shortly. Thanks for pointing this out.
…ests, e.g. Debian Wheezy.
|
@domob1812 can you test again on Wheezy? |
|
Works for me now. (As before, I only tested the plain interface without TLS.) |
|
@phelixbtc Can we merge this? |
|
I would prefer all the https and fingerprinting stuff to go into a separate, reusable module. E.g. I could then use it for the obfuscated api stuff, too. I have not yet tested it on Windows. |
|
@phelixbtc Just to be clear, the only HTTPS-specific code is the 1-line function at https://github.com/namecoin/nmcontrol/pull/58/files#diff-f0642a76a70a05557239a272a3cd10c6R19 , plus the 3 lines at https://github.com/namecoin/nmcontrol/pull/58/files#diff-f0642a76a70a05557239a272a3cd10c6R44 , plus (optionally, just for debug) the few lines at https://github.com/namecoin/nmcontrol/pull/58/files#diff-f0642a76a70a05557239a272a3cd10c6R69 . Are you requesting that those go into a separate module? (No objection if so, just want to be sure I understand what you're asking.) |
|
@phelixbtc Also, are you intending for the obfuscated API to be added to NMControl? If so, can you make another ticket for that? |
|
@phelixbtc I split off the HTTPS code. Anything else needed before we merge? |
I will take a look and test this PR asap. |
|
Why do you not create a new session for every request? Isn't it the default anyway? Is there a benefit to keeping sessions? |
I have the OpenSSL module installed and can import it, though. Do I have a wrong version of requests installed? Setting globals like this seems somewhat hackish to me. Maybe there is another way like this https://gist.github.com/paxswill/5ed7273059b34e90b184 ? |
|
This could make checking the fingerprint easier: http://stackoverflow.com/questions/26479039/python-requests-direct-pem-pinning-with-self-signed-cert |
Using a single session allows the HTTP connection to be reused, which reduces latency. This is particularly important on HTTPS and Tor connections, where latency is greater. See http://docs.python-requests.org/en/latest/user/advanced/#keep-alive
Hmm, I'll check and get back to you on that.
I initially tried that. Problem is that until about a month ago (when I submitted a bug report) that method didn't support SHA256 fingerprints (only SHA1 and MD5 were supported). The SHA256 support is in Fedora now, but won't be added to Debian Jessie, so I'm not using that method since PyOpenSSL provides better compatibility. |
|
@phelixbtc What version of the requests module do you have? You're on Windows, right? Also try this: |
|
I had to install ndg-httpsclient to get around it. Also I updated from requests 2.5.3 to 2.6. Unfortunately requests in the callback function it breaks on:
|
|
Hmm, I'll check on this and get back to you. |
|
@phelixbtc As far as I can tell, get_servername should be being called on a Connection object, not a Context object. What version of PyOpenSSL are you on? How exactly did you trigger this error? |
|
I refactored the code quite a bit but I don't see what could have gone wrong: https://gist.github.com/phelixbtc/0230e872f8c199a5057c It's probably not related but I am quite certain the private callback function is not meant to be patched like this. https://pythonhosted.org/pyOpenSSL/api/ssl.html#OpenSSL.SSL.Context.set_verify |
|
Also:
|
|
@phelixbtc thanks for posting the code you're using; I'll see if I can reproduce this on my end this weekend. |
|
@phelixbtc I'm on PyOpenSSL 0.13.1, with requests 2.5.0, and I don't get that error when executing the file you posted. I do get this: Because no fingerprint is specified. When I add the fingerprint "NONE" for namecoin.org, that error goes away as well. At no point am I getting the error you posted. PyOpenSSL 0.14 isn't in Fedora 21 repos yet, so it's not easy for me to test that. If necessary I will try to find a way to get that tested. |
|
@phelixbtc I just tested in PyOpenSSL 0.14, and I can confirm that I get the same error you do. Now that I can reproduce it, I will work on a fix. |
|
@phelixbtc See https://github.com/pyca/pyopenssl/blob/master/ChangeLog#L32 . What we're encountering is a known bug in PyOpenSSL 0.14, which has since been fixed in their master branch. I don't think there's a workaround (the Context class doesn't have a way to access the corresponding Connection), but given that even Fedora isn't using that version of PyOpenSSL (and Fedora is among the first to upgrade things), I don't think we need to worry about it. If I add some code to detect this broken PyOpenSSL version and display a more informative error, can we merge this? |
|
@phelixbtc I just added a useful error message when the broken PyOpenSSL version is in use. Anything left before we merge? |
|
@phelixbtc FYI PyOpenSSL 0.15 and higher should fix the issue. (0.15.1 is the latest, it came out April 14.) Can we merge this? |
|
with my slightly updated test file https://gist.github.com/phelixbtc/0230e872f8c199a5057c I get this error: versions: Traceback (most recent call last): it works without TLS (http://). I'm on Python 2.7.10. (2.7.9 threw the same error). |
|
@phelixbtc Hmm, I'll see if I can figure out what's going on there. That traceback looks like the connection timed out or something... have you tried a different URL to see what happens? |
|
Yep, same same (https://bitcointalk.org) |
|
If the core client will not support it for much longer maybe we should let this one go.... ? |
|
If it works for you with HTTP, I'm OK with merging my PR now and worrying about HTTPS later. On May 29, 2015 7:04:13 AM CDT, phelixbtc notifications@github.com wrote:
Sent from my Android device with K-9 Mail. Please excuse my brevity. |
|
Hello @phelixbtc . At the IRC meeting yesterday, the consensus was to merge this (given that HTTP works fine, and that's what 99% of people use), and deal with HTTPS issues as a separate PR. If you'd like to NACK this, please do so in the next 48 hours, otherwise we'll merge. Thanks. |
|
For now there is no advantage vs an extremely simple version using only builtins? Also I am worried this will complicate Tor support. |
|
Hello @phelixnmc . Generally requests is simpler / more pythonic code than using the alternatives. TLS is possible with requests (and is working for me, I'm not sure why you're having trouble with it), and not really doable properly with the alternative. I'm not sure about Tor, I'll get back to you on that. |
|
Interesting. So according to urllib3/urllib3#486 , requests does not support SOCKS proxies, and it doesn't seem to be a high priority for them. This is problematic. So we have one method that doesn't work with SOCKS, and another method that doesn't work with TLS. I'm not really sure what we should do here; neither is really a good option AFAICT. |
|
TLS can be configured to work safely since Python 2.7.9 and does so out of the box since 2.7.10. Only how to cleanly check fingerprints we don't know (maybe should ask on stackoverflow). But I have a feeling it's not meant to be used like that but that you are rather supposed to add a cert locally (e.g. gives expiry info). |
|
@phelixnmc I'm looking at the Python 2.7.10 docs for urllib: https://docs.python.org/2/library/urllib.html It says at the top:
That gives me the impression that urllib doesn't behave safely for HTTPS by default. Is that impression incorrect? |
|
You need to use urllib2. import urllib2 |
|
@phelixbtc Can you provide a reference for urllib2 having native SOCKS support? A quick web search suggests that it's only doable using a third-party module, which isn't really what we want, and which I'm guessing works equally well with urllib3. |
|
No, seems to need extra work: http://fitblip.pub/2012/11/13/proxying-dns-with-python/ |
|
@phelixbtc Ugh, that is a really ugly hack. I also suspect that it is equally applicable to urllib3, and I am not at all sure that it's safe in either case. In any event, I am not really comfortable making decisions about what modules to use based on assumptions of using ugly hacks like that. There does not appear to be a good solution right now, but I think the best option available is to get urllib3+requests to implement native SOCKS support (they seem open to accepting pull requests for this). Maybe FSM or NMDF could place a small BountySource bounty on it, if the urllib3 people think it would be productive. Thoughts? |
|
@phelix Thoughts on what we should do here? Neither urllib2 nor urllib3 has proper support for SOCKS. |
|
Looks like my bounty on urllib3 is getting some people interested in finishing it off. They already have a working implementation, they're just trying to get tests added before merging. Given that, I think we can continue using requests without problems for Tor. @phelix Any further objections to merging? |
|
Let me recall the advantage of using this over the standard lib... I think it is that this let's you check the fingerprint before giving away information about the request? On the other hand side TLS is not working (excepts for me as described above) which is working fine with up to date standard libs. Probably a cleaner solution for this problem would be to add a custom certificate to the system.
So far I am not convinced to use this in Windows builds. If it is helpful on Linux it's fine with me but I would like to use standard libs for API server access on Windows (as in hyperion). |
|
@phelixbtc The method you linked is not secure; it does not restrict the certificates that are accepted. requests is more Pythonic and is recommended by official Python documentation: https://docs.python.org/2/library/urllib2.html .
The pending changes to urllib3 are only needed for SOCKS support; TLS should work fine on existing Debian. In contrast, urllib2 will probably never have support for SOCKS. I'm fine with investigating the TLS issue you described, but I don't think it's a blocker for merging the REST code. |
|
@JeremyRand What do you mean by
|
4 participants
I believe this code works, except for the TLS code (which I'm leaving for later since TLS won't be in Bitcoin Core for much longer, and supporting TLS proxies is of secondary importance). The TLS code that I have already is still there so that we can finish it later, but shows a warning when enabled (since end users shouldn't be touching it).
@domob1812 , can you test this?