Thoughts on a library redesign for release 1.0.0

[dmjones]

We plan to redesign crypto11 to address architectural flaws and to simplify its usage. This would be a backwards-incompatible change, leading to a v1.0.0 release.

I'm cc-ing non-Thales folks who have contributed or commented on the library before, in the hope of some feedback or suggestions: @optnfast, @sitano, @cbroglie, @bernard-wagner.

Abandon the singleton pattern

Each instance of crypto11 will have its own configuration. We will extend the configuration object so tokens can be selected by slot number.

Stop randomly generating attribute values

Passing nil for id or label to the key generation functions will no longer generate random values. Instead, it will result in CKA_ID or CKA_LABEL being unset.

Add ...WithAttributes function variants

This is addresses my concerns in #15. All key generation functions will have a WithAttributes variant, which accepts a slice of attributes to apply to the secret key. For key pair functions, a separate slice of attributes will apply to the public key.

There will be a corresponding FindKeyPairWithAttributes function.

Remove ...WithSession functions

To manage the thread safety aspects of crypto11, we assume sessions are managed by our pool and are for the exclusive use of crypto11. Accepting sessions from the caller could easily break that guarantee.

If anyone is actively using these functions, it would be great to understand the use case.

Remove ...OnSlot functions

We can specify a slot number in the config, so we don't need these functions any more.

Don't provide access to the PKCS11 context

The current library returns the underlying miekg/pkcs11 context object. For the reasons listed above, relating to thread safety, I think this should be removed.

In fact, I'd suggest the public API doesn't rely on any structures from miekg/pkcs11. It's confusing for new users to mix and match data structures from two libraries, only one of which is under our control.

Add all the certificate functions from #20

We can add all the certificate finding and importing code from #20. (Don't worry @bernard-wagner, I can refactor this time!).

I'm no longer concerned about supporting importing certificates, because a WithAttributes variant can be supported.

Keep session pools as they are now

Looking at the comments in #9, I think everyone is now happy with the session pool configuration options. The move away from a singleton pattern should allow callers to set sensible values for each instance of crypto11. Comments very welcome on this, though.

[dmjones]

Other issues that can be closed or rolled into this change:

• Add information about Configure and ConfigureFromFile to README #30
• Return error when Configure or ConfigureFromFile has not been called #29
• Add error return to PKCS11Session.Close (breaking change) #25
• Consistent error behavior in Configure() #4

[cbroglie]

This all sounds good to me @dmjones. Thank you for reaching out.

[sitano]

Some thoughts on your points:

• It's a good point that public API must be clear of internal pkcs11 structs. The library it self provided convenient abstractions over sessions handling. Yet there can be a usage pattern that someone accesses a pkcs11 context to work with it directly, but session handling left to the lib. Maybe its worth to save this opportunity for the users.
• Pool as external dependency still is a question for me.
• Maybe the interface between the pool and lib worth be abstracted.
• A lot will depend on how the session.go will be rewritten.

[dmjones]

@sitano Thanks for your feedback.

Yet there can be a usage pattern that someone accesses a pkcs11 context to work with it directly, but session handling left to the lib. Maybe its worth to save this opportunity for the users.

I agree with this possibility. But I worry more about failing to keep thread safety promises if users do the wrong thing with the context. I'm still inclined to remove this capability.

• Pool as external dependency still is a question for me.
• Maybe the interface between the pool and lib worth be abstracted.
• A lot will depend on how the session.go will be rewritten.

My plan was to keep the session handling unchanged. Moving away from a singleton pattern means the existing pool settings can be changed for each instance, rather than being global. Can you suggest any concrete changes you would like to see, beyond that?

[sitano]

@dmjones no, I don't have any concrete things on mind currently)

[dmjones]

Majority of these changes implemented in #40.