Hi Grant! On Tue, Jan 24, 2017 at 08:31:46PM +0000, Grant Zhang wrote: > This patch adds the global 'ssl-engine' keyword. First arg is an engine > identifier followed by a list of default_algorithms the engine will > operate. > > If the openssl version is too old, an error is reported when the option > is used.
Thanks. I'm having just a few comments but overall that looks good. > +ssl-engine <name> default_algorithms <comma-seperated list of algorithms> > + Sets the OpenSSL engine to <name>. List of valid values for <name> may be > + obtained using the command "openssl engine". This statement may be used > + multiple times, it will simply enable multiple crypto engines. Referencing > an > + unsupported engine will prevent haproxy from starting. Note that many > engines > + will lead to lower HTTPS performance than pure software with recent > + processors. The command default_algorithms sets the default algorithms an > + ENGINE will supply using the OPENSSL function ENGINE_set_default_string(). > + A value of "ALL" uses the engine for all cryptographic operations. A comma- > + seperated list of different algorithms may be specified, including: RSA, > DSA, > + DH, EC, RAND, CIPHERS, DIGESTS, PKEY, PKEY_CRYPTO, PKEY_ASN1. This is the > + same format that openssl configuration file uses: > + https://www.openssl.org/docs/man1.0.2/apps/config.html > + Do you think it would make sense to consider that if no list of algo is specified then it defaults to all ? I tend to think probably not from a technical perspective but maybe for some users it can make sense. Also this "default_algorithms" name is quite long (and I've seen many people who don't know how to write "algorithm"). Maybe we should call it "algo" like in the compression. Or maybe "enable" to mention that what follows is a list of the algorithms we want to enable on this engine ? Any other idea ? (BTW we avoid use of underscores in new keywords, they mix poorly with the dashes). > +static int ssl_init_engines(void) > +{ > + int err_code = 0; > + struct cond_wordlist *wl, *wlb; > + > + ENGINE_load_builtin_engines(); > + > + list_for_each_entry_safe(wl, wlb, &openssl_engines, list) { > + Alert("ssl-engine: %s, def_algorithm: %s\n", wl->s, (char > *)wl->cond); I think the alert above was only for debugging purposes during development ? > + err_code = ssl_init_single_engine(wl->s, wl->cond); > + if (err_code == ERR_ABORT) > + break; > + } > + return err_code; > +} > +#endif /* OPENSSL_NO_ENGINE */ > + > /* > * This function returns the number of seconds elapsed > * since the Epoch, 1970-01-01 00:00:00 +0000 (UTC) and the > @@ -6375,6 +6436,37 @@ static int ssl_parse_global_ca_crt_base(char **args, > int section_type, struct pr > return 0; > } > > +/* parse the "ssl-engine" keyword in global section. > + * Returns <0 on alert, >0 on warning, 0 on success. > + */ > +static int ssl_parse_global_ssl_engine(char **args, int section_type, struct > proxy *curpx, > + struct proxy *defpx, const char > *file, int line, > + char **err) > +{ > + struct cond_wordlist *wl; > + > + if (*(args[1]) == 0) { > + memprintf(err, "global statement '%s' expects a valid engine > name as an argument.", args[0]); > + return -1; > + } > + > + if (strncmp(args[2], "default_algorithms", 18) != 0) { Be careful on this one, it's rarely a good idea to use strncmp() to match a keyword because it will also match longer keywords and will silently errors in config files, until the moment we fix it and these configs break (eg: "default_algorithms" followed by an invisible non-breakable space). Please use strcmp() instead. > + memprintf(err, "global statement '%s' expects to have > default_algorithms keyword.", args[0]); > + return -1; > + } > + > + if (*(args[3]) == 0) { > + memprintf(err, "global statement '%s' expects algorithm names > as an argument.", args[0]); > + return -1; > + } > + > + wl = calloc(1, sizeof(*wl)); > + wl->cond = strdup(args[3]); > + wl->s = strdup(args[1]); This "cond" made me think quite a bit until I realized you simply reused a closely related structure. I understand the convenience, but it's confusing. What I suggest is that you create a new structure called "arg1_wordlist" which takes a "void *arg1" instead of "void *cond". This way later we may add other types. While you're at it, please rename this "s" field to "word" so that we know which one is the word we're supposed to be looking for. > @@ -7067,6 +7165,9 @@ static void __ssl_sock_init(void) > __attribute__((destructor)) > static void __ssl_sock_deinit(void) > { > +#ifndef OPENSSL_NO_ENGINE > + struct cond_wordlist *wl, *wlb; > +#endif > #if (defined SSL_CTRL_SET_TLSEXT_HOSTNAME && !defined > SSL_NO_GENERATE_CERTIFICATES) > lru64_destroy(ssl_ctx_lru_tree); > #endif > @@ -7093,6 +7194,15 @@ static void __ssl_sock_deinit(void) > } > #endif > > +#ifndef OPENSSL_NO_ENGINE > + /* free up engine list */ > + list_for_each_entry_safe(wl, wlb, &openssl_engines, list) { > + free(wl->cond); > + free(wl->s); > + LIST_DEL(&wl->list); > + free(wl); > + } > +#endif Above, given the low cost of the code and of this openssl_engines list head, I suggest that you simply remove the #ifndef all around. When there's no engine, it will simply try to iterate over an empty list but it will make the code more readable. ssl_sock.c is probably the file totalizing the highest number of #if so the more we can kill, the better. Thanks! Willy
