Re: [PATCH] MINOR: ssl: deduplicate ca-file
On Thu, Nov 28, 2019 at 11:13:18AM +0100, William Lallemand wrote: > On Wed, Nov 27, 2019 at 05:10:10PM +0100, Emmanuel Hocdet wrote: > > > > Patches update, should address William’s comments. > > > > Thanks, merged! > It seems to have break the build on centos 6, could you take a look at this ticket? https://github.com/haproxy/haproxy/issues/385 Thanks -- William Lallemand
Re: [PATCH] MINOR: ssl: deduplicate ca-file
On Wed, Nov 27, 2019 at 05:10:10PM +0100, Emmanuel Hocdet wrote: > > Patches update, should address William’s comments. > Thanks, merged! -- William Lallemand
Re: [PATCH] MINOR: ssl: deduplicate ca-file
Hi William,
> Le 22 nov. 2019 à 17:34, William Lallemand 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(_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(_tree, _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(_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(_name_tree, _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
Re: [PATCH] MINOR: ssl: deduplicate ca-file
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(_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(_tree, _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(_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(_name_tree, _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
Re: [PATCH] MINOR: ssl: deduplicate ca-file
Fix bad merge from my branch, > Le 22 nov. 2019 à 11:35, Emmanuel Hocdet a écrit : > > > Patches update with compat lib-ssl and crl-file. > Deduplicate Verify-stuff in memory will prevent file access when updating a > certificate with CLI. 0001-MINOR-ssl-deduplicate-ca-file.patch Description: Binary data 0002-MINOR-ssl-compute-ca-list-from-deduplicate-ca-file.patch Description: Binary data 0003-MINOR-ssl-deduplicate-crl-file.patch Description: Binary data
Re: [PATCH] MINOR: ssl: deduplicate ca-file
Hi, > Le 29 oct. 2019 à 07:59, Willy Tarreau a écrit : > > Please, let's revisit this after the release. The only people able to > have a look at this and to have an opinion on it are all busy finishing > this release. > Patches update with compat lib-ssl and crl-file. Deduplicate Verify-stuff in memory will prevent file access when updating a certificate with CLI. ++ Manu 0001-MINOR-ssl-deduplicate-ca-file.patch Description: Binary data 0002-MINOR-ssl-compute-ca-list-from-deduplicate-ca-file.patch Description: Binary data 0003-MINOR-ssl-deduplicate-crl-file.patch Description: Binary data
Re: [PATCH] MINOR: ssl: deduplicate ca-file
Hi Manu, On Fri, Oct 25, 2019 at 05:51:31PM +0200, Emmanuel Hocdet wrote: > Hi, > > add a second patch to address ca-list case. Please, let's revisit this after the release. The only people able to have a look at this and to have an opinion on it are all busy finishing this release. Thanks, Willy
Re: [PATCH] MINOR: ssl: deduplicate ca-file
Hi, add a second patch to address ca-list case. ++ Manu > Le 24 oct. 2019 à 12:14, Emmanuel Hocdet a écrit : > > Hi, > > Little patch with big win when ca-file is used in server line. > > ++ > Manu > > <0001-MINOR-ssl-deduplicate-ca-file.patch> > 0001-MINOR-ssl-deduplicate-ca-file.patch Description: Binary data 0002-MINOR-ssl-compute-ca-list-from-deduplicate-ca-file.patch Description: Binary data
[PATCH] MINOR: ssl: deduplicate ca-file
Hi, Little patch with big win when ca-file is used in server line. ++ Manu 0001-MINOR-ssl-deduplicate-ca-file.patch Description: Binary data
