Re: mod_ssl: inline SSL_X509_INFO_load_path(); please review

[Kaspar Brand]

On 01.05.2015 17:11, Stefan Sperling wrote:
> I believe SSL_X509_INFO_load_path() should be inlined into
> its only caller.

I'm +1 for this. The "Low-Level CA Certificate Loading" part in
ssl_util_ssl.c is / was only used by ssl_init_proxy_certs, so I would be
in favor of also moving SSL_X509_INFO_load_file to ssl_engine_init.c
(and making it static).

> Regarding the removed comment about merging the dir-read loop
> with another one: I don't think that's worth it.

If we can get rid of code duplication on this occasion, then I think we
should do so - it makes future maintenance easier if there is common
code for loading CA files from a directory, be that for client
authentication (SSLCACertificatePath) or for proxy connections
(SSLProxyCACertificatePath).

Kaspar

mod_ssl: inline SSL_X509_INFO_load_path(); please review

[Stefan Sperling]

I believe SSL_X509_INFO_load_path() should be inlined into
its only caller. I'd like some eyes on this change since
it's not just mechanical.

The desired behaviour seems to be load as many certs as possible
from a directory, looping over its file entries. Ignore errors,
e.g. in case the file is not a cert. The replaced function returned
a boolean which was never checked.

Regarding the removed comment about merging the dir-read loop
with another one: I don't think that's worth it.

Index: modules/ssl/ssl_engine_init.c
===
--- modules/ssl/ssl_engine_init.c   (revision 1677159)
+++ modules/ssl/ssl_engine_init.c   (working copy)
@@ -1247,7 +1247,26 @@ static apr_status_t ssl_init_proxy_certs(server_re
 }
 
 if (pkp->cert_path) {
-SSL_X509_INFO_load_path(ptemp, sk, pkp->cert_path);
+apr_dir_t *dir;
+apr_finfo_t dirent;
+apr_int32_t finfo_flags = APR_FINFO_TYPE|APR_FINFO_NAME;
+
+if (apr_dir_open(&dir, pkp->cert_path, ptemp) == APR_SUCCESS) {
+while ((apr_dir_read(&dirent, finfo_flags, dir)) == APR_SUCCESS) {
+const char *fullname;
+
+if (dirent.filetype == APR_DIR) {
+continue; /* don't try to load directories */
+}
+
+fullname = apr_pstrcat(ptemp,
+   pkp->cert_path, "/", dirent.name,
+   NULL);
+modssl_X509_INFO_load_file(ptemp, sk, fullname);
+}
+
+apr_dir_close(dir);
+}
 }
 
 if ((ncerts = sk_X509_INFO_num(sk)) <= 0) {
Index: modules/ssl/ssl_util_ssl.c
===
--- modules/ssl/ssl_util_ssl.c  (revision 1677159)
+++ modules/ssl/ssl_util_ssl.c  (working copy)
@@ -441,43 +441,6 @@ BOOL modssl_X509_INFO_load_file(apr_pool_t *ptemp,
 return TRUE;
 }
 
-BOOL SSL_X509_INFO_load_path(apr_pool_t *ptemp,
- STACK_OF(X509_INFO) *sk,
- const char *pathname)
-{
-/* XXX: this dir read code is exactly the same as that in
- * ssl_engine_init.c, only the call to handle the fullname is different,
- * should fold the duplication.
- */
-apr_dir_t *dir;
-apr_finfo_t dirent;
-apr_int32_t finfo_flags = APR_FINFO_TYPE|APR_FINFO_NAME;
-const char *fullname;
-BOOL ok = FALSE;
-
-if (apr_dir_open(&dir, pathname, ptemp) != APR_SUCCESS) {
-return FALSE;
-}
-
-while ((apr_dir_read(&dirent, finfo_flags, dir)) == APR_SUCCESS) {
-if (dirent.filetype == APR_DIR) {
-continue; /* don't try to load directories */
-}
-
-fullname = apr_pstrcat(ptemp,
-   pathname, "/", dirent.name,
-   NULL);
-
-if (modssl_X509_INFO_load_file(ptemp, sk, fullname)) {
-ok = TRUE;
-}
-}
-
-apr_dir_close(dir);
-
-return ok;
-}
-
 /*  _
 **
 **  Custom (EC)DH parameter support
Index: modules/ssl/ssl_util_ssl.h
===
--- modules/ssl/ssl_util_ssl.h  (revision 1677159)
+++ modules/ssl/ssl_util_ssl.h  (working copy)
@@ -68,7 +68,6 @@ char   *modssl_X509_NAME_to_string(apr_pool_t
 BOOLmodssl_X509_getSAN(apr_pool_t *, X509 *, int, int, 
apr_array_header_t **);
 BOOLmodssl_X509_match_name(apr_pool_t *, X509 *, const char *, BOOL, 
server_rec *);
 BOOLmodssl_X509_INFO_load_file(apr_pool_t *, STACK_OF(X509_INFO) *, 
const char *);
-BOOLSSL_X509_INFO_load_path(apr_pool_t *, STACK_OF(X509_INFO) *, const 
char *);
 int SSL_CTX_use_certificate_chain(SSL_CTX *, char *, int, 
pem_password_cb *);
 char   *SSL_SESSION_id2sz(unsigned char *, int, char *, int);