python-hyper / hyper

HTTP/2 for Python.
http://hyper.rtfd.org/en/latest/
MIT License
1.05k stars 192 forks source link

Support for client side certificate in requests adapter. #199

Closed mylh closed 8 years ago

mylh commented 8 years ago

Ability to pass cert parameter when using hyper with requests:

requests.get(url, cert=(cert_file, key_file))
requests.post(url, data, cert=single_file)
Lukasa commented 8 years ago

Thanks @mylh! This is a reasonable idea, but it'd be a lot better if we plumbed support for it through into hyper's main API. That'd avoid the awkwardness associated with having a lot of the code dumped in to the one function here.

mylh commented 8 years ago

So you suggest adding cert param into HTTPConnection init method?

Lukasa commented 8 years ago

That's one possibility. The other possibility is to change the secure argument to accept a boolean or an SSLContext object.

mylh commented 8 years ago

I like the requests approach that hides complexity of SSL from end user, you can just pass path to a file(s) and don't care about any SSL-related libraries. So I think it is better to support similar interface. We have ssl_context param in init already which is used here to pass SSL context with loaded keys. Unfortunately I don't understand completely your idea with secure argument

Lukasa commented 8 years ago

@mylh So it's worth knowing that this library is not requests: it exposes a lot more complexity than requests does, by design. The goal is that requests would be used to hide that complexity from the users.

I had forgotten the ssl_context argument, however. In that case, you should add a new cert argument to hyper.tls.init_context, and then use that appropriately. =)

mylh commented 8 years ago

moved logic into tls.init_context, added test case

Lukasa commented 8 years ago

@mylh Looks like there are some problems with some of the tests. Mind fixing them up?

mylh commented 8 years ago

@Lukasa I woud like to but from what I see this is the original development branch that is not passing tests. The code in this commit is unrelated to the test failures. What you suggest?

Lukasa commented 8 years ago

These commits are relevant to the test failures, I'm afraid. For example this build has a problem with cert passphrases. And this build demonstrates that some code coverage is missing.

Those both need to be fixed. There are some other failures that are the result of the development branch being broken which I'll ignore. =)

mylh commented 8 years ago

I've got it :) Looks like everything is ok now.

Lukasa commented 8 years ago

Awesome, thanks! I have a few small stylistic notes I'll make inline, but this is very nearly ready for merge.

mylh commented 8 years ago

well, I'm not really an expert in SSL so I can only imagine writing integration test to try to connect with client certificate to some protected server. I'm pretty sure this is not what you need?.. I just saw the same kind of test in test_hyper_SSLContext.py...

Lukasa commented 8 years ago

@mylh No, we'd want to do something substantially more controlled. Such a test is likely to be a bit annoying to write, so you're welcome to avoid doing it.

Lukasa commented 8 years ago

Thanks @mylh, I've merged your changes along with some extra tests I added in #215. :sparkles: :cake: :sparkles:

mylh commented 8 years ago

Great @Lukasa Thank you for your work on the project :)