Fix #101: Add support for PIN callback

[matthauck]

PIN_GET_CALLBACK retrieves a PIN, and PIN_DONE_CALLBACK releases it so that the application does not need to keep the PIN in memory in clear text if it does not want to.

The ugliest part of this PR is the casting of the function pointer to support the proper parameters we need to send back and forth between the application and libp11, but I do not see any way around this.

[matthauck]

This is to address issue #101

[matthauck]

Would like to add tests here as well. Saw mention of it in the README.md, but didn't see a testing framework in place or examples of how to run the tests that are there...

[mtrojnar]

I think adding new engine ctrls is a very bad idea, because it assumes the application is written for this specific engine. It may work for you, but it is not a generic solution for other users. The existing LOAD_CERT_CTRL is already bad enough. We should not make things even worse.

Instead, we should use SSL_get_default_passwd_cb() to retrieve the registered callback if PKCS#11 requests a pin, and no pin was specified with existing methods,

[matthauck]

(fyi, rebased from master, cleaned up commits, and added some tests)

re. comments above:

I think adding new engine ctrls is a very bad idea, because it assumes the application is written for this specific engine.

True, it does tie the application more to the implementation, and this is unfortunate. However, this is inevitable to some degree, right (i.e. even MODULE_PATH and PIN currently)? Seems like anyone using an engine that requires any amount of configuration would be stuck in the same situation of supporting particularities of that engine they want to use.

Instead, we should use SSL_get_default_passwd_cb()

This may be an option for people who can upgrade to OpenSSL 1.1, but this will not be most people for a long time I think. I'm also struggling to see how that function is relevant here -- feels like that would be overloading that function with something it is not intended for -- passwords for encrypted SSL keys is not the same thing as PCKS11 slot passwords, no? I'm also not sure that callback would provide enough context to make the right decision on the password to use.

[dwmw2]

So, tell me the story about how your callback relates to a generic OpenSSL facility to use a cert/key by filename or PKCS#11 URI without the application having to know or care what it is. Is this going to be usable in that context, when this is just one of the types of thing that the "certificate" string might be referencing? Or is it something completely different that is going to have to be marked obsolete when we move over?

[dwmw2]

I think adding new engine ctrls is a very bad idea, because it assumes the application is written for this specific engine.

@mtrojnar FWIW until recently applications did have to be written for this specific engine, because they had to do all that messing around to load it and load the PKCS#11 provider module manually. It's only fairly recently that we've fixed it to just work with ENGINE_by_id("pkcs11") and no more setup than that.

[matthauck]

So, tell me the story about how your callback relates to a generic OpenSSL facility...

I would argue that such a facility would be a layer on top of lower-level functions in OpenSSL that still do the exact thing you want them to do. Reading a PKCS8 key vs. PKCS1 key vs. PKCS11 key are all going to be different routines. I'm all for a GuessAndReadMyKey! api as long as there is still a DontTryToGuessTheFormat_IWillTellYou api. So, the way the password callback will work at the higher level may be a bit different from how it would look at the lower level.

This PR is about adding it to the lower level. It exposes a "get password" and "release password" pattern that would hopefully also exist for other lower-level APIs as well.

Or is it something completely different that is going to have to be marked obsolete when we move over?

The most obsolete-worthy change here is the callback parameters, which in this case do not correspond to the key URI but to the slot id/label on account of the internals of pkcs11 and the fact that the password is really not on the key, but on the slot. This maybe different from other file-based keys. PKCS12 may be another interesting one to think about in the password encrypting the key store may be different (I think) from the password encrypting the key inside the store.

The trickiest forward-looking thing to accomplish with a PR like this, I suppose, would be coming up with the right callback API to indicate what kind of data will be passed out to the application to figure out which password to provide in a way that can be shared across key encoding types but also reflect each of their nuances.

Maybe the higher level API ends up just exposing a single "get" and "release" callback that passes an indicator (enum?) indicating key type and some structure that can store some kind of generic information that will be interpreted differently depending on key type? Behind the scenes, the higher level callback can interface with the other lower-level callbacks based on their individual needs?

Release Callback

One last thing. I do want to emphasize a bit the idea of a "release callback". I'm not sure this is on your radar, but I think it will be important for some who don't want to keep the password around in memory. Maybe it is done like I have it in this PR w/ two very similar callbacks -- maybe it is done with a single callback with an additional enum/bool/int parameter that indicates whether it is a "get" or a "release".

[matthauck]

Note: This is one area I wasn't entirely sure if I made the right change or not. The code was conflicting with the comment below. It is here required that ctx->pin be non-null, but below, PKCS11_login is called with the expectation that it may be null. The comment seemed more correct since a null pin should be allowed, so I took out this ctx->pin != NULL check.

[mtrojnar]

I think adding new engine ctrls is a very bad idea, because it assumes the application is written for this specific engine.

True, it does tie the application more to the implementation, and this is unfortunate. However, this is inevitable to some degree, right (i.e. even MODULE_PATH and PIN currently)? Seems like anyone using an engine that requires any amount of configuration would be stuck in the same situation of supporting particularities of that engine they want to use.

Well... Not really. Stunnel and several other applications supported MODULE_PATH and PIN with a generic text key/value interface, without a single line of code specific to engine_pkcs11. This method does not work with binary structures or callbacks.

Instead, we should use SSL_get_default_passwd_cb()

Oops. I was wrong. I reviewed the code. The pubkey_function and privkey_function functions of the OpenSSL engine interface already receive a UI_METHOD * parameter. Currently, this UI method is only directly invoked by the engine when retrieving the key, and never passed to libp11.

The solution would be to add a new libp11 function:

void PKCS11_set_ui_method(PKCS11_KEY *key, UI_METHOD *ui_method);

and invoke it from pkcs11_load_key() for the chosen key.
libp11 could then invoke the UI method every time it needs a PIN.

[dwmw2]

True, it does tie the application more to the implementation, and this is unfortunate. However, this is inevitable to some degree, right (i.e. even MODULE_PATH and PIN currently)?

Hm, didn't spot that before. No, MODULE_PATH is definitely not "currently"; it is a historical brain-damage. On systems with p11-kit we load p11-kit-proxy.so and it loads the tokens which are indicated by the system configuration. Nobody should ever mess with MODULE_PATH.

And as noted above, one can also handle the pin with a pin-value= field in the URI. I understand the concern about it "accidentally" bring printed if it's there... but if we can guarantee that libp11 and the engine won't print a PIN from a URI, then perhaps that becomes the easiest way to handle per-token PINs?

[matthauck]

We set MODULE_PATH because it says to on your README, and we do not want to bring in other dependencies. It provides a very nice programmatic way to configure pkcs11 engine from an application's point of view without depending on (read: worrying about) openssl system configuration. As an application I would rather configure a library with code than with a configuration file any day of the week.

Would you be open to accepting a patch that uses UI_METHOD* instead of the current callback scheme? This still may not provide the way to accomplish the "release callback" to clean up memory, but I'd have to see.

I understand your push back and wanting the solution to work for the community and not just for me. =) I do think this is generally useful for being able to better manage the lifetime of passwords in memory. It should be discouraged to have password floating around in memory as plain text. But at the same time, I totally get that you want to make sure it fits with the future vision of libp11.

[matthauck]

@mtrojnar How would you recommend configuring the engine to know whether to always query the UI_METHOD or whether to query it once then cache it?

[matthauck]

argh. If only I had known about UI_METHOD*! 😥 It actually does everything I need it to, and better. =)

1. Allows me to tie the authentication to the key URI and not to the slot which is an implementation detail really
2. Ties in with the openssl interface and doesn't rely on libp11 internals
3. Works today!

One open question I have is how to make libp11 stop caching the password. Any suggestions for how I can approach that in an acceptable way?

[matthauck]

Moving discussion back to #101