[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