Preserve configured order of intermediate CA certificate chain

[rousskov]

https_port ... tls-cert=signing,itsIssuer,itsIssuerIssuer.pem

The order was reversed in commit cf48712, probably by accident. Wrong
order violates TLS protocol and breaks TLS clients that are incapable of
reordering received intermediate CAs. Squid deployments that use
wrong-order bundles (to compensate for this bug) should reorder their
bundles when deploying this fix (or wait for Squid to order certificates
correctly, regardless of the bundle order -- a work in progress).

This is a Measurement Factory project.

[rousskov]

PR description: ... or wait for Squid to order certificates correctly, regardless of the bundle order -- a work in progress

Those changes are nearly ready. I extracted this order fix and the #955 changes from a large/complex patch that improves certificate handling. That patch is finishing internal review/testing cycles. Factory will post the corresponding PR (or PRs) when these two surgical fixes are closed.

[yadij]

Veto. The relevant RFC 8446 text is "Each following certificate SHOULD directly certify the one immediately preceding it."

Note the words preceding it. That means the chain is required to have the following syntax:
cert -> issuer CA 1 -> issuer CA 2 -> (optional) root CA

The value stored in latestCert is verified by X509_check_issued() to be certified by the previous entry appended to chain (ie its tail). Which makes emplace_back the correct RFC compliant logic if we assume the input file is correct.

[rousskov]

Veto. The relevant RFC 8446 text is "Each following certificate SHOULD directly certify the one immediately preceding it."

Note the words preceding it. That means the chain is required to have the following syntax: cert -> issuer CA 1 -> issuer CA 2 -> (optional) root CA

Official code violates the above RFC requirement. Official code reverses the RFC-prescribed chain order to become: (optional) root CA -> issuer CA 2 -> issuer CA 1.

Here is a simplified (but equivalent) version of the official code and the proposed change, preserving unfortunate official variable names:

  ca = loadNextCertificateFromTheBundle();
  X509_check_issued(ca, latestCert); // i.e. ca signed/issued/certified latestCert
- chain.emplace_front(ca); // XXX: LIFO; Root (the last certificate to load) becomes the first chain entry
+ chain.emplace_back(ca); // OK: FIFO; Root is the last chain entry
  latestCert = ca;

The value stored in latestCert is verified by X509_check_issued() to be certified by the previous entry appended to chain (ie its tail).

I cannot find a way to interpret what you are saying so that it matches official code. There is an interpretation that matches PR code, but that does not explain your veto.

Here is what is actually going on in this very confusing code, in more precise/clear terms:

• Squid makes sure that the newly/just loaded-from-the-bundle certificate (ca) signed/issued/certified the previously loaded certificate (misnamed latestCert). This requires that the bundle uses the RFC-recommended on-the-wire certificate order. OK.
• Official Squid then makes ca the very first certificate in the chain. Bug! For example, the root CA certificate¹ is loaded last but becomes the very first certificate in the final chain. It should be the last!
• PR code then makes ca the very last certificate in the chain. OK.

Which makes emplace_back the correct RFC compliant logic if we assume the input file is correct.

Exactly! Why are you vetoing the PR that does what you describe as "correct RFC compliant logic"? Here is what the PR diff says, verbatim:

-    chain.emplace_front(latestCert);
+    chain.emplace_back(latestCert);

Footnotes

1. Assuming, for simplicity of the example sake, that the root CA certificate is present in the bundle and is chained by Squid. ↩

[yadij]

Sorry, misread the patch. :( cleared for merge now.