Hi, I haven't had time to look at the entire patch set, so this is just some remarks on the points that Emeric mentioned.
>> Code initially use 'ctx->default_passwd_callback_userdata' to allow us to >> manage a further way to manage passphrase via configuration. > > As far as I could tell, this field was not getting set anywhere in the > code. It¹s set with SSL_CTX_set_default_passwd_cb, which I did not find in > the codebase. I had then assumed that this was just a harmless copy/paste > of example code, which generally always use this CB. I faced the same issue with the patch I sent earlier to provide support for OpenSSL 1.1.x, because the default_passwd_callback_userdata can't be accessed anymore (opaque struct). I couldn't find a place where callback/callback_userdata are used in the code either, so I just replaced them with NULL, which is equivalent when the callback is not set. Note that PEM_read_bio_PrivateKey accepts a pem_password_cb and the associated user data, so I don't think it's an issue if we want to handle password-protected key in a different way in the future. >> I notice also you continue to load DH parameters for each files. It was >> not a big deal by the past because for each file correspond a uniq >> certificate. >> >> With your patch, i don't know what will be the behavior. In the way it is >> loaded on SSL_CTX, the DH parameter doesn't seem specific to the used >> DSA/RSA/ECDSA certificate. So which one will be used, first or latest >> loaded? >> >> Some users will expect to use the DH parameter defined in the same file >> than the used certificate, but i'm really not sure it will be the case. > > Unsure about this. I can change it to only load the first time the context > is created. The last DH parameters set with SSL_CTX_set_tmp_dh() will be used, because the DSA/RSA/ECDSA certs will share the same SSL_CTX (that's the whole point actually) and OpenSSL only supports one in a SSL_CTX. We could change the way DH parameters are loaded for a bind, splitting the certificate and the DH parameters in separate files. It would be clearer but would not play nice with existing configuration. Maybe we should just issue a warning when we detect that different DH parameters are being loaded on the same SSL_CTX (ie, check in ssl_sock_load_dh_params() if the SSL_CTX already has a tmp_dh, and if so check if it's a different one)?
signature.asc
Description: OpenPGP digital signature
