Move TLS/SSL http_port config values from libanyp objects to libsecurity by yadij · Pull Request #51 · squid-cache/squid

yadij · 2017-08-21T20:56:55Z

These are most of the minor shuffling prerequisite for the proposal to allow generate-host-certificates to set a CA filename. These are required in libsecurity in order to prevent circular dependencies between libsecurity, libssl and libanyp.

Also contains some improvements to how configuration errors are displayed for these affected settings and some bugs fixed where the configured values were handled incorrectly.

rousskov

FWIW, the PortCfg class itself does not belong to anyp IMHO.

I can only do a superficial review. Hopefully, Christos will look at this as well.

If you are going to keep individual/smaller commits when merging, then please merge fixes into the corresponding individual/smaller commits. I think such "selecting squashing" can be done via rebase.

rousskov · 2017-08-22T01:26:56Z

src/cache_cf.cc

    } else if (strncmp(token, "dynamic_cert_mem_cache_size=", 28) == 0) {
        parseBytesOptionValue(&s->dynamicCertMemCacheSize, B_BYTES_STR, token + 28);
-#endif
+        if (s->dynamicCertMemCacheSize == std::numeric_limits<size_t>::max()) {


I do not understand why maximum size_t value is important here. The corresponding error message talks about (anticipated?) memory allocation failure but there are many other size_t values for which most systems would not be able to allocate memory. It feels like there is a bug somewhere. I know you are just moving a similar comparison, but do we actually understand what is going on here? If not, we should at least add an XXX comment.

I'm not completely sure either - at least no enough to remove it. Seems the value parsing used may be setting max() when invalid values are configured , sometimes, maybe, if the wrappers and macros switching stdlib, Squid, and math library parsers around expand that way.

Please add an XXX comment then. For example:

// XXX: parseBytesOptionValue() self_destruct()s on invalid values, // probably making this comparison and misleading ERROR unnecessary.

rousskov · 2017-08-22T01:32:52Z

src/security/ServerOptions.cc

+#if USE_OPENSSL
+        staticContextSessionId = SBuf(token+8);
+        if (staticContextSessionId.length() > SSL_MAX_SSL_SESSION_ID_LENGTH) {
+            debugs(83, DBG_CRITICAL, "FATAL: Option 'context=' value is too long. Maximum " << SSL_MAX_SSL_SESSION_ID_LENGTH << " characters.");


Please avoid duplication (and improve diagnostics) by using token instead of 'context='.

The value of this parameter is a security secret, effectively a password for the session. So causing it to be logged is not a good idea. A small amount of "duplication" by naming just the option in text seems a reasonable tradeoff for protecting that.

I should mention it is also the reason I'm not doing the usual thing of storing the value for correct config dumps when OpenSSL is not built in.

rousskov · 2017-08-22T01:33:23Z

src/security/ServerOptions.cc

+            self_destruct();
+        }
+#else
+        debugs(83, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: Option 'context=' requires --with-openssl. Ignoring.");


Please avoid duplication (and improve diagnostics) by using token instead of 'context='.

The value of this parameter is a security secret, effectively a password for the session.

Are you sure? This is not my area of the expertise, but based on my superficial web search, this context is not a secret value but a weak mechanism for detecting misconfiguration -- it can be used to detect accidental loading of stale/wrong session caches. If my interpretation of their source code documentation is accurate, Tomcat servers "usually" set it to a "host:port combination". Does knowing the session context ID enable an otherwise unavailable attack vector?

Tomcat usage is consistent with Browser and Server people scoping TLS things to the origin hostname+port. We should probably be doing the same as our default, but that is a separate issue to fix.

My understanding is that this context string is an extra key to be added to the session hash - it works towards sandboxing of sessions intended for separate purposes. To prevent reuse of a session from one service in some other unintended service offered by the same organisation and/or server. (eg. the virtual host origin in Tomcats case).

If an anyone can figure out what the value is they have better ability to reuse the sessions in other places that use the same value. Since we are offering it as arbitrary configurable string to admin we cannot know for sure whether they are using it for weak origin-scoping like Tomcat, or for more critical uses like scoping TLS authentication within an enterprise.

NP: if you are reading that "uniquely identifies this context" the 'unique' part is not as unique as it may seem. Any two contexts A and B with identical crypto values usually can accept and re-use sessions from the other (as seen in 0-RTT traffic normally) - when this string is set in one that ceases to be possible - A can then only deal with A sessions, and B with B sessions ... until the other also gets the identical string set - in which case it goes back to both being indistinguishable. So the meaning of "unique" in that doc hinges on the meaning of "this context".
So I'm failing to see how this would detect a stale session cache entry unless one was regularly changing the string value, and doing that would break all the fresh TLS sessions at the instant of changing as well. Using it with normal values of host:port will not detect the age/staleness of any particular session cache entry - only the origin it was created for.

If an anyone can figure out what the value is they have better ability to reuse the sessions in other places that use the same value.

I do not think knowing a constant (session context ID) inside an encrypted value (session state) makes it any easier to reuse that session with malicious intent.

Since we are offering it as arbitrary configurable string to admin we cannot know for sure whether they are using it for weak origin-scoping like Tomcat, or for more critical uses like scoping TLS authentication within an enterprise.

If knowing the value does not weaken security, then (for this discussion) it does not matter what the value is used for.

I'm failing to see how this would detect a stale session cache entry unless one was regularly changing the string value ... Using it with normal values of host:port will not detect the age/staleness of any particular session cache entry - only the origin it was created for.

I believe you have pretty much addressed your own doubts -- the context ID can be used to detect stale values if and only if the ID changes when the old context becomes stale. What "stale" means depends on the deployment, of course. In some cases, it could be a change in server host name or port. In some other cases, it could be a date change. In yet other cases, it could be the version change of some monitoring software. The actual staleness definition does not matter for this discussion.

I do not consider this particular problem important. If you insist on "hiding" the context ID value, then I am not going to block this PR because of the resulting minor code duplication. Please add a comment to document the rationale:

// to hide its arguably sensitive value, do not print `token` here

rousskov · 2017-08-22T01:39:40Z

src/security/PeerOptions.h

    void updateTlsVersionLimits();

+    /// setup the library specific 'options=' parameters for the given context
+    void updateContextOptions(Security::ContextPointer &, bool) const;


It is impossible to guess the meaning of the anonymous boolean parameter IMO.

rousskov · 2017-08-22T01:43:00Z

src/security/ServerOptions.cc


    // parse the server-only options
-    if (strncmp(token, "dh=", 3) == 0) {
+    if (strncmp(token, "context=", 8) == 0) {


You may be able to minimize (perceived) code changes if you move context parsing lower, leaving dh parsing unchanged.

rousskov · 2017-08-22T01:46:43Z

src/ssl/stub_libsslutil.cc

-X509 * Ssl::signRequest(X509_REQ_Pointer const &, Security::CertPointer const &, EVP_PKEY_Pointer const &, ASN1_TIME *, BIGNUM const *) STUB_RETVAL(NULL)
-bool Ssl::generateSslCertificateAndPrivateKey(char const *, Security::CertPointer const &, EVP_PKEY_Pointer const &, Security::CertPointer &, EVP_PKEY_Pointer &, BIGNUM const *) STUB_RETVAL(false)
-void Ssl::readCertAndPrivateKeyFromFiles(Security::CertPointer &, EVP_PKEY_Pointer &, char const *, char const *) STUB
+X509_REQ * Ssl::createNewX509Request(Security::PrivateKeyPointer const &, const char *) STUB_RETVAL(NULL)


Please use nullptr in changed code.

rousskov · 2017-08-22T01:46:54Z

src/ssl/stub_libsslutil.cc

+bool Ssl::writeCertAndPrivateKeyToMemory(Security::CertPointer const &, Security::PrivateKeyPointer const &, std::string &) STUB_RETVAL(false)
+bool Ssl::writeCertAndPrivateKeyToFile(Security::CertPointer const &, Security::PrivateKeyPointer const &, char const *) STUB_RETVAL(false)
+bool Ssl::readCertAndPrivateKeyFromMemory(Security::CertPointer &, Security::PrivateKeyPointer &, char const *) STUB_RETVAL(false)
+X509 * Ssl::signRequest(X509_REQ_Pointer const &, Security::CertPointer const &, Security::PrivateKeyPointer const &, ASN1_TIME *, BIGNUM const *) STUB_RETVAL(NULL)


Please use nullptr in changed code.

so much for scripted changes getting through audit. done

rousskov · 2017-08-22T01:51:38Z

src/anyp/PortCfg.h

-    Security::CertPointer untrustedSigningCert; ///< x509 certificate for signing untrusted generated certificates
-    Ssl::EVP_PKEY_Pointer untrustedSignPkey; ///< private key for signing untrusted generated certificates
-#endif
+    size_t dynamicCertMemCacheSize = 4*1024*1024; ///< max size of generated certificates memory cache (4 MB default)


Separating dynamicCertMemCacheSize from the rest of .secure options feels wrong. Please move dynamicCertMemCacheSize into .secure as well if possible, even if dynamicCertMemCacheSize does not require USE_OPENSSL.

It is not actually a TLS/SSL thing. It is just a per-port cache structure managed through SMP and Runners regardless of whether TLS/SSL is even used on the port. That seems like a bit of a design mistake, but not one I'm fixing in this PR.

TLS parameters, keys, certificates, sessions, etc. are all various aspects of secure connections. Their configuration belongs to .secure. Whether the certificate cache (incorrectly) exists when TLS is unused is irrelevant. I agree that fixing that specific bug can be declared out of scope, but one should either move all the security-related configuration parameters or keep them as is. A minor bug cannot justify splitting related parameters, introducing another, much bigger bug. Besides, I do not see how that bug prevents you from keeping all the parameters together.

fine, done.

rousskov · 2017-08-22T02:07:19Z

src/security/PeerOptions.cc

+void
+Security::PeerOptions::updateContextOptions(Security::ContextPointer &ctx, bool setOptions) const
+{
+#if USE_OPENSSL


I would move this GnutTLS comment lower, outside the USE_OPENSSL block. Adding an #else clause just for this comment may be a good idea.

okay, done.

yadij · 2017-08-22T05:01:43Z

I'm planning on a squash merge for all this. Left the individual commits since I thought it would help the audit to see how the bits fit together and which changes were bugs vs shuffling vs polish.

rousskov

Thank you for addressing many of my previous review requests. This review focuses on the remaining points, only two of which are significant (dynamicCertMemCacheSize location and updateContextOptions() documentation).

rousskov · 2017-08-23T17:55:47Z

src/cache_cf.cc

    } else if (strncmp(token, "dynamic_cert_mem_cache_size=", 28) == 0) {
        parseBytesOptionValue(&s->dynamicCertMemCacheSize, B_BYTES_STR, token + 28);
-#endif
+        if (s->dynamicCertMemCacheSize == std::numeric_limits<size_t>::max()) {


Please add an XXX comment then. For example:

// XXX: parseBytesOptionValue() self_destruct()s on invalid values, // probably making this comparison and misleading ERROR unnecessary.

rousskov · 2017-08-23T18:06:10Z

src/anyp/PortCfg.h

-    Security::CertPointer untrustedSigningCert; ///< x509 certificate for signing untrusted generated certificates
-    Ssl::EVP_PKEY_Pointer untrustedSignPkey; ///< private key for signing untrusted generated certificates
-#endif
+    size_t dynamicCertMemCacheSize = 4*1024*1024; ///< max size of generated certificates memory cache (4 MB default)


TLS parameters, keys, certificates, sessions, etc. are all various aspects of secure connections. Their configuration belongs to .secure. Whether the certificate cache (incorrectly) exists when TLS is unused is irrelevant. I agree that fixing that specific bug can be declared out of scope, but one should either move all the security-related configuration parameters or keep them as is. A minor bug cannot justify splitting related parameters, introducing another, much bigger bug. Besides, I do not see how that bug prevents you from keeping all the parameters together.

rousskov · 2017-08-23T18:21:48Z

src/security/PeerOptions.h

    void updateTlsVersionLimits();

+    /// Setup the library specific 'options=' parameters for the given context.
+    /// \param setOptions false when the context options are to be set to 0.


AFAICT, false setOptions does not set options to zero! It passes a zero value to SSL_CTX_set_options(), and calling that function with a zero value is actually a no-op:

SSL_CTX_set_options() adds the options set via bitmask in options to ctx. Options already set before are not cleared!

If we assume (I have not checked) that the callers are correct, then the updateContextOptions() method documentation should be fixed, and the method should do nothing when setOptions is false. BTW, fixing this will help you avoid one "unused parameter" warning in updateContextOptions().

rousskov · 2017-08-23T18:27:17Z

src/security/ServerOptions.cc

+            self_destruct();
+        }
+#else
+        debugs(83, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: Option 'context=' requires --with-openssl. Ignoring.");


The value of this parameter is a security secret, effectively a password for the session.

Are you sure? This is not my area of the expertise, but based on my superficial web search, this context is not a secret value but a weak mechanism for detecting misconfiguration -- it can be used to detect accidental loading of stale/wrong session caches. If my interpretation of their source code documentation is accurate, Tomcat servers "usually" set it to a "host:port combination". Does knowing the session context ID enable an otherwise unavailable attack vector?

rousskov · 2017-08-23T19:19:23Z

I'm planning on a squash merge for all this.

Sounds good.

rousskov · 2017-08-23T19:19:34Z

I have requested @chtsanti review, but he may not be available for a week or so. Please feel free to remove that request once you have my vote.

rousskov · 2017-08-25T22:40:23Z

For the record, I do not think that PortCfg belongs to anyp/ and ServerOptions belong to security/. The linking problems you have complained about during this PR could be caused, in part, by misplacing high-level configuration objects into low-level protocol libraries. I am not suggesting that we fix that here and now, but we may want to fix that at some point to stop struggling with these linking problems.

chtsanti · 2017-08-29T16:08:25Z

src/ssl/support.cc

-                if (!sk_X509_push(chain, ca))
-                    debugs(83, DBG_IMPORTANT, "WARNING: unable to add CA certificate to cert chain");
+                Security::CertPointer newEntry;
+                newEntry.resetAndLock(ca);


The PEM_read_bio_X509 return locked X509 object, you should not use resetAndLock method. Simply replace the 1230 and 1231 lines with a line:
Security::CertPointer newEntry(ca)

chtsanti

Just one locking issue.
The rest of the patch looks ok to me.

rousskov · 2017-08-30T14:47:48Z

@chtsanti, please note that, when posting a code comment, you can click "Start a review" (instead of "Add single comment") button and then close the review with a summary. This way your individual change request(s) will be grouped under the summary, all associated with a particular commit. This procedure also allows you to edit individual comments before they are shown to everybody -- the comments are privately accumulated until you close the review.

rousskov

I found one more bug (missing assignments in the assignment operator), one risky design (generateHostCertificates()), and suggested a few polishing touches. I hope this is the last round for this PR.

rousskov · 2017-08-30T20:24:50Z

src/security/ServerOptions.cc

        clientCaStack = nullptr;
 #endif
+        staticContextSessionId = old.staticContextSessionId;
+        dynamicCertMemCacheSize = old.dynamicCertMemCacheSize;


Please copy the other newly added members as well.

Still open.

Oops, stupid of me. Fixed now.

rousskov · 2017-08-30T20:52:36Z

src/ssl/helper.cc

@@ -87,10 +87,9 @@ void Ssl::Helper::Init()
    assert(ssl_crtd == NULL);

    // we need to start ssl_crtd only if some port(s) need to bump SSL *and* generate certificates


This misleading comment became stale. Please remove or rephrase it. I suggest the former. However, a different or no fix may be needed here after the generateHostCertificates() issue is settled in an unexpected way.

Still open.

rousskov · 2017-08-30T20:56:09Z

src/ssl/support.cc

    }

-    maybeSetupRsaCallback(ctx);
+    Ssl::MaybeSetupRsaCallback(ctx);


In most cases, it is best not to explicitly mention namespace X inside namespace X. There are many offenders in the code you have to change; consider fixing those that you change for other reasons.

What make this practice of implicitly shadowing symbol names better than defining explicitly which symbol is being used?

I am aware that explicit full-naming is not strictly necessary. However doing so increases code clarity, self-documentation and is not harmful AFAIK. It just replicates explicitly what the compiler should be doing, without relying on that implicit behaviour. By contrast relying on implicit compiler behaviours opens the code to bugs in the toolchain, or IDE environments auto-spelling correction. So "best" seems a bit strong to use describing the minimalist coding style.

What make this practice of implicitly shadowing symbol names better than defining explicitly which symbol is being used?

Your specific question is too loaded to provide a meaningful answer, but I can answer a similar "Why is it usually best to not explicitly mention namespace X inside namespace X?" question with these points:

It is not required. With all other factors being equal, less ink is better.

It is not useful: The reader already knows the context from the method namespace (or equivalent).

It is harmful: The explicit mention of X may confuse the reader into thinking that the surrounding code is not inside X or that there is some naming conflict that should be resolved by explicitly mentioning X.

It complicates code refactoring by requiring more adjustments if the code is moved into a different namespace and/or X is renamed.

It increases code inconsistencies because only some developers will add (only) some unnecessary namespace prefixes.

implicitly shadowing symbol names

There should be no shadowing in most cases:

If both X::foo() and ::foo() exist, then (in most cases) something went wrong at the design stage because such shadowing is (in most cases) too risky/dangerous since the compiler will not be able to identify the cases of the wrong foo() used.

When both X::foo() and Y::foo() exist, one does not shadow the other. This is a quite common case because different libraries/scopes (X vs Y) often use the same names (foo).

than defining explicitly which symbol is being used

Both foo() and X::foo() either define explicitly which symbol is being used or not, depending on your definition of "explicit". If by "explicit" you meant something like "in a context-independent way" then neither expression is "explicit": To remove namespace dependencies, you have to write ::X::foo() AFAICT.

Still open.

IME the only solution to collisions when they do occur is explicit X::foo() and ::foo() use in callers. The various compiler and linker combinations do a bunch of slightly different symbol resolving , so the outcome of which gets picked is not as predictable as one might expect from simple local tests.

I think we may have some collisions between objects built before and after the patching. But that should not be too hard to resolve with a clean build. Sooo going with your way for now. I'd hate to be the one debugging it though. FTR; if this does go wrong the potential side effect is OpenSSL not calling our callbacks and doing its own thing to verify a TLS/SSL connections ciphers.

Addressed (as far as this particular instance of unnecessary namespace prefixes is concerned; I will try to mark several other occurrences separately).

the only solution to collisions when they do occur is explicit X::foo() and ::foo() use in callers

IMO, we should not rely on humans to remember about (and detect/fix) silent collisions when writing innocent caller code. We should instead design our APIs so that undetected collisions become nearly impossible. If both X::foo() and ::foo() may be present (and can compile) inside the same reasonable caller code (leading to two different results), then something went wrong at the design level. Such a problem should be solved at the design level (instead of adding "X::" prefixes in random places in hope to work around future problems).

if this does go wrong [a disaster beyond your imagination will occur]

If omitting Ssl:: from Ssl::MaybeSetupRsaCallback() compiles but causes runtime bugs, then we need to fix that (and/or some other) API (instead of hoping that some human will predict and overt that disaster by adding "Ssl::" prefixes in all the right places).

rousskov · 2017-08-30T21:11:12Z

src/client_side.cc

                s->flags.tunnelSslBumping = false;
            }
-            if (!s->secure.staticContext && !s->generateHostCertificates) {
+            if (!s->secure.staticContext && !s->generateHostCertificates()) {


The cumulative changes broke the original flaky logic further. Squid used to warn that it "Will not bump SSL due to TLS initialization failure" when bumping relied on static context and that static context initialization failed. Now Squid will also issue the same warning when Squid disables SslBump due to lack of ssl_bump rules because the previous if statement body will make !s->generateHostCertificates() true. In many such cases, this extra warning would be wrong because there was no "TLS initialization failure".

If you follow the generateHostCertificates fix outlines elsewhere, then you can fix the above by using secure.certificateGenerationRequested here instead. Further polishing can be done by taking into account flags.tunnelSslBumping value, but I do not insist on that because it may require more changes to take care of s->secure.encryptTransport that is only reset in this if statement and not the earlier one.

Presumed addressed.

rousskov · 2017-08-30T22:37:35Z

src/anyp/PortCfg.h

+    /// \returns true if this port requires auto-generated host certificates
+    bool generateHostCertificates() const {
+        // TODO also return true on https_port with vhost set
+        return flags.tunnelSslBumping && secure.generateHostCertificates;


This a.x is y && b.x design often leads to bugs because developers cannot remember which 'x' member to use, cfg.generateHostCertificates() or secure.generateHostCertificates. This PR may not have such problems, but it is only a matter of time IMO.

Also, mixing user configuration (secure.generateHostCertificates) and dynamically computed state (flags.tunnelSslBumping) in one configuration-like getter method leads to confusion when reporting configuration-related problems (this confusion is already present in this PR as documented in another comment).

On the other hand, the original code was also far from perfect because it could misinterpret .generateHostCertificates as an instruction to generate those certificates, even though SslBump was off.

There are several ways to address this problem. One of them is to rename secure.generateHostCertificates to secure.certificateGenerationRequested, rename PortCfg::generateHostCertificates() to generateCertificates(), and then adjust the rest of the code as needed. The different spelling and the Requested suffix should reduce the chance of misusing the configuration field in a context that requires a generateCertificates() call instead (and vice versa!).

If you do not want to address this problem in this PR, then I suggest leaving the old imperfect code (manipulating two independent data members without a new combining method) as is.

"generateHostCertificates" is the name of squid.conf parameter "generate-host-certificates=". What you are proposing here requires a matching change to the Squid UI / squid.conf option naming it to certificate-generation-requested=on/off. Which does not make sense linguistically as it is not a polite request - it is an admin requirement to generate certs. ssl-bump later determines the how, but that is another story.

I'm choosing to drop the PortCfg wrapper method. It was to de-duplicate boolean conditions which you have pointed out should not have been the same anyway.

What you are proposing here requires a matching change to the Squid UI / squid.conf option naming

It does not. When needed, we can use different names for parameter configuration and storage. A typical use case is when Squid must compute a derived value from the configuration parameter before using that configuration parameter. This is exactly what we are dealing with here as well. The complication here is that the raw configuration value is also useful in some context. Using two different names is a natural solution.

BTW, I am not married to the "Requested" suffix. For example, "Configured" can be used as well if you prefer. The lack of "Config::" or similar context helps making the name for the configured value ambiguous.

As I said, there are other solutions to this problem; some of them do not require different names. The solution I outlined is probably the simplest I could come up with.

I'm choosing to drop the PortCfg wrapper method. It was to de-duplicate boolean conditions which you have pointed out should not have been the same anyway.

OK, your call.

rousskov · 2017-08-30T22:52:03Z

src/security/ServerOptions.cc

+{
+#if USE_OPENSSL
+    if (!staticContextSessionId.isEmpty())
+        SSL_CTX_set_session_id_context(ctx.get(), reinterpret_cast<const unsigned char*>(staticContextSessionId.rawContent()), staticContextSessionId.length());


Use static_cast if possible. It catches more casting bugs than reinterpret_cast does.

Still open.

this one needs the reinterpret due to signed to unsigned conversion.
compile errors "invalid static_cast" appear otherwise

rousskov · 2017-08-30T23:00:07Z

src/security/forward.h

+CtoCpp1(EVP_PKEY_free, EVP_PKEY *)
+typedef Security::LockingPointer<EVP_PKEY, EVP_PKEY_free_cpp, HardFun<int, EVP_PKEY *, EVP_PKEY_up_ref> > PrivateKeyPointer;
+#else
+typedef void *PrivateKeyPointer;


As discussed in pr #38, this PrivateKeyPointer is not compatible with the primary PrivateKeyPointer declaration because the former lacks self-initialization. If possible, use a smart pointer here. The old comment from pr #38 has specific suggestions.

Fixing that coding style is out of scope for this PR. As before I'm open to a dedicated PR fixing the issue for all of them, but I'm against a piecewise approach for that fix. This PR is going with the existing slightly-broken definition so all the Pointer behave consistently (albeit broken in what is increasingly becoming a rare build case).

This may be of interest. The std:: smart pointers all share this property.
https://stackoverflow.com/questions/19840937/should-stdunique-ptrvoid-be-permitted

I'm against a piecewise approach for that fix.

I can accept that argument here if the fix is difficult. In that case, please consider adding an XXX. For example:

typedef void *PrivateKeyPointer; // XXX: incompatible with the other PrivateKeyPointer declaration (lacks self-initialization)

yadij · 2017-10-02T08:26:28Z

I believe all the outstanding issues are now sorted, one way or another.

rousskov · 2017-10-03T20:08:12Z

@yadij: I believe all the outstanding issues are now sorted, one way or another.

I found several that were not explicitly marked as addressed and appear to be still open/relevant as of 8c8e860. For your convenience, I have explicitly marked the latter ones with a "Still open" comment.

Please note that I have not checked whether all Christos comments are addressed, but please double check that they are.

yadij · 2017-10-15T20:20:30Z

Double and triple checked Christos change too.

yadij · 2017-10-16T19:33:54Z

I've now also moved the PortCfg::configureSslServerContext out so there are no more TLS/SSL specific things in the PortCfg object.

You may want to review that last commit. The bulk of it is pretty much a copy-paste. It also allows configuring https_port in non-OpenSSL builds, which should now mention that OpenSSL is needed to configure those ports. The case of no TLS/SSL library at all remains unchanged: 'unknown directive'.

rousskov

I left one minor error message polishing remark.

I did not see any more bugs, but I cannot say I had enough patience to carefully go through all the changes one more time. I did notice many new excessive namespace prefixes in several files, but I think they should be removed in a dedicated (to that file or files) PR for consistency sake, so I am not marking them in this PR.

rousskov · 2017-10-17T00:19:30Z

src/security/ServerOptions.cc

+        Security::KeyData &keys = certs.front();
+        Ssl::readCertChainAndPrivateKeyFromFiles(signingCert, signPkey, certsToChain, keys.certFile.c_str(), keys.privateKeyFile.c_str());
+#else
+        char buf[128];


The missing OpenSSL support for ... phrasing can be easily misinterpreted as if Squid currently lacks code that would allow this port to work when Squid is built with OpenSSL. Or as if OpenSSL itself cannot support this configuration yet. I suggest rephrasing this error message as --with-open-ssl required for ... or something like that.

yadij · 2017-10-17T01:16:14Z

Christos just waiting on your review update it seems.

rousskov · 2017-10-31T17:29:13Z

@yadij, if Semaphore CI is stuck, then I recommend [manually squashing all changes and force-]pushing an updated branch to tickle it.

commit 89539a0 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Tue Oct 17 14:10:36 2017 +1300 Change error message on https_port use without OpenSSL commit 0e20026 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Tue Oct 17 07:37:40 2017 +1300 Move PortCfg::configureSslServerContext to ServerOptions::createSigningContexts commit 87b4314 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Mon Oct 16 09:37:07 2017 +1300 Remove Ssl:: prefix from RSA callback function use commit b0acd5f Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Mon Oct 16 08:42:13 2017 +1300 Fix copy operator missing members commit d8c29fc Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Mon Oct 2 21:07:48 2017 +1300 Add XXX for PrivateKeyPointer commit e0264d6 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Mon Oct 2 20:45:01 2017 +1300 Revert 0ebcaa8 commit 914de31 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Mon Oct 2 20:06:40 2017 +1300 Cleanup diff against master commit 76e009d Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Thu Aug 31 06:42:10 2017 +1200 Move dynamic_cert_mem_cache_size= to libsecurity commit 3927f3e Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Thu Aug 31 06:10:49 2017 +1200 Fix locking after PEM_read_bio_X509 commit 45aa1f9 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Thu Aug 31 04:49:05 2017 +1200 Document for context= not being logged commit 26d02fc Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Sat Aug 26 21:36:25 2017 +1200 Convert Ssl::chainCertificatesToSSLContext to Security::ServerOptions commit b80162c Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Sat Aug 26 19:07:50 2017 +1200 Fix EVP_PKEY_Pointer uses after rebase commit 0bcc980 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Thu Aug 24 16:53:22 2017 +1200 Fix bool parameter of PeerOptions::updateContextOptions() The 0 value is apparently to cause a no-op. We can do the same simply by not calling the update function and avoid needing the parameter. commit 0bc7a5b Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Thu Aug 24 16:18:15 2017 +1200 Add XXX marker requested in audit It is not known for certain why the max() check was every done. A breif code review shows no particular need, but there may be deeper issues hidden by some macro wrappers. Needs some good testing to remove. commit 7234764 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Tue Aug 22 17:12:34 2017 +1200 Audit requested changes commit 8f28c45 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Tue Aug 22 08:40:05 2017 +1200 Make dynamic_certificate_mem_cache_size= option always configurable OpenSSL is not required to configure this setting. It is just meaningless without cert generation, which can happen even when OpenSSL is compiled in. Move the size_t max() check to configuration time so that correct values can always be printed on config dumps and an error message produced at a meaningful time instead of silently overriding the admins config at memory initialization time. Fix a bug on split-stack operating systems where PortCfg are cloned but the configured value was not copied to the cloned port. This may have resulted in memory corruption for some configurations. commit 122eb58 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Tue Aug 22 07:22:41 2017 +1200 Remove obsolete reference to AnyP::PortCfg::sslContext commit e5776ff Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Mon Aug 21 07:36:38 2017 +1200 Move configureSslContext() into Security::ServerOptions Now that all the configuration settings are stored in ServerOptions this function to update a given context can be made a method. Which prevents circular dependency between libanyp and libsecurity. commit 2b21887 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Sun Aug 20 23:46:50 2017 +1200 Add MaybeSetupRsaCallback() to libsslsquid.la API commit fd817b9 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Sun Aug 20 23:39:21 2017 +1200 De-duplicate calls to SSL_CTX_set_options commit 2b51ce7 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Sun Aug 20 23:27:05 2017 +1200 Move port cert and key Pointers to Security::ServerOptions commit 54036ed Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Sun Aug 20 12:50:06 2017 +1200 Convert Ssl::createSSLContext() and related functions use Security::ServerOptions ... instead of AnyP::PortCfg objects. commit 111b71c Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Sun Aug 20 09:51:28 2017 +1200 Remove unused 'alternate code' in configureSslContext() commit 88d349a Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Sun Aug 20 09:50:13 2017 +1200 Update configureSslContext() to take a Security::ServerOptions ... instead of an AnyP::PortCfg object for TLS settings commit 9487adc Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Sun Aug 20 09:36:19 2017 +1200 Move TLS/SSL session ID string to Security::ServerOptions Add an update method for setting the configured session ID ('sslcontext=' parameter) into a Security::ContextPointer object. Use an SBuf to manage the string memory since libssl SSL_CTX_set_session_id_context() takes its own copy. Removing several uses of xstrdup() and *free(). Make misconfiguration a fatal error. Ignoring the error produced by OpenSSL as Squid was previously doing leads to OpenSSL ignoring the given ID. Which in turn can result in TLS handshake errors when clients attempt to reuse previous sessions. commit e58a6f7 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Sat Aug 19 04:29:00 2017 +1200 Use a Security::CertList to store a listening ports additional CA chain More portable with standard STL operations and no longer necessary to cope with OpenSSL specific memory management problems. commit cc97f2a Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Sun Aug 13 00:24:51 2017 +1200 Move Ssl::EVP_PKEY_Pointer into libsecurity as Security::PrivateKeyPointer commit a007240 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Tue Aug 8 23:19:58 2017 +1200 Add generateHostCertificates() method to PortCfg commit b0844c2 Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Tue Aug 8 07:48:37 2017 +1200 Make generateHostCertificates public for ssl/helper.cc commit 2b7e6db Author: Amos Jeffries <amosjeffries@squid-cache.org> Date: Tue Aug 8 07:36:48 2017 +1200 Move generateHostCertificates to Security::ServerOptions

These are most of the minor shuffling prerequisite for the proposal to allow generate-host-certificates to set a CA filename. These are required in libsecurity in order to prevent circular dependencies between libsecurity, libssl and libanyp. Also contains some improvements to how configuration errors are displayed for these affected settings and some bugs fixed where the configured values were handled incorrectly.

rousskov · 2022-01-05T18:17:21Z

src/ssl/support.cc

-            while (X509 *ca = PEM_read_bio_X509(bio.get(), NULL, NULL, NULL)) {
-                if (!sk_X509_push(chain, ca))
-                    debugs(83, DBG_IMPORTANT, "WARNING: unable to add CA certificate to cert chain");
-            }
+            while (X509 *ca = PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr))
+                chain.emplace_front(Security::CertPointer(ca));


AFAICT, this change reverses as-configured chain order because sk_X509_push() is FIFO while emplace_front() is LIFO. See #956 for the proposed fix.

rousskov requested changes Aug 22, 2017

View reviewed changes

rousskov requested a review from chtsanti August 22, 2017 02:08

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 Aug 22, 2017

yadij force-pushed the tls-dynamic-cert-ui branch from dda29cd to a4cecf0 Compare August 22, 2017 05:38

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

rousskov requested changes Aug 23, 2017

View reviewed changes

rousskov added the S-waiting-for-author author action is expected (and usually required) label Aug 23, 2017

yadij force-pushed the tls-dynamic-cert-ui branch from a4cecf0 to 988c908 Compare August 24, 2017 04:08

chtsanti reviewed Aug 29, 2017

View reviewed changes

chtsanti requested changes Aug 30, 2017

View reviewed changes

rousskov removed the S-waiting-for-more-reviewers needs a reviewer and/or a second opinion label Aug 30, 2017

yadij force-pushed the tls-dynamic-cert-ui branch 2 times, most recently from 01f5a46 to 2ea1a37 Compare August 30, 2017 16:46

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 Aug 30, 2017

rousskov requested changes Aug 30, 2017

View reviewed changes

rousskov added the S-waiting-for-author author action is expected (and usually required) label Aug 30, 2017

yadij force-pushed the tls-dynamic-cert-ui branch from 70b536e to 1d82fed Compare September 21, 2017 14:12

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

rousskov added the S-waiting-for-author author action is expected (and usually required) label Oct 3, 2017

yadij force-pushed the tls-dynamic-cert-ui branch from 8c8e860 to 44efa12 Compare October 4, 2017 03:17

kinkie mentioned this pull request Oct 5, 2017

Bug 4718: Support filling raw buffer space of shared SBufs #64

Merged

yadij force-pushed the tls-dynamic-cert-ui branch from 44efa12 to b0e69b5 Compare October 11, 2017 11:04

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

rousskov reviewed Oct 17, 2017

View reviewed changes

rousskov approved these changes Oct 17, 2017

View reviewed changes

yadij force-pushed the tls-dynamic-cert-ui branch from 1caeda6 to 107bfaa Compare October 18, 2017 02:09

yadij force-pushed the tls-dynamic-cert-ui branch 2 times, most recently from 5f68cb8 to 89539a0 Compare October 27, 2017 15:24

chtsanti approved these changes Oct 31, 2017

View reviewed changes

yadij removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Oct 31, 2017

rousskov added the S-waiting-for-committer privileged action is expected (and usually required) label Oct 31, 2017

yadij force-pushed the tls-dynamic-cert-ui branch from 89539a0 to 72af380 Compare November 1, 2017 13:39

yadij merged commit cf48712 into squid-cache:master Nov 2, 2017

yadij deleted the tls-dynamic-cert-ui branch November 2, 2017 08:15

rousskov removed the S-waiting-for-committer privileged action is expected (and usually required) label Jan 5, 2022

rousskov reviewed Jan 5, 2022

View reviewed changes

		@@ -87,10 +87,9 @@ void Ssl::Helper::Init()
		assert(ssl_crtd == NULL);

		// we need to start ssl_crtd only if some port(s) need to bump SSL and generate certificates

Conversation

yadij commented Aug 21, 2017

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!

yadij Aug 22, 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

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 commented Aug 22, 2017

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!

rousskov commented Aug 23, 2017

Uh oh!

rousskov commented Aug 23, 2017

Uh oh!

rousskov commented Aug 25, 2017

Uh oh!

chtsanti Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

yadij Aug 22, 2017 •

edited

Loading

chtsanti Aug 29, 2017 •

edited

Loading