m-click / requests_pkcs12

Add PKCS#12 support to the Python requests library in a clean way, without monkey patching or temporary files
ISC License
123 stars 33 forks source link

Enable non-encrypted certificates #45

Closed olehb007 closed 1 year ago

olehb007 commented 1 year ago

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.

vog commented 1 year ago

Thanks for your contribution! Alas, this patch has two issues:

olehb007 commented 1 year ago

In init the pkcs12_password_bytes variable will be uninitialized if pkcs12_password is None.

variable assigned inside an if is still scoped to a function, class, or module.

The encryption will be turned off when pkcs12_password_bytes is an empty byte sequence, not just when it's None.

It's fine, pkcs12_password='' and pkcs12_password=None are the same case.

vog commented 1 year ago

In __init__ the pkcs12_password_bytes variable will be uninitialized if pkcs12_password is None.

variable assigned inside an if is still scoped to a function, class, or module.

To clarify:

$ python
>>> import requests_pkcs12
>>> requests_pkcs12.get('https://example.com', pkcs12_data='', pkcs12_password=None)
Traceback (most recent call last):
  File "", line 1, in 
  File "/.../requests_pkcs12.py", line 131, in get
    return request('get', *args, **kwargs)
  File "/.../requests_pkcs12.py", line 117, in request
    pkcs12_adapter = Pkcs12Adapter(
  File "/.../requests_pkcs12.py", line 94, in __init__
    self.ssl_context = create_sslcontext(pkcs12_data, pkcs12_password_bytes, ssl_protocol)
UnboundLocalError: local variable 'pkcs12_password_bytes' referenced before assignment

If you provide a fix, please also adjust selftest() to cover this case, such that these types of bugs are caught on make check.

olehb007 commented 1 year ago

Right, not professional from my side. Please take a look on fix :)

vog commented 1 year ago

The encryption will be turned off when pkcs12_password_bytes is an empty byte sequence, not just when it's None.

It's fine, pkcs12_password='' and pkcs12_password=None are the same case.

It is true that pkcs12_password='' and pkcs12_password=None are currently handled identically. But this is exactly the issue. According to @ggoulart in his comment https://github.com/m-click/requests_pkcs12/issues/44#issuecomment-1560653230 these cases should be handled differently:

Regarding the second (None) case: What would be the use case? Isn't it more likely that the caller forgot or mistyped the password parameter?

olehb007 commented 1 year ago

On pkcs12_password == None it is fine to turn off the encryption entirely, even though I don't think this is a good idea. Regarding the second (None) case: What would be the use case? Isn't it more likely that the caller forgot or mistyped the password parameter?

I'm reading certificate from memory, which is already decrypted, e.g. Azure KeyVault.

olehb007 commented 1 year ago

I've changed empty password handling, please check.

olehb007 commented 1 year ago

@vog, could you please take a look on PR?

vog commented 1 year ago

@vog, could you please take a look on PR?

Sorry for the late response. There are some minor issues regarding error handling and coding style. I'll provide a full review tomorrow.

vog commented 1 year ago

Workload is sometimes hard to predict.

Anyways, here is the review.

@olehb007 Again, thanks for your work! Would you like to fix this pull request yourself?

vog commented 1 year ago

@olehb007 I'd propose to rebase and force-push your pull request, rather than merging it.

However, more importantly: Are you planning to finish implementing the feedback, or do you want somebody else finishing this PR? Either is fine for me, I just need to know.

olehb007 commented 1 year ago

@olehb007 I'd propose to rebase and force-push your pull request, rather than merging it.

However, more importantly: Are you planning to finish implementing the feedback, or do you want somebody else finishing this PR? Either is fine for me, I just need to know.

Sorry, I was on vacations. Please take a look now

olehb007 commented 1 year ago

Should I squash commits? I'm not very familiar with github model :)

vog commented 1 year ago

Should I squash commits? I'm not very familiar with github model :)

Thanks! I'll have a look at this. Don't worry about squashing, I'll take care of that.

vog commented 1 year ago

@olehb007 This looks very good. There were just a few final minor issues, then I could include your work.

vog commented 1 year ago

@olehb007 Again, thanks for your contribution! Please see the above linked commits for my final fixes.

I just released version 1.18 that contains your improvements:

olehb007 commented 1 year ago

Thank you very much!