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

Can't specify both fingerprint and SSLContext for client request #3679

Open
jeremy-hiatt opened this issue Apr 5, 2019 · 7 comments
Open
Assignees
Labels
Milestone

Comments

@jeremy-hiatt
Copy link

Long story short

It appears that 4.0 will remove all the SSL parameters from the TCPConnector class except for a single ssl parameter (#2626). This appears to be quite limiting: for example, it would no longer be possible to supply a client certificate and simultaneously pin the expected server certificate. The former requires you to pass the SSLContext configured with the client certificate as the ssl parameter, but for the latter you would have to pass a Fingerprint. Is there a recommendation for how this could be achieved?

Expected behaviour

Should be possible to make a request with BOTH a client certificate and a pinned server certificate.

Actual behaviour

It appears this will be removed with no alternative offered in 4.0.

Steps to reproduce

N/A

Your environment

client

@webknjaz webknjaz added question StackOverflow server client and removed server labels Apr 5, 2019
@webknjaz
Copy link
Member

webknjaz commented Apr 5, 2019

It looks like those were dropped in #3548

@webknjaz webknjaz modified the milestones: 3.5, 4.0 Apr 5, 2019
@webknjaz webknjaz added the bug label Apr 5, 2019
@webknjaz webknjaz modified the milestones: 3.5, 4.0 Apr 5, 2019
@webknjaz
Copy link
Member

webknjaz commented Apr 5, 2019

@asvetlov it looks like an API design regression: we should probably separate ssl context and fingerprint.

Another thought: one arg to rule them all smells like a godobject antipattern. Maybe it's a wrong decision after all?

@jeremy-hiatt
Copy link
Author

Following up on this...

I see a couple of options here:

  1. Revert back to a pattern where SSL is configured through multiple non-orthogonal parameters.
  2. Find a way to (ab)use inheritance to allow a single parameter to specify the fingerprint and configure the SSLContext as desired.

I'm happy to submit a patch if we can reach agreement on the best path forward.

@webknjaz
Copy link
Member

webknjaz commented Jul 2, 2019

Yeah, Andrew and I discussed this privately and thought that it probably needs to be done on the stdlib side and is probably in the scope of https://www.python.org/dev/peps/pep-0543/ (which I notified @tiran about, I think he wanted to add come hook for an extra validation callback).

Personally, I think that it's probably mostly fine to extend SSLContext but Andrew seems to dislike this idea.

@jeremy-hiatt
Copy link
Author

Thanks for the update. I did some experimentation with the multiple inheritance route; unfortunately it doesn't look easy to create an object that looks like both a Fingerprint and an SSLContext. I think Andrew's instincts were correct there.

For now I can work around the issue by implementing a subclass of TCPConnector and extending _get_fingerprint() to check an additional attribute.

@tiran
Copy link

tiran commented Jul 3, 2019

It's currently not possible to implement cert pinning with Python's ssl module. To implement it correctly, the pinned cert must be verified during the handshake. In case of an error, the handshake must be aborted before a client cert is verified and error must be signalled with a bad_certificate alert.

@webknjaz
Copy link
Member

webknjaz commented Jul 4, 2019

@tiran is that because the client cert may contain sensitive data?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants