Hi William,
> Le 23 janv. 2020 à 16:20, William Lallemand <wlallem...@haproxy.com> a écrit :
>
> 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"
>>
>
> 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.
>
Okay, or refuse the second one.
To be clear, 'issuer-path' can be confused, i could be named
'issuers-chain-path’.
>> - 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.
>
Indeed, is not related. It’s more optimal when PEM is parsed.
Key is parsed first, Cert next after rewind the BIO at begining.
>> + /* 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.
>
It’s not really magic, it’s based on X509, how certificate are build, and
and how to check(find) an issuer for a certificate (via X509_check_issued with
openssl).
It’s what libssl do to verify certificat, find the appropriate root CA (loaded
via ca-file in
HAproxy).
For certificats generated via the same issuer, there are no reason to not
complete it
with the same chain. At worst, not used the functionality (include a chain who
match the issuer).
I have experienced this for years on our multi-tenant platform. Customers
upload
their certificates (the chain are ignored) and we accept it if we can verify it.
For that we need, in addition to "ca-certificates » pkg, all intermediate
certificates from
accepted CA (Comodo, LetsEncrypt, CAcert, Cloudflare, …).
When PEM file are generated for HAproxy, the chain of issuers is construct from
all this intermediate certificates. This guarantees to have a correct chain for
all certificates,
and chain can be updated automatically.
When intermediate certificates repository is updated (by humans) we can have
root
certificate (detected and ignored), or more than one issuer for a certificate
(sha1/sha256 ...):
the better (valid) issuer is chosen.
(1) For haproxy, I didn't want to do that initially, because its’ tricky and
too "magic ». Add such
feature without change the initial behaviour:
Have PEM with Key+Cert+Chain or have Key+Cert and complete the Chain (via the
start of
the chain - the issuer -).
> 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.
>
(default-server -> ssl-default-bind-options)
(‘issuer’ -> ‘issuers-chain’ to be less confused)
I don’t like that very much. We can have a lot of certificats with different
issuer, and only crt-list
with this issuers-chain param could do the job. It’s very unwieldy. On the
other hand it takes
away any discovery, It might be more in line with how HAproxy deal with ssl
files.
It could looks like ca-file, but ca-file is a PEM who can contient all CA-root.
A issuers-file could have all issuers in a raw, but we need to extract the good
chain
from that file (1). To update issuers-file payload could be heavy.
> 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?
>
ocsp_issuer is only one certificate, the issuer from the certificate. It can be
found
via chain->chain.
(in chain, issuer from the certificate should be the first one, the next
certificate
should be the issuer from the first issuer,…. if not the case ssl client can
be not
happy). Actually HAProxy don’t check this.
> - it looks like your code is only filling the chain with one certificate,
> shouldn't it try to resolve the whole chain ?
>
cf (1)
>> 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.
>
With this command alone, update issuer-chain requires updating certificate to
take effect. It not break consistency: reload will add issuers-chain and update
chain when certificate is initialised.
We could have an update command for all certificates to update the chain.
I suppose you target a named issuers-chain as you suggest to do such direct
update.
++
Manu
