GnuTLS implementation for TLS listening ports and client connections by yadij · Pull Request #81 · squid-cache/squid

yadij · 2017-11-18T14:58:02Z

Move the http_port cert= and key= options logic to libsecurity and add GnuTLS implementation for PEM file loading. Also adds some extra debugging to clarify listening port initialization problems with the PEM files.

Enable most of the http(s)_port listening socket logic to always build except where OpenSSL-specific dependency still exists. It may seem reasonable to leave it optionally excluded for minimal builds, however a minimal proxy that does not support HTTPS in any way is increasingly useless in the modern web so preference is given to building the generic TLS related code. This also simplifies the required testing to detect code portability issues.

GnuTLS implementation is added for https_port configured with static cert=/key= parameters and the resulting TLS handshake behaviour. Squid built with GnuTLS can now act as useful parent proxies behind a SSL-Bump'ing frontend or for other clients which require a TLS explicit proxy.

Also fixes the definitions for the CertPointer and PrivateKeyPointer.

rousskov

I agree that it would be good for @chtsanti to review this, but I know he has a lot on his plate right now. I am submitting this review in hope to help you make progress with this PR while you are waiting for Christos.

Also fixes the definitions for the CertPointer and PrivateKeyPointer.

Thank you! I hope this fix works well with all important compilers.

Enable most of the http(s)_port listening socket logic to always build except where OpenSSL-specific dependency still exists.

"Always build" sounds like a good thing, but after reading the proposed code, I fear that a more accurate description would be closer to "make more non-OpenSSL TLS code compile even though it does not really work yet". I did my best to give specific/constructive code comments, but I fear a deeper problem/conflict here.

IMO, added code must work (correctly) when reached. It must not ignore or hide errors. It should do something useful (other than producing errors). It should be reachable. Naturally, there are exceptions to this rule of thumb, but those exceptions should be backed by some very convincing arguments. This PR appears to add code that compiles but, for every HTTPS connections produces the following runtime ERRORs:

"HTTPS not supported by this Squid" (when built without OpenSSL and without GnuTLS support) or
"clientNegotiateSSL session logics not implemented" (when built with GnuTLS support)

If the above is happening in PR code, then I have to question the value of supporting such totally broken/unusable configurations (at the expense of making the code a lot more complex). If the above is not happening because the corresponding tryTlsHandshake() (or its caller) code is never reached in those two cases, then I have to question the value of adding so much unreachable (and complex!) code.

GnuTLS implementation is added for the basic https_port logic pathway and TLS handshake.

Does this PR add working, production-ready support for any Squid features when Squid is built with GnuTLS? If yes, what are they? I cannot tell whether the current "basic https_port logic pathway" and "TLS handshake" descriptions mean something like "some parts of some TLS-related features are working with GnuTLS now (but nothing new fully works)" or "Squid features X and Y can now be used in production with GnuTLS". IMO, the difference between the two cases is critical for admins and reviewers alike, but the PR description does not have enough information for me to understand support for which Squid features (if any) was actually added to GnuTLS builds.

rousskov · 2017-11-25T19:51:16Z

src/cf.data.pre

 	TLS / SSL Options:

-	   cert=	Path to SSL certificate (PEM format).
+	   tls-cert=	Path to TLS server certificate (PEM format).


AFAICT, technically, there is no such thing as a "server certificate" in this context. A server may use a certificate, of course, but that use does not change the certificate itself. There is also a TLS Server Certificate message (containing X509 certificates), but this option is not about that message, of course.

Similarly, there is no "TLS certificate" or "SSL certificate". Most certificates we deal with are X509 certificates that have nothing to do with SSL or TLS. SSL and TLS use X509 format of public key certificates, of course, but that does not make the certificate they use a "TLS certificate".

I suggest leaving this option as is, but if you have to change it, describe it as "Public key certificate in PEM format. Used to ..." or something like that.

Essentially the same can be said about tls-key.

There have been a few issues in squid-users already from people working with that idea that all certs are equivalent and encountering "invalid certificate" errors as a result.

Certificates containing TLS extension fields cannot always be used by SSL protocol. Likewise minimal certificates for SSL lack fields TLS is increasingly making mandatory at the higher protocol level. That small but critical detail is making the idea that there is no distinction between the protocols and server vs client certs be false.

Certificates loaded here are increasingly generated for TLS use and contain the KeyUsage extension field. When that is used only certificates marked as being for servers can be added to a server context. Admin configuring a cert= with a X509 cert marked for client-only usage will get errors from the library on each client connection attempt. It needs to be not marked at all ("SSL certificate") or marked with TLS extensions for server usage ("TLS server certificate").

Yes, many certificates are unusable in a given context. IMHO, that fact does not justify using misleading/inaccurate (for this context) terminology such as "server certificate", "TLS certificate", or "SSL certificate".

If you think that cert= documentation should describe required (or even desirable) certificate properties, I do not think anybody would object to such a change. My complaint was not about adding such descriptions.

Fair enough. I've applied a change to call the server port certificates just 'X.509 certificate' and describe the requirements in separate paragraphs.

rousskov · 2017-11-25T19:52:30Z

src/cf.data.pre

-			if not specified, the certificate file is
-			assumed to be a combined certificate and
-			key file.
+			When OpenSSL or LibreSSL is used this file may also


I do not think we should mention LibreSSL in squid.conf because I do not think we should imply any support for that library (until we have the resources to properly support it).

This is one of the few things that have been tested with LibreSSL. Removed anyway.

Thank you for addressing this request.

The admin does not know what has been tested, and the boundaries of the tested "it" are always vague. Once they see "LibreSSL" mentioned like this, many admins naturally expect LibreSSL to be supported and work in general.

rousskov · 2017-11-25T20:04:31Z

src/client_side.cc

-static int
-Squid_SSL_accept(ConnStateData *conn, PF *callback)
+static bool
+tryTlsHandshake(ConnStateData *conn, PF *callback)


Please remove "try" from the function name. We sometimes use the "try" prefix for throwing routines that have a try/catch wrapper around them, but the use case is (currently) different here (and even the original try/catch use case is rather questionable as far as naming is concerned -- fooOrThrow() is often a better name in those cases).

Please use "accept" instead of "handshake" in the function name. There are several kinds of handshakes in this context. Saying "accept" here clarifies things a lot!

Overall, it seems like the proposed monolith tryTlsHandshake() function should be replaced with:

one TlsAcceptOrThrow() function per OpenSSL/GnuTLS/none library plus

a generic try/catch TlsAccept() wrapper that closes the connection on failures.

This function is where the "TLS connection" concept using handshakes as a pseudo-accept() starts to fall down. It is the callback run after TCP accept(). It attempts to perform a TLS handshake, but may result in 0-N handshakes happening - in that 0 case there is "anonymous TLS", aka TLS with no session / "connection".

It just happens that OpenSSL API is not capable of the latter TLS/1.2+ feature so it uses the badly named function "SSL_accept()" and dies if no handshake occurs.

I'm using 'attempt' instead of 'try', but not adding 'accept' into the naming.

This function is where the "TLS connection" concept using handshakes as a pseudo-accept() starts to fall down

I see no evidence of that. Yes, accepts may happen at different layers (e.g., TCP, TLS, and application). There is nothing wrong or even confusing about that IMO.

It attempts to perform a TLS handshake, but may result in 0-N handshakes happening

Counting the number of handshakes sounds like splitting hairs to me. I doubt the concept of a handshake (if it is formally defined) allows for such counting and even if it is, there seems to be no point in counting individual "shakes" when we are talking about naming a function that does them.

that 0 case there is "anonymous TLS", aka TLS with no session / "connection".

My quick search for these terms failed to find any relevant matches. If you are sure this is something we should be discussing, please provide pointers defining the relevant "anonymous TLS" concept.

I'm using 'attempt' instead of 'try', but not adding 'accept' into the naming.

I am considering this problem as unresolved. I am not married to the word "accept" (even though the term fits the bill very well), but we should (continue to) distinguish client-Squid handshakes from Squid-server handshakes.

rousskov · 2017-11-25T20:10:44Z

src/client_side.cc

- * \retval -1 on error
+ * \retval true on success
+ * \retval false when needs more data
+ * \retval false when an error occured and closes the TCP connection


To fix spelling and improve grammar, please s/occured and closes the TCP connection/occurred (also closes the TCP connection)/

AFAICT, the correct way to fix this API is throw an exception on errors instead of overloading a false result with two very different outcomes.

In this case the caller was closing the TCP connection based on numeric result, so in order not to change OpenSSL build behaviour I just moved that particular action into the function. The callers only needs to handle the boolean result with no new side effects or code being skipped by a throw.

Adding a TODO note about this future fix though. The behaviour change should be done in dedicated fix without other code changes adding possible confusion.

The end result of your changes is an introduction of bad API. It does not really matter whether the changes were well-intentioned, small, moving-only, etc.

The original API was not ideal, but it was acceptable. The new API is not. I see two reasonable choices:

Keep the old API: You can still close the connection inside the function, before returning -1 of course. I am not asking you to undo that "just moved that particular action" change.

Fix the API.

I agree that fixing the API by throwing on errors is not a trivial change, and I do not insist on that change to be done inside this PR. However, the first alternative does not seem to complicate this PR at all. If that is true, please undo the API change (i.e., follow the first bullet) or implement any better option that I have missed instead.

rousskov · 2017-11-25T20:14:49Z

src/client_side.cc

    int fd = conn->clientConnection->fd;
-    auto ssl = fd_table[fd].ssl.get();
-    int ret;
+    auto session = fd_table[fd].ssl.get();


I do not see a reason to rename this variable in this PR, but if you do, please s/session/tls/ to avoid introducing a TLS session concept where none exists (yet).

This is not "a TLS". It is a pointer used to access session data which is being initialized by the handshake - if it succeeds. We cannot help that OpenSSL uses "SSL" to name its API object.

You seem to reopen the TLS connection-vs-session discussions that, I thought, were already resolved on squid-dev. The object in question is not a session. It is a connection. I do not insist on calling it "ssl" or "tls". I insist on not calling it "session". If you prefer "tlsConnection" or "tlsConn", I am fine with that.

rousskov · 2017-11-26T02:10:19Z

src/security/KeyData.cc

+        }
+    }
+    gnutls_free(data.data);
+#endif


Seems like we are missing an #else case here.

Aye, added extra debugging and all ifdef in this file now have an else. Even if only to state what is not implemented or unreachable.

I am worried about sweeping the "nobody cares about #else" problem under the rug. If we do not think this through now, we may significantly increase the cost of supporting TLS-free builds, not to mention adding support for another TLS library. Just adding debugs() lines now will hide any non-TLS and 3rd-library support problems from users and developers when Squid is built without TLS support or we start adding 3rd library support.

Is it possible to raise the bar by making Security code fail to compile if it is used when it should not be? We did a similar thing with several Security pointer types already -- they cannot be dereferences without a compilation problem. Can we do the same with KeyData and such? I think it should be possible without significant changes. For example, how about using the following sketch in KeyData.h?

#if HAVE_TLS_SUPPORT class KeyData ... #else class KeyData {}; #endif

The HAVE_TLS_SUPPORT (or a similar macro) will be auto-computed based on USE_OPENSSL and USE_GNUTLS macros, of course. When adding support for a 3rd TLS library API, the developer will only need to adjust that (trivial) macro computation code to trigger all the compiler errors they must fix.

The whole KeyData.cc body can be surrounded by the HAVE_TLS_SUPPORT guard while all #if switches with look something like this:

#if USE_OPENSSL ... #elsif USE_GNUTLS ... #else #error missing code for your current TLS library #endif

rousskov · 2017-11-26T02:13:09Z

src/security/KeyData.cc

+#if USE_OPENSSL
+    return X509_check_private_key(cert.get(), pkey.get());
+#elif USE_GNUTLS
+    return true; // TODO find GnuTLS equivalent check


Feels like an XXX to me, not a TODO.

I thought so too until I read what that OpenSSL function actually does - which is purely to check that parameters like KeyUsage if any on the key are also on the cert .... there can apparently be additional ones on the cert not in the key, and the two cert/key items can be completely unrelated while still passing this check.

The GnuTLS API does most of this kind of check automatically when you add that cert+key pair to the context instead of being explicitly done by us. So I think we are safe enough leaving it to do that for now.

The check this call seemed to be doing (verify the cert and key were actually a matching pair) is something we probably should implement for nicer squid.conf error messages. But that is also missing for OpenSSL so out of scope in this PR. I have an XXX in the caller about doing something better than this function can provide right now to verify the config details.

that OpenSSL function [...] is purely to check that parameters like KeyUsage if any on the key are also on the cert

AFAICT, comparing parameters is not the only check that X509_check_private_key() performs in OpenSSL v1.1.0f. The function also checks that the public key of the given certificate matches the given private key (via an EVP_PKEY_cmp() call). For example, for RSA keys, EVP_PKEY_cmp() uses rsa_pub_cmp() to compare the two "raw" key values:

static int rsa_pub_cmp(const EVP_PKEY *a, const EVP_PKEY *b) { if (BN_cmp(b->pkey.rsa->n, a->pkey.rsa->n) != 0 || BN_cmp(b->pkey.rsa->e, a->pkey.rsa->e) != 0) return 0; return 1; }

Why is OpenSSL comparing public and private keys? According to EVP_PKEY_cmp() manual page, "Since OpenSSL private keys contain public key components too the function
EVP_PKEY_cmp() can also be used to determine if a private key matches
a public key".

My code interpretation seems to match X509_check_private_key() documentation which says that the function returns success "if the keys match each other". There is a separate function for comparing just the parameters.

Did I misinterpret what X509_check_private_key() does?

The section "BUGS" in that documentation you referenced explicitly mentions that passing it two public keys (x and k) generated with the same parameters will return success.

With the new structure of KeyData.cc members I have inlined this check to the applicable USE_OPENSSL code block.

rousskov · 2017-11-26T02:18:40Z

src/security/KeyData.cc

+
+/// verify that a private key and cert match
+static bool
+checkPrivateKey(Security::CertPointer &cert, Security::PrivateKeyPointer &pkey)


If possible, please make this into a private constant KeyData method and avoid passing these parameters.

rousskov · 2017-11-26T02:22:47Z

src/security/KeyData.cc

+    gnutls_x509_crt_t certificate;
+    x = gnutls_pcert_export_x509(&pcrt, &certificate);
+    if (x != GNUTLS_E_SUCCESS) {
+        certificate = nullptr; // paranoid: just in case the *_t ptr is undefined after deinit.


Missing error reporting? Why is this error handling should be any different from all the other errors before it?

AFAIK this function call is just a data copy from one structure to another. So it fails only if the input was loaded with non-X509 types (ie. incompatible with X.509). We are loading the data immediately prior with explicit GNUTLS_X509_FMT_PEM flag to ensure that only X.509 succeeds in the earlier call.

The remaining edge case is a non-existing certificate. The error for that is in the else clause of the next if-statement block shared by both GnuTLS and OpenSSL, all we need do to trigger that is set the pointer to nil (as done for the paranoid case)

I am surprised that you are trying to justify the lack of error reporting by speculating that the function usually does not fail. Looking at GnuTLS sources, I quickly lost count of the different error conditions gnutls_pcert_export_x509() and its callees detect! The function is a lot more complex than a simple copy and even if it were that simple, the simplicity is not a valid excuse to avoid error reporting, especially in code that already detects the error condition and that already reports other similar errors.

fine then, added.

rousskov · 2017-11-26T02:27:00Z

src/security/KeyData.cc

+        certificate = nullptr; // paranoid: just in case the *_t ptr is undefined after deinit.
+    }
+#else
+    // to simplify and prevent 'undefined variable errors' in the next code block


IMO, this method's layout makes things mode complex/confusing than they actually are. Please give each of the three library cases (OpenSSL/GnutTLS/other) its own method implementation. They share nothing worth preserving AFAICT!

The design for libsecurity code is to have each behavioural action be presented by a single generic function/method with internal ifdef split for how that behaviour is implemented by the supported libraries. Using library-specific methods is what we are trying to move away from.

I would like to move the load-key and load-chain into separate functions. But right now the BIO object used by OpenSSL implementation currently gets in the way of splitting off the chain load as it would have to be passed around between the cert and chain methods when no other library API does so.

If you like I can split off the key loading into a generic method whose OpenSSL implementation is just a call to ReadPrivateKeyFromFile(). I was thinking of leaving that until the ReadPrivateKeyFromFile() code could be cleanly moved into KeyData as the OpenSSL implementation of that method.

The design for libsecurity code is to have each behavioural action be presented by a single generic function/method with internal ifdef split for how that behaviour is implemented by the supported libraries. Using library-specific methods is what we are trying to move away from.

We should not mechanically apply Security API principles to that API implementation. Yes, there should be one Security::foo() API for all. However, how that Security::foo() is implemented is a different story. If it makes sense to use library-specific implementations for some good reason, then we should use library-specific implementations.

When there is virtually no shared code or overlap between library-specific code implementing Security::foo(), it is better to #ifdef whole method bodies rather than force readers to navigate a complicated maze of #ifdef code blocks that depend on each other.

If you like I can split off the key loading into a generic method whose OpenSSL implementation is just a call to ReadPrivateKeyFromFile().

To address this particular change request, please refactor loadX509CertFromFile() to follow this pattern:

Security::KeyData::loadX509CertFromFile() { ... optional shared code ... #if USE_OPENSSL ... OpenSSL-specific code ... #elif USE_GNUTLS ... GnuTLS-specific code ... #else ... code specific to disabled SSL support ... #endif ... optional shared code ... }

The insides of each of those #ifdef sections is outside this change request scope.

Refactored.

Notice that layout currently results in public certs being sent twice in TLS handshakes. Because of the required multiple BIO objects. I have left a few XXX in the OpenSSL code for that and a few other checks that seem to be worthwhile, but out of scope for this PR.

yadij · 2017-11-30T14:15:23Z

In reply to your overview questions:

Does this PR add working, production-ready support for any Squid features when Squid is built with GnuTLS? If yes, what are they?

While location and symbol naming has changed the code is intended to be the same as the previously existing OpenSSL-only logic which has been in production use. Each piece of GnuTLS code has been tested as equivalent to the matching OpenSSL code to the best of my abilities.

I cannot tell whether the current "basic https_port logic pathway" and "TLS handshake" descriptions mean something like "some parts of some TLS-related features are working with GnuTLS now (but nothing new fully works)" or "Squid features X and Y can now be used in production with GnuTLS". IMO, the difference between the two cases is critical for admins and reviewers alike, but the PR description does not have enough information for me to understand support for which Squid features (if any) was actually added to GnuTLS builds.

I mean the code which performs the below (and no more) is made to be working for GnuTLS builds by this PR:

load squid.conf settings
generate a static TLS server context based on those settings
open a listening port
accept TCP client connections on that port
receiveTLS ClientHello data
send TLS ServerHello
perform I/O over resulting TLS encrypted TCP connection

All options documented with tls-* prefix work with either OpenSSL or GnuTLS as of the commit where they were renamed, and have the documented behaviour. Most of which is unchanged from OpenSSL-only behaviour. Any TLS/SSL related feature involving options not yet documented in such manner should be considered "not yet implemented" and should produce an error if used in GnuTLS builds.

There is one brand new behaviour added in this PR because the GnuTLS API makes it trivial to implement. That is configuring multiple cert=/key= parameters for the https_port. When configured in this manner Squid should be able to present ServerHello for any of those cert's domains based on the client request.

chtsanti

Looks that the code is equivalent for openSSL, so I am not expecting any problem.

My I ask, from the code I am understanding that the gnutls supports multiple certificates per port tls context. What does it mean? Does the gnutls selects the correct certificate for the SNI name? Or they are just chained when sent to the client?

Move the logic for loading cert/key from PEM file from libsslsquid to libsecurity. Also, fix CertPointer and PrivateKeyPointer definitions for non-OpenSSL builds.

…extCertChain

…::ServerOptions

rousskov · 2018-02-01T17:37:09Z

yadij dismissed rousskov’s stale review 15 hours ago

For the record, this PR was committed without addressing my concerns.

@yadij, please do not dismiss others' reviews without even discussing your intent to do so and allowing the reviewer to update their review.

badfiles · 2018-02-01T19:48:34Z

This commit breaks running with existing certs. My cert is not accepted w/o explaining why.

rousskov · 2018-02-01T20:32:34Z

@badfiles, are you sure it is this commit and this commit alone? In other words, does reversing this commit and only this commit resolve the problem in your environment?

Did you adjust your configuration in any way?

My cert is not accepted

Please detail whether it is Squid or the client/browser that does not "accept" your certificate. Providing error details and/or posting an ALL,9 log while reproducing the problem (with no other traffic) may also help.

badfiles · 2018-02-01T20:51:24Z

This commit only. I changed cert= to tls-cert= in config.
Squid does not start with the cert that properly worked before this commit, so I am not sure about browser acceptance.
It says FATAL: No valid signing certificate configured for HTTP_port 0.0.0.0:3128
config strings

http_port  3128 ssl-bump generate-host-certificates=on dynamic_cert_mem_cache_size=4MB tls-cert=/usr/etc/squid.pem
https_port 9443 intercept ssl-bump generate-host-certificates=on dynamic_cert_mem_cache_size=4MB tls-cert=/usr/etc/squid.pem

Rolling back po previously built version (before this commit)) solves the issue (with cert=).

yadij · 2018-02-02T01:05:30Z

hmm, what OS are you using?

yadij · 2018-02-02T03:41:45Z

Also, you should not have had to change it to tls-cert= to load the config. If you have built with the OpenSSL support needed to do cert generation the old config settings for OpenSSL-only builds should still work just fine.

badfiles · 2018-02-02T07:26:37Z

w/o changing config same error
FATAL: No valid signing certificate configured for HTTP_port 0.0.0.0:3128
OS linux ubuntu 16.04

ldd squid
	linux-vdso.so.1 =>  (0x00007ffc173ce000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f518c4d2000)
	libnettle.so.6 => /usr/lib/x86_64-linux-gnu/libnettle.so.6 (0x00007f518c29c000)
	libxml2.so.2 => /usr/lib/x86_64-linux-gnu/libxml2.so.2 (0x00007f518bee1000)
	libexpat.so.1 => /lib/x86_64-linux-gnu/libexpat.so.1 (0x00007f518bcb8000)
	libssl.so.1.0.0 => /lib/x86_64-linux-gnu/libssl.so.1.0.0 (0x00007f518ba4f000)
	libcrypto.so.1.0.0 => /lib/x86_64-linux-gnu/libcrypto.so.1.0.0 (0x00007f518b60b000)
	libgssapi_krb5.so.2 => /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2 (0x00007f518b3c1000)
	libkrb5.so.3 => /usr/lib/x86_64-linux-gnu/libkrb5.so.3 (0x00007f518b0ef000)
	libcom_err.so.2 => /lib/x86_64-linux-gnu/libcom_err.so.2 (0x00007f518aeeb000)
	libcap.so.2 => /lib/x86_64-linux-gnu/libcap.so.2 (0x00007f518ace5000)
	librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f518aadd000)
	libltdl.so.7 => /usr/lib/x86_64-linux-gnu/libltdl.so.7 (0x00007f518a8d3000)
	libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f518a551000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f518a248000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f518a032000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f5189c68000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f518c6ef000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f5189a64000)
	libicuuc.so.55 => /usr/lib/x86_64-linux-gnu/libicuuc.so.55 (0x00007f51896d0000)
	libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007f51894b6000)
	liblzma.so.5 => /lib/x86_64-linux-gnu/liblzma.so.5 (0x00007f5189294000)
	libk5crypto.so.3 => /usr/lib/x86_64-linux-gnu/libk5crypto.so.3 (0x00007f5189065000)
	libkrb5support.so.0 => /usr/lib/x86_64-linux-gnu/libkrb5support.so.0 (0x00007f5188e5a000)
	libkeyutils.so.1 => /lib/x86_64-linux-gnu/libkeyutils.so.1 (0x00007f5188c56000)
	libresolv.so.2 => /lib/x86_64-linux-gnu/libresolv.so.2 (0x00007f5188a3b000)
	libicudata.so.55 => /usr/lib/x86_64-linux-gnu/libicudata.so.55 (0x00007f5186f84000)

chtsanti · 2018-02-02T10:54:54Z

I am confirming the problem. I am getting the following errors:
ERROR: Ignoring http_port [::]:8082 due to TLS context initialization failure.
ERROR: Ignoring https_port [::]:8083 due to TLS context initialization failure.

rousskov · 2018-02-02T15:30:18Z

@yadij, please fix or revert ASAP.

yadij · 2018-02-03T13:50:16Z

@badfiles, @christos, I have opened PR #144 for the changes that fix it for me. Please test and followup there.

chtsanti · 2018-02-06T09:45:18Z

I applied the patch in PR #144 and run 1-2 tests. Still there are problems.
Peek and splice does not work, and the bumped connections rejected.

The problem in this patch looks that it is the tlsAttemptHandshake method, which return a true/false value where the old Squid_SSL_accept returned tri-state value, -1,0,1. But I did not run more tests and checks to see the exact problem. Moreover maybe there are other problems too.

I must apologize for my previous positive vote, but when I am reviewing a PR even if I am trying to check for bugs, and I am not running tests. I am mostly just checking how the patch affects the design, or causes problems difficult to be detected and solved in the future. But I am expecting that the author has run 1-2 tests.
Is it OK, should the reviewers test the code?

yadij · 2018-02-06T10:54:08Z

@christos, In the case of the SSL/TLS and libsecurity changes specifically, please do run tests as well. I am pretty sure yours will be different from mine and catch different things. In general it is not a requirement, but these changes are a bit special being security+privacy+crypto.

(I am testing what I can, but there are govt restrictions on crypto MITM that I have to work around.)

I am suspecting the latest issues are with the call in client_side.cc line 3310. The behaviour there is a bit weird. Given that you have written a handshake parser do we actually need to pass this to the library anymore?

chtsanti · 2018-02-06T11:19:15Z

Given that you have written a handshake parser do we actually need to pass this to the library anymore?

The parser just do simple parsing. OpenSSL api does more tests than just parsing, checks the openSSL versions, the used algorithms, possible attacks and many other. It is a long discussion if you want to disable these checks...

yadij · 2018-02-06T16:01:15Z

Okay, I'm reverting that particular API change for now in the PR #144 fixes. We can have the longer discussion about fixing it at some future time, there is a TODO about that already.

Can you review & test the PR 144 changes please to see if there is anything else you can find. Thanks.

rousskov · 2018-02-06T16:06:15Z

@chtsanti: I must apologize for my previous positive vote, but when I am reviewing a PR even if I am trying to check for bugs, and I am not running tests.

I do not think reviewers should apologize for not running tests. Testing is the responsibility of the PR author, not a reviewer. You simply do not have enough resources to properly review and test most SSL-related PRs in addition to your own work!

@yadij: I am testing what I can, but there are govt restrictions on crypto MITM that I have to work around

Government restrictions cannot justify posting and committing complex untested modifications of highly popular TLS-related features while essentially relying on others to test those modifications. We need to change the current status quo. I have sent you an email with specific suggestions to avoid airing dirty laundry here.

Meanwhile, please consider reverting this broken PR commit.

michaeladm · 2018-02-09T16:09:34Z

After this "GnuTLS implementation" improvement, ssl_bump function has stopped working.

root@frw:/# service squid start
Starting squid.
FATAL: No valid signing certificate configured for HTTPS_port 127.0.0.1:3129
Squid Cache (Version 5.0.0-20180207-rfbf55b6): Terminated abnormally.
CPU Usage: 0.039 seconds = 0.039 user + 0.000 sys
Maximum Resident Size: 41824 KB
Page faults with physical i/o: 0
/usr/local/etc/rc.d/squid: WARNING: failed to start squid

How now to make self-signed certificates?!
I did not succeed. Tried openssl and certtool tools.
If I edit cert-template to basicConstraints = CA:FALSE then squid starts, but ports do not up:
cache.log

ERROR: Ignoring https_port 127.0.0.1:3129 due to TLS context initialization failure.

yadij · 2018-02-09T17:32:19Z

@michaeladm, the fixes for those issues are in PR 144 which is currently stuck awaiting review and confirmation from people affected as to whether the issue is fully fixed for them. If you have any other issues related to this commit which can be replicated after applying the PR 144 patch please mention it in that PR.

rousskov · 2018-02-09T17:41:49Z

PR #144 is stuck awaiting tests (it also lacks a review but that is a minor secondary issue in this case). @michaeladm, if you can thoroughly test the changes in PR #144 in your SslBump setup, please do so and report your results (there).

…squid-cache#81) Move the http_port cert= and key= options logic to libsecurity and add GnuTLS implementation for PEM file loading. Also adds some extra debugging to clarify listening port initialization problems with the PEM files. Enable most of the http(s)_port listening socket logic to always build except where OpenSSL-specific dependency still exists. It may seem reasonable to leave it optionally excluded for minimal builds, however a minimal proxy that does not support HTTPS in any way is increasingly useless in the modern web so preference is given to building the generic TLS related code. This also simplifies the required testing to detect code portability issues. GnuTLS implementation is added for https_port configured with static cert=/key= parameters and the resulting TLS handshake behaviour. Squid built with GnuTLS can now act as useful parent proxies behind a SSL-Bump'ing frontend or for other clients which require a TLS explicit proxy. Also fixes the definitions for the CertPointer and PrivateKeyPointer.

…#81) (#152) Move the http_port cert= and key= options logic to libsecurity and add GnuTLS implementation for PEM file loading. Also adds some extra debugging to clarify listening port initialization problems with the PEM files. Enable most of the http(s)_port listening socket logic to always build except where OpenSSL-specific dependency still exists. It may seem reasonable to leave it optionally excluded for minimal builds, however a minimal proxy that does not support HTTPS in any way is increasingly useless in the modern web so preference is given to building the generic TLS related code. This also simplifies the required testing to detect code portability issues. GnuTLS implementation is added for https_port configured with static cert=/key= parameters and the resulting TLS handshake behaviour. Squid built with GnuTLS can now act as useful parent proxies behind a SSL-Bump'ing frontend or for other clients which require a TLS explicit proxy. Also fixes the definitions for the CertPointer and PrivateKeyPointer.

Commit 51e09c0 (GitHub PR #81) added GnuTLS functions available starting with GnuTLS v3.4.0 but did not bump the ./configure check accordingly.

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.

yadij force-pushed the tls-dynamic-cert-ui branch from 1ab5953 to f832ff1 Compare November 18, 2017 14:59

yadij requested a review from chtsanti November 22, 2017 14:03

rousskov previously requested changes Nov 26, 2017

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 labels Nov 26, 2017

yadij force-pushed the tls-dynamic-cert-ui branch from cdde7d8 to af2c1b2 Compare November 30, 2017 13:23

yadij force-pushed the tls-dynamic-cert-ui branch 2 times, most recently from abe7545 to dd3ddb4 Compare December 12, 2017 03:01

yadij removed the S-waiting-for-author author action is expected (and usually required) label Dec 14, 2017

yadij force-pushed the tls-dynamic-cert-ui branch from dd3ddb4 to 82188ed Compare December 14, 2017 16:00

yadij force-pushed the tls-dynamic-cert-ui branch 2 times, most recently from 4381371 to 4965572 Compare January 6, 2018 13:58

yadij mentioned this pull request Jan 17, 2018

ESI: remove custom parser #128

Merged

yadij force-pushed the tls-dynamic-cert-ui branch 3 times, most recently from f3f0d2e to 1e807cd Compare January 24, 2018 02:19

chtsanti reviewed Jan 31, 2018

View reviewed changes

yadij added 12 commits February 1, 2018 14:27

TLS: Add GnuTLS definition for PrivateKeyPointer

5b0114d

GnuTLS support for https_port cert= key= file loading

30253de

Move the logic for loading cert/key from PEM file from libsslsquid to libsecurity. Also, fix CertPointer and PrivateKeyPointer definitions for non-OpenSSL builds.

Improve const correctness for KeyData::loadFromFiles()

faf5ee6

Move Ssl::addChainToSslContext to Security::ServerOptions::updateCont…

7dc5c8c

…extCertChain

Move more TLS context updates from Ssl::InitServerContext to Security…

92ae304

…::ServerOptions

Use gnutls_x509_privkey_t instead of gnutls_privkey_t for storage

1cb5083

Move cert= and key= loading to Security::ServerOptions

1b1e4b1

Docs: Update desriptions for http_port cert= and key=

56a9ed5

Always build https_port listening logic

23d001e

GnuTLS implementation for server TLS handshake

b9365e3

Polish the tryTlsHandshake code

88c76c3

Use a KeyData to hold signing CA parsed details

e819fc7

yadij merged commit 51e09c0 into squid-cache:master Feb 1, 2018

yadij deleted the tls-dynamic-cert-ui branch February 1, 2018 09:52

rousskov mentioned this pull request Feb 21, 2018

Fixed writing clean swap file broken in 4310f8b #155

Merged

yadij mentioned this pull request Feb 25, 2018

Bump minimum required GnuTLS version to 3.4.0 #156

Merged

squid-anubis pushed a commit that referenced this pull request Feb 26, 2018

Bump minimum required GnuTLS version to 3.4.0 (#156)

a08fc4e

Commit 51e09c0 (GitHub PR #81) added GnuTLS functions available starting with GnuTLS v3.4.0 but did not bump the ./configure check accordingly.

yadij mentioned this pull request Apr 15, 2018

Bug 4831: filter chain certificates for validity when loading #187

Closed

Conversation

yadij commented Nov 18, 2017 • 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!

rousskov Nov 27, 2017 • 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!

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

yadij commented Nov 18, 2017 •

edited

Loading

rousskov Nov 27, 2017 •

edited

Loading