Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PKCS11-based HSM interface #229

Closed

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Apr 6, 2020

Fixes issue #:
Supersedes #170 and #221 (big kudos to @tanishqjasoria for excellent groundwork!)
Bases on / incorporates #228 (needs rebase here after #228 is merged) (rebased on #228)

Description of the changes being introduced by the pull request:

Add public interface to:

  • list available HSMs (get_hsms), and
  • keys on a given HSM (get_keys_on_hsm),
  • sign data with a private key on the HSM, exporting the signature into a securesystemslib-like format (create_signature), and to
  • export a key from the HSM using a sslib-like format (export_pubkey) to be used for signature verification.

Signature verification can be performed with the existing securesystemslib.keys.verify_signature function.

Currently the HSM interface supports ecdsa keys and the "ecdsa-sha2-nistp256" and "ecdsa-sha2-nistp384" signature schemes.

Any interaction with the HSM interface require a one-time call to load_pkcs11_lib passing it the path to a PKCS11 dynamic library.

The commit also adds a basic test loop (sign + export key -> verify) using SoftHSM, as well interface tests that check that the public functions error gracefully if optional dependencies are not installed.

TODO:

  • error handling

    • PKCS11 seems brittle, make sure that there are no state problems (e.g. openSession on a slot requires prior call to et slots, etc.) and we always get back the values we expect, i.e. handle hsm return invalid / incomplete data (e.g. on getAttributeValue), and fail gracefully if not
      HINT: check CKR_<RETURN VALUE TYPE> constants
    • revise error messages and exception taxonomy
    • Add HSM_INFO_SCHEMA, HSM_KEY_INFO_SCHEMA, HSM_KEY_ID_SCHEMA and check_match on them.
  • docs

    • flesh out function docstrings and add code comments
    • add links to PKCS11 specs
    • add installation and usage docs, e.g. replace test_hsm.TestECDSAOnLUKPUEHsYubiKey with some instructions for YubiKey in README.md

Please verify and check that the pull request fulfils 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

@lukpueh
Copy link
Member Author

lukpueh commented Apr 6, 2020

Snippets to test with YubiKey on PIV slot

# Create key and sign it (the latter is important!)
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
import securesystemslib.hsm
import securesystemslib.keys

sslib_key_id = \
    "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
scheme = securesystemslib.hsm.ECDSA_SHA2_NISTP256
data = b"data to be signed"

user_pin = "USER PIN" # TODO: Replace
pkcs11_so_path = "/path/to/pkcs11/shared/object/or/dynamic/lib" # TODO: replace
# E.g. /usr/local/Cellar/yubico-piv-tool/2.0.0/lib/libykcs11.dylib when
# installing with brew

# Init PKCS11
securesystemslib.hsm.load_pkcs11_lib(pkcs11_so_path)

# Get YubiKey assuming it is on the 1st slot
hsm_info = securesystemslib.hsm.get_hsms().pop()

key_infos = securesystemslib.hsm.get_keys_on_hsm(hsm_info)

# Grab keyid by label (NOTE: I promise better tooling ;)
hsm_key_id = [key_info["key_id"] for key_info in key_infos if key_info["label"] == "Public key for Digital Signature"].pop()

# Export public key
pub = securesystemslib.hsm.export_pubkey(
    hsm_info, hsm_key_id, scheme, sslib_key_id)

# Create signature
sig = securesystemslib.hsm.create_signature(
    hsm_info, hsm_key_id, user_pin, data, scheme, sslib_key_id)

# Verify signature
sig_valid = securesystemslib.keys.verify_signature(pub, sig, data)

assert sig_valid == True

lukpueh and others added 2 commits April 9, 2020 10:11
- pykcs11 to communicate with HSM (requires native swig and pkcs11
  libraries, kept optional to allow pure Python sslib install)
- asn1crypto to decode ecdsa public keys exported from HSM
- unrelated: update enum34

TODO:
Add installation docs (native dependencies, extras install)
Add public interface to
- list available HSMs (get_hsms), and
- keys on a given HSM (get_keys_on_hsm),
- sign data with a private key on the HSM, exporting the signature
  into a securesystemslib-liek format (create_signature), and to
- export a key from the HSM using a sslib-like format
  (export_pubkey) to be used for signature verification.

Signature verification can be performed with the existing
securesystemslib.keys.verify_signature() function.

Currently the HSM interface supports ecdsa keys and the
"ecdsa-sha2-nistp256" and "ecdsa-sha2-nistp384" signature schemes.

The commit also adds a basic test loop (sign + export key ->
verify) using SoftHSM, as well interface tests that check that the
public functions error gracefully if optional dependencies are not
installed.

TODO:

- error handling
  - PKCS11 seems brittle, make sure that there are no state
    problems (e.g. openSession on a slot requires prior call to get
    slots, etc.) and we always get back the values we expect, i.e.
    handle hsm return invalid / incomplete data (e.g. on
    getAttributeValue), and fail gracefully if not
    HINT: check `CKR_<RETURN VALUE TYPE>` constants
  - revise error messages and exception taxonomy
  - Add HSM_INFO_SCHEMA and HSM_INFO_SCHEMA, HSM_KEY_INFO_SCHEMA,
    HSM_KEY_ID_SCHEMA and check_match on them.

- docs
  - flesh out function docstrings and add code comments
  - add links to PKCS11 specs
  - add installation and usage docs, e.g. replace
    test_hsm.TestECDSAOnLUKPUEHsYubiKey with some instructions for
    YubiKey in README.md

- testing
  - check coverage
  - trigger edge cases (see error handling above)

Co-authored-by: Tanishq Jasoria <jasoriatanishq@gmail.com>
@jku
Copy link
Collaborator

jku commented Oct 21, 2022

slowly moving towards actually testing something... Documenting on the way: this is likely not useful to anyone else:

  • I think this also generates key and signs a cert (I don't have a management key set and yubico-piv-tool does not seem to like that -- this might be a deprecated feature that I should not be using but 🤷 ):

    ykman piv keys generate --algorithm ECCP256 9c pubkey.pem
    ykman piv certificates generate --subject "yubico" 9c pubkey.pem
    
  • I just noticed the step "manually dlopen a library" and ...
    securesystemslib.hsm.load_pkcs11_lib("/usr/local/Cellar/yubico-piv-tool/2.0.0/lib/libykcs11.dylib")
    Is this really the best we can do -- it feels barbaric?
    On linux "/usr/lib/x86_64-linux-gnu/opensc-pkcs11.so" seems to work

  • The label on my HSM key is not what's on your script (maybe because different tool created it)

  • create_signature() fails with PyKCS11.PyKCS11Error: CKR_USER_NOT_LOGGED_IN -- the pin is correct as I get a different error if I use the wrong pin... I wonder if my key is really in working order

@jku
Copy link
Collaborator

jku commented Oct 24, 2022

Continuing documenting my attempt at testing:

Yubico does not document which tool should be used for this but I've now switched to yubico-piv-tool instead of ykman to reproduce Lukas's steps, I have reset the "management key" and...

~/src/securesystemslib$ yubico-piv-tool -a generate -s 9c -A ECCP256 -k -o public.pem
Enter management key: 
Successfully generated a new private key.
~/src/securesystemslib$ 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
Enter PIN: 
Successfully verified PIN.
Failed signing certificate.

Even full verbosity gives nothing useful. This UX is worse than pgp AND it also doesn't work. I am not sure if I can do anything more at the moment

@jku
Copy link
Collaborator

jku commented Nov 7, 2022

So I was finally able to run the snippet succesfully (thanks lukas):

  • yubico-piv-tool is 2.2.0 (on debian) apparently creates broken keys that fail self-sign: installing to 2.3.0 and creating th key there helped
  • the code does not work with opensc-pkcs11.so: signing fails with CKR_USER_NOT_LOGGED_IN. It does owrk with the yubico module. I am able to use pkcs11-tool to sign and verify with either module

So, using yubico-piv-tool 2.3.0 and Yubicos libykcs11.so as the pkcs library, the example code worked.

  • The first issue pivtool 2.2.0 being broken does not need fixing (or can be addressed with a note in docs)
  • the pkcs module issue maybe should be somehow mitigated: I don't understand who is supposed to choose the PKCS11 module and why?

Copy link
Collaborator

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few years later, some review comments (I'm mostly looking at this from the POV of the Signer interface and #447):

I think we should not expose all of this as a new module -- especially things like get_keys_on_hsm(), get_hsms() seem like things that should not be public API here

HSMSigner on the other hand sounds like a reasonable thing: The API would include

  • key and device ids as constructor arguments (optional if PyKCS supports that)
  • PKCS module loaded using environment variable (optional if there are reasonable defaults for some systems)
  • PIN as constructor variable

This would be compatible with #447 with a URI like hsm:device/0/slot/c9 or hsm:?slot=c9&device=0, and pin getting requested with secrets_handler()

After implementing HSMSigner, the remaining public functionality in this PR would then be export_pubkey(). Interestingly this same issue comes up with KMS keys... Maybe public key export could actually be a method on the Signer as well: not all implementations would support it but some could?

Another thing that comes up with KMS keys as well is the need to prehash the payload. We should add that support somewhere -- figuring out the algorithm from the public key is currently very annoying. Can we have Key.get_payload_hash_algorithm()?

Left also some code comments -- they may be obsolete so feel free to disregard

Comment on lines +136 to +142
# If path is not passed PyKCS11 consults the PYKCS11LIB env var
if path is None:
PKCS11.load()

else:
securesystemslib.formats.PATH_SCHEMA.check_match(path)
PKCS11.load(path)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just PKCS11.load(path)? docs seem to imply that None is perfectly vaid here

Comment on lines +146 to +148
except PyKCS11.PyKCS11Error as e:
PKCS11_DYN_LIB = False
raise
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you set PKCS11_DYN_LIB = False before the try block, this except is not needed

import binascii

if not six.PY2:
import asn1crypto.keys
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asn1crypto is now a dependency for all securesystemslib users?

@@ -100,10 +100,11 @@
'Topic :: Software Development'
],
install_requires = ['six>=1.11.0', 'subprocess32; python_version < "3"',
'python-dateutil>=2.8.0'],
'python-dateutil>=2.8.0', 'asn1crypto; python_version > "3"'],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned this elsewhere but this seems wrong: it should be only needed for "pkcs11" extra, right?

@lukpueh lukpueh mentioned this pull request Dec 1, 2022
@lukpueh
Copy link
Member Author

lukpueh commented Dec 1, 2022

Superseded by #472, which adds the relevant parts of this implementation to the new Signer/Key API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants