[squid-dev] [PATCH] YesNoNone class updates

Amos Jeffries squid3 at treenet.co.nz
Thu Jan 14 19:31:16 UTC 2016


On 15/01/2016 6:26 a.m., Alex Rousskov wrote:
> On 01/13/2016 03:56 PM, Amos Jeffries wrote:
>> On 13/01/2016 5:11 a.m., Alex Rousskov wrote:
>>> On 01/11/2016 06:36 PM, Amos Jeffries wrote:
>>>> Distinction is made between implicit and explicit configuration.
>>>
>>> Why do we need this increased complexity?
> 
>> This was written in context of using it to complete the final audit
>> request for the bug 4005 patch instead of multiple bools.
> 
> Noted, thank you.
> 
> 
>>>> +bool
>>>> +YesNoNone::operator !() const
>>>> +{
>>>> +    assert(option != YesNoNone::optUnspecified);
>>>> +    return option == YesNoNone::optDisabled;
>>>>  }
>>>
>>> Please never reverse the existing operator bool code. Inline the
>>> negation of that operator call instead.
> 
> [ excuses snipped without review ]
> 
> If you cannot simply negate the existing "bool" conversion operator, for
> any reason, then you must not add the "bool" operator in the first place!
> 
> 
> After further consideration, I think I can make an even stronger and
> simpler rule for the logical not operator in new code:
> 
> There is one and only one reason to add a logical not operator to a
> C++11 class: Optimization of a boolean conversion operator (for rare
> cases where "!foo" is much faster to compute than "foo").
> 
> Please correct me if I missed any other cases where an explicit logical
> not operator should be added to a C++11 class.
> 
> 
>> +    /** forbidden operation.
>> +     * Use x.configure(bool) when the value is configured.
>> +     * Use x = YesNoNone(bool) to assign defaults.
>> +     */
>> +    YesNoNone &operator =(bool) = delete;
> 
> Why do we need to delete something that does not exist? If you do not
> add the above code, do you really gain the ability to assign booleans to
> YesNoNone objects? If not, please remove.
> 

We have a bool constructor and implicit copy-assignment. Which tempts
the compiler to try bool constructing then copying. Which in turn either
works with the copy-assignment or produces some long errors about
move-assignment, depending on whether the compiler version prefers to
try move or copy assign first.

Versus:
  error: use of deleted function ‘YesNoNone& YesNoNone::operator=(bool)’



> 
>> +    uint8_t setWith; ///< initialized instead of configured
> 
> Stale comment. Consider using "how the value was set" description instead.
> 
> Stale type. Should be SetWith AFAICT.
> 

Done and done.

> 
>> +    enum SetWith : uint8_t { optUnspecified = 0, optImplicit = 1, optConfigured = 2 };
> 
> 
> Why do we need to specify the exact enum storage size in this case? If
> there is a good reason, please add a source code comment to explain. If
> there is not, remove storage type specification.
> 

Default enum size seems to be a full 32/64-bit int. This gets reasonable
savings without going down so far as to expend performance handling
uncommon type sizes.


> BTW, if you need to minimize storage overhead at the expense of
> value-testing performance, you can encode all the information (both
> setWith and option members) in 8 bits, of course.
> 
> Please consider renaming SetWith to something that matches the enum
> values better: "set with configured" does not work well. I suggest
> "SetHow" and optUnspecified, optImplicitly, and optConfigured values.
> 

Sure. Done.

> 
>> +    bool option; ///< configured value or false
> 
> Or an implicitly set value. I would rephrase the description to
> something like:
> 
> bool option; /// specified yes/no value; meaningless if optUnspecified
> 

Ok. Done.

> 
>> +    bool option; ///< configured value or false
>> +    uint8_t setWith; ///< initialized instead of configured
> 
> Please reorder to minimize padding, especially if setWith type changes.
> 

Do you have a good way to test for that?

I'm only aware of the GCC option that causes warnings across lots of the
Squid code and thus -Werror issues.


> 
>> +    Config.memShared = YesNoNone(false);
>> -        writeableCfg().connectionEncryption.configure(true);
>> +        writeableCfg().connectionEncryption = YesNoNone(true);
> 
> The new code that configures "implicit" values looks unnecessary
> awkward/complex. I suggest adding a method to change the value (in
> addition to the constructor):
> 
>     Config.memShared.defaultTo(false);
>     writeableCfg().connectionEncryption.defaultTo(true);
> 
> As an added benefit, the method can throw if optConfigured values are
> reset using defaultTo() calls!
> 

Hmm. Okay. Done.

> 
>> -YesNoNone::operator void*() const
>> -{
>> -    assert(option != 0); // must call configure() first
>> -    return option > 0 ? (void*)this : NULL;
>> -}
> 
>> +    explicit operator bool() const {
>> +        assert(setWith != optUnspecified);
>> +        return option;
>> +    }
> 
> To reduce chances for getting another CVE for a reconfiguration crash, I
> suggest that new configuration-related code does not assert() but throws
> (e.g., via Must()). Whether that implies moving the above code back to
> .cc file is your call.

I am a little wary that this could lead to mysterious uncaught
exceptions when these config values are tested by code outside the
AsyncCall protections.

But going with it anyway.

> 
>> If there are no objections I will apply this patch and the bug 4005 fix
>> that depends on it tomorrow.
> 
> I object to the current YesNoNone patch, but I hope that the above
> comments give you enough info to polish it without another review round.
> 

New one attached then, in case you have time.


> FWIW, I have not seen a bug 4005 patch that depends on this. The old mk2
> does not seem to use the changes introduced here. On the other hand, I
> cannot promise to review the updated patch so others should decide
> whether they have to see it before it goes in.
> 

K. It is much the same as the adaptation config usage in these latest
patches. But with a member initialization in PeerOptions ctor as well.

Amos

-------------- next part --------------
=== modified file 'src/Makefile.am'
--- src/Makefile.am	2016-01-05 13:00:50 +0000
+++ src/Makefile.am	2016-01-13 06:15:50 +0000
@@ -230,8 +230,6 @@
 	AsyncEngine.h \
 	cache_cf.h \
 	AuthReg.h \
-	YesNoNone.h \
-	YesNoNone.cc \
 	RefreshPattern.h \
 	cache_cf.cc \
 	CacheDigest.h \
@@ -920,7 +918,8 @@
 	tests/testSBufList \
 	tests/testConfigParser \
 	tests/testStatHist \
-	tests/testLookupTable
+	tests/testLookupTable \
+	tests/testYesNoNone
 
 if HAVE_FS_ROCK
 check_PROGRAMS += tests/testRock
@@ -1009,7 +1008,6 @@
 	log/access_log.h \
 	tests/stub_access_log.cc \
 	cache_cf.h \
-	YesNoNone.h \
 	tests/stub_cache_cf.cc \
 	tests/stub_cache_manager.cc \
 	tests/stub_comm.cc \
@@ -1131,7 +1129,6 @@
 	log/access_log.h \
 	tests/stub_access_log.cc \
 	cache_cf.h \
-	YesNoNone.h \
 	tests/stub_cache_cf.cc \
 	tests/stub_client_side.cc \
 	tests/stub_debug.cc \
@@ -1265,8 +1262,6 @@
 	cache_manager.cc \
 	cache_cf.h \
 	AuthReg.h \
-	YesNoNone.h \
-	YesNoNone.cc \
 	RefreshPattern.h \
 	cache_cf.cc \
 	CachePeer.cc \
@@ -1582,7 +1577,6 @@
 	tests/stub_access_log.cc \
 	tests/stub_acl.cc \
 	cache_cf.h \
-	YesNoNone.h \
 	tests/stub_cache_cf.cc \
 	tests/stub_cache_manager.cc \
 	tests/stub_client_db.cc \
@@ -1698,8 +1692,6 @@
 	tests/stub_CacheDigest.cc \
 	cache_cf.h \
 	AuthReg.h \
-	YesNoNone.h \
-	YesNoNone.cc \
 	RefreshPattern.h \
 	cache_cf.cc \
 	CachePeer.cc \
@@ -1947,8 +1939,6 @@
 	cache_manager.cc \
 	cache_cf.h \
 	AuthReg.h \
-	YesNoNone.h \
-	YesNoNone.cc \
 	RefreshPattern.h \
 	cache_cf.cc \
 	CachePeer.cc \
@@ -2190,8 +2180,6 @@
 	BodyPipe.cc \
 	cache_cf.h \
 	AuthReg.h \
-	YesNoNone.h \
-	YesNoNone.cc \
 	RefreshPattern.h \
 	cache_cf.cc \
 	CachePeer.cc \
@@ -2460,7 +2448,6 @@
 	mime_header.h \
 	String.cc \
 	cache_cf.h \
-	YesNoNone.h \
 	$(SBUF_SOURCE) \
 	SBufAlgos.h \
 	SBufAlgos.cc \
@@ -2526,8 +2513,6 @@
 	cache_manager.cc \
 	cache_cf.h \
 	AuthReg.h \
-	YesNoNone.h \
-	YesNoNone.cc \
 	RefreshPattern.h \
 	cache_cf.cc \
 	debug.cc \
@@ -2867,7 +2852,6 @@
 	tests/stub_access_log.cc \
 	tests/stub_acl.cc \
 	cache_cf.h \
-	YesNoNone.h \
 	tests/stub_cache_cf.cc \
 	tests/stub_cache_manager.cc \
 	tests/stub_client_side_request.cc \
@@ -2973,7 +2957,6 @@
 	tests/testString.cc \
 	tests/testString.h \
 	cache_cf.h \
-	YesNoNone.h \
 	tests/stub_cache_cf.cc \
 	tests/stub_cache_manager.cc \
 	tests/stub_cbdata.cc \
@@ -3094,7 +3077,6 @@
 	ConfigOption.cc \
 	tests/stub_acl.cc \
 	cache_cf.h \
-	YesNoNone.h \
 	tests/stub_cache_cf.cc \
 	tests/stub_helper.cc \
 	cbdata.cc \
@@ -3294,7 +3276,6 @@
 	log/access_log.h \
 	tests/stub_access_log.cc \
 	cache_cf.h \
-	YesNoNone.h \
 	tests/stub_cache_cf.cc \
 	client_db.h \
 	tests/stub_cache_manager.cc \
@@ -3377,8 +3358,6 @@
 	BodyPipe.cc \
 	cache_cf.h \
 	AuthReg.h \
-	YesNoNone.h \
-	YesNoNone.cc \
 	RefreshPattern.h \
 	cache_cf.cc \
 	tests/stub_cache_manager.cc \
@@ -3719,7 +3698,6 @@
 	tests/testConfigParser.cc \
 	tests/testConfigParser.h \
 	cache_cf.h \
-	YesNoNone.h \
 	tests/stub_cache_cf.cc \
 	tests/stub_cache_manager.cc \
 	tests/stub_cbdata.cc \
@@ -3825,6 +3803,18 @@
 	$(XTRA_LIBS)
 tests_testEnumIterator_DEPENDENCIES =
 
+tests_testYesNoNone_SOURCES = \
+	tests/testYesNoNone.cc \
+	tests/testYesNoNone.h
+nodist_tests_testYesNoNone_SOURCES = \
+	tests/STUB.h \
+	base/YesNoNone.h
+tests_testYesNoNone_LDADD= \
+	$(SQUID_CPPUNIT_LIBS) \
+	$(COMPAT_LIB) \
+	$(XTRA_LIBS)
+tests_testYesNoNone_LDFLAGS = $(LIBADD_DL)
+
 TESTS += testHeaders
 
 ## Special Universal .h dependency test script

=== modified file 'src/SquidConfig.h'
--- src/SquidConfig.h	2016-01-01 00:12:18 +0000
+++ src/SquidConfig.h	2016-01-13 06:15:33 +0000
@@ -11,6 +11,7 @@
 
 #include "acl/forward.h"
 #include "base/RefCount.h"
+#include "base/YesNoNone.h"
 #include "ClientDelayConfig.h"
 #include "DelayConfig.h"
 #include "helper/ChildConfig.h"
@@ -23,7 +24,6 @@
 #include "ssl/support.h"
 #endif
 #include "store/forward.h"
-#include "YesNoNone.h"
 
 #if USE_OPENSSL
 class sslproxy_cert_sign;

=== removed file 'src/YesNoNone.cc'
--- src/YesNoNone.cc	2016-01-01 00:12:18 +0000
+++ src/YesNoNone.cc	1970-01-01 00:00:00 +0000
@@ -1,23 +0,0 @@
-/*
- * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
- *
- * Squid software is distributed under GPLv2+ license and includes
- * contributions from numerous individuals and organizations.
- * Please see the COPYING and CONTRIBUTORS files for details.
- */
-
-#include "squid.h"
-#include "YesNoNone.h"
-
-YesNoNone::operator void*() const
-{
-    assert(option != 0); // must call configure() first
-    return option > 0 ? (void*)this : NULL;
-}
-
-void
-YesNoNone::configure(bool beSet)
-{
-    option = beSet ? +1 : -1;
-}
-

=== modified file 'src/adaptation/ServiceConfig.h'
--- src/adaptation/ServiceConfig.h	2016-01-13 10:10:20 +0000
+++ src/adaptation/ServiceConfig.h	2016-01-13 17:06:05 +0000
@@ -11,9 +11,9 @@
 
 #include "adaptation/Elements.h"
 #include "base/RefCount.h"
+#include "base/YesNoNone.h"
 #include "security/PeerOptions.h"
 #include "SquidString.h"
-#include "YesNoNone.h"
 
 namespace Adaptation
 {

=== modified file 'src/adaptation/ecap/ServiceRep.cc'
--- src/adaptation/ecap/ServiceRep.cc	2016-01-13 10:10:20 +0000
+++ src/adaptation/ecap/ServiceRep.cc	2016-01-14 18:32:31 +0000
@@ -171,7 +171,7 @@
 {
     Adaptation::Service::finalize();
     if (!cfg().connectionEncryption.configured())
-        writeableCfg().connectionEncryption.configure(true);
+        writeableCfg().connectionEncryption.defaultTo(true);
     theService = FindAdapterService(cfg().uri);
     if (theService) {
         try {

=== modified file 'src/adaptation/icap/ServiceRep.cc'
--- src/adaptation/icap/ServiceRep.cc	2016-01-13 12:12:11 +0000
+++ src/adaptation/icap/ServiceRep.cc	2016-01-14 18:32:47 +0000
@@ -85,10 +85,10 @@
     if (cfg().secure.encryptTransport) {
         debugs(3, DBG_IMPORTANT, "Initializing service " << cfg().resource << " SSL context");
         sslContext = writeableCfg().secure.createClientContext(true);
-        if (!cfg().connectionEncryption.configured())
-            writeableCfg().connectionEncryption.configure(true);
-    } else if (!cfg().connectionEncryption.configured())
-        writeableCfg().connectionEncryption.configure(false);
+    }
+
+    if (!cfg().connectionEncryption.configured())
+        writeableCfg().connectionEncryption.defaultTo(cfg().secure.encryptTransport);
 
     theSessionFailures.configure(TheConfig.oldest_service_failure > 0 ?
                                  TheConfig.oldest_service_failure : -1);

=== modified file 'src/base/Makefile.am'
--- src/base/Makefile.am	2016-01-01 00:12:18 +0000
+++ src/base/Makefile.am	2016-01-13 06:16:06 +0000
@@ -38,4 +38,5 @@
 	Subscription.h \
 	TextException.cc \
 	TextException.h \
-	TidyPointer.h
+	TidyPointer.h \
+	YesNoNone.h

=== renamed file 'src/YesNoNone.h' => 'src/base/YesNoNone.h'
--- src/YesNoNone.h	2016-01-01 00:12:18 +0000
+++ src/base/YesNoNone.h	2016-01-14 19:19:25 +0000
@@ -9,27 +9,68 @@
 #ifndef SQUID_YESNONONE_H_
 #define SQUID_YESNONONE_H_
 
-/// Used for boolean enabled/disabled options with complex default logic.
-/// Allows Squid to compute the right default after configuration.
-/// Checks that not-yet-defined option values are not used.
+#include "base/TextException.h"
+
+// TODO: generalize / template to non-boolean option types
+// and make YesNoNone the boolean instance of the template
+
+/**
+ * Used for boolean enabled/disabled options with complex default logic.
+ * Allows Squid to compute the right default after configuration.
+ * Checks that not-yet-defined option values are not used.
+ * Allows for implicit default Yes/No values to be used by initialization
+ * without configure() being called, but not dumped as squid.conf content.
+ */
 class YesNoNone
 {
-// TODO: generalize to non-boolean option types
+    enum SetHow : uint8_t { optUnspecified = 0, optImplicit = 1, optConfigured = 2 };
+
 public:
-    YesNoNone(): option(0) {}
-
-    /// returns true iff enabled; asserts if the option has not been configured
-    operator void *() const; // TODO: use a fancy/safer version of the operator
-
-    /// enables or disables the option;
-    void configure(bool beSet);
-
-    /// whether the option was enabled or disabled, by user or Squid
-    bool configured() const { return option != 0; }
+    // this constructor initializes to 'unspecified' state
+    YesNoNone():
+        setHow_(optUnspecified),
+        option(false)
+    {}
+
+    // this constructor initializes to 'implicit' state
+    explicit YesNoNone(bool beSet):
+        setHow_(optImplicit),
+        option(beSet)
+    {}
+
+    /** forbidden operation.
+     * Use x.configure(bool) when the value is configured.
+     * Use x.defaultTo(bool) to assign defaults.
+     */
+    YesNoNone &operator =(bool) = delete;
+
+    /// the boolean equivalent of the value stored.
+    /// asserts if the value has not been set.
+    explicit operator bool() const {
+        Must(setHow_ != optUnspecified);
+        return option;
+    }
+
+    /// enables or disables the option; updating to 'configured' state
+    void configure(bool beSet) {
+        setHow_ = optConfigured;
+        option = beSet;
+    }
+
+    /// enables or disables the option; updating to 'implicit' state
+    void defaultTo(bool beSet) {
+        Must(setHow_ != optConfigured);
+        setHow_ = optImplicit;
+        option = beSet;
+    }
+
+    /// whether the option was enabled or disabled,
+    /// by squid.conf values resulting in explicit configure() usage.
+    bool configured() const {return setHow_ == optConfigured;}
 
 private:
-    enum { optUnspecified = -1, optDisabled = 0, optEnabled = 1 };
-    int option; ///< configured value or zero
+    SetHow setHow_; ///< how the option was set
+    bool option; ///< specified yes/no value; meaningless if optUnspecified
 };
 
 #endif /* SQUID_YESNONONE_H_ */

=== modified file 'src/tests/stub_cache_cf.cc'
--- src/tests/stub_cache_cf.cc	2016-01-01 00:12:18 +0000
+++ src/tests/stub_cache_cf.cc	2016-01-13 22:14:44 +0000
@@ -12,7 +12,6 @@
 #include "acl/Acl.h"
 #include "ConfigParser.h"
 #include "wordlist.h"
-#include "YesNoNone.h"
 
 #define STUB_API "cache_cf.cc"
 #include "tests/STUB.h"
@@ -28,5 +27,4 @@
 void ConfigParser::ParseUShort(unsigned short *var) STUB
 void dump_acl_access(StoreEntry * entry, const char *name, acl_access * head) STUB
 void dump_acl_list(StoreEntry*, ACLList*) STUB
-YesNoNone::operator void*() const { STUB_NOP; return NULL; }
 

=== modified file 'src/tests/testRock.cc'
--- src/tests/testRock.cc	2016-01-01 00:12:18 +0000
+++ src/tests/testRock.cc	2016-01-14 18:32:16 +0000
@@ -57,6 +57,8 @@
     if (0 > system ("rm -rf " TESTDIR))
         throw std::runtime_error("Failed to clean test work directory");
 
+    Config.memShared.defaultTo(false);
+
     // use current directory for shared segments (on path-based OSes)
     Ipc::Mem::Segment::BasePath = getcwd(cwd,MAXPATHLEN);
     if (Ipc::Mem::Segment::BasePath == NULL)

=== modified file 'src/tests/testStoreController.cc'
--- src/tests/testStoreController.cc	2016-01-01 00:12:18 +0000
+++ src/tests/testStoreController.cc	2016-01-14 18:32:09 +0000
@@ -55,6 +55,8 @@
     if (inited)
         return;
 
+    Config.memShared.defaultTo(false);
+
     Mem::Init();
 
     Config.Store.avgObjectSize = 1024;

=== modified file 'src/tests/testUfs.cc'
--- src/tests/testUfs.cc	2016-01-01 00:12:18 +0000
+++ src/tests/testUfs.cc	2016-01-14 18:31:50 +0000
@@ -65,6 +65,8 @@
     Config.replPolicy = new RemovalPolicySettings;
     Config.replPolicy->type = xstrdup("lru");
 
+    Config.memShared.defaultTo(false);
+
     /* garh garh */
     storeReplAdd("lru", createRemovalPolicy_lru);
 

=== added file 'src/tests/testYesNoNone.cc'
--- src/tests/testYesNoNone.cc	1970-01-01 00:00:00 +0000
+++ src/tests/testYesNoNone.cc	2016-01-14 19:07:43 +0000
@@ -0,0 +1,56 @@
+/*
+ * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#include "squid.h"
+#include "base/YesNoNone.h"
+#include "tests/testYesNoNone.h"
+#include "unitTestMain.h"
+
+#include <stdexcept>
+
+CPPUNIT_TEST_SUITE_REGISTRATION( testYesNoNone );
+
+void
+testYesNoNone::testBasics()
+{
+    // unconfigured, non-implicit
+    {
+        YesNoNone v;
+        CPPUNIT_ASSERT_EQUAL(false, v.configured());
+        // cannot test the value it is 'undefined' and will assert
+    }
+    // implicit dtor test
+
+    // unconfigured, implicit true
+    {
+        YesNoNone v(true);
+        CPPUNIT_ASSERT_EQUAL(false, v.configured());
+        CPPUNIT_ASSERT(v);
+        CPPUNIT_ASSERT_EQUAL(true, static_cast<bool>(v));
+
+        // check explicit setter method
+        v.configure(false);
+        CPPUNIT_ASSERT_EQUAL(true, v.configured());
+        CPPUNIT_ASSERT(!v);
+        CPPUNIT_ASSERT_EQUAL(false, static_cast<bool>(v));
+    }
+
+    // unconfigured, implicit false
+    {
+        YesNoNone v(false);
+        CPPUNIT_ASSERT_EQUAL(false, v.configured());
+        CPPUNIT_ASSERT(!v);
+        CPPUNIT_ASSERT_EQUAL(false, static_cast<bool>(v));
+
+        // check assignment operator
+        v = YesNoNone(true);
+        CPPUNIT_ASSERT_EQUAL(false, v.configured());
+        CPPUNIT_ASSERT(v);
+        CPPUNIT_ASSERT_EQUAL(true, static_cast<bool>(v));
+    }
+}

=== added file 'src/tests/testYesNoNone.h'
--- src/tests/testYesNoNone.h	1970-01-01 00:00:00 +0000
+++ src/tests/testYesNoNone.h	2016-01-11 10:48:11 +0000
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#ifndef SQUID_SRC_TESTS_TESTYESNONONE_H
+#define SQUID_SRC_TESTS_TESTYESNONONE_H
+
+#include <cppunit/extensions/HelperMacros.h>
+
+/*
+ * demonstration test file, as new idioms are made they will
+ * be shown in the testYesNoNone source.
+ */
+
+class testYesNoNone : public CPPUNIT_NS::TestFixture
+{
+    CPPUNIT_TEST_SUITE( testYesNoNone );
+    /* note the statement here and then the actual prototype below */
+    CPPUNIT_TEST( testBasics );
+    CPPUNIT_TEST_SUITE_END();
+
+public:
+
+protected:
+    void testBasics();
+};
+
+#endif /* SQUID_SRC_TESTS_TESTYESNONONE_H */
+

-------------- next part --------------
=== modified file 'doc/release-notes/release-4.sgml'
--- doc/release-notes/release-4.sgml	2016-01-06 16:10:48 +0000
+++ doc/release-notes/release-4.sgml	2016-01-08 08:49:26 +0000
@@ -91,6 +91,13 @@
    to configure the minimum version the TLS negotiation will allow to be used
    when an old TLS version is requested by the remote endpoint.
 
+<p>The system Trusted CA are no longer used by default when verifying client
+   certificates. The <em>cafile=</em> option should be used instead to load
+   the specific CA which signed acceptible client certificates explicitly,
+   even if that CA is one of the system Trusted CA.
+   The <em>tls-default-ca</em> option can be used to restore the old
+   behaviour explicitly if needed.
+
 
 <sect1>MSNT-multi-domain helper removal
 <p>The <em>basic_msnt_multi_domain_auth</em> helper has been removed. The
@@ -161,7 +168,7 @@
 	<p>New option <em>auth-no-keytab</em> to let GSSAPI implementation determine
 	   which Kerberos credentials to use, instead of specifying a keytab.
 	<p>New option <em>tls-min-version=1.N</em> to set minimum TLS version allowed.
-	<p>New option <em>tls-no-default-ca</em> replaces <em>sslflags=NO_DEFAULT_CA</em>
+	<p>New option <em>tls-default-ca</em> replaces <em>sslflags=NO_DEFAULT_CA</em>
 	<p>New option <em>tls-no-npn</em> to disable sending TLS NPN extension.
 	<p>All <em>ssloptions=</em> values for SSLv2 configuration or disabling
 	   have been removed.
@@ -176,18 +183,20 @@
 
 	<tag>http_port</tag>
 	<p>New option <em>tls-min-version=1.N</em> to set minimum TLS version allowed.
-	<p>New option <em>tls-no-default-ca</em> replaces <em>sslflags=NO_DEFAULT_CA</em>
+	<p>New option <em>tls-default-ca</em> replaces <em>sslflags=NO_DEFAULT_CA</em>
 	<p>New option <em>tls-no-npn</em> to disable sending TLS NPN extension.
 	<p>All <em>option=</em> values for SSLv2 configuration or disabling
 	   have been removed.
 	<p>Removed <em>version=</em> option. Use <em>tls-options=</em> instead.
 	<p>Manual squid.conf update may be required on upgrade.
 	<p>Replaced <em>cafile=</em> with <em>tls-cafile=</em> which takes multiple entries.
-	<p>New option <em>tls-no-default-ca</em> replaces <em>sslflags=NO_DEFAULT_CA</em>
+	<p>New option <em>tls-default-ca</em> replaces <em>sslflags=NO_DEFAULT_CA</em>,
+	   the default is also changed to OFF.
 
 	<tag>https_port</tag>
 	<p>New option <em>tls-min-version=1.N</em> to set minimum TLS version allowed.
-	<p>New option <em>tls-no-default-ca</em> replaces <em>sslflags=NO_DEFAULT_CA</em>
+	<p>New option <em>tls-default-ca</em> replaces <em>sslflags=NO_DEFAULT_CA</em>,
+	   the default is also changed to OFF.
 	<p>New option <em>tls-no-npn</em> to disable sending TLS NPN extension.
 	<p>All <em>options=</em> values for SSLv2
 	   configuration or disabling have been removed.
@@ -210,6 +219,8 @@
 	   certificate(s) to verify the server certificate.
 	<p>New <em>tls-crlfile=</em> option to set a file with a CRL to verify the
 	   server certificate.
+	<p>New <em>tls-default-ca</em> option to use the system Trusted CA to
+	   verify the server certificate.
 	<p>New <em>tls-domain=</em> option to verify the server certificate domain.
 
 	<tag>logformat</tag>

=== modified file 'src/cf.data.pre'
--- src/cf.data.pre	2016-01-13 10:10:20 +0000
+++ src/cf.data.pre	2016-01-13 22:28:33 +0000
@@ -2013,8 +2013,8 @@
 				Verify CRL lists for all certificates in the
 				client certificate chain.
 
-	   tls-no-default-ca
-			Do not use the system default Trusted CA.
+	   tls-default-ca[=off]
+			Whether to use the system Trusted CAs. Default is OFF.
 
 	   tls-no-npn	Do not use the TLS NPN extension to advertise HTTP/1.1.
 
@@ -2551,7 +2551,8 @@
 				Don't verify the peer certificate
 				matches the server name
 	
-	no-default-ca	Do not use the system default Trusted CA.
+	default-ca[=off]
+			Whether to use the system Trusted CAs. Default is ON.
 	
 	domain= 	The peer name as advertised in its certificate.
 			Used for verifying the correctness of the received peer
@@ -3326,9 +3327,9 @@
 			See MS KB document Q307347 for details on this header.
 			If set to auto the header will only be added if the
 			request is forwarded as a https:// URL.
-
-	tls-no-default-ca
-			Do not use the system default Trusted CA.	
+	
+	tls-default-ca[=off]
+			Whether to use the system Trusted CAs. Default is ON.
 	
 	tls-no-npn	Do not use the TLS NPN extension to advertise HTTP/1.1.
 
@@ -8527,7 +8528,7 @@
 			the icap server certificate.
 			Use to specify intermediate CA certificate(s) if not sent
 			by the server. Or the full CA chain for the server when
-			using the tls-no-default-ca flag.
+			using the tls-default-ca=off flag.
 			May be repeated to load multiple files.
 
 	tls-capath=...	A directory containing additional CA certificates to
@@ -8546,8 +8547,8 @@
 				Don't verify the icap server certificate
 				matches the server name
 
-	tls-no-default-ca
-			Do no use the system default Trusted CA.
+	tls-default-ca[=off]
+			Whether to use the system Trusted CAs. Default is ON.
 
 	tls-domain=	The icap server name as advertised in it's certificate.
 			Used for verifying the correctness of the received icap

=== modified file 'src/security/PeerOptions.cc'
--- src/security/PeerOptions.cc	2016-01-12 17:35:06 +0000
+++ src/security/PeerOptions.cc	2016-01-13 05:07:30 +0000
@@ -91,8 +91,14 @@
         }
         sslFlags = SBuf(token + 6);
         parsedFlags = parseFlags();
-    } else if (strncmp(token, "no-default-ca", 13) == 0) {
-        flags.tlsDefaultCa = false;
+    } else if (strncmp(token, "default-ca=off", 14) == 0 || strncmp(token, "no-default-ca", 13) == 0) {
+        if (flags.tlsDefaultCa.configured() && flags.tlsDefaultCa)
+            fatalf("ERROR: previous default-ca settings conflict with %s", token);
+        flags.tlsDefaultCa.configure(false);
+    } else if (strncmp(token, "default-ca=on", 13) == 0 || strncmp(token, "default-ca", 10) == 0) {
+        if (flags.tlsDefaultCa.configured() && !flags.tlsDefaultCa)
+            fatalf("ERROR: previous default-ca settings conflict with %s", token);
+        flags.tlsDefaultCa.configure(true);
     } else if (strncmp(token, "domain=", 7) == 0) {
         sslDomain = SBuf(token + 7);
     } else if (strncmp(token, "no-npn", 6) == 0) {
@@ -140,8 +146,14 @@
     if (!sslFlags.isEmpty())
         p->appendf(" %sflags=" SQUIDSBUFPH, pfx, SQUIDSBUFPRINT(sslFlags));
 
-    if (!flags.tlsDefaultCa)
-        p->appendf(" %sno-default-ca", pfx);
+    if (flags.tlsDefaultCa.configured()) {
+        // default ON for peers / upstream servers
+        // default OFF for listening ports
+        if (flags.tlsDefaultCa)
+            p->appendf(" %sdefault-ca", pfx);
+        else
+            p->appendf(" %sdefault-ca=off", pfx);
+    }
 
     if (!flags.tlsNpn)
         p->appendf(" %sno-npn", pfx);
@@ -500,8 +512,10 @@
         if (!found)
             fatalf("Unknown TLS flag '" SQUIDSBUFPH "'", SQUIDSBUFPRINT(tok.remaining()));
         if (found == SSL_FLAG_NO_DEFAULT_CA) {
-            debugs(83, DBG_PARSE_NOTE(2), "UPGRADE WARNING: flags=NO_DEFAULT_CA is deprecated. Use tls-no-default-ca instead.");
-            flags.tlsDefaultCa = false;
+            if (flags.tlsDefaultCa.configured() && flags.tlsDefaultCa)
+                fatal("ERROR: previous default-ca settings conflict with sslflags=NO_DEFAULT_CA");
+            debugs(83, DBG_PARSE_NOTE(2), "WARNING: flags=NO_DEFAULT_CA is deprecated. Use tls-default-ca=off instead.");
+            flags.tlsDefaultCa.configure(false);
         } else
             fl |= found;
     } while (tok.skipOne(delims));

=== modified file 'src/security/PeerOptions.h'
--- src/security/PeerOptions.h	2016-01-01 00:12:18 +0000
+++ src/security/PeerOptions.h	2016-01-13 06:15:24 +0000
@@ -9,6 +9,7 @@
 #ifndef SQUID_SRC_SECURITY_PEEROPTIONS_H
 #define SQUID_SRC_SECURITY_PEEROPTIONS_H
 
+#include "base/YesNoNone.h"
 #include "ConfigParser.h"
 #include "security/KeyData.h"
 
@@ -75,7 +76,7 @@
     std::list<SBuf> caFiles;  ///< paths of files containing trusted Certificate Authority
     Security::CertRevokeList parsedCrl; ///< CRL to use when verifying the remote end certificate
 
-private:
+protected:
     int sslVersion;
 
     /// flags governing Squid internal TLS operations
@@ -83,7 +84,7 @@
         flags_() : tlsDefaultCa(true), tlsNpn(true) {}
 
         /// whether to use the system default Trusted CA when verifying the remote end certificate
-        bool tlsDefaultCa;
+        YesNoNone tlsDefaultCa;
 
         /// whether to use the TLS NPN extension on these connections
         bool tlsNpn;

=== modified file 'src/security/ServerOptions.h'
--- src/security/ServerOptions.h	2016-01-01 00:12:18 +0000
+++ src/security/ServerOptions.h	2016-01-14 18:32:23 +0000
@@ -18,7 +18,11 @@
 class ServerOptions : public PeerOptions
 {
 public:
-    ServerOptions() : PeerOptions() {}
+    ServerOptions() : PeerOptions() {
+        // Bug 4005: dynamic contexts use a lot of memory and it
+        // is more secure to have only a small set of trusted CA.
+        flags.tlsDefaultCa.defaultTo(false);
+    }
     explicit ServerOptions(const Security::ServerOptions &);
     virtual ~ServerOptions() = default;
 



More information about the squid-dev mailing list