https://issues.apache.org/bugzilla/show_bug.cgi?id=54357
--- Comment #15 from Kaspar Brand <[email protected]> --- (In reply to Alex Bligh from comment #14) > Any feedback on the above? It's on the right track, I think. Comments about v2: - Keying the array/hash on the cert's SHA-1 digest is fine. What it also means is that we don't need to store it on a per-vhost basis (modssl_pk_server_t), it's sufficient to put it into SSLModConfigRec (similar to stapling_cache, this also avoids duplicate storage in case a certificate is used for more than one vhost). I would suggest to rename it to "stapling_cert_info", furthermore, to make its specific purpose more explicit. - Initialization, i.e. apr_hash_make(), is best done in ssl_engine_config.c (in ssl_config_global_create() in this case). You don't need to check for apr_hash_make() or apr_pcalloc() failures, since httpd configures a global OOM handler for these APR failures. - In ssl_stapling_init_cert(), there's a simpler way of achieving memory management by APR: instead of if (aia) { /* Ugly: ensure memory managed by apr */ char *uri; uri = sk_OPENSSL_STRING_pop(aia); cinf->uri = apr_pstrdup(p, uri); OPENSSL_free(uri); } you can simply say: if (aia) { cinf->uri = apr_pstrdup(p, sk_OPENSSL_STRING_value(aia, 0)); X509_email_free(aia); } (Make sure to free "aia" - the "1" in X509_get1_ocsp(x) means that you own the copy.) - In stapling_get_cert_info(), I suggest to check for X509_digest() returning 1 before proceeding, e.g.: if ((x == NULL) || (X509_digest(x, EVP_sha1(), idx, NULL) != 1)) return NULL; - In the certinfo struct, I wonder if we still need to explicitly keep the "idx" member, as we have to calculate the SHA-1 hash on the fly in stapling_get_cert_info() anyway. Could you try to figure out if dropping "idx" from the struct is feasible? (BTW, instead of using "20", I would prefer SHA_DIGEST_LENGTH being used, and the "Cached info stored in certificate ex_info" comment should be reworded.) - In ssl_engine_init.c, there's a second call of ssl_stapling_init_cert() which needs adjustment as well (for the OpenSSL 1.0.2 or later case). - Also remove the ssl_stapling_ex_init() declaration from ssl_private.h. Thanks for your work on getting this fixed, much appreciated. -- You are receiving this mail because: You are the assignee for the bug. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
