Reduce code duplication in ACLCertificateData::parse() (#1242)

ACLs ca_cert and user_cert already prohibit key changes. And
server_cert_fingerprint ACL only supports one option name spelling, so,
while "-sha1" is not, technically, a "key", it still cannot be
"changed". Adjust SetAclKey() (added in recent commit 4a3b853) and reuse
it to implement those existing "key change" checks.

diff --git a/src/ConfigParser.cc b/src/ConfigParser.cc
index dee50561fee2a37f21a38a6c23d0f58fb3eac328..c65124ece68ee163691c526cd225f5a78aa1a614 100644 (file)
--- a/src/ConfigParser.cc
+++ b/src/ConfigParser.cc
@@ -504,35 +504,6 @@ ConfigParser::regex(const char *expectedRegexDescription)
     return std::unique_ptr<RegexPattern>(new RegexPattern(pattern, flags));
 }
 
-void
-ConfigParser::SetAclKey(SBuf &keyStorage, const char *keyParameterName)
-{
-    extern const char *AclMatchedName;
-
-    const auto newKey = strtokFile();
-    if (!newKey) {
-        throw TextException(ToSBuf("An acl declaration is missing a ", keyParameterName,
-                                   Debug::Extra, "ACL name: ", AclMatchedName),
-                            Here());
-    }
-
-    if (keyStorage.isEmpty()) {
-        keyStorage = newKey;
-        return;
-    }
-
-    if (keyStorage.caseCmp(newKey) == 0)
-        return; // no change
-
-    throw TextException(ToSBuf("Attempt to change the value of the ", keyParameterName, " argument in a subsequent acl declaration:",
-                               Debug::Extra, "previously seen value: ", keyStorage,
-                               Debug::Extra, "new/conflicting value: ", newKey,
-                               Debug::Extra, "ACL name: ", AclMatchedName,
-                               Debug::Extra, "advice: Use a dedicated ACL name for each distinct ", keyParameterName,
-                               " (and group those ACLs together using an 'any-of' ACL)."),
-                        Here());
-}
-
 CachePeer &
 ConfigParser::cachePeer(const char *peerNameTokenDescription)
 {

diff --git a/src/ConfigParser.h b/src/ConfigParser.h
index 54693b021ecdc087f08c057aa9ec080d46bbcf31..2580b15887c266488cb4cf62599103ebfd3870eb 100644 (file)
--- a/src/ConfigParser.h
+++ b/src/ConfigParser.h
@@ -129,13 +129,6 @@ public:
      */
     static bool NextKvPair(char * &key, char * &value);
 
-    // TODO: Convert into a non-static method after exposing the current parser.
-    /// Extract, validate, and store the ACL key parameter for ACL types
-    /// declared using "acl aclname type key argument..." declaration that
-    /// require unique key values for each aclname+type combination.
-    /// Key comparison is case-insensitive.
-    static void SetAclKey(SBuf &keyStorage, const char *keyParameterName);
-
     /**
      * Preview the next token. The next NextToken() and strtokFile() call
      * will return the same token.

diff --git a/src/acl/Acl.cc b/src/acl/Acl.cc
index 5972585e5b280904d1b4b896976e2ac7619257e5..f466c6712ee41a26224ad23f31c6367c67bb7745 100644 (file)
--- a/src/acl/Acl.cc
+++ b/src/acl/Acl.cc
@@ -75,6 +75,32 @@ Acl::RegisterMaker(TypeName typeName, Maker maker)
     TheMakers().emplace(typeName, maker);
 }
 
+void
+Acl::SetKey(SBuf &keyStorage, const char *keyParameterName, const char *newKey)
+{
+    if (!newKey) {
+        throw TextException(ToSBuf("An acl declaration is missing a ", keyParameterName,
+                                   Debug::Extra, "ACL name: ", AclMatchedName),
+                            Here());
+    }
+
+    if (keyStorage.isEmpty()) {
+        keyStorage = newKey;
+        return;
+    }
+
+    if (keyStorage.caseCmp(newKey) == 0)
+        return; // no change
+
+    throw TextException(ToSBuf("Attempt to change the value of the ", keyParameterName, " argument in a subsequent acl declaration:",
+                               Debug::Extra, "previously seen value: ", keyStorage,
+                               Debug::Extra, "new/conflicting value: ", newKey,
+                               Debug::Extra, "ACL name: ", AclMatchedName,
+                               Debug::Extra, "advice: Use a dedicated ACL name for each distinct ", keyParameterName,
+                               " (and group those ACLs together using an 'any-of' ACL)."),
+                        Here());
+}
+
 void *
 ACL::operator new (size_t)
 {

diff --git a/src/acl/Acl.h b/src/acl/Acl.h
index 0f58f7df33889f3a3665327ddfb184f0eebd49db..d30e0ac7fe20bc4f805172455808767c922aa643 100644 (file)
--- a/src/acl/Acl.h
+++ b/src/acl/Acl.h
@@ -30,6 +30,12 @@ typedef ACL *(*Maker)(TypeName typeName);
 /// use the given ACL Maker for all ACLs of the named type
 void RegisterMaker(TypeName typeName, Maker maker);
 
+/// Validate and store the ACL key parameter for ACL types
+/// declared using "acl aclname type key argument..." declaration that
+/// require unique key values (if any) for each aclname+type combination.
+/// Key comparison is case-insensitive.
+void SetKey(SBuf &keyStorage, const char *keyParameterName, const char *newKey);
+
 } // namespace Acl
 
 /// A configurable condition. A node in the ACL expression tree.

diff --git a/src/acl/CertificateData.cc b/src/acl/CertificateData.cc
index 50f602d0ec501f4e5b6b84a698261e10870f5b43..64ed18828149c9ee887cc39e16f8429ca87cbfba 100644 (file)
--- a/src/acl/CertificateData.cc
+++ b/src/acl/CertificateData.cc
@@ -16,7 +16,10 @@
 #include "debug/Stream.h"
 #include "wordlist.h"
 
-ACLCertificateData::ACLCertificateData(Ssl::GETX509ATTRIBUTE *sslStrategy, const char *attrs, bool optionalAttr) : validAttributesStr(attrs), attributeIsOptional(optionalAttr), attribute (nullptr), values (), sslAttributeCall (sslStrategy)
+ACLCertificateData::ACLCertificateData(Ssl::GETX509ATTRIBUTE * const sslStrategy, const char * const attrs, const bool optionalAttr):
+    validAttributesStr(attrs),
+    attributeIsOptional(optionalAttr),
+    sslAttributeCall(sslStrategy)
 {
     if (attrs) {
         size_t current = 0;
@@ -37,11 +40,6 @@ xRefFree(T &thing)
     xfree (thing);
 }
 
-ACLCertificateData::~ACLCertificateData()
-{
-    safe_free (attribute);
-}
-
 template<class T>
 inline int
 splaystrcmp (T&l, T&r)
@@ -55,8 +53,8 @@ ACLCertificateData::match(X509 *cert)
     if (!cert)
         return 0;
 
-    char const *value = sslAttributeCall(cert, attribute);
-    debugs(28, 6, (attribute ? attribute : "value") << "=" << value);
+    const auto value = sslAttributeCall(cert, attribute.c_str());
+    debugs(28, 6, (attribute.isEmpty() ? attribute.c_str() : "value") << "=" << value);
     if (value == nullptr)
         return 0;
 
@@ -68,7 +66,7 @@ ACLCertificateData::dump() const
 {
     SBufList sl;
     if (validAttributesStr)
-        sl.push_back(SBuf(attribute));
+        sl.push_back(attribute);
 
     sl.splice(sl.end(),values.dump());
     return sl;
@@ -107,14 +105,10 @@ ACLCertificateData::parse()
                 return;
             }
 
-            /* an acl must use consistent attributes in all config lines */
-            if (attribute) {
-                if (strcasecmp(newAttribute, attribute) != 0) {
-                    debugs(28, DBG_CRITICAL, "FATAL: An acl must use consistent attributes in all config lines (" << newAttribute << "!=" << attribute << ").");
-                    self_destruct();
-                    return;
-                }
-            } else {
+            // If attribute has been set already, then we do not need to call OBJ_create()
+            // below because either we did that for the same attribute when we set it, or
+            // Acl::SetKey() below will reject this new/different attribute spelling.
+            if (attribute.isEmpty()) {
                 if (strcasecmp(newAttribute, "DN") != 0) {
                     int nid = OBJ_txt2nid(newAttribute);
                     if (nid == 0) {
@@ -136,8 +130,9 @@ ACLCertificateData::parse()
                         return;
                     }
                 }
-                attribute = xstrdup(newAttribute);
             }
+
+            Acl::SetKey(attribute, "SSL certificate attribute", newAttribute);
         }
     }
 

diff --git a/src/acl/CertificateData.h b/src/acl/CertificateData.h
index 78171298c8fa4a434b035ab0fcadabd50586c773..d9622976fa4a9c5efcc4c4c7213b8c32b151df06 100644 (file)
--- a/src/acl/CertificateData.h
+++ b/src/acl/CertificateData.h
@@ -23,7 +23,6 @@ class ACLCertificateData : public ACLData<X509 *>
 
 public:
     ACLCertificateData(Ssl::GETX509ATTRIBUTE *, const char *attributes, bool optionalAttr = false);
-    ~ACLCertificateData() override;
     bool match(X509 *) override;
     SBufList dump() const override;
     void parse() override;
@@ -38,7 +37,7 @@ public:
     std::list<std::string> validAttributes;
     /// True if the attribute is optional (-xxx options)
     bool attributeIsOptional;
-    char *attribute;
+    SBuf attribute;
     ACLStringData values;
 
 private:

diff --git a/src/acl/HttpHeaderData.cc b/src/acl/HttpHeaderData.cc
index 7301f2688763978bf9bbd1f4f255984a57526f35..2db401e1436bf9844045f285310983c3a3f7d22b 100644 (file)
--- a/src/acl/HttpHeaderData.cc
+++ b/src/acl/HttpHeaderData.cc
@@ -75,7 +75,7 @@ ACLHTTPHeaderData::lineOptions()
 void
 ACLHTTPHeaderData::parse()
 {
-    ConfigParser::SetAclKey(hdrName, "header-name");
+    Acl::SetKey(hdrName, "header-name", ConfigParser::strtokFile());
     hdrId = Http::HeaderLookupTable.lookup(hdrName).id;
     regex_rule->parse();
 }

diff --git a/src/acl/NoteData.cc b/src/acl/NoteData.cc
index c8f91f17199f845cd403219a32f30590677351b6..b959649f1b35e1a50b2c1af55f354b8866e39137 100644 (file)
--- a/src/acl/NoteData.cc
+++ b/src/acl/NoteData.cc
@@ -47,7 +47,7 @@ ACLNoteData::dump() const
 void
 ACLNoteData::parse()
 {
-    ConfigParser::SetAclKey(name, "annotation name");
+    Acl::SetKey(name, "annotation name", ConfigParser::strtokFile());
     values->parse();
 }