Errors loading certificates after tls-cert= changes by yadij · Pull Request #144 · squid-cache/squid

yadij · 2018-02-03T13:49:27Z

Fixes for the following log entries when a self-signed CA, or certificate restricted to signing-only is configured in squid.conf after commit 51e09c0:

Certificate is self-signed, will not be chained
FATAL: No valid signing certificate configured for HTTP_port 0.0.0.0:3128
ERROR: Ignoring http_port [::]:8082 due to TLS context initialization failure.
ERROR: Ignoring https_port [::]:8083 due to TLS context initialization failure.

... and one typo fix in debugs found while investigating.

badfiles · 2018-02-03T15:24:25Z

Configuration loads now (I tried my old cert+key pem file with tls-cert= and cert=), bumping works.
My cert was expired and self-signed, probably that was the reason it did not load before.
I quit using bump mode, I use splice for now. But for testing this patch I turned bumping back on.

michaeladm

It works!!! Thanks.
...for ssl bumping use openssl tool to generate certificate. If use certtool of gnutls package for generating certs - bumping not working with this certs.

rousskov

I am approving this PR in hope that it restores SslBump functionality, but my understanding of these changes is very superficial. This is not a proper review, but a problem mitigation in an emergency.

I also left a few polishing comments and questions inlined, but they may be affected by my lack of in-depth understanding of the buggy code.

rousskov · 2018-02-09T21:11:58Z

src/client_side.cc


    // Here squid should have all of the client hello message so the
-    // tlsAttemptHandshake() should return false;
+    // tlsAttemptHandshake() should return 0.


s/0/1/ because tlsAttemptHandshake() is now documented to return 1 on success (not 0).

rousskov · 2018-02-09T21:13:35Z

src/client_side.cc

    // This block exist only to force openSSL parse client hello and detect
    // ERR_SECURE_ACCEPT_FAIL error, which should be checked and splice if required.
-    if (!tlsAttemptHandshake(this, nullptr)) {
+    if (tlsAttemptHandshake(this, nullptr) < 0) {


According to the source code comment, squid should have all of the client hello message. Thus, zero (i.e., need more data) is not a good/expected tlsAttemptHandshake() outcome here. I do not know whether the comment or this condition is wrong, but the two do not match.

Indeed what I thought earlier. However, if that were true the changed API would have worked.

I hindsight this call / attempt is expected to result in either a WANT_READ or WANT_WRITE occurance (ie. wanting the server Hello data). Which is 0, not 1.

So the expected result is non-true / 0 / "fail" to continue with the FwdState call, versus -1 / error / reject to be tunneled. It should never be true / 1 / success here because the handshake is incomplete.

Your response does not match the mismatch problem I described. You are saying that the code works correctly. I have not said that the code does not work correctly.

You said the comment and code condition do no match. Due to what the code is doing the comment accurately describes the input state, why the call is being done and expected output. So they do match IMO - they are also the same as previously existing before PR 81 changes.

So what wrongness do you see in either the comment or the condition?

I have already answered your question in my original comment. This mismatch is not important enough for me to continue this discussion -- if you do not see the mismatch I have described, then it will stay in the code.

All I see in your original comment is a statement, then jumping to an incorrect conclusion.

Your second comment then rejects my reply on grounds that I don't believe you outright and provide a solution to the non-existent problem, which you wrongly concluded as existing. Instead I provide the logic by which your conclusion is proven false and the problem non-existent.

I believe this is important to settle ASAP because the behaviour here is key to auto-tunneling decisions in Squid. A flaw here allows clients to bypass all TLS security checking - or wrongly rejects TLS traffic. If there is something my logic missed out that your hidden logic used, I'd like to know.

If there is something my logic missed out that your hidden logic used, I'd like to know.

Given the relative unimportance of this particular change request, and the significant complexity of its environment, I am (still) not interested in continuing this conversation. I am also content with you thinking that you have proven me wrong. This change request should be considered addressed.

rousskov · 2018-02-09T21:28:57Z

src/security/ServerOptions.cc

+    }

-    } else if (!createStaticServerContext(port)) {
+    if (!certs.empty() && !createStaticServerContext(port)) {


Is it possible that generateHostCertificates is false and certs are empty? If it is possible, then please add a source code comment (or adjust the code) to clarify what happens in that case. If it is impossible, then please replace the !certs.empty() part of the condition with an assert.

It is possible when generate-host-certificates=off is configured and tls-cert= is omitted.

That is one of the correct situations where one of these errors is expected to be shown:
FATAL: No valid signing certificate configured for HTTP_port 0.0.0.0:3128
ERROR: Ignoring http_port [::]:8082 due to TLS context initialization failure.

(which one depends on other details)

If it is possible, then please add a source code comment (or adjust the code) to clarify what happens in that case. In other words, please adjust the code so that the reader does not need to ask the question I have asked.

rousskov · 2018-02-09T21:35:53Z

src/client_side.cc

    ConnStateData *conn = (ConnStateData *)data;

-    if (!tlsAttemptHandshake(conn, clientNegotiateSSL))
+    int ret = tlsAttemptHandshake(conn, clientNegotiateSSL);


Please make ret constant if possible.

This check is not needed when loading the initial cert portion of a PEM file as it will be performed later when loading the chain and was causing self-signed CA to be rejected incorrectly.

... if a cert= is provided. SSL-Bump still (for now) requires a static context as fallback when generate fails.

* Remove self-signed CA check This check is not needed when loading the initial cert portion of a PEM file as it will be performed later when loading the chain and was causing self-signed CA to be rejected incorrectly. * Fix a typo in debugs output * Always generate static context from tls-cert= parameter ... if a cert= is provided. SSL-Bump still (for now) requires a static context as fallback when generate fails. * Revert tlsAttemptHandshake to Squid_SSL_Accept API * Update const correctness * Document when initialization is skipped

yadij mentioned this pull request Feb 3, 2018

GnuTLS implementation for TLS listening ports and client connections #81

Merged

rousskov requested a review from chtsanti February 3, 2018 17:54

yadij force-pushed the v5-cert-errors branch from 988a464 to 04f3665 Compare February 6, 2018 15:57

michaeladm reviewed Feb 9, 2018

View reviewed changes

rousskov approved these changes Feb 9, 2018

View reviewed changes

yadij added 5 commits February 10, 2018 20:55

Remove self-signed CA check

a6b6643

This check is not needed when loading the initial cert portion of a PEM file as it will be performed later when loading the chain and was causing self-signed CA to be rejected incorrectly.

Fix a typo in debugs output

bf62dc6

Always generate static context from tls-cert= parameter

da1a7be

... if a cert= is provided. SSL-Bump still (for now) requires a static context as fallback when generate fails.

Revert tlsAttemptHandshake to Squid_SSL_Accept API

ced10f5

Update const correctness

2c642ad

yadij force-pushed the v5-cert-errors branch from 48f3ab8 to 2c642ad Compare February 10, 2018 07:55

Document when initialization is skipped

52e8812

yadij merged commit 1700fab into squid-cache:master Feb 12, 2018

yadij deleted the v5-cert-errors branch February 12, 2018 15:05

rousskov mentioned this pull request Feb 22, 2018

Add regression testing for https_port certificate loading #154

Open

Conversation

yadij commented Feb 3, 2018

Uh oh!

badfiles commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaeladm left a comment • 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 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!

yadij Feb 10, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadij Feb 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov Feb 10, 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

badfiles commented Feb 3, 2018 •

edited

Loading

michaeladm left a comment •

edited

Loading

yadij Feb 10, 2018 •

edited

Loading

yadij Feb 11, 2018 •

edited

Loading

rousskov Feb 10, 2018 •

edited

Loading