Hsm identification by jku · Pull Request #526 · secure-systems-lab/securesystemslib

jku · 2023-03-03T18:02:34Z

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

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 contains token field filters Example URI: "hsm:2?label=YubiKey+PIV+%2315835999" This would use keyid 2 from a token with label "YubiKey+PIV+%2315835999". The example is also what gets automatically created on HSMSigner.import_(). Other fields can also be used -- I believe there is no standard for these so this seemed sensible. 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. import_() stays backwards compatible, and old URIs keep working. The constructor has a new required argument (this could be fixed but I didn't see it as that important).

jku · 2023-03-04T09:23:57Z

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?

securesystemslib/signer/_hsm_signer.py

tests/test_hsm_signer.py

jku · 2023-03-06T10:48:06Z

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 · 2023-03-06T11:04:22Z

Okay, thanks for the explanation.

lukpueh · 2023-03-06T11:09:42Z

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.

This is (mostly) useful for testing as softhsm always has one uninitialized token. By skipping those we can actually test the default import_(). Also remove a debug print() call.

jku · 2023-03-06T12:01:56Z

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 · 2023-03-06T12:04:18Z

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.

securesystemslib/signer/_hsm_signer.py

Refactor duplicated code into _find_pkcs_slot() Tweak some docstrings, mostly to make it clear when "slot" refers to a PKCS slot (a smart card reader) and when it's a PIV slot (which roughly maps to PKCS CKA_ID, the keyid).

jku · 2023-03-07T12:49:28Z

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

jku added 2 commits March 4, 2023 11:06

HSMSigner: Tweak docstring

9a3ab72

jku force-pushed the hsm-identification branch from 24dd312 to 9a3ab72 Compare March 4, 2023 09:14

jku marked this pull request as ready for review March 4, 2023 09:24

lukpueh requested changes Mar 6, 2023

View reviewed changes

securesystemslib/signer/_hsm_signer.py Outdated Show resolved Hide resolved

securesystemslib/signer/_hsm_signer.py Outdated Show resolved Hide resolved

tests/test_hsm_signer.py Outdated Show resolved Hide resolved

tests/test_hsm_signer.py Outdated Show resolved Hide resolved

HSMSigner: Skip uninitialized tokens

c660d3b

This is (mostly) useful for testing as softhsm always has one uninitialized token. By skipping those we can actually test the default import_(). Also remove a debug print() call.

jku force-pushed the hsm-identification branch from 4e00571 to c660d3b Compare March 6, 2023 11:59

jku requested a review from lukpueh March 6, 2023 12:36

jku mentioned this pull request Mar 6, 2023

git-repo: decide on support for multiple keys jku/repository-playground#51

Closed

lukpueh approved these changes Mar 7, 2023

View reviewed changes

HSMSigner: Refactor slot lookup

23fa243

Refactor duplicated code into _find_pkcs_slot() Tweak some docstrings, mostly to make it clear when "slot" refers to a PKCS slot (a smart card reader) and when it's a PIV slot (which roughly maps to PKCS CKA_ID, the keyid).

jku merged commit 1695e45 into secure-systems-lab:main Mar 7, 2023

Conversation

jku commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

jku commented Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukpueh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jku commented Mar 6, 2023

Uh oh!

lukpueh commented Mar 6, 2023

Uh oh!

lukpueh commented Mar 6, 2023 • edited by jku Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jku commented Mar 6, 2023

Uh oh!

jku commented Mar 6, 2023

Uh oh!

lukpueh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jku commented Mar 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jku commented Mar 3, 2023 •

edited

Loading

jku commented Mar 4, 2023 •

edited

Loading

lukpueh commented Mar 6, 2023 •

edited by jku

Loading