Hsm identification

[jku]

I bought a second yubikey so need a way to tell them apart... so: Define a URI that contains identifying information for the HSM token:

• URI path is the keyid
• URI query can contain any token field filters (this is a bit overkill but since there's apparently no standards for this it seemed reasonable)

Example URI: hsm:2?label=YubiKey+PIV+%2315835999

This would use keyid 2 from a token with label "YubiKey PIV #15835999". This example is also what gets created by HSMSigner.import_() by default.

import_() stays backwards compatible, and old URIs keep working but there are some changes in functionality:

• Running import_() now fails if there are more than 1 tokens (but a filter can be provided there as well). Because of this the tests needed some changes (softhsm creates a new token in InitToken???) -- unfortunately this means the default import_() filter is not tested as I couldn't figure out how to remove the extra softhsm token.
• The constructor has a new required argument (this could be fixed but I didn't see it as that important).

I can be persuaded to do these differently.

Please verify and check that the pull request fulfills the following requirements:

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

[jku]

Three questions:

• is filtering tokens with a dictionary overkill and should I have just hard coded filtering on label for now?
• is the "API break" ok or should I try to keep the constructor compatible (my reasoning was that the real API is the private key URL and import_() which do stay compatible)
• is it a softhsm bug that initializing token 0 also creates a unitialized token 1? it makes testing the default import_() case a bit tricky

[lukpueh]

Cool stuff!

I think the filter dictionary is quite neat and doesn't require that much more code compared to a hardcoded a label filter. Plus, your argument that there is no one obvious way to filter tokens makes sense. So even if it's a bit over kill, I'm fine with it.

But I don't understand the purpose of _build_token_filter. Couldn't you just not filter, if no filter is provided?

Could you also re-explain the issue you had with suddenly(?) SoftHSM creating two slots? And, why that doesn't let you test the default case?

[jku]

Could you also re-explain the issue you had with suddenly(?) SoftHSM creating two slots? And, why that doesn't let you test the default case?

What I mean is that before initToken() there is a slot 0 already with some default token I suppose. Then when we call lib.initToken(0,...) that slot 0 contains our testing token. Unfortunately a new slot 1 also appears with some default content.

Since I'm now trying to be more strict about finding a specific key during import_(), I can't use the normal import_() style in tests (this is what would work in normal applications):

uri, key = HSMSigner.import_()

because that now fails since there are two slots and two tokens that could be used and I think the safe option is to fail in that case. So to work around I have to do

uri, key = HSMSigner.import_(self.hsm_keyid_default, self.token_filter)

which does not test the default.

[lukpueh]

Okay, thanks for the explanation.

[lukpueh]

is the "API break" ok or should I try to keep the constructor compatible (my reasoning was that the real API is the private key URL and import_() which do stay compatible)

I'm fine with the API break regardless.

is it a softhsm bug that initializing token 0 also creates a unitialized token 1? it makes testing the default import_() case a bit tricky

I'm fine with ticketizing this issue, unless you have resources to debug SoftHSM.

[jku]

oops sorry for editing your comment by accident

I tried to rply to this:

I'm fine with ticketizing this issue, unless you have resources to debug SoftHSM.

This turns out to be a softhsm feature (that allows you to "create" all the new tokens you want without a separate API): the way to distinguish the uninitialized token from usable tokens is PyKCS11.CKF_TOKEN_INITIALIZED flag which we could check for. I'll try this.

[jku]

Checking for CKF_TOKEN_INITIALIZED works and is likely a good check for real tokens as well, so that issue should be resolved. Thanks for asking the right questions :)

[lukpueh]

Very nice enhancement of the hsm signer! I'll approve despite a few minor nits. Although the double negation in the docstring seems like a mistake and should probably be fixed. The rest is nice to have.

[jku]

Refactoring suggestion was good, I even expanded that a bit. There is now a _find_pkcs_slot(filters: dict[str, str]) -> int and that makes the calling code much easier to reason about.

The failing test is Windows struggling with gpg -- I'm merging this