Invalid empty password

[ggoulart]

In version 1.16 the argument pkcs12_password can not be an empty string, but in version 1.15 it works.
This is the error received in version 1.16 raise ValueError("Password must be 1 or more bytes.")

This is an expected behavior?

[vog]

No, this change in behavior is unintentional.

The only relevant change from 1.15 to 1.16 was to drop pyOpenSSL support and to use cryptography instead. The relevant commit is:

• Commit: Remove support for pyOpenSSL
  • from pull request: Remove urllib3 and pyopenssl direct dependency #43
  • from issue: DeprecationWarning forurllib3.contrib.pyopenssl #40

Pull requests with improvements to create_sslcontext() are welcome.

[vog]

As a side note, I also don't like how we need to go through temporary files to use cryptography, with pyOpenSSL this wasn't necessary, and maybe cryptography provides a way without that as well, perhaps we just overlooked something here.

[yonting]

I take responsibility for the problematic commits. The exception is raised by

requests_pkcs12/requests_pkcs12.py

Line 52 in 2049d6b

cryptography.hazmat.primitives.serialization.BestAvailableEncryption(password=pkcs12_password_bytes)

. I dunno how to fix this. cryptography.hazmat.primitives.serialization.NoEncryption maybe works but maybe not wise. Using empty string passwords is considered a bad practice so the "cryptography" decided to put in place some enforcement.

Regarding the side note I conducted some research into avoiding temporary files. This was a longstanding feature request for the standard library ssl: python/cpython#60691. and urllib3/urllib3#2680 (comment). Now closed. This might all be old news to you folk.

Maybe the only options to avoiding temporary files are to find or wait for another library that implements the now deprecated functionality of urllib3.contrib.pyopenssl, or attempt to sustain the patching in this repo.

[vog]

. I dunno how to fix this. cryptography.hazmat.primitives.serialization.NoEncryption maybe works but maybe not wise. Using empty string passwords is considered a bad practice so the "cryptography" decided to put in place some enforcement.

I'm not against this, but would like to point out that this would only make sense if it was equivalent to an empty password in the pyOpenSSL implementation. Otherwise it would not be backwards compatible and hence still make old code fail.

@ggoulart What kind of PKCS12 files did you open by providing an empty string as password? Are they just unencrypted, or do they use some kind of encryption, just with a dead simple password?

Regarding the side note I conducted some research into avoiding temporary files. This was a longstanding feature request for the standard library ssl: python/cpython#60691. and urllib3/urllib3#2680 (comment). Now closed.

Okay, so it seems we didn't overlook anything, it just doesn't exist. This is too bad.

[ggoulart]

Hi @vog I'm using a pfx certificate with an empty string as password. Yes, it is encryption with a dead simple password.

[olehb007]

I have a case with no password when loading certificate from memory, creating pull request. If needed it can be bound specifically pkcs12_data parameter. Pull request #45

[gprestes]

Hi @vog, I am working together with @ggoulart. We are getting the file from a Key Vault and file is retrieved passwordless. We could re-add the password, but since you said this is not the intended behaviour we will stick with the previous version while there is not a fix.

[vog]

I'm a bit confused. What exactly do you mean by "could re-add the password"?

[vog]

@ggoulart @gprestes Could you please have a look at the latest version 1.18? Does this work for you as expected, or are there still issues?

[magnethy]

I have the same issue for 1.18. Works when downgrading to 1.15. Running Python 3.9 and did a clean dependency installation.

Traceback (most recent call last):
    [...]
    rval = requests_pkcs12.post(url, json=body, headers=self._get_api_headers(), pkcs12_filename=self.cert_path, pkcs12_password='')
  File "/home/site/wwwroot/.python_packages/lib/site-packages/requests_pkcs12.py", line 168, in post
    return request('post', *args, **kwargs)
  File "/home/site/wwwroot/.python_packages/lib/site-packages/requests_pkcs12.py", line 140, in request
    pkcs12_adapter = Pkcs12Adapter(
  File "/home/site/wwwroot/.python_packages/lib/site-packages/requests_pkcs12.py", line 99, in __init__
    self.ssl_context = create_sslcontext(pkcs12_data, pkcs12_password_bytes, ssl_protocol)
  File "/home/site/wwwroot/.python_packages/lib/site-packages/requests_pkcs12.py", line 53, in create_sslcontext
    cryptography.hazmat.primitives.serialization.BestAvailableEncryption(password=pkcs12_password_bytes)
  File "/home/site/wwwroot/.python_packages/lib/site-packages/cryptography/hazmat/primitives/_serialization.py", line 67, in __init__
    raise ValueError("Password must be 1 or more bytes.")
ValueError: Password must be 1 or more bytes.```

[vog]

I think I found an acceptable solution for this messy situation.

If verified that the old pyOpenSSL based implementation in 1.15 behaves identically for an empty password and a None password. So I believe it is safe to treat those cases equally, i.e. to treat an empty password as if it was None.

[vog]

@ggoulart @gprestes @yonting @olehb007 @magnethy

Please have a look at the latest version 1.19.

Does this fix the problem for all of you, or are there still issues?

[vog]

@ggoulart @gprestes @yonting @olehb007 @magnethy

Meanwhile I added some more improvements with respect to password handling, improved the README, and released 1.20 as well as 1.21, but the question remains:

Does this fix the problem for all of you, or are there still issues?

[vog]

@ggoulart @gprestes @yonting @olehb007 @magnethy

As there was no further feedback, I'm closing this issue for now. Please feel free to open another ticket if you still have trouble with the latest requests_pkcs12 version.