Add pkcs12 support #234

rashley-iqt · 2018-12-13T16:30:27Z

An adapter to be used in cases where a user needs to authenticate to a service using a .p12/.pfx certificate and it is not desirable or feasible to first convert that certificate into a .pem format. Born out of a use case in which the existence of temporary files containing cert data were deemed too great a risk.

Allow users for whom decrypting a .p12/pfx certificate into a .pem file is not desireable or feasible to pass a pkcs12 certificate as part of their request.

rather than import from ssl use _ssl in order to ensure pulling constants properly from OpnenSSL

…12.0 Due to the use of the PyOpenSSlContext object, introduced in urllib3 version 1.19, use of the Pkcs12Adapter class can not be used by versions of requests prior to 2.12.0.

sigmavirus24 · 2018-12-14T12:33:36Z

I think this looks okay to me (There are some style nits but those aren't as important to me in the context of this PR). I'm asking someone else with a bit more experience around OpenSSL and Pkcs12 to review this to make sure I'm not missing something crucial.

alex · 2018-12-14T13:15:25Z

Hey, providing some feedback at @sigmavirus24's request.

I don't think it makes sense to organize this as an HttpAdapter, that'd mean you need one for every serialization format. Organizing this to just deserialize the data and then pass that to an HttpAdapter that can take keys/certs as bytes (can the primary one do this?)
I wouldn't use the pyOpenSSL PKCS#12 implementation. pyca/cryptography's next release includes PKCS#12 support, so I'd wait for that https://cryptography.io/en/latest/hazmat/primitives/asymmetric/serialization/#pkcs12 instead of using pyOpenSSL.
FWIW, stdlib SSL (and I assume therefore requests) supports password protected private keys, so it shouldn't be necessary to use PKCS#12 if that's all you need.

rashley-iqt · 2018-12-14T18:57:44Z

@alex thanks for the feedback, I really appreciate it.

My understanding of the primary HttpAdapter, and somebody please correct me if I am wrong about this, is that it will accept a string path to a certificate file that must be in .pem format. I implemented is as an HttpAdapter because based on this comment and this comment it seemed to be the preferred method for handling this type of scenario. I agree that it wouldn't be a good idea to need a separate HttpAdapter for every potential serialization format and I would actually like to grow this adapter out to support other formats, such as PKCS#11. If there is interest on the part of @sigmavirus24 and the other maintainers, would it be acceptable to treat this as a starting point and add other serialization methods? If so, would it make sense to change the names of files and classes to make that intention clearer?
I would actually prefer to use pyca/cryptography but I don't know when a version that will support my needs will be released. Once that version is available I am willing to commit to refactoring my code to use it instead. Do you have any insight into when that might be?
For my current project .p12 files are a strict requirement and decomposing them into temporary .pem files is not an option for us, hence the need for this adapter.

alex · 2018-12-14T20:29:05Z

I think the right move would be to add support for loading keys/certs from bytes in memory, instead of requiring every serialization format to have it's own adapter.
Probably January

rashley-iqt · 2018-12-17T16:47:35Z

@alex you should be more like this?

import requests
from OpenSSL.crypto import load_pkcs12
from requests_toolbelt.adapters import X509Adapter

p12 = load_pkcs12(pkcs12_data, pkcs12_password_bytes)
cert = p12.get_certificate()
private_key = p12.get_privatekey()

s = requests.Session()
a = X509Adapter(max_retries=3, cert_bytes=cert, private_key_bytes=private_key)
s.mount('https://', a)

alex · 2018-12-17T16:56:07Z

Something like that, yeah! And maybe it should even be on the default requests.Session or something.

rashley-iqt · 2018-12-17T16:59:43Z

I would agree, but that is a question for @sigmavirus24 because then we would need to move from the toolbelt to requests/requests since that is where HttpAdapter lives.

sigmavirus24 · 2018-12-17T20:09:36Z

I'd be happy to have that in the toolbelt today. And work towards getting it exempted from requests' feature-freeze for sometime in the future if we can show there's a lot of usage of it.

@alex

Per @alex's comment, widen the adapter to accept a generic X.509 certificate and private key as byte arrays regardless of serialization or encoding method.

sigmavirus24

Also, @alex could you give this a second pass?

docs/adapters.rst

sigmavirus24 · 2018-12-20T13:07:48Z

docs/adapters.rst

@@ -21,7 +21,7 @@ with requests. The transport adapters are all kept in

 - :class:`requests_toolbelt.adapters.host_header_ssl.HostHeaderSSLAdapter`

- :class:`requests_toolbelt.adapters.pkcs12_adapter.Pkcs12Adapter`
+- :class:`requests_toolbelt.adapters.x509_adapter.X509Adapter`


This pointed out to me the module name. The other adapter modules don't have _adapter in the name. Would you be comfortable renaming the module to be x509 so this reads as requests_toolbelt.adapters.x509.X509Adapter?

sigmavirus24 · 2018-12-20T13:08:30Z

docs/adapters.rst

 Example usage:

 .. code-block:: python

-      from requests_toolbelt.adapters import Pkcs12Adapter
+      import requests
+      from requests_toolbelt.adapters import X509Adapter


I think this should be:

Suggested change

from requests_toolbelt.adapters import X509Adapter

from requests_toolbelt.adapters.x509 import X509Adapter

sigmavirus24 · 2018-12-20T13:10:08Z

requests_toolbelt/__init__.py

@@ -28,7 +28,7 @@
 __all__ = [
    'GuessAuth', 'MultipartEncoder', 'MultipartEncoderMonitor',
    'MultipartDecoder', 'SSLAdapter', 'SourceAddressAdapter',
-    'Pkcs12Adapter', 'StreamingIterator', 'user_agent',
+    'X509Adapter', 'StreamingIterator', 'user_agent',


I think we stopped adding new things to the top level exports purposefully. We've instead started documenting importing the class from the fully-qualified path to reduce the impact on importing this module (that way people only import what they need and don't incur the costs of the rest of the library). Please revert this change.

This especially important since we're importing OpenSSL and cryptography in the adapter's module and not everyone will have that installed. By default importing this will make that a hard requirement rather than an optional one.

sigmavirus24 · 2018-12-20T13:13:43Z

requests_toolbelt/adapters/x509_adapter.py

+from .. import exceptions as exc
+
+try:
+    from _ssl import PROTOCOL_TLS as PROTOCOL


I'm curious what the reasoning is to use _ssl here instead of ssl.

i used _ssl because i didn't need any of the actual implementation portions of ssl, just some of the constants that are defined in _ssl and passed through ssl. It also solved an issue i was having trying to import directly from ssl that only occurred in the tox py27 environment.

I'm surprised that you ran into issues with importing ssl on Py27. Would you be willing to add a comment explaining that? I just don't want to come back to this in the future, change the import and break something for someone.

What version of Python 2.7 are you using by the way?

I'll add a comment explaining that. I'm using 2.7.15, but it actually happened under tox in 2.7.14. I tried several things to try to correct it, but the switch to using _ssl was the only thing that worked on the 2.7.x and 3.x lines.

sigmavirus24 · 2018-12-20T13:19:21Z

requests_toolbelt/adapters/x509_adapter.py

+    :param pool_block: Whether the connection pool should block for
+            connections.
+
+    :param \**kwargs:


From here to the usage section doesn't follow the documentation style of the library, let's change this to:

Suggested change

:param \**kwargs:

:param bytes cert_bytes:

(your description here)

:param bytes pk_bytes:

(your description here)

:param password:

(your description here)

:param encoding:

(your description here sans the type)

:type encoding:

:class:`cryptography.hazmat.primitives.serialization.Encoding`

sigmavirus24 · 2018-12-20T13:20:34Z

requests_toolbelt/adapters/x509_adapter.py

+def check_cert_dates(cert):
+    """Verify that the supplied client cert is not invalid."""
+
+    if (


What if we did

Suggested change

if (

now = datetime.datetime.utcnow()

if cert.not_valid_after < now or cert.not_valid_before > now:

requests_toolbelt/adapters/x509_adapter.py

sigmavirus24 · 2018-12-20T13:28:50Z

tests/test_x509_adapter.py

+
+REQUESTS_SUPPORTS_SSL_CONTEXT = requests.__build__ >= 0x021200
+
+@pytest.mark.skipif(not REQUESTS_SUPPORTS_SSL_CONTEXT,


Let's also add 1 test that is skipped that looks vaguely like:

from requests_toolbelt import exceptions as exc @pytest.mark.skipif(REQUESTS_SUPPORTS_SSL_CONTEXT, reason="Will not raise exc") def test_requires_new_enough_requests(): with pytest.raises(exc.VersionMismatchError): X509Adapter()

This will ensure we don't regress the fact that we check the version.

Co-Authored-By: rashley-iqt <37374243+rashley-iqt@users.noreply.github.com>

rashley-iqt · 2018-12-20T17:40:26Z

@sigmavirus24 i've got the changes you requested mostly implemented, but I am currently struggling with betamax. When i perform my tests locally and can access the server i am using for testing the tests pass. However, when I disconnect the network and rely on the cassette file the tests fail. I notice that the http_interactions property is being populated with an empty array which makes me think that I have done something incorrect in terms of how I have the recorder configured but I am not sure what. Do you have any thoughts or advice? Is there a log that is populated somewhere?

@sigmavirus24

Updates to reflect changes requested by @sigmavirus24

rashley-iqt · 2018-12-21T15:29:17Z

@sigmavirus24 I figured it out and everything is passing now as well as all of the requested changes being made.

sigmavirus24

Just a couple things remaining if you wouldn't mind cleaning them up. Otherwise this looks good

sigmavirus24 · 2018-12-28T15:49:21Z

requests_toolbelt/adapters/x509.py

+    :param \**kwargs:
+        see below
+
+    :Keyword Arguments:


It seems my request about this documentation being corrected was lost.

sigmavirus24 · 2018-12-28T15:49:53Z

requests_toolbelt/adapters/x509.py

+def check_cert_dates(cert):
+    """Verify that the supplied client cert is not invalid."""
+
+    if (


It also seems my request to import the datetime module and find utcnow() was also lost.

sigmavirus24 · 2018-12-28T15:51:09Z

requests_toolbelt/adapters/x509_adapter.py

+            connections.
+
+    :param bytes cert_bytes:
+        bytes object containing contents of a cryptography.x509Certificate


I see what happened there are two files here, the x509_adapter and x509 are not in sync properly and one has the changes I wanted while the other has the module name i wanted. Can you reconcile these please?

sorry, something went weird with the rename and merge. I've corrected this.

No need to apologize :) Thanks for fixing it up

Addressed all of my feedback

sigmavirus24 · 2018-12-29T15:14:58Z

Alex gave this an informal 👍 on IRC, so I'm going to merge it. Thanks @rashley-iqt for following through and tackling this.

rashley-iqt · 2018-12-29T16:29:46Z

thank you guys for helping me get it ready for use.

rashley-iqt · 2019-01-28T12:18:31Z

@sigmavirus24 do you have any idea when the next release of the toolbelt is scheduled for?

sigmavirus24 · 2019-01-28T14:46:20Z

There isn't a schedule. We need to put together release notes though before we can release. If you want to send a PR with those notes then that can help accelerate this as the weather where I am should keep me outside shoveling snow probably through the end of my life 😜

ryanwilsonperkin · 2019-01-29T19:02:15Z

Hey folks, I suspect this might have introduced a breaking change into this project. We're installing this package as a dependency of twine (just a simple pip install twine) and now finding that we're getting an error as a result of trying to import pyopenssl.

Full stack trace:

Traceback (most recent call last):
  File "/usr/local/bin/twine", line 6, in <module>
    from twine.__main__ import main
  File "/usr/local/lib/python3.6/site-packages/twine/__main__.py", line 23, in <module>
    from twine.cli import dispatch
  File "/usr/local/lib/python3.6/site-packages/twine/cli.py", line 23, in <module>
    import requests_toolbelt
  File "/usr/local/lib/python3.6/site-packages/requests_toolbelt/__init__.py", line 12, in <module>
    from .adapters import SSLAdapter, SourceAddressAdapter
  File "/usr/local/lib/python3.6/site-packages/requests_toolbelt/adapters/__init__.py", line 12, in <module>
    from .ssl import SSLAdapter
  File "/usr/local/lib/python3.6/site-packages/requests_toolbelt/adapters/ssl.py", line 16, in <module>
    from .._compat import poolmanager
  File "/usr/local/lib/python3.6/site-packages/requests_toolbelt/_compat.py", line 59, in <module>
    from urllib3.contrib.pyopenssl import PyOpenSSLContext
  File "/usr/local/lib/python3.6/site-packages/urllib3/contrib/pyopenssl.py", line 46, in <module>
    import OpenSSL.SSL
ModuleNotFoundError: No module named 'OpenSSL'

rashley-iqt added 6 commits November 30, 2018 13:21

Allow users to authenticate requests using a pkcs12 cert

4fb1c45

Allow users for whom decrypting a .p12/pfx certificate into a .pem file is not desireable or feasible to pass a pkcs12 certificate as part of their request.

Correct improper example and update tox deps

f044fce

Correct import to work properly in tox py27 env

b4bb4ab

rather than import from ssl use _ssl in order to ensure pulling constants properly from OpnenSSL

Restrict use of Pkcs12Adapter to versions of requests greater than 2.…

910b871

…12.0 Due to the use of the PyOpenSSlContext object, introduced in urllib3 version 1.19, use of the Pkcs12Adapter class can not be used by versions of requests prior to 2.12.0.

Correct issues flagged by Flake8 and add myself to Authors

481f371

Correct typos

27ae29c

rashley-iqt changed the title ~~Add pfx support~~ Add pkcs12 support Dec 13, 2018

Convert PKCS12Adapter to generic X509Adapter

c9b69a5

Per @alex's comment, widen the adapter to accept a generic X.509 certificate and private key as byte arrays regardless of serialization or encoding method.

sigmavirus24 requested changes Dec 20, 2018

View reviewed changes

sigmavirus24 and others added 2 commits December 20, 2018 10:19

Update docs/adapters.rst

4cb1ef8

Co-Authored-By: rashley-iqt <37374243+rashley-iqt@users.noreply.github.com>

Update docs/adapters.rst

81cf8b9

Co-Authored-By: rashley-iqt <37374243+rashley-iqt@users.noreply.github.com>

rashley-iqt added 3 commits December 20, 2018 15:05

Incorporate feedback

d752ac7

Updates to reflect changes requested by @sigmavirus24

Correct merge conflict

b26889c

Correct incorrect parameter for test

26b8c2e

Add comment as requested by @sigmavirus24

e6339cb

rashley-iqt force-pushed the add-pfx-support branch from 0c5357b to e6339cb Compare December 28, 2018 12:36

sigmavirus24 previously requested changes Dec 28, 2018

View reviewed changes

rashley-iqt added 2 commits December 29, 2018 08:43

cleanup merge artifacts

fa48716

cleanup merge issues

d71c3b8

sigmavirus24 merged commit 675fab7 into requests:master Dec 29, 2018

rashley-iqt deleted the add-pfx-support branch January 23, 2019 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pkcs12 support #234

Add pkcs12 support #234

rashley-iqt commented Dec 13, 2018 •

edited

Loading

sigmavirus24 commented Dec 14, 2018

alex commented Dec 14, 2018

rashley-iqt commented Dec 14, 2018

alex commented Dec 14, 2018

rashley-iqt commented Dec 17, 2018

alex commented Dec 17, 2018

rashley-iqt commented Dec 17, 2018

sigmavirus24 commented Dec 17, 2018

sigmavirus24 left a comment

sigmavirus24 Dec 20, 2018

sigmavirus24 Dec 20, 2018

sigmavirus24 Dec 20, 2018

sigmavirus24 Dec 20, 2018

sigmavirus24 Dec 20, 2018

rashley-iqt Dec 20, 2018

sigmavirus24 Dec 22, 2018

rashley-iqt Dec 28, 2018

sigmavirus24 Dec 20, 2018

sigmavirus24 Dec 20, 2018

sigmavirus24 Dec 20, 2018

rashley-iqt commented Dec 20, 2018

rashley-iqt commented Dec 21, 2018

sigmavirus24 left a comment

sigmavirus24 Dec 28, 2018

sigmavirus24 Dec 28, 2018

sigmavirus24 Dec 28, 2018

rashley-iqt Dec 29, 2018

sigmavirus24 Dec 29, 2018

sigmavirus24 commented Dec 29, 2018

rashley-iqt commented Dec 29, 2018

rashley-iqt commented Jan 28, 2019

sigmavirus24 commented Jan 28, 2019

ryanwilsonperkin commented Jan 29, 2019

	from requests_toolbelt.adapters import X509Adapter
	from requests_toolbelt.adapters.x509 import X509Adapter

-    :param \**kwargs:
+    :param bytes cert_bytes:
+        (your description here)
+    :param bytes pk_bytes:
+        (your description here)
+    :param password:
+        (your description here)
+    :param encoding:
+        (your description here sans the type)
+    :type encoding:
+        :class:`cryptography.hazmat.primitives.serialization.Encoding`

	if (
	now = datetime.datetime.utcnow()
	if cert.not_valid_after < now or cert.not_valid_before > now:


		REQUESTS_SUPPORTS_SSL_CONTEXT = requests.__build__ >= 0x021200

		@pytest.mark.skipif(not REQUESTS_SUPPORTS_SSL_CONTEXT,

Add pkcs12 support #234

Add pkcs12 support #234

Conversation

rashley-iqt commented Dec 13, 2018 • edited Loading

sigmavirus24 commented Dec 14, 2018

alex commented Dec 14, 2018

rashley-iqt commented Dec 14, 2018

alex commented Dec 14, 2018

rashley-iqt commented Dec 17, 2018

alex commented Dec 17, 2018

rashley-iqt commented Dec 17, 2018

sigmavirus24 commented Dec 17, 2018

sigmavirus24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rashley-iqt commented Dec 20, 2018

rashley-iqt commented Dec 21, 2018

sigmavirus24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigmavirus24 commented Dec 29, 2018

rashley-iqt commented Dec 29, 2018

rashley-iqt commented Jan 28, 2019

sigmavirus24 commented Jan 28, 2019

ryanwilsonperkin commented Jan 29, 2019

rashley-iqt commented Dec 13, 2018 •

edited

Loading