Hi Manu,
I have a few questions/remarks below:
> Subject: [PATCH 1/3] MINOR: ssl: deduplicate ca-file
> [...]
>
> +static int ssl_store_load_locations_file(X509_STORE **store_ptr, char *path)
> +{
> + struct ebmb_node *eb;
> + struct cafile_entry *ca_e;
> +
> + eb = ebst_lookup(&cafile_tree, path);
> + if (eb)
> + ca_e = ebmb_entry(eb, struct cafile_entry, node);
> + else {
> + X509_STORE *store = X509_STORE_new();
> + if (X509_STORE_load_locations(store, path, NULL)) {
> + int pathlen;
> + pathlen = strlen(path);
> + ca_e = calloc(1, sizeof(*ca_e) + pathlen + 1);
You probably want to check the return of calloc here.
> + memcpy(ca_e->path, path, pathlen + 1);
> + ca_e->ca_store = store;
> + ebst_insert(&cafile_tree, &ca_e->node);
> + } else {
> + X509_STORE_free(store);
> + return 0;
> + }
> + }
> + *store_ptr = ca_e->ca_store;
> + return 1;
> +}
> +
> Subject: [PATCH 2/3] MINOR: ssl: compute ca-list from deduplicate ca-file
> [...]
>
> +/* Duplicate ca_name is tracking with ebtree. It's simplify openssl
> compatibility */
> +static STACK_OF(X509_NAME)* ssl_load_client_ca_file(char *path)
> +{
> [...]
> +
> + skn = sk_X509_NAME_new_null();
> + /* take x509 from cafile_tree */
> + objs = X509_STORE_get0_objects(ca_e->ca_store);
> + for (i = 0; i < sk_X509_OBJECT_num(objs); i++) {
> + x = X509_OBJECT_get0_X509(sk_X509_OBJECT_value(objs,
> i));
> + if (!x)
> + continue;
> + xn = X509_get_subject_name(x);
> + if (!xn)
> + continue;
> + /* Check for duplicates. */
> + key = X509_NAME_hash(xn);
> + for (node = eb64_lookup(&ca_name_tree, key); node; node
> = eb64_next(node)) {
> + ca_name = container_of(node, typeof(*ca_name),
> node);
> + if (X509_NAME_cmp(xn, ca_name->xname) == 0)
> + continue;
> + }
I'm not sure this part is right. I think you wanted to skip pushing the object
if it was already in the tree, but instead you are doing a continue in the
inner loop.
Also I'm not sure why we have to deduplicate the entries? Isn't it the job of
openssl to do that?
> + xn = X509_NAME_dup(xn);
> + sk_X509_NAME_push(skn, xn);
> + ca_name = calloc(1, sizeof *ca_name);
Same issue with calloc there.
> + ca_name->node.key = key;
> + ca_name->xname = xn;
> + eb64_insert(&ca_name_tree, &ca_name->node);
After digging into this part of the code, I think it's safer to split the logic
in two parts, like what has been done for the ckch.
The access to the file system should be done in the configuration parsing, so
we don't access at all to the files in ssl_sock_prepare_ctx().
And we should only have a lookup in ssl_sock_prepare_ctx().
Thanks!
--
William Lallemand
