From: Eduard Bagdasaryan Date: Sat, 28 Jan 2023 05:34:27 +0000 (+0000) Subject: Reduce code duplication in ACLCertificateData::parse() (#1242) X-Git-Tag: SQUID_6_0_1~30 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=0b5786d3ad61e139d0f533450b64d6376614a26a;p=thirdparty%2Fsquid.git 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 dee50561fe..c65124ece6 100644 --- a/src/ConfigParser.cc +++ b/src/ConfigParser.cc @@ -504,35 +504,6 @@ ConfigParser::regex(const char *expectedRegexDescription) return std::unique_ptr(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 54693b021e..2580b15887 100644 --- 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 5972585e5b..f466c6712e 100644 --- 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 0f58f7df33..d30e0ac7fe 100644 --- 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 50f602d0ec..64ed188281 100644 --- 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 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 78171298c8..d9622976fa 100644 --- a/src/acl/CertificateData.h +++ b/src/acl/CertificateData.h @@ -23,7 +23,6 @@ class ACLCertificateData : public ACLData 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 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 7301f26887..2db401e143 100644 --- 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 c8f91f1719..b959649f1b 100644 --- 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(); }