Ruediger Pluem wrote:
+ res = apr_evp_factory_create(f, conf->keyfile, conf->certfile, NULL, + NULL, NULL, conf->digest, APR_EVP_FACTORY_ASYM, r->pool);How do you know that a conf->keyfile was set?
You don't need to know, either option is valid - your options are the cert and key together in the same certfile with keyfile NULL, or the cert and key specified separately in two different files. apr_evp_factory_create() is expected to behave rationally when keyfile is NULL.
+ if (APR_ENOTIMPL == res) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, res, r, LOG_PREFIX+ "generic public/private key encryption is not supported by " + "this version of APR. session encryption not possible");+ } + } + if (conf->passphrase) { + *key = APR_EVP_KEY_SYM; + res = apr_evp_factory_create(f, NULL, NULL, conf->cipher,+ conf->passphrase, NULL, conf->digest, APR_EVP_FACTORY_SYM, r->pool);Hm. Why not supplying conf->engine if set?
An oversight, will fix.
According to the documentation of APR-UTIL the passphrase parameter is used for assymetrical encryption (I assume to decrypt the private key). So why passing it along here?And what happens above if the private key is password protected?
Let me look at this again - I wrote the asymmetrical support for apr-util a while back based on the capabilities of the not-yet-released v0.9.9 of openssl, and this stuff is marked as experimental at this point.
What is supposed to happen is that if the certfile is set, asymmetrical encryption should be tried, and if certfile is not set, symmetrical encryption should be tried. The passphrase for both asymmetrical and symmetrical encryption should be set in conf->passphrase.
Will fix.
+ /* don't attempt to encrypt an empty string, trying to do so causes a segfault */+ if (!in || !*in) { + return APR_SUCCESS;But should we set *out to something meaningful like "" or NULL?
We probably should to be safe and set it. Fixed.
+ /* decrypt the given string */+ res = apr_evp_crypt(e, &decrypted, &decryptedlen, (unsigned char *) decoded, decodedlen);+ if (res) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, res, r, LOG_PREFIX + "decrypt: attempt to decrypt failed"); + return res;Don't we need to cleanup the factory here? Don't we need to call apr_evp_crypt_cleanup in this case?
We do. Fixed.
+ if (conf->passphrase_set || conf->certfile_set) { + res = encrypt_string(r, z->encoded, &encoded);Why not supplying conf to encrypt_string / decrypt_string?
An oversight while refactoring, will fix.
+ /* default cipher AES256-SHA */ + new->cipher = DEFAULT_CIPHER; + new->cipher_set = 1;This seems wrong to me. By this the cipher is set to the default value every you do NOT set it. Even if it was set differently in an enclosing container.
Yes, this is correct - the DEFAULT_CIPHER should only apply when you do NOT set it as you have said, because it is the default. If someone uses the SessionCryptoCipher directive, it will override this default.
The "_set" flag pattern solves some subtle config bugs in the server caused at merge time.
When a merge happens, an additional config is overlaid on top of the base config so far. If you use the simple premise that if the additional config is not null it should override the base config, and you have set a default, you'll find that the default finds its way into the additional config, which is then merged over the base config, effectively "undoing" the config underneath, replacing the base config with the default without the admin having explicitly asked for this.
In some places in the server, the addition of an unrelated directive within the same config structure can suddenly cause other directives to "disappear" and revert to the defaults, this is why.
+ new->digest_set = 1; + + /* initialise SSL */ + apr_ssl_init();Do we need to call this over and over again each we create a dir_config?
We need to call this at least once, and doing this in the config means that it can be guaranteed to be done by the time the first request comes.
Unless there is something I missed (quite possible), there is not currently a hook that gets run when the config is complete. This would be very useful to initialise things once after all the config directives are parsed.
Regards, Graham --
smime.p7s
Description: S/MIME Cryptographic Signature
