Implement all miekg/pkcs11 Ctx functions

[JeremyRand]

I noticed today that @bernard-wagner forked this repo last year and added a bunch of missing functions from the pkcs11 spec. That's awesome! I'd really like to merge these changes, assuming they pass a cursory review. Bernard, can you comment on how well-tested these changes are? Are there any known issues that should give us pause in merging them? Is there a recommended way to test them? (Maybe with SoftHSM?) May I ask what you're using pkcs11mod for? (Always curious to hear what my code is being used for in the wild.)

[bernard-wagner]

Hi. Sorry, totally forgot about making a MR for the changes I made.

The changes have been pretty thoroughly tested. I just pushed the last bit of changes that we still outstanding. I needed to implement DecryptNull, SignNull, etc. to pass SoftHSMs benchmarks.

Trying to tidy up a last bit of my stuff before pushing what I have done with the library, but two main things are:
Implement a multiplexing gRPC network HSM so that Kubernetes Pods can make use of an HSM.
Implemented PKCS11 frontend with Azure KeyVault, Google KMS and AWS as backends, allowing KMS services to be used by openssl, ssh and gpg.

If you give me a few days, I will try and get documentation together about how to test with miekg and softhsm.

Test framework:
softhsm benchmark->pkcs11mod->miekg->softhsm

[JeremyRand]

Hi. Sorry, totally forgot about making a MR for the changes I made.

No worries.

The changes have been pretty thoroughly tested. I just pushed the last bit of changes that we still outstanding. I needed to implement DecryptNull, SignNull, etc. to pass SoftHSMs benchmarks.

Ah, excellent!

Trying to tidy up a last bit of my stuff before pushing what I have done with the library, but two main things are:
Implement a multiplexing gRPC network HSM so that Kubernetes Pods can make use of an HSM.
Implemented PKCS11 frontend with Azure KeyVault, Google KMS and AWS as backends, allowing KMS services to be used by openssl, ssh and gpg.

Oh wow, those are quite exciting!

If you give me a few days, I will try and get documentation together about how to test with miekg and softhsm.

Test framework:
softhsm benchmark->pkcs11mod->miekg->softhsm

Yeah that sounds great. Thanks for all your work on this.

[JeremyRand]

@bernard-wagner Maybe I'm missing something obvious, but what's the origin of EncryptNull/DecryptNull/DigestNull/SignNull? I don't see them in miekg/pkcs11, so I'm not sure how the Backend interface can possibly be compatible with pkcs11.Ctx with those members present.

[bernard-wagner]

@bernard-wagner Maybe I'm missing something obvious, but what's the origin of EncryptNull/DecryptNull/DigestNull/SignNull? I don't see them in miekg/pkcs11, so I'm not sure how the Backend interface can possibly be compatible with pkcs11.Ctx with those members present.

Miekg does not implement those, but they are part of the PKCS11 spec. https://www.cryptsoft.com/pkcs11doc/v220/group__SEC__11__2__CONVENTIONS__FOR__FUNCTIONS__RETURNING__OUTPUT__IN__A__VARIABLE__LENGTH__BUFFER.html

Welcome to remove them again, but I needed to extend pkcs11.Ctx to work for all operations.

bernard-wagner/pkcs11@c8e6cef

[JeremyRand]

Miekg does not implement those, but they are part of the PKCS11 spec. https://www.cryptsoft.com/pkcs11doc/v220/group__SEC__11__2__CONVENTIONS__FOR__FUNCTIONS__RETURNING__OUTPUT__IN__A__VARIABLE__LENGTH__BUFFER.html

Welcome to remove them again, but I needed to extend pkcs11.Ctx to work for all operations.

bernard-wagner/pkcs11@c8e6cef

Ah, I see, they're not separate functions in the PKCS11 spec, but rather a different usage of the same functions, which doesn't map to miekg/pkcs11's syntax. That's why I missed them when I grepped the spec.

A quick search of Miek's repo suggests no one has suggested adding those. Would it be feasible to submit your pkcs11 patch to Miek, and see if he's okay with merging it? I'm not opposed to the concept of supporting that usage, but it's probably better for everyone involved to get the relevant changes merged to pkcs11 rather than have pkcs11mod do its own thing without coordination with Miek. If Miek rejects the patch for some reason, then we can figure out what to do about it here.

[bernard-wagner]

https://github.com/bernard-wagner/pkcs11mod-testing

Not cleanest solution, would be great if we can bring in the testing into the actual pkcs11mod package.

[JeremyRand]

@bernard-wagner Okay cool. Let's see, is it okay to split this PR into 2 PR's, with this PR not including the Null functions that upstream miekg/pkcs11 doesn't have, and a 2nd PR adding those functions? That way we can get most of this merged quickly, without blocking everything on dealing with the extra functions.

[bernard-wagner]

@bernard-wagner Okay cool. Let's see, is it okay to split this PR into 2 PR's, with this PR not including the Null functions that upstream miekg/pkcs11 doesn't have, and a 2nd PR adding those functions? That way we can get most of this merged quickly, without blocking everything on dealing with the extra functions.

Removed the additional Backend methods. Will need to consider how to test all the changes without the additional methods. opendnssec/pkcs11-testing relies on changes to run successfully.

[bernard-wagner]

@JeremyRand thoughts on the current state of the MR and possible testing plan solutions?

[JeremyRand]

@bernard-wagner Sorry for the delay on this. So here's a question. I'm not very familiar with the Null variants of those functions, but it looks to me like they're more efficient special cases of the 3-function workflows that are already there. Is it possible to implement the Null variants (with less efficiency) in terms of the 3-function workflow? If so, would such an implementation pass the tests?

[bernard-wagner]

@JeremyRand this would be pretty tricky. For example, the behaviour of Sign is to automatically invoke SignFinal, unless the output PTR is null (which I named SignNull) in which case it will return the length required for the output buffer. Most of the pkcs11 benchmarking tools I have found make use of this variable length buffer feature. For full e2e testing we will most likely need to build a custom test harness that does not make use of variable length buffers.

[JeremyRand]

@bernard-wagner If I understand correctly, the problem is that passing the non-null variant of Sign to miekg/pkcs11 will result in SignFinal being implicitly called by the PKCS#11 module that miekg/pkcs11 has open, meaning that SignInit would have to be called again? Can we work around it like this?

1. User calls pkcs11mod's null variant of Sign.
2. pkcs11mod calls backend's non-null variant of Sign, resulting in a byte slice, and the backend's SignFinal being implicitly called.
3. pkcs11mod returns the length of the byte slice to the user, and caches the byte slice.
4. User calls pkcs11mod's non-null variant of Sign.
5. pkcs11mod returns the cached byte slice, without calling the backend. Both pkcs11mod and the backend are now in sync again with respect to the signing operation being finalized.

AFAICT the behavior that the user sees should be end-to-end consistent regardless of whether pkcs11mod is in the middle. Am I missing something?

[JeremyRand]

https://github.com/bernard-wagner/pkcs11mod-testing

Not cleanest solution, would be great if we can bring in the testing into the actual pkcs11mod package.

@bernard-wagner I just gave this testing framework a quick glance-over, and it looks okay to me. I'd have no objection to adding that framework to pkcs11mod, maybe running on Cirrus CI. Can you update your testing repo to add the LGPLv3+ license, so that it can be added here without any conflicts? (I don't see any license on your repo right now, so it's technically All Rights Reserved.)

[JeremyRand]

This PR doesn't build with latest miekg/pkcs11 due to some minor cgo refactors that Miek made. I've fixed those errors, and also implemented the Null solution from #7 (comment) ; new PR is #31 . Closing this PR in favor of the new one.