Hi William, > Le 22 nov. 2019 à 17:34, William Lallemand <[email protected]> a écrit : > > 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. >
yep, introduce a ‘for' around a ‘continue’ to remplace sk_X509_NAME_find is not the best thing to do… i will fix this. > 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); > Is what openssl do when used upper layer API. Openssl doesn’t refcount xname or dup xname in push(). Without dup xname, ca_e->ca_list and ca_e->ca_store will share xnames: should not in openssl word. > Same issue with calloc there. > >> + ca_name->node.key = key; >> + ca_name->xname = xn; >> + eb64_insert(&ca_name_tree, &ca_name->node); > ok i will adress allocs. > > 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(). > Agree, this will be cleaner to integrate with existing code. The initial purpose of the patch is to greatly speedup start and greatly reduce memory footprint when ca-file is used in big configurations (can be unusable without). Keep the initial patch easily to make backport should help. What do you think? ++ Manu
