Use PROTECTED_AUTH to activate the PIN Pad

[pquL1]

Description of the changes

Use PROTECTED_AUTH instead of None as the user pin to activate the PIN Pad with PKCS#11 Devices. Resolves #133

Checklist

Please go over this checklist to increase the chances of your PR being worked on in a timely manner. Deviations are allowed with proper justification (see previous section).

• [x] I have read the project's CoC and contribution guidelines.
• [x] I understand and agree to the terms in the Developer Certificate of Origin as applied to this contribution.
• [ ] All new code in this PR has full test coverage.

For bug fixes (delete if not applicable)

• [ ] My changes do not affect any public API or CLI semantics.
• [ ] My PR adds regression tests (i.e. tests that fail if the bug fix is not applied).
• [ ] All new code in this PR has full test coverage.

[MatthiasValvekens]

Thanks, @pquL1!

Hmm, unfortunately this breaks in some cases where None would otherwise work. I'm getting this error with one of my devices after applying your change:

pkcs11.exceptions.ArgumentsBad: Protected authentication is not supported by loaded module

For context: this module handles its own PIN dialog, but doesn't expose that fact in any discernible way in the PKCS#11 API. Maybe it should, though. I'll investigate.

On the other hand, even if the module I'm using is somehow bugged and/or weird, there probably exist PKCS#11 implementations that don't require any authentication at all (e.g. signing hardware intended for unattended operation). Passing PROTECTED_AUTH to those also doesn't seem right.

Anyway, the fact remains that there exist implementations that work with PROTECTED_AUTH but not with None and vice versa (unfortunately). I'll try to come up with some tweaks to your PR to allow both variants to be used.

[MatthiasValvekens]

Update: I've been working on some tooling to automate PKCS#11 testing lately, in particular to be able to run it in CI (see MatthiasValvekens/certomancer@d0e8996 and MatthiasValvekens/certomancer@1fea804). I'll get back to this PR after I get that set up!

[codecov-commenter]

Codecov Report

Merging #134 (5d593ee) into master (bcb7554) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #134   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files          75       75           
  Lines       11191    11211   +20     
=======================================
+ Hits        11045    11065   +20     
  Misses        146      146           

Flag	Coverage Δ
unittests	98.69% <100.00%> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pyhanko/config.py	100.00% <100.00%> (ø)
pyhanko/sign/pkcs11.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[MatthiasValvekens]

So, I've turned the prompt-pin config setting from a boolean into a tri-state enum with values prompt, defer and skip. prompt and skip correspond to the old true and false, respectively (and passing in boolean values will get you those results, for compatibility's sake). The new defer option should trigger the behaviour you want.

Can you check if that works as expected on your end?

---

FWIW, I added a test against SoftHSM with PROTECTED_AUTH, but since SoftHSM doesn't support the PROTECTED_AUTHENTICATION_PATH flag at all, the test is of the "I expect this operation to fail in this very specific way" type, so it's not quite the same thing.

[pquL1]

Hi @MatthiasValvekens, unfortunately it doesn't work. I haven't been able to figure out what's going on, but regardless of the setting for prompt_pin, I now get pkcs11.exceptions.PKCS11Error: No token with label 123456789 found (even if I let it promt the pin and enter the correct one). The tri-state enum seems to work (I think it does select PROTECTED_AUTH correctly), but something else seems to have gone awry with the PCKS11 setup so it never really gets that far. I first thought my device had an issue, but it works fine with the regular pyhanko version.

[MatthiasValvekens]

Hmm, that's curious. Could you try applying your original change (the None -> PROTECTED_AUTH one) on the tip of the current master branch and run that? If that also doesn't work, then I probably inadvertently changed something while refactoring things to make them more testable...

Are you specifying both a slot index and a token label, or only a label?

[pquL1]

Seems like it's the same problem with the current master branch. I'm only supplying the token label.

[MatthiasValvekens]

That's really weird. So, you mentioned letting the device prompt for PIN and then getting the error? Because the routine that raises that exception runs before the token.open(...) method is ever called... https://github.com/MatthiasValvekens/pyHanko/blob/b831bf1a6d22111f634fa3ad1cdc1d6516905d04/pyhanko/sign/pkcs11.py#L52-L87

I'll try debugging this further tomorrow, but since it's kind of hard to repro without having your setup in front of me, it would be extremely helpful if you could help me figure out which of the commits between @551c8bc and the current master branch ended up breaking things... :/. It's probably something silly.

Anyway, big thanks for the input so far!

[MatthiasValvekens]

Hmmm, I'm not immediately seeing what could cause the issue you mentioned. Would you mind sharing the config and CLI invocation you're using?

[pquL1]

Hi @MatthiasValvekens, 60a9dbc introduced the issue. Starting with that commit, my device doesn't work at all anymore. Even if I set prompt_pin to true, it doesn't find the token. 551c8bc0 still works.

My PKCS11 config is

        module-path: /usr/local/lib/libpkcs11tcos3SigG_x86_x64-1.8.0.dylib
        token-label: 123456789
        cert-label: ABC
        key-label: XYZ
        raw-mechanism: True
        prompt_pin: True <-- using this for testing

My CLI invocation is long, but with regard to PKCS11, it only specifies the p11-setup.

Thanks for your efforts to solve this!

[pquL1]

Hi Matthias, I think I found the issue. I didn't put quotes around my token label in the config and after the change it seems to have been interpreted as an int so the comparison failed.

With quotes around the label, the deferred PIN entry works! Very neat!

One more thing: I get a Segmentation fault: 11 with almost every signature I make. The signature comes out fine, though. I haven't been able to figure out what causes it. However, sometimes I don't get the error and I think that this often times goes along with the PIN PAD activating faster than usual (quite difficult to know for sure because I can't intentionally reproduce it yet). I would guess that this has nothing to do with anything in this issue but I still thought I'd mention it, maybe it rings a bell...

[MatthiasValvekens]

Hi @pquL1, thanks a lot!

Hi Matthias, I think I found the issue. I didn't put quotes around my token label in the config and after the change it seems to have been interpreted as an int so the comparison failed.

Huh, weird. I didn't really touch that part of the config deserialisation logic, but YAML sometimes does bizarre things... Glad you got it sorted, though.

One more thing: I get a Segmentation fault: 11 with almost every signature I make. The signature comes out fine, though. I haven't been able to figure out what causes it. However, sometimes I don't get the error and I think that this often times goes along with the PIN PAD activating faster than usual (quite difficult to know for sure because I can't intentionally reproduce it yet). I would guess that this has nothing to do with anything in this issue but I still thought I'd mention it, maybe it rings a bell...

Interesting... I have some stress tests in the test suite that also segfault more often than I'd like them to... I wasn't sure if it was a bug in python-pkcs11, in SoftHSM or in the way pyHanko calls them, though. There's a chance that those are related. I never run into issues triggering signatures one by one, though, so it might be something else.

Let me know if you manage to figure out anything more!