From 58103bc8e60a22d3d7dafade708115dfb7d8e135 Mon Sep 17 00:00:00 2001 From: William Lallemand Date: Thu, 2 May 2024 17:46:06 +0200 Subject: [PATCH] MINOR: ssl: ckch_conf_cmp() compare multiple ckch_conf structures The ckch_conf_cmp() function allow to compare multiple ckch_conf structures in order to check that multiple usage of the same crt in the configuration uses the same ckch_conf definition. A crt-list allows to use "crt-store" keywords that defines a ckch_store, that can lead to inconsistencies when a crt is called multiple time with different parameters. This function compare and dump a list of differences in the err variable to be output as error. The variant ckch_conf_cmp_empty() compares the ckch_conf structure to an empty one, which is useful for bind lines, that are not able to have crt-store keywords. These functions are used when a crt-store is already inialized and we need to verify if the parameters are compatible. ckch_conf_cmp() handles multiple cases: - When the previous ckch_conf was declared with CKCH_CONF_SET_EMPTY, we can't define any new keyword in the next initialisation - When the previous ckch_conf was declared with keywords in a crtlist (CKCH_CONF_SET_CRTLIST), the next initialisation must have the exact same keywords. - When the previous ckch_conf was declared in a "crt-store" (CKCH_CONF_SET_CRTSTORE), the next initialisaton could use no keyword at all or the exact same keywords. --- include/haproxy/ssl_ckch-t.h | 10 +++ include/haproxy/ssl_ckch.h | 2 + src/ssl_ckch.c | 119 +++++++++++++++++++++++++++++++++++ src/ssl_crtlist.c | 14 ++++- src/ssl_sock.c | 7 +++ 5 files changed, 149 insertions(+), 3 deletions(-) diff --git a/include/haproxy/ssl_ckch-t.h b/include/haproxy/ssl_ckch-t.h index 0f95c4857..0e501e556 100644 --- a/include/haproxy/ssl_ckch-t.h +++ b/include/haproxy/ssl_ckch-t.h @@ -161,6 +161,16 @@ enum { CERT_TYPE_MAX, }; +/* + * When crt-store options are set from a crt-list, the crt-store options must be explicit everywhere. + * When crt-store options are set from a crt-store, the crt-store options can be empty, or the exact same + */ +enum { + CKCH_CONF_SET_EMPTY = 0, /* config is empty */ + CKCH_CONF_SET_CRTLIST = 1, /* config is set from a crt-list */ + CKCH_CONF_SET_CRTSTORE = 2, /* config is defined in a crt-store */ +}; + struct cert_exts { const char *ext; int type; diff --git a/include/haproxy/ssl_ckch.h b/include/haproxy/ssl_ckch.h index 4ee3541cf..e6356637f 100644 --- a/include/haproxy/ssl_ckch.h +++ b/include/haproxy/ssl_ckch.h @@ -50,6 +50,8 @@ int ckch_store_load_files(struct ckch_conf *f, struct ckch_store *c, int cli, ch int ckch_conf_parse(char **args, int cur_arg, struct ckch_conf *f, int *found, const char *file, int linenum, char **err); void ckch_conf_clean(struct ckch_conf *conf); +int ckch_conf_cmp(struct ckch_conf *conf1, struct ckch_conf *conf2, char **err); +int ckch_conf_cmp_empty(struct ckch_conf *prev, char **err); /* ckch_inst functions */ void ckch_inst_free(struct ckch_inst *inst); diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c index a72da2415..a354b9e3a 100644 --- a/src/ssl_ckch.c +++ b/src/ssl_ckch.c @@ -1003,6 +1003,8 @@ struct ckch_store *ckch_store_new_load_files_path(char *path, char **err) if (ssl_sock_load_files_into_ckch(path, ckchs->data, err) == 1) goto end; + ckchs->conf.used = CKCH_CONF_SET_EMPTY; + /* insert into the ckchs tree */ memcpy(ckchs->path, path, strlen(path) + 1); ebst_insert(&ckchs_tree, &ckchs->node); @@ -4147,6 +4149,122 @@ out: return err_code; } +/* + * Check if ckch_conf and are compatible: + * + * new \ prev | EMPTY | CRTLIST | CRTSTORE + * ---------------------------------------- + * EMPTY | OK | X | OK + * ---------------------------------------- + * CRTLIST | X | CMP | CMP + * ---------------------------------------- + * + * Return: + * 1 when the 2 structures have different variables or are incompatible + * 0 when the 2 structures have equal variables or are compatibles + */ +int ckch_conf_cmp(struct ckch_conf *prev, struct ckch_conf *new, char **err) +{ + int ret = 0; + int i; + + /* compatibility check */ + + if (prev->used == CKCH_CONF_SET_EMPTY) { + if (new->used == CKCH_CONF_SET_CRTLIST) { + memprintf(err, "%sCan't use the certificate previously defined without any keyword with these keywords:\n", *err ? *err : ""); + ret = 1; + } + if (new->used == CKCH_CONF_SET_EMPTY) + return 0; + + } else if (prev->used == CKCH_CONF_SET_CRTLIST) { + if (new->used == CKCH_CONF_SET_EMPTY) { + memprintf(err, "%sCan't use the certificate previously defined with keywords without these keywords:\n", *err ? *err : ""); + ret = 1; + } + } else if (prev->used == CKCH_CONF_SET_CRTSTORE) { + if (new->used == CKCH_CONF_SET_EMPTY) + return 0; + } + + + for (i = 0; ckch_conf_kws[i].name != NULL; i++) { + + if (strcmp(ckch_conf_kws[i].name, "crt") == 0) + continue; + + switch (ckch_conf_kws[i].type) { + case PARSE_TYPE_STR: { + char *avail1, *avail2; + avail1 = prev ? *(char **)((intptr_t)prev + (ptrdiff_t)ckch_conf_kws[i].offset) : NULL; + avail2 = new ? *(char **)((intptr_t)new + (ptrdiff_t)ckch_conf_kws[i].offset) : NULL; + + /* must alert when strcmp is wrong, or when one of the field is NULL */ + if (((avail1 && avail2) && strcmp(avail1, avail2) != 0) || (!!avail1 ^ !!avail2)) { + memprintf(err, "%s- different parameter '%s' : previously '%s' vs '%s'\n", *err ? *err : "", ckch_conf_kws[i].name, avail1, avail2); + ret = 1; + } + } + break; + + default: + break; + } + /* special case for ocsp-update and default */ + if (strcmp(ckch_conf_kws[i].name, "ocsp-update") == 0) { + int o1, o2; /* ocsp-update from the configuration */ + int q1, q2; /* final ocsp-update value (from default) */ + + + o1 = prev ? *(int *)((intptr_t)prev + (ptrdiff_t)ckch_conf_kws[i].offset) : 0; + o2 = new ? *(int *)((intptr_t)new + (ptrdiff_t)ckch_conf_kws[i].offset) : 0; + + q1 = (o1 == SSL_SOCK_OCSP_UPDATE_DFLT) ? global_ssl.ocsp_update.mode : o1; + q2 = (o2 == SSL_SOCK_OCSP_UPDATE_DFLT) ? global_ssl.ocsp_update.mode : o2; + + if (q1 != q2) { + int j = 1; + int o = o1; + int q = q1; + memprintf(err, "%s- different parameter '%s' : previously ", *err ? *err : "", ckch_conf_kws[i].name); + + do { + switch (o) { + case SSL_SOCK_OCSP_UPDATE_DFLT: + memprintf(err, "%s'default' (ocsp-update.mode %s)", *err ? *err : "", (q > 0) ? "on" : "off"); + break; + case SSL_SOCK_OCSP_UPDATE_ON: + memprintf(err, "%s'%s'", *err ? *err : "", "on"); + break; + case SSL_SOCK_OCSP_UPDATE_OFF: + memprintf(err, "%s'%s'", *err ? *err : "", "off"); + break; + } + o = o2; + q = q2; + if (j) + memprintf(err, "%s vs ", *err ? *err : ""); + } while (j--); + memprintf(err, "%s\n", *err ? *err : ""); + ret = 1; + } + } + } + +out: + return ret; +} + +/* + * Compare a previously generated ckch_conf with an empty one, using ckch_conf_cmp(). + */ +int ckch_conf_cmp_empty(struct ckch_conf *prev, char **err) +{ + struct ckch_conf new = {}; + + return ckch_conf_cmp(prev, &new, err); +} /* parse ckch_conf keywords for crt-list */ int ckch_conf_parse(char **args, int cur_arg, struct ckch_conf *f, int *found, const char *file, int linenum, char **err) @@ -4315,6 +4433,7 @@ static int crtstore_parse_load(char **args, int section_type, struct proxy *curp goto out; c->conf = f; + c->conf.used = CKCH_CONF_SET_CRTSTORE; if (ebst_insert(&ckchs_tree, &c->node) != &c->node) { memprintf(err,"parsing [%s:%d] : '%s' in section 'crt-store': store '%s' was already defined.", diff --git a/src/ssl_crtlist.c b/src/ssl_crtlist.c index a0dfbcf66..d8e100318 100644 --- a/src/ssl_crtlist.c +++ b/src/ssl_crtlist.c @@ -472,9 +472,11 @@ int crtlist_parse_line(char *line, char **crt_path, struct crtlist_entry *entry, if (cfgerr & ERR_FATAL) goto error; - cc->used = 1; - if (newarg) /* skip 2 words if the keyword was found */ - cur_arg += 2; + if (newarg) { + cur_arg += 2; /* skip 2 words if the keyword was found */ + cc->used = CKCH_CONF_SET_CRTLIST; /* if they are options they must be used everywhere */ + } + } out: if (!cfgerr && !newarg) { @@ -697,6 +699,12 @@ int crtlist_parse_file(char *file, struct bind_conf *bind_conf, struct proxy *cu } } else { + if (ckch_conf_cmp(&ckchs->conf, &cc, err) != 0) { + memprintf(err, "'%s' in crt-list '%s' line %d, is already defined with incompatible parameters:\n %s", crt_path, file, linenum, err ? *err : ""); + cfgerr |= ERR_ALERT | ERR_FATAL; + goto error; + } + entry->node.key = ckchs; entry->crtlist = newlist; diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 6a94e8b27..b2d643c51 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -3843,6 +3843,13 @@ int ssl_sock_load_cert(char *path, struct bind_conf *bind_conf, int is_default, } if ((ckchs = ckchs_lookup(path))) { + + cfgerr |= ckch_conf_cmp_empty(&ckchs->conf, err); + if (cfgerr & ERR_CODE) { + memprintf(err, "Can't load '%s', is already defined with incompatible parameters:\n %s", path, err ? *err : ""); + return cfgerr; + } + /* we found the ckchs in the tree, we can use it directly */ cfgerr |= ssl_sock_load_ckchs(path, ckchs, bind_conf, NULL, NULL, 0, is_default, &ckch_inst, err);