Implicit sign_init in sign operation prevents some use cases

[simo5]

The PKCS#11 spec allows keys to require context specific authentication before each operation.

When a key has the CKA_ALWAYS_AUTHENTICATE flag these operations needs to be performed exactly in this order:

C_SignInit
C_login (with CKU_CONTEXT_SPECIFIC and User's PIN)
C_Sign

however session.sign always performs a C_SignInit implicitly.

In some cases you can do use multi-part operations via:

session.sign_init
session.login (see above)
session.sign_update
session.sign_final

But not all mechanisms and (not all tokens) support multi-part operations.

A possible solution is to add a new sign_single() operation that does one shot like sign() except it leaves out calling sign_init().

Another possible way is to create a new high level sign_with_context operation which does the login with context sequence internally, however this may not the only case where you need to separate sign_init from sign so probably not a good idea.

I think similar issues are present for all other APIs (verify/encrypt/decrypt/etc...)

[keldonin]

My two cents:

A high-level approach looks interesting. It would shield some of the low-level machinery from the end user. There is an ongoing PR (#354) that introduces a Credential
type; referring to it, as this could be used an Option<&T> argument when building the context, and support all cases including authenticated path (assuming the PR is approved and merged).

Alternatively, augmenting the current APIs with e.g. an Option<&Credential> could fit the bill, but yes, that's quite a breaking change.

[simo5]

Well keep in mind the app may need to query the user to get credentials, so an Option<&Credential> wouldn't really cover all use cases ...

Also keep in mind that a failure to authenticate leaves the context "dirty" right now, a new sign() on the same session will fail with CKR_OPERATION_ACTIVE because there is an active session and a non fatal error was returned that does not invalidate the operation; but the current code does not handle it, and does not give a way to the application to handle it, beyond tossing the session.

[keldonin]

Well keep in mind the app may need to query the user to get credentials, so an Option<&Credential> wouldn't really cover all use cases ...

The proposed PR is rather using Into<Credential<'a>>, which leaves the freedom to provide a PIN or use authenticated path at context creation time. Do you see cases where this wouldn't work?

Also keep in mind that a failure to authenticate leaves the context "dirty" right now, a new sign() on the same session will fail with CKR_OPERATION_ACTIVE because there is an active session and a non fatal error was returned that does not invalidate the operation; but the current code does not handle it, and does not give a way to the application to handle it, beyond tossing the session.

Fair point. Opportunity for a call to C_SessionCancel() ?

[simo5]

Well keep in mind the app may need to query the user to get credentials, so an Option<&Credential> wouldn't really cover all use cases ...

The proposed PR is rather using Into<Credential<'a>>, which leaves the freedom to provide a PIN or use authenticated path at context creation time. Do you see cases where this wouldn't work?

As I said the application may need to request a PIN to the user, it won't know if it needs to until it sees the error, which it can;t see because sign_init() hasn't been executed yet.

For those case when a pin is already available it would work, for cases where you want to force the pin to be re-request when per-key auth is needed, then it won't.

Also keep in mind that a failure to authenticate leaves the context "dirty" right now, a new sign() on the same session will fail with CKR_OPERATION_ACTIVE because there is an active session and a non fatal error was returned that does not invalidate the operation; but the current code does not handle it, and does not give a way to the application to handle it, beyond tossing the session.

Fair point. Opportunity for a call to C_SessionCancel() ?

Sure if you want to also lose all other operations and potentially ephemeral keys stored in the session ?

[simo5]

To be clear, I think sign() as is is fine for naive applications, but for non-naice one I think you need to offer sign_init+sign_single (whatever the naming will be).

Ah and of course in sing() you need to catch CKR_USER_NOT_LOGGED_IN on calling C_Sign and either call C_SignInit with pMechanism sell to null to abort the operation (this is the only method for tokens with 2.40 interface) or C_sessionCancel with the CKF_SIGN flag

[simo5]

Fair point. Opportunity for a call to C_SessionCancel() ?

Sure if you want to also lose all other operations and potentially ephemeral keys stored in the session ?

Ignore this, I read SessionClose() :-(

C_SessionCancel() is fine for > 3.0 tokens, but I think you should call it automatically in sign(), otherwise it becomes awkward to catch this in naive applications.

I think you should either provide "batteries included" apis that take care of always leaving the session in a good state or should let application dig down in the bowels of pkcs#11 .. half&half is disorienting IMO.

[simo5]

potentially a better abstraction for this kind of operations is to create a high level object, for Signature is could be Sign(), this object would ingest a reference to the session.
The creation of Sign will automatically handle sign_init(), then you can have sign() or update()/final() calls from it:

let s = Sign(session, mech, key_handle);

then:

let sig = s.sign(data);

or:

s.update(data);
....
let sig = s.finalize();


In the case of user auth required then an app could call s.user_auth(pin) on error ...

;potentially you could also offer a s.reinit() function that allows to re-init without having to pass again all the data (assuming you can keep references to the mech data in the internal Sign structure)

[hug-dev]

A possible solution is to add a new sign_single() operation that does one shot like sign() except it leaves out calling sign_init().

I think that seems like a good and simple option! I would leave out any high-level, automatic, solution until we have had more users complaining/requesting for such :)

Would be nice to make sure that the documentation of the function is clear so that users don't use this one by mistake instead of sign though!