On Fri, Jan 24, 2020 at 01:22:05PM +0100, Emmanuel Hocdet wrote: > > Hi William, > Hello Manu!
> > Le 23 janv. 2020 à 16:20, William Lallemand <[email protected]> a > > écrit : > > > > 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. That's really important to be able to overwrite the previous value, because the global section could be superseded. For example in the ALOHA appliances, there is a global.def file which is a configuration file with default values, and these values can be overwritten in the haproxy.cfg. > To be clear, 'issuer-path' can be confused, i could be named > 'issuers-chain-path’. > That might be a good idea. > >> - 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. > Oh, that's right. We can probably update the documentation by explaining why it's more efficient in this order. I might be wrong but I think the previous order was mandatory because of the way the certificates were loaded before, because it wasn't using the BIO, but the high level functions of openssl. > > 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). I understand the principle, but my fear is the fact that its a global context only referenced by IDs, and you don't specified which file are used in the configuration. > 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 -). > Ok I understand your usage better, it completely makes sense. So the provider will fill itself the directory with trusted files, this is not something which will be filled by the customer. > > 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. That's my concern, but with your explanations I understand that the usage is not really compatible with the way haproxy deals with ssl files. But we should handle it the same way as we plan to handle the certificate directories. > 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. > > > - 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. In fact I didn't see that there wasn't a filename argument on the command, and the 'set' word was kind of missleading because it usually means that you will create or replace something with a given name or position, not add something to an existing list. I'm not sure this is a good idea because you lose the knowledge of what was in which file. What we are trying to do with the certificates and the CLI, is to be able to do a 'reload' of the filesystem, but without reloading haproxy. You could imagine an haproxy helper (let's say `haproxyctl cert reload`) that will scan the changes on the filesystem and apply the changes without launching a new process, so it could be a file removed, updated or added. Little question, isn't there a case where you want to replace an issuer, with a certificate having the same SKID, for example if the issuer expired but not the end certificate? In this case you don't want to do a "set ssl cert". I'm seeing another issue, let's say that you have a certificate X, generated with the issuer Z. You put X in the configuration file, but you forgot to put Z in the issuer-path. So you use "set ssl issuer" to add it in memory. But at this point you don't need to update X in memory because it didn't change, so can't simply use the 'set ssl cert' command. And what about removing an issuer? In fact we should be able to do exactly the same operations than with the filesystem + reload with the same effects. > 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. We could have something called "commit ssl issuer" which will update every ckch. It will be a little bit complicated, because you need to know which certificate used what, so you need to link the ckch_store to the issuer, and update every ckch_store and the ckch_inst linked to it. It's an extra iteration more that what does 'commit ssl cert". But you also need to complete the chains of the certificates that were missing there issuer on startup. Regards, -- William Lallemand
