PeculiarVentures / graphene-cli

The graphene-cli is a cross-platform command line tool for working with PKCS#11 devices
MIT License
5 stars 11 forks source link

Duplicate Key IDs being generated #13

Open Matt561 opened 3 years ago

Matt561 commented 3 years ago

Hi there,

I'm encountering a potential bug using a LunaHSM. When generating new key pairs using the object generate ... command I am getting unique handles but duplicate ID values when looking at the object info.

This seems to happen when I create a key pair, back out of graphene and re enter it.

My application uses the graphene-cli tool to programmatically run commands. This means that each time I want to generate a key pair or sign data I have to reopen graphene (npx graphene) and load the module.

This bug doesn't happen when I generate all keys sequentially within the same instance of graphene.

Seems that reloading the module or reopening the slot causes the IDs on generated objects to return to 0100000000000000.

Also, when signing data using existing objects, the param is called --handle but when passing in the handle value (ex. 1) it cannot find the signing key. Passing in the full ID of 0100000000000000 works as expected though.

These 2 things in conjunction prevent me from accurately selecting a key to sign with since I have duplicate IDs. Seems random which key actually gets selected when it's time to sign.

I would greatly appreciate any help you can provide.

Matt561 commented 3 years ago

I'm noticing that when I generate a new object without adding the -token argument I'm getting the error

CKR_ATTRIBUTE_TYPE_INVALID

passing in either true or false for this allows a key to be generated following the same unexpected behavior mentioned in my last comment.

Using the provided ckdemo cli tool generates the objects (ecdsa secp256k1 key pairs) correctly. It is not recommended in the Luna documentation to use this cli tool in production however.

Hope this helps a bit more!

rmhrisk commented 3 years ago

@Matt561 The CKR_ATTRIBUTE_TYPE_INVALID error may be the HSM saying it requires that the param associated with the -token param as required. The HSM basically has a set of (poorly documented) criteria it needs to be present for each call and each HSM will vary.

Maybe capture a log using https://github.com/Pkcs11Interop/pkcs11-logger to see the difference in API calling behaviors to help localize whats going on.

Matt561 commented 3 years ago

I appreciate the quick response and will look into this library you mentioned.

I'm also curious if you'd know why key pairings generated with Luna's provided ckdemo cli-tool have an empty or blank ID when calling "object info -i" with the graphene-cli tool.

Thinking it maybe has to do with one of the flags selected when generating the keys.

I spent more time on the duplicate Key ID found and haven't made significant progress. I found using ckdemo generates keys successfully but the IDs are missing so I can't use graphene to select and sign.

I will continue on this tomorrow and keep this thread posted if I find anything.

Let me know if any additional findings regarding this issue are found.

Thanks again!

Matt561 commented 3 years ago

@rmhrisk @microshine When signing the param is called --handle yet it is the ID (ex: 0100000000000000) that is accepted. Not the handle as described in the object info -i command.

I may be confusing the two (ID/Handle) but as I understand it, the handle will persist across sessions (assuming token -true is set) and the ID resets back to 0100000000000000 between sessions (opening/closing slot).

If the Handle is the unique identifier how come it cannot be used to select a key when signing?

I'm am trying to get down to the bottom of this asap so that I am not forced to look elsewhere for a pkcs11 implementation.

Since pkcs11 can't guarantee that keys are grabbed in same order, it would be nice to be able to set a label when generating with the cli

Again, I appreciate any support you can provide.

Matt561 commented 3 years ago

Found the cause of the CKR_ATTRIBUTE_TYPE_INVALID error.

Inside of gen.ts when generating an ecdsa key with token true flag. A key with the attributes of private and sensitive is created.

This combination actually soft locked me out of my hsm and required me to reach out to Thales.

They responded saying that a key can't be private and also not sensitive (contradiction).

Will probably to try forking this repo if possible

rmhrisk commented 3 years ago

Each HSM will have its own interpretation of flags; file a bug for that specific thing and we can get a fix for it. PR welcome but please propose a solution first.

microshine commented 3 years ago

I'm looking through the source code. The generate command creates keys with TEST_KEY_ID identifier. And doesn't allow to manage template attributes. The token flag is only supported.

TEST_KEY_ID could be a reason for the CKR_ATTRIBUTE_TYPE_INVALID error. I'm using SoftHSM and it throws that error if a token with the same ID already exists.

I think we need to update generate command and support other key attributes (eg sensitive, key usages, labels).

Examples

Generate RSA2048 key with encrypt, decrypt, sign and verify key usages.

> object generate -a rsa-2048 --token --sensitive --label "My key" --id 01020304 --usage sved
| Name                 | Value                     |
|----------------------|---------------------------|
| Handle               | 4                         |
| Class                | PRIVATE_KEY               |
| Label                | RSA-2048-privKey          |
| Token                | true                      |
| Private              | true                      |
| Modifiable           | true                      |
| ID                   | 0300000000000000          |
| Mechanism            | RSA                       |
| Local                | true                      |
| Sensitive            | true                      |
| Extractable          | false                     |
| Derive               | false                     |
| Decrypt              | true                      |
| Sign                 | true                      |
| Sign recover         | true                      |
| Unwrap               | true                      |

Some providers don't allow to set false values to object attributes. We could use --no-disabled flag for that

Generate ECDSA key with sign, verify key usages, default label and id

> object generate -a ecdsa-secp256r1 --token --sensitive --usage sv --no-disabled

Also, we need to refuse to Handle usage like object selector. It would be better to use CKA_ID for that. There is only one problem. CKA_ID is not unique. Certificate, private key, and public key may use the same IDs. We could use object type option for that.

> object info --id 01020304 --type private
| Name                 | Value                     |
|----------------------|---------------------------|
| Handle               | 4                         |
| Class                | PRIVATE_KEY               |
| Label                | RSA-2048-privKey          |
| Token                | true                      |
| Private              | true                      |
| Modifiable           | true                      |
| ID                   | 0300000000000000          |
| Mechanism            | RSA                       |
| Local                | true                      |
| Sensitive            | true                      |
| Extractable          | false                     |
| Derive               | false                     |
| Decrypt              | true                      |
| Sign                 | true                      |
| Sign recover         | true                      |
| Unwrap               | true                      |
Matt561 commented 3 years ago

What would the --type private selector provide? In this case is it only grabbing objects with private attribute set to true? Or is it grabbing the private key?

If we can't control which ID is assigned to new keys we could set labels to be unique values and select keys based on the labels. Users would just need to make sure they assign unique labels.

Also, I've noticed when opening a new session and running object list before generating a new key the IDs gets set to the correct value.

For example: without running object list I always get the TEST_KEY_ID value of 0100000000000000 when generating new objects within a new session.

But listing the objects first ensures the ID of the next generated key is correct 0300000000000000 for the private key of the second key pairing.

@microshine will you implement the above changes? If so how long do you estimate it taking?

The reason I ask is that i will work on it if you don't as I could use exactly what you've described asap.

I'm wondering if modifications to the parent graphene library will be necessary to accommodate these changes?

microshine commented 3 years ago

What would the --type private selector provide? In this case is it only grabbing objects with private attribute set to true? Or is it grabbing the private key?

It's grabbing the private key. Option type could be:

If we can't control which ID is assigned to new keys we could set labels to be unique values and select keys based on the labels. Users would just need to make sure they assign unique labels.

Maybe. But you need to set 2 labels for key pair generation (one for the private key and the other for the public key). I prefer to use CKA_ID for that. By default, it could be SHA1 from the public key or a randomly generated value.

What if support 2 types of identification (ID (CKA_ID) or Handle)? For Handle usage option type is not required

object info --id|-id 0102030405
object info --handle|-H 0100000000000000

For example: without running object list I always get the TEST_KEY_ID value of 0100000000000000 when generating new objects within a new session.

It prints <handle> <class> <label> image

will you implement the above changes? If so how long do you estimate it taking?

I'm busy on another project and can't implement changes shortly. PR is appreciated

I'm wondering if modifications to the parent graphene library will be necessary to accommodate these changes?

The parent graphene library doesn't require to be changed for these changes