Bug 4831: filter chain certificates for validity when loading by yadij · Pull Request #187 · squid-cache/squid

yadij · 2018-04-15T08:47:31Z

51e09c0 adding
GnuTLS support required splitting the way
certificate chains were loaded. This resulted in the
leaf certificate being added twice at the prefix of a
chain in the serverHello.

It turns out that some recipients validate strictly that the
chain delivered by a serverHello does not contain extra
certificates and reject the handshake if they do.

This patch implements the XXX about filtering certificates
for chain sequence order and self-sign properties, added
in the initial PR. Resolving the bug 4831 regression and also
reporting failures at startup/reconfigure for admins.

Also, add debug display of certificate names for simpler
detection and administrative fix when loaded files fail
these tests.

rousskov

This PR uses DBG_IMPORTANT for things that represent boring "applying your configuration" facts. IMHO, those facts should be logged at level 2+.

I also have several questions/concerns about the validation logic, but it is possible that they are all bogus/invalid -- this is not my area of expertise, and I cannot quickly understand what exactly various certificates represent in relation to squid.conf. AFAICT, this PR does not return us to pre- #81 logic (e.g., it adds more tests that were not there before #81), which worries me quite a bit.

@chtsanti, please review this. Ignore my review comments though -- you know this code much better.

rousskov · 2018-04-30T15:51:19Z

src/security/KeyData.cc

-        debugs(83, 5, "Certificate is self-signed, will not be chained");
-    else {
+    if (X509_check_issued(cert.get(), cert.get()) == X509_V_OK) {
+        char *nameStr = X509_NAME_oneline(X509_get_subject_name(cert.get()), nullptr, 0);


It looks like we need an X509_NAME_oneline() wrapper (or equivalent) that returns SBuf: The related official code has memory leaks and wrong free functions, while the PR code is not exception safe. I do not insist on adding that wrapper in this PR.

There is code already abstracting this for the ACL lookups. Using that starts to get into dependency issues and porting more stuff to libsecurity. I put off all that to simplify this bug fix.

rousskov · 2018-04-30T15:56:50Z

src/security/KeyData.cc

-    else {
+    if (X509_check_issued(cert.get(), cert.get()) == X509_V_OK) {
+        char *nameStr = X509_NAME_oneline(X509_get_subject_name(cert.get()), nullptr, 0);
+        debugs(83, DBG_IMPORTANT, "Certificate is self-signed, will not be chained: " << nameStr);


Why is this DBG_IMPORTANT? If many chains are expected to have self-signed certificates, then I do not think we should inform the user about this boring fact, especially in rather cryptic and negative will not be chained terms.

We have had quite a few user questions and some confusion about why PEM files are not working the way they expected where it turned out to be misconfiguration with self-signed certs or incorrect chain ordering. These debugs are added (and at that level specifically) to make it clear when the problem is a misconfiguration and what certs Squid is actually loading.

We cannot successfully address admin confusion by polluting logs in benign cases. If Squid can detect something unusual and/or potentially dangerous, sure, let's complain at level 1! Otherwise, excessive logging will only hide important information. In this particular case, if, for example, 95+% of chains are going to contain a self-signed certificate (without any harm!), then we should not log about that boring fact at level 1.

very well. Dropped these into parse notes.

rousskov · 2018-04-30T15:57:57Z

src/security/KeyData.cc

+        debugs(83, DBG_IMPORTANT, "Certificate is self-signed, will not be chained: " << nameStr);
+        OPENSSL_free(nameStr);
+    } else {
+        debugs(83, DBG_IMPORTANT, "Using certificate chain in " << certFile);


If this is a boring "I am using what you have configured" fact, then it should not be logged at the DBG_IMPORTANT level.

same as above

Yes, the same principle applies here. In this particular case, there is absolutely nothing unusual or problematic going on whatsoever (AFAICT). If an admin wants to know what certificates are stored in their chain, they can either elevate Squid debugging level or use external X509 analysis tools like openssl x509.

I have long advocated for the development of guiding principles for critical and important debugging. While the decision is always made on a case-by-case basis, good general principles are often easy to apply. In complex cases, they make discussions a lot more productive. "Do not report application of seemingly valid configuration options" is a starting point for one of such principles.

rousskov · 2018-04-30T15:59:22Z

src/security/KeyData.cc

+    } else {
+        debugs(83, DBG_IMPORTANT, "Using certificate chain in " << certFile);
        // and add to the chain any other certificate exist in the file
+        X509 *lastCert = cert.get();


If possible, please use a smart pointer for the lastCert instead of creating a possibly dangling copy of a smart-pointer-controlled raw pointer.

done. This required rearranging the emplace and saving of ca later to avoid double-free of loaded CA certs.

rousskov · 2018-04-30T16:00:46Z

src/security/KeyData.cc

+            // get Issuer name of the cert for debug display.
+            char *nameStr = X509_NAME_oneline(X509_get_subject_name(ca), nullptr, 0);
+
+            // self-signed certificates are not valid in a sent chain.


Please s/chain./chain/.

It would be nice to cite the RFC or somesuch that prohibits sending root certificates.

FTR: this is based on OpenSSL test tool producing "validation error" if a self-signed certificate exists in the server provided chain.

rousskov · 2018-04-30T16:28:29Z

src/security/KeyData.cc

+            // checks that the chained certs are actually part of a chain for validating cert.
+            if (X509_check_issued(lastCert, ca) == X509_V_OK) {
+                debugs(83, DBG_IMPORTANT, "Adding issuer CA: " << nameStr);
+                chain.emplace_front(Security::CertPointer(ca));


If reversing the order of loaded certificates is intentional, then please add a comment explaining why the order should be reversed. Otherwise, add the newly loaded certificates to the end of the chain.

it is. comment added.

rousskov · 2018-04-30T16:31:36Z

src/security/KeyData.cc

+                chain.emplace_front(Security::CertPointer(ca));
+                lastCert = ca;
+            } else {
+                debugs(83, DBG_IMPORTANT, "Ignoring non-issuer CA: " << nameStr);


Please specify the file name where the ignored CA was found. If the initial certificate in the chain may come from a different file than the chained certificates, then please specify that certificate subject and/or file as well. Without those details, it may be very difficult for an admin to understand where the problem is when multiple certificates/chains are in use for multiple ports.

rousskov · 2018-04-30T16:34:00Z

src/security/KeyData.cc

+
+            // checks that the chained certs are actually part of a chain for validating cert.
+            if (X509_check_issued(lastCert, ca) == X509_V_OK) {
+                debugs(83, DBG_IMPORTANT, "Adding issuer CA: " << nameStr);


This boring "I am using what you have configured" fact should not be DBG_IMPORTANT IMHO.

switched to parse notes.

rousskov · 2018-04-30T16:34:38Z

src/security/KeyData.cc

+            if (X509_check_issued(ca, ca) == X509_V_OK) {
+                debugs(83, 5, "CA " << nameStr << " is self-signed, will not be chained: " << nameStr);
+                OPENSSL_free(nameStr);
+                continue;


It feels wrong that we set lastCert to cert but then may skip a few different self-signed certificates, meaning that our lastCert is not "last" anymore. Perhaps lastCert should be reset to the last self-signed certificate skipped or something like that?

It is the last cert currently in the chain - initially just the signing cert, then the CA for that and so on. Renamed to latestCert ie. latest added to the chain, to avoid positional implications.

rousskov · 2018-04-30T16:37:31Z

src/security/KeyData.cc

+            if (X509_check_issued(ca, ca) == X509_V_OK) {
+                debugs(83, 5, "CA " << nameStr << " is self-signed, will not be chained: " << nameStr);
+                OPENSSL_free(nameStr);
+                continue;


Is it possible that we skip or ignore all certificates (here and/or further below)? If yes, should we still check that the last skipped/ignored CA certificate has issued the signing certificate?

it is intentional to skip all self-signed CA, irrelevant CA, non-CA entries and chain that are misconfigured/misordered. Each entry loaded is only accepted if it is the next CA in the chain and lastCert/latestCert incremented to always point at the latest chain entry - the one for which the next loop iteration is now seeking a relevant intermediary CA.

rousskov · 2018-04-30T16:49:10Z

In PR descriptions/commit messages, please use commit IDs (i.e., SHAs) instead of (or, if you prefer, in addition to) PR IDs. Commit IDs are more permanent, independent from a 3rd party (GitHub), and easier to access when working with sources.

yadij · 2018-05-07T05:09:42Z

FYI: what I have been finding is the opposite to be true. A commit message for master saying hash XYZ is actually referring to v4.0 branch commit hash UVW, or some other commit hash in the authors local repo. Whereas the PR numbers are being encoded permanently in the commit titles by github.

rousskov · 2018-05-07T14:58:37Z

Alex: Commit IDs are more permanent, independent from a 3rd party (GitHub), and easier to access when working with sources.

Amos: what I have been finding is the opposite to be true

No, the opposite is not true. I am pretty sure the facts, as I stated them, are correct. What you describe does happen, but does not change the facts I have stated. More on that at the end of this comment.

Here are the rules of thumb for using a commit hash in the official repository (commit message or source code):

Should not refer to any commit outside of the official Squid repository. If such references are necessary, they must be augmented with the repository information.
If referring to a non-master commit in the official Squid repository, the reference should be augmented with the official branch name (for human convenience -- git itself does not care).
If referring to a master commit in the official Squid repository, the reference may be augmented with the "master" branch name. Omitting augmentation reduces noise, but adding it may be a good idea in some cases (e.g., where multiple commits from multiple branches are mentioned in the same context).

When working on feature branches, it is a good idea to augment any feature branch commit reference with "this branch" or a similar marker so that it is easier to spot and remove such references later. However, authors should keep in mind that feature branch references are volatile because rebasing and/or squashing invalidates them. (Official branches are never rebased or squashed, of course.)

What you describe in your comment are all violations of the first two rules. The second rule violations are relatively minor inconveniences, but both should be corrected by humans during PR review. Eventually, Anubis should warn humans about suspicious commit usage in proposed commit messages.

yadij · 2018-05-07T16:10:26Z

So tell me, working from commit 3868775 (in v4.0 branch) how would you identify what its master branch commit was? and was that change applied to any other official branches than v4.0 ?
Without using the PR number in the subject as a cross-reference point.

rousskov · 2018-05-07T17:25:09Z

working from commit 3868775 (in v4.0 branch) how would you identify what its master branch commit was?

If commit 3868775 is a port of master commit X, then commit 3868775 message should say that it is a port of X explicitly. AFAICT, it does not. The lack of that reference means that either commit 3868775 message is defective or there was no corresponding master commit at the time when 3868775 was committed to v4.0.

In case of a defective commit message, it is usually possible to find the matching master commit by looking for similar commit messages, of course, but that is just a workaround for a porting defect.

I believe your specific example deals with a defective commit message, but I tried to be more general in my answer.

and was that change applied to any other official branches than v4.0?

Similarly, Any ported change should reference the master commit it ports.

Needless to say, it is often possible to reference the master commit by referring to its GitHub PR, but, as I said earlier, using GitHub PR numbers is not the best primary referencing approach overall. PR numbers may be added to commit references, especially if that helps GitHub to cross-reference commits.

yadij · 2018-05-08T05:27:57Z

So you are requiring that I personally edit every code submission myself in order to backport. That will result in myself or squidadm being the only code committers. That commit was done with "git cherry-pick -ff ..." to ensure the authorship did not get altered. See examples in dd0be78 (aka 2d680fd) and 6375c3b (aka ac93e02 aka 51e09c0).

My point is that labeling to say it was a backport is not done by git, its just a pile of hashes. The cross-reference is done by github using the PR numbers which you can see in 6375c3b commit "(#81) (#152)" <- original master commit documented at PR #81 and a backport which made changes documented at PR #152.

rousskov · 2018-05-08T15:03:27Z

So you are requiring that I personally edit every code submission myself in order to backport.

I do not. There is no need for any manual edits if porting is already automated AFAICT.

That commit was done with "git cherry-pick -ff ..." to ensure the authorship did not get altered.

When committing the ported code (after or without git cherry-pick), the authorship can be preserved with --author. There is also --date for preserving the original author date. The script will need to run git commit explicitly instead of relying on -ff doing that implicitly, of course, but that is not a big deal (and has advantages).

The cross-reference is done by github using the PR numbers

Not exactly. GitHub blindly adds its own PR numbers to any PR-based commit. GitHub does not know about commit cross-referencing. The "cross-reference" effect you are talking about happens when the original commit has an original PR number. It is convenient in some cases, confusing in others, and results in excessively long commit titles in most. We should not be relying on GitHub PRs for cross-referencing, but we can certainly preserve original PR numbers for convenience of users browsing commits on the web. To address all the problems discussed so far, the ported commit description may look like this:

Original commit message title (without the trailing original GitHub PR number)

Original commit message body

Ported from master commit XXX (#original PR number).

GitHub or Anubis will add the PR number to the above title, of course.

Note that the above sketch works for both "fast-forwarded" and manually edited backports. If you want to distinguish the two cases (which may be a good idea!), then you can use "Copied from" and "Based on" phrases (or a similar).

Thank you for addressing my concerns.

rousskov · 2018-05-21T17:54:37Z

I am still worried that by introducing these new checks, we are improving interoperation with some clients while (silently) breaking some other "currently working" deployments, but I cannot substantiate that concern with any specific examples. It may be a good idea to preempt complains with documentation of what the affected squid.conf parameters should (not) contain exactly, but perhaps such documentation improvements are ouside this PR scope.

yadij · 2018-05-21T18:13:59Z

It was not "silent" until you had me push the warnings down into the quiet debug areas.

I think the documentation about these is relevant in the wiki pages mentioning chains and my release announcement text. So not in scope here.

rousskov · 2018-05-21T19:15:36Z

It was not "silent" until you had me push the warnings down into the quiet debug areas.

In hope to save time for more important issues, I am leaving this misinterpretation unchallenged.

yadij · 2018-05-28T09:06:49Z

I intend to retract the requested review in two days and let anubis take it from there.

@chtsanti if you just need more time to get around to this, please let me know ASAP.

chtsanti

I think we should allow CA root certificates in chain.

chtsanti · 2018-05-29T12:56:15Z

src/security/KeyData.cc

+
+            // self-signed certificates are not valid in a sent chain
+            if (X509_check_issued(ca, ca) == X509_V_OK) {
+                debugs(83, DBG_PARSE_NOTE(2), "CA " << nameStr << " is self-signed, will not be chained: " << nameStr);


The RFC 5246 says about certificate chain:

Each following certificate MUST directly certify the one preceding it. Because certificate validation requires that root keys be distributed independently, the self-signed certificate that specifies the root certificate authority MAY be omitted from the chain, under the assumption that the remote end must already possess it in order to validate it in any case.

My sense is that we should allow the CA root certificates. Just it should be the last one.

If there are no known use cases where a single root certificate in the right position breaks things, then I agree that we should allow it. Otherwise, I am not sure what the correct solution is.

I'm not sure what the exact tool output was earlier in my experimentation to find the actual bug. It seems to be working now for the tests I am doing regularly. So hiding the self-signed checks inside pre-processor blocks in hopes that we can remove them later instead of having anyone need to enable.

51e09c0 adding GnuTLS support required splitting the way certificate chains were loaded. This resulted in the leaf certificate being added twice at the prefix of a chain in the serverHello. It turns out that some recipients validate strictly that the chain delivered by a serverHello does not contain extra certificates and reject the handshake if they do. This patch implements the XXX about filtering certificates for chain sequence order and self-sign properties, added in the initial PR. Resolving the bug 4831 regression and also reporting failures at startup/reconfigure for admins. Also, add debug display of certificate names for simpler detection and administrative fix when loaded files fail these tests.

PR squid-cache#81 adding GnuTLS support required splitting the way certificate chains were loaded. This resulted in the leaf certificate being added twice at the prefix of a chain in te servreHello. It turns out that some recipients validate strictly that the chain delivered by a serverHello does not contain extra certificates and reject the handshake if they do. This patch implements the XXX about filtering certificates for chain sequence order and self-sign properties added in the initial PR. Resolving the bug 4831 regression and also reporting failures at startup/reconfigure for admins. Also, add debug display of certificate names for simpler detection and administratve fix when loaded files fail these tests.

Also remove XXX this bug fix is resolving and fix parameters on X509_check_issued()

Unless Squid is built with -DTLS_CHAIN_NO_SELFSIGNED=1. The expected behaviour is that chains will continue to reject chain certs following a self-signed certificate, but the root CA itself will be delivered to clients if configured.

51e09c0 adding GnuTLS support required splitting the way certificate chains were loaded. This resulted in the leaf certificate being added twice at the prefix of a chain in the serverHello. It turns out that some recipients validate strictly that the chain delivered by a serverHello does not contain extra certificates and reject the handshake if they do. This patch implements the XXX about filtering certificates for chain sequence order and self-sign properties, added in the initial PR. Resolving the bug 4831 regression and also reporting failures at startup/reconfigure for admins. Also, add debug display of certificate names for simpler detection and administrative fix when loaded files fail these tests.

…cache#187) 51e09c0 adding GnuTLS support required splitting the way certificate chains were loaded. This resulted in the leaf certificate being added twice at the prefix of a chain in the serverHello. It turns out that some recipients validate strictly that the chain delivered by a serverHello does not contain extra certificates and reject the handshake if they do. This patch implements the XXX about filtering certificates for chain sequence order and self-sign properties, added in the initial PR. Resolving the bug 4831 regression and also reporting failures at startup/reconfigure for admins. Also, add debug display of certificate names for simpler detection and administrative fix when loaded files fail these tests.

51e09c0 adding GnuTLS support required splitting the way certificate chains were loaded. This resulted in the leaf certificate being added twice at the prefix of a chain in the serverHello. It turns out that some recipients validate strictly that the chain delivered by a serverHello does not contain extra certificates and reject the handshake if they do. This patch implements the XXX about filtering certificates for chain sequence order and self-sign properties, added in the initial PR. Resolving the bug 4831 regression and also reporting failures at startup/reconfigure for admins. Also, add debug display of certificate names for simpler detection and administrative fix when loaded files fail these tests.

yadij requested a review from chtsanti April 15, 2018 08:47

yadij added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Apr 16, 2018

yadij force-pushed the bug4831 branch from fcd631a to 7f503e9 Compare April 17, 2018 14:22

rousskov previously requested changes Apr 30, 2018

View reviewed changes

rousskov added S-waiting-for-author author action is expected (and usually required) S-waiting-for-more-reviewers needs a reviewer and/or a second opinion and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Apr 30, 2018

yadij force-pushed the bug4831 branch from 7f503e9 to e694086 Compare May 8, 2018 16:05

yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels May 8, 2018

rousskov removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label May 21, 2018

yadij removed the S-waiting-for-more-reviewers needs a reviewer and/or a second opinion label May 25, 2018

chtsanti reviewed May 29, 2018

View reviewed changes

yadij added 3 commits June 5, 2018 18:11

Audit requested updates

4bbfd74

Also remove XXX this bug fix is resolving and fix parameters on X509_check_issued()

Allow self-signed certs to be chained

9b77c6b

Unless Squid is built with -DTLS_CHAIN_NO_SELFSIGNED=1. The expected behaviour is that chains will continue to reject chain certs following a self-signed certificate, but the root CA itself will be delivered to clients if configured.

yadij force-pushed the bug4831 branch from dee9c84 to 9b77c6b Compare June 5, 2018 06:11

squid-anubis removed the M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Jun 5, 2018

chtsanti approved these changes Jun 5, 2018

View reviewed changes

yadij added the M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels label Jun 5, 2018

squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Jun 5, 2018

yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Jun 5, 2018

squid-anubis closed this Jun 5, 2018

yadij deleted the bug4831 branch June 6, 2018 04:55

yadij mentioned this pull request Jun 4, 2022

Do not send more than one self-signed certificate #1058

Closed

Conversation

yadij commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadij May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadij May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov commented Apr 30, 2018

Uh oh!

yadij commented May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rousskov commented May 7, 2018

Uh oh!

yadij commented May 7, 2018

Uh oh!

rousskov commented May 7, 2018

Uh oh!

yadij commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rousskov commented May 8, 2018

Uh oh!

rousskov commented May 21, 2018

Uh oh!

yadij commented May 21, 2018

Uh oh!

rousskov commented May 21, 2018

Uh oh!

yadij commented May 28, 2018

Uh oh!

chtsanti left a comment

yadij commented Apr 15, 2018 •

edited

Loading

yadij May 8, 2018 •

edited

Loading

yadij May 8, 2018 •

edited

Loading

yadij commented May 7, 2018 •

edited

Loading

yadij commented May 8, 2018 •

edited

Loading