[PATCH] allow to use engine keyform for server private key

[Maxim Dounin]

Hello!

On Fri, Aug 01, 2014 at 09:20:02AM +0400, Dmitrii Pichulin wrote:

> On 31.07.2014 17:49, Maxim Dounin wrote:
> >> + if (engine == NULL) {
> >> + ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> >> + "ENGINE_by_id(\"%s\") failed", p);
> >> + return NGX_ERROR;
> >> + }
> >> +
> >> + p[last - p] = ':';
> >> +
> >> + if (passwords) {
> >> + pwd = passwords->elts;
> >> +
> >> + ngx_cpystrn(pwd_buf, pwd->data, pwd->len + 1);
> >> +
> >> + pwd_data.password = pwd_buf;
> >> + } else {
> >> + pwd_data.password = NULL;
> >> + }
> >> + pwd_data.prompt_info = NULL;
> >> +
> >> + last++;
> >> +
> >> + private_key = ENGINE_load_private_key(engine, last, 0,
> >> + (void *) &pwd_data);
> > I don't see how it's expected to work. You only pass private data
> > for UI callbacks, but not callbacks itself.
> >
> > Anyway, proper implementation of passing key passwords into an engine
> > seems to be rather big, and as per my reading of the code under
> > crypto/engine won't work with most of the engines anyway. It
> > might be better idea to don't try to do this for now.
> 
> Maxim, our vision is based on 2 implementations of engines as previously
> noted:
> 1) gost_capi — doesn't support external passwords
> 2) opensc — with such code from get_pin function
> (https://github.com/OpenSC/engine_pkcs11/blob/master/src/engine_pkcs11.c):
> 
> /* either get the pin code from the supplied callback data, or get the pin
> * via asking our self. In both cases keep a copy of the pin code in the
> * pin variable (strdup'ed copy). */
> static int get_pin(UI_METHOD * ui_method, void *callback_data)
> {
> UI *ui;
> struct {
> const void *password;
> const char *prompt_info;
> } *mycb = callback_data;
> 
> /* pin in the call back data, copy and use */
> if (mycb != NULL && mycb->password) {
> pin = (char *)calloc(MAX_PIN_LENGTH, sizeof(char));
> if (!pin)
> return 0;
> strncpy(pin,mycb->password,MAX_PIN_LENGTH);
> pin_length = MAX_PIN_LENGTH;
> return 1;
> }
> ...
> 
> As you can see, there's no need for ui_method if a password is present.

The code in the get_pin() function provided looks utterly wrong - 
it tries to access private data of UI_METHOD, assuming some 
structure.   This may work if stucture match (it seems to be 
copied from PW_CB_DATA used by openssl apps, see apps/apps.[ch]), 
but will result in segmentation fault if it doesn't.

Proper engine behaviour would be to use provided UI_METHOD's 
callbacks to ask for a password, using the 
UI_INPUT_FLAG_DEFAULT_PWD flag, see engines/e_chil.c for an 
example.  This seems to be understood at least by openssl itself 
and curl, see here:
 
http://curl.haxx.se/mail/lib-2013-08/0265.html

> We suggest to implement something like this:
> 
> typedef struct {
> const void *password;
> const char *prompt_info;
> ngx_array_t *passwords;
> ngx_uint_t position;
> } ngx_openssl_pw_cb_data_ex;
> 
> In this case, our ui_read implementation can run through all passwords,
> while supporting the basics.

There is no need to provide complex structure as there should be a 
loop to call ENGINE_load_private_keyi() multiple times anyway.

> Or it would be better to pass nothing for now?

Given the fact how crappy underlying code in engines is, I 
seriously doubt it worth supporting.  Better to don't try for now.

-- 
Maxim Dounin
http://nginx.org/