On Tue, Jan 21, 2020 at 12:42:04PM +0100, Emmanuel Hocdet wrote:
> Hi,
>
> Patches updated, depend on "[PATCH] BUG/MINOR: ssl:
> ssl_sock_load_pem_into_ckch is not consistent"
>
> ++
> Manu
>
Hello,
It could be great to share more of the certificates in memory, but some points
are
confusing in my opinion:
> +issuer-path <dir>
> + Assigns a directory to load certificate chain for issuer completion. All
> + files must be in PEM format. For certificates loaded with "crt" or
> "crt-list",
> + if certificate chain is not included in PEM (also commonly known as
> intermediate
> + certificate), haproxy will complete chain if issuer match the first
> certificate
> + of the chain loaded with "issuer-path". "issuer-path" directive can be set
> + several times.
> +
That's not a good idea to be able to add a new path to the list each time this
keyword is found, this is not how the configuration works in the global
section, a new line with the keyword should remove the previous one.
> - PEM files into one (e.g. cat cert.pem key.pem > combined.pem). If your CA
> + PEM files into one (e.g. cat key.pem cert.pem > combined.pem). If your CA
I'm not sure to understand why this was changed, maybe by mistake? If not could
you elaborate in the commit message, or split this part in another commit if
it's not related to the feature.
> + /* Find Certificate Chain in global */
> + if (chain == NULL) {
> + AUTHORITY_KEYID *akid;
> + akid = X509_get_ext_d2i(cert, NID_authority_key_identifier,
> NULL, NULL);
> + if (akid) {
> + struct issuer_chain *issuer;
> + struct eb64_node *node;
> + u64 hk;
> + hk = XXH64(ASN1_STRING_get0_data(akid->keyid),
> ASN1_STRING_length(akid->keyid), 0);
> + for (node = eb64_lookup(&global_ssl.cert_issuer_tree,
> hk); node; node = eb64_next(node)) {
> + issuer = container_of(node, typeof(*issuer),
> node);
> + if
> (X509_check_issued(sk_X509_value(issuer->chain, 0), cert) == X509_V_OK) {
> + chain =
> X509_chain_up_ref(issuer->chain);
> + break;
> + }
> + }
> + AUTHORITY_KEYID_free(akid);
> + }
> + }
I'm not sure about this, what's bothering me is that the certificate is not
found using a filename, but only found by looking for the SKID. So it's kind of
magic because it's not named in the configuration. It can be bothering if you
don't want to complete the chain for some of the certificates, or if you have a
multi-tenant configuration.
Maybe we should use a keyword on the bind line to load it instead of this, like
`ssl crt foo.pem issuer bar.pem` and this keyword could be applied to a lot
of certificates by using the default-server keyword.
Also I want to clarify some confusion with the issuer / chain:
- the keyword "issuer" is used for the ocsp response, it's used as the issuer
of the ocsp response, but we could probably use it as a certificate to
complete the chain and split the PEM right?
- it looks like your code is only filling the chain with one certificate,
shouldn't it try to resolve the whole chain ?
> Subject: [PATCH 2/2] MINOR: ssl/cli: 'set ssl issuer' load an issuer
> certificate from the CLI
>
> $ echo -e "set ssl issuer <<\n$(cat chain.pem)\n" | \
> socat stdio /var/run/haproxy.stat
> Issuer loaded
>
> The operation update/load issuer certificate in memory. It's the CLI
> version from "issuer-path" directive. It's usefull with "set ssl cert"
> to update certificate without chain certificates. The chain certificates
> will be shared between certificates with same issuer.
I think there is a consistency problem there, because it does not recreate
the ckchs. What we are trying to do with the CLI, is to have the same effect as
a reload of the process. So it should be able to find which certificate is
using it, and update it.
Regards,
--
William Lallemand
