Add HSMSigner

[lukpueh]

based on #229
updated PR title and description on 12/7/2022
un-draft on 12/12/2022

--
Description
Adds basic HSMSigner implementation to generate signatures on a hardware security module and to export the corresponding public key (as SSlibKey).

HSMSigner uses the first token it finds, if multiple tokens are available, and can be instantiated directly or with Signer.from_priv_key_uri("hsm:", public_key, secret_handler). The former can be passed a keyid, the latter uses the key on the digital signature default piv slot 9c.

Supported keys are ecdsa on SECG curves secp256r1 (NIST P-256) or secp384r1 (NIST P-384), which correspond to securesystemslib signing schemes "ecdsa-sha2-nistp256" and "ecdsa-sha2-nistp384".

Tests are performed on SoftHSM (virtual hsm).

Future work

• If we want to support schemes that allow arbitrary token reader slots, e.g.
  hsm:YubiKey%20PIV%20%2315835999?piv_slot=9c we need to change the HSMSigner constructor, to e.g. add an hsm_label argument. This could be an optional non-API-breaking addition.
• If we want to export public keys automatically, e.g. if no key is passed to 'from_priv_key_uri', we also need to pass the sslib keyid via uri parameter, or use generate a random/canonical keyid (the choice should maybe also be parameterizable

Notes to reviewer

• Keeping this as draft until #442 is merged (please ignore Jussi's commits until then)
• The first of my commits is what started this PR. Everything afterwards is new.
• I don't recommend reviewing commit by commit. They tell an engineering story, but are not all relevant for reviewers.
• If desired, I can spend some time on rewriting the commit history, which will likely be just squashing everything. :)
• Ping me if you have questions.

[lukpueh]

Usage example (tried on YubiKey 5 Nano)

# Create and sign key
yubico-piv-tool -a generate -s 9c -A ECCP256 -k -o public.pem
yubico-piv-tool -a verify-pin -a selfsign -s 9c -i public.pem -S /CN=a.b.c/OU=abc/O=b.c/ -o cert.pem
yubico-piv-tool -k -a import-certificate -s 9c -i cert.pem

from PyKCS11 import PyKCS11
from securesystemslib.signer import HSMKey, HSMSigner


lib = "<PATH/TO/PKCS11/LIBRARY>"
#     e.g. "/usr/local/Cellar/yubico-piv-tool/2.3.0/lib/libykcs11.dylib" (via `brew`)
hsm_keyid = "<HSM KEY IDTUPLE>"
#     e.g. `(2,)`; see `pkcs15-tool --list-key`
pin = "<YOUR USER PIN>"

sslib_keyid = "0"*64

pkcs11 = PyKCS11.PyKCS11Lib()
pkcs11.load(lib)
slot = pkcs11.getSlotList(tokenPresent=True)[0]
session = pkcs11.openSession(slot)

pubkey = HSMKey.from_hsm(session, hsm_keyid, sslib_keyid)

session.login(pin)
signer = HSMSigner(session, hsm_keyid, pubkey)
sig = signer.sign(b"DATA")
session.logout()
session.closeSession()

pubkey.verify_signature(sig, b"DATA")

[jku]

Very cool

So I suppose the issues to solve are following (and they don't all need to be solved here: we can merge new signers without issues now since they don't really affect the existing one):

• locating the pkcs11 module path: If PYKCS11LIB (or some other) environment variable works, I think it would be fine to just document that
• identifying the correct private key (aka what needs to be in private key URI for this signer). I'm really not sure what these are exactly so have no opinions... Have you found good docs on what these really are?
  • id tuple?
  • slot?
  • label?
• key import: I think this should be a Signer feature. This way Key can be a common SSLibKey:
  • first create a Signer -- maybe no need for new functions even: if your constructor gets a public_key, use that. If public_key is None, construct it from the HSM content
  • then imported_pubkey = signer.public_key
• pkcs11 session: this seems like it should be a HSMSigner implementation detail?
• pin entry: SecretHandler in Signer should work for this

[lukpueh]

• locating the pkcs11 module path: If PYKCS11LIB (or some other) environment variable works, I think it would be fine to just document that

Yes, I think that documenting PYKCS11LIB is good enough, plus some hints on how to locate the path.

• identifying the correct private key (aka what needs to be in private key URI for this signer). I'm really not sure what these are exactly so have no opinions... Have you found good docs on what these really are?

  • id tuple?
  • slot?

Haven't found any good docs, but I think id should be fine for the key. For the slot/token, I am not sure. The slot ID can change between sessions. So maybe label would work.

• key import: I think this should be a Signer feature. This way Key can be a common SSLibKey:

  • first create a Signer -- maybe no need for new functions even: if your constructor gets a public_key, use that. If public_key is None, construct it from the HSM content
  • then imported_pubkey = signer.public_key

Great idea!

• pkcs11 session: this seems like it should be a HSMSigner implementation detail?

Agreed, this is just a crutch until I have figured out a good way to identify the token. That's also why the PR is still a draft.

• pin entry: SecretHandler in Signer should work for this

Yes, but as an argument to `from_priv_key_uri akin to SSlibSigner, right? The HSMSigner could still receive the pin as constructor argument. Or would you just pass on the handler?

[lukpueh]

Some comments about testing, CI and dependencies...

[lukpueh]

I had to lower this because aggregate_tests does not include tests for new HSM code, thus the coverage of this run drops, even though that code is tested (see [testenv:hsm] below).

[lukpueh]

This still needs to be 95%, but the reason changed. The PR now also pulls in #442, which is excluded from aggregate_tests. I should be able to increase the number was that #442 is merged.

[jku]

identifying the correct private key (aka what needs to be in private key URI for this signer). I'm really not sure what these are exactly so have no opinions... Have you found good docs on what these really are?

• id tuple?
• slot?
• label?

So first confusion source is that pkcs11 slot seems to be completely different from what yubikey docs call a slot??? I suppose PKCS11 slot is something that can e.g. change over reboots etc?

So we want to be able to potentially select

• token (but a good first implemenation would be to assume only one token is available)
• the key within the token (by CKA_ID or CKA_LABEL?) which maybe maps to yubico slots like "9c" . Can the user see CKA_ID or CKA_LABEL anywhere? (Potential first implementation: If we can tell which CKA_ID is "9c" we could start by only supporting "9c"?)

[jku]

The HSMSigner could still receive the pin as constructor argument. Or would you just pass on the handler?

🤷 I think handler as argument to constructor makes sense?

[lukpueh]

identifying the correct private key (aka what needs to be in private key URI for this signer). I'm really not sure what these are exactly so have no opinions... Have you found good docs on what these really are?

• id tuple?
• slot?
• label?

So first confusion source is that pkcs11 slot seems to be completely different from what yubikey docs call a slot??? I suppose PKCS11 slot is something that can e.g. change over reboots etc?

Yeah, YubiKey docs refer to PIV certificate slots (PIV is a separate standard). In PKCS11 slots and tokens are defined as:

• Slot -- A logical reader that potentially contains a token.
• Token -- The logical view of a cryptographic device defined by Cryptoki.

Not sure what you mean by reboot, but I saw that slot ids can change even between pkcs11 sessions.

So we want to be able to potentially select

• token (but a good first implemenation would be to assume only one token is available)

👍

• the key within the token (by CKA_ID or CKA_LABEL?) which maybe maps to yubico slots like "9c" . Can the user see CKA_ID or CKA_LABEL anywhere? (Potential first implementation: If we can tell which CKA_ID is "9c" we could start by only supporting "9c"?)

In my example I just used "9c" when creating the key, because YubiKey docs suggest that it is semantically the correct slot for a signing key. I don't think our API needs to care about the semantics of PIV slots. They are not related to the hsm keyid.

The hsm keyid can be determined e.g. with

 $ pkcs11-tool -lO
Using slot 0 with a present token (0x0)
Logging in to "host.example.com".
Please enter User PIN:
Private Key Object; EC
  [...]
  ID:         03
  [...]
Public Key Object; EC  EC_POINT 256 bits
  [...]
  ID:         03
  [...]

It's the same for public and private key and remains stable.

[jku]

When I use pkcs-tool -lO, the results depend quite a bit on the pkcs module but both opensc and yubico seem to have a Digital Signing key in ID 02. This seems to match the table at https://developers.yubico.com/yubico-piv-tool/YKCS11/:

ykcs11 id   PIV
1           9a
2           9c
3           9d
4           9e
5 - 24      82 - 95
25          f9

I was hoping this would be true, that the key ids would be predictable... Are you seeing the signing key at id 3 or was that just an example of the output? Does the ID depend on the module you use? I was really hoping we could avoid handwave documentation like "now find out which CKA_ID your key ended up in and insert that in the configuration file"

[lukpueh]

Thanks for digging, @jku! Just tried pkcs11-tool with libykcs11.dylib instead of the default opensc-pkcs11.so module and now I see a lot more keys on the yubikey, and the mapping seems to match the table. 🎉

$ pkcs11-tool --module /usr/local/lib/libykcs11.dylib -O
[...]
Public Key Object; EC  EC_POINT 256 bits
  label:      Public key for Digital Signature
  ID:         02
  [...]
Public Key Object; EC  EC_POINT 256 bits
  label:      Public key for Key Management
  ID:         03
  [...]

I was already wondering, why I can pass IDs 2 and 3, to sign, when, according to pkcs11-tool, I only have one signing key at ID 3.

[jku]

So then the version 1 implementation could

• assume first token is the one we want
• assume key id is 02
• document that this has been tested with libykcs and PIV slot "9c"

then the first version would need no path or query params for the private key URI: pkcs11: or hsm: would be enough. Then later we could add optional paths/params if needed: hsm:YubiKey%20PIV%20%2315835999?piv_slot=9c (here the path is the url encoded token label and piv_slot is a query param)

[lukpueh]

Just rebased on top of #445 to pull in 16734bb (for sub-modules) and added a bunch of WIP-commits based on our latest discussion. Will ping for re-review when ready.

[lukpueh]

Okay, this is ready for re-review. See updated PR description for what changed.

[jku]

Looks good to merge to me. Left comments but I don't think any of them are blockers -- although the optional-dependencies discussion might be good to have before merge

[jku]

I think calling secrets_handler() from the constructor might make more sense but this is more like a nit since the constructor is "less public api" than this method is...

The idea behind SecretsHandler is that "pin" or other secrets don't have to be part of the API: that at least theoretically the code can dynamically decide if it needs a pin or not (variable authentication methods are definitely a feature in systems like this, although I don't know if it makes sense with yubikeys), and when it needs it

[lukpueh]

Fixed in 8283650

[jku]

this is a different idea than what I had planned for GCPSigner but I think that's fine for now... I don't think we are sure yet what really works.

My design was

signer = GCPSigner(uri) # when public_key arg is not given, import it
pubkey = signer.public_key

with the logic that the constructor is implementation specific already, but we could make the public_key accessor a part of Signer API: that way implementation specific API does not grow (and import errors are only handled once).

Originally I also thought I would always need a Signer and Key at same time anyway, but I think this is not necessarily true. This would support your classmethod design: if I sometimes only need a Key, why do I need to construct a Signer first?

[jku]

If signer construction was required to import a Key, then signer construction should not trigger a pin request meaning this would mean a bit of refactoring: I'm not asking for that, just thinking out loud

[lukpueh]

I agree with you. Let's fix this with #466. Until then, do you think it's better to merge as non-public _pubkey_from_hsm?

[jku]

I think it's fine as is: if we fix before next release it's a non-issue; if we fix in some later release it's a minor API break

[jku]

all three of the imports get their own errors, own try-except blocks and own error handling. Since only one error is ever reported, this duplication isn't really that useful.

How about just a single HSM_IMPORT_ERROR to simplify the error handling (the separate try-except blocks might still be nice for readability)?

[lukpueh]

I think the separation makes sense wrt current extra dependency handling. Let's fix those together.

[jku]

feels like this isn't the correct way to use this system. pykcs11 without asn1 doesn't make sense so why offer that as option?

[lukpueh]

asn1crypto is only an immediate dependency for pubkey export. For signing we don't use it.

[lukpueh]

But I agree, telling users to run

pip install securesystemslib[crypto, asn1, pykcs11]

is not a lot better, than

pip install cryptography asn1crypto pykcs11 securesystemslib

[lukpueh]

Listing extra dependencies by feature, otoh, seems prone to redundancy. cryptography, for instance, is used for many different features.

[lukpueh]

Just force-pushed a pure rebase without changes and marked as non-draft.

[lukpueh]

Thanks for the review, @jku! I addressed your comments wrt docs and secrets_handler. Would you mind if we take a look at extra dependency definition (and related error handling) in a follow-up? It's not obvious to me, what the best solution is.

[jku]

I'm happy with this, LGTM.