From ec514665efd0ea35baf8433f9ab8ae59d070994b Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Fri, 22 Aug 2025 12:26:05 +0200 Subject: [PATCH] [#3860] Checkpoint: todo vendor VALUE --- src/hooks/dhcp/radius/client_attribute.h | 14 +++ src/hooks/dhcp/radius/client_dictionary.cc | 6 +- src/hooks/dhcp/radius/radius_parsers.cc | 13 ++- .../dhcp/radius/tests/config_unittests.cc | 105 +++++++++++++++++- .../dhcp/radius/tests/dictionary_unittests.cc | 20 ++-- 5 files changed, 139 insertions(+), 19 deletions(-) diff --git a/src/hooks/dhcp/radius/client_attribute.h b/src/hooks/dhcp/radius/client_attribute.h index db00b2e2e5..8d2879b1ca 100644 --- a/src/hooks/dhcp/radius/client_attribute.h +++ b/src/hooks/dhcp/radius/client_attribute.h @@ -162,6 +162,7 @@ public: /// @brief From Vendor ID and string data with type. /// /// @note Requires the type to be of a standard vsa attribute. + /// @note Used only in unit tests. /// /// @param type type of attribute. /// @param vendor vendor id. @@ -190,6 +191,8 @@ public: /// @brief Returns text representation of the attribute. /// + /// @note Used for logs. + /// /// @param indent number of spaces before printing text. /// @return string with text representation. virtual std::string toText(size_t indent = 0) const = 0; @@ -260,10 +263,15 @@ public: /// @brief Type. const uint8_t type_; + /// @note No need for a vendor member as vendor attributes + /// are only temporary. + private: /// @brief From text with definition. /// + /// Dispatch over the value type. + /// /// @param def pointer to attribute definition. /// @param value textual value. /// @return pointer to the attribute. @@ -273,6 +281,8 @@ private: /// @brief From bytes with definition. /// + /// Dispatch over the value type. + /// /// @param def pointer to attribute definition. /// @param value binary value. /// @return pointer to the attribute. @@ -805,6 +815,8 @@ private: /// @brief Collection of attributes. +/// +/// Designed to not handle vendor attributes so can be keyed by type only. class Attributes : public data::CfgToElement { public: @@ -891,6 +903,8 @@ public: /// @brief Returns text representation of the collection. /// + /// @note Used for logs. + /// /// @param indent number of spaces before printing text. /// @return string with text representation. std::string toText(size_t indent = 0) const; diff --git a/src/hooks/dhcp/radius/client_dictionary.cc b/src/hooks/dhcp/radius/client_dictionary.cc index 4a48c27b4c..2706111460 100644 --- a/src/hooks/dhcp/radius/client_dictionary.cc +++ b/src/hooks/dhcp/radius/client_dictionary.cc @@ -254,11 +254,15 @@ AttrDefs::parseLine(const string& line, uint32_t& vendor, unsigned int depth) { } // Integer constant definition. if (tokens[0] == "VALUE") { + if (vendor != 0) { + // Ignore vendor constant definitions. + return; + } if (tokens.size() != 4) { isc_throw(Unexpected, "expected 4 tokens, got " << tokens.size()); } const string& attr_str = tokens[1]; - AttrDefPtr attr = getByName(attr_str); + AttrDefPtr attr = getByName(attr_str/*, vendor*/); if (!attr) { isc_throw(Unexpected, "unknown attribute '" << attr_str << "'"); } diff --git a/src/hooks/dhcp/radius/radius_parsers.cc b/src/hooks/dhcp/radius/radius_parsers.cc index 261a8fd448..dc4526bbe8 100644 --- a/src/hooks/dhcp/radius/radius_parsers.cc +++ b/src/hooks/dhcp/radius/radius_parsers.cc @@ -519,7 +519,15 @@ RadiusAttributeParser::parse(const RadiusServicePtr& service, // vendor. uint32_t vendor = 0; - const string& vendor_txt = getString(attr, "vendor"); + const ConstElementPtr& vendor_elem = attr->get("vendor"); + if (!vendor_elem) { + // Should not happen as it is added by setDefaults. + isc_throw(Unexpected, "no vendor parameter"); + } else if (vendor_elem->getType() != Element::string) { + // Expected to be a common error. + isc_throw(TypeError, "vendor parameter must be a string"); + } + const string& vendor_txt = vendor_elem->stringValue(); if (!vendor_txt.empty()) { IntCstDefPtr vendor_cst = AttrDefs::instance().getByName(PW_VENDOR_SPECIFIC, vendor_txt); @@ -534,7 +542,8 @@ RadiusAttributeParser::parse(const RadiusServicePtr& service, } vendor = static_cast(val); } catch (...) { - isc_throw(ConfigError, "can't parse vendor " << vendor_txt); + isc_throw(ConfigError, "can't parse vendor '" + << vendor_txt << "'"); } } } diff --git a/src/hooks/dhcp/radius/tests/config_unittests.cc b/src/hooks/dhcp/radius/tests/config_unittests.cc index 4753d208e4..1a9f85943c 100644 --- a/src/hooks/dhcp/radius/tests/config_unittests.cc +++ b/src/hooks/dhcp/radius/tests/config_unittests.cc @@ -53,12 +53,30 @@ public: virtual ~ConfigTest() { impl_.reset(); HostDataSourceFactory::deregisterFactory("cache"); + static_cast(remove(TEST_FILE)); + } + + /// @brief writes specified content to a file. + /// + /// @param file_name name of file to be written. + /// @param content content to be written to file. + void writeFile(const std::string& file_name, const std::string& content) { + static_cast(remove(file_name.c_str())); + ofstream out(file_name.c_str(), ios::trunc); + EXPECT_TRUE(out.is_open()); + out << content; + out.close(); } /// @brief Radius implementation. RadiusImpl& impl_; + + /// Name of a dictionary file used during tests. + static const char* TEST_FILE; }; +const char* ConfigTest::TEST_FILE = "test-dictonary"; + // Verify that a configuration must be a map. TEST_F(ConfigTest, notMap) { ElementPtr config = Element::createList(); @@ -177,6 +195,15 @@ TEST_F(ConfigTest, badDictionary) { expected += "No such file or directory"; EXPECT_THROW_MSG(impl_.init(config), ConfigError, expected); + impl_.reset(); + string dict = "BEGIN-VENDOR 1234\n"; + writeFile(TEST_FILE, dict); + config = Element::createMap(); + config->set("dictionary", Element::create(string(TEST_FILE))); + expected = "vendor definitions were not properly closed: "; + expected += "vendor 1234 is still open"; + EXPECT_THROW_MSG(impl_.init(config), ConfigError, expected); + impl_.reset(); config = Element::createMap(); config->set("dictionary", Element::create(string(TEST_DICTIONARY))); @@ -730,6 +757,12 @@ TEST_F(ConfigTest, attribute) { ElementPtr config = Element::createMap(); EXPECT_NO_THROW(impl_.init(config)); + // Add vendor too. + IntCstDefPtr cstv(new IntCstDef(PW_VENDOR_SPECIFIC, "DSL-Forum", 3561)); + ASSERT_NO_THROW(AttrDefs::instance().add(cstv)); + AttrDefPtr defv(new AttrDef(1, "Agent-Circuit-Id", PW_TYPE_STRING, 3561)); + ASSERT_NO_THROW(AttrDefs::instance().add(defv)); + RadiusServicePtr srv(new RadiusService("test")); ASSERT_TRUE(srv); RadiusAttributeParser parser; @@ -740,6 +773,17 @@ TEST_F(ConfigTest, attribute) { "get(string) called on a non-map Element"); EXPECT_TRUE(srv->attributes_.empty()); + // Vendor must be a string. + attr = Element::createMap(); + attr->set("vendor", Element::create(1234)); + EXPECT_THROW_MSG(parser.parse(srv, attr), TypeError, + "vendor parameter must be a string"); + + // Named vendor must be known. + attr->set("vendor", Element::create("foobar")); + EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, + "can't parse vendor 'foobar'"); + // Name or type is required. attr = Element::createMap(); attr->set("data", Element::create("foobar")); @@ -752,12 +796,33 @@ TEST_F(ConfigTest, attribute) { EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, "attribute 'No-Such-Attribute' is unknown"); + attr->set("name", Element::create("User-Name")); + attr->set("vendor", Element::create("DSL-Forum")); + EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, + "attribute 'User-Name' in vendor 'DSL-Forum' is unknown"); + attr->set("vendor", Element::create("1234")); + EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, + "attribute 'User-Name' in vendor '1234' is unknown"); + attr->set("vendor", Element::create("")); + // Type and name must match. attr->set("name", Element::create("User-Name")); attr->set("type", Element::create(123)); EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, "'User-Name' attribute has type 1, not 123"); + attr->set("name", Element::create("Agent-Circuit-Id")); + attr->set("vendor", Element::create("DSL-Forum")); + string expected = "'Agent-Circuit-Id' attribute in vendor 'DSL-Forum' "; + expected += "has type 1, not 123"; + EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, expected); + attr->set("vendor", Element::create("3561")); + expected = "'Agent-Circuit-Id' attribute in vendor '3561' "; + expected += "has type 1, not 123"; + EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, expected); + attr->set("name", Element::create("User-Name")); + attr->set("vendor", Element::create("")); + // Type must be between 0 and 255. attr = Element::createMap(); attr->set("data", Element::create("foobar")); @@ -777,6 +842,12 @@ TEST_F(ConfigTest, attribute) { attr->set("type", Element::create(234)); EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, "attribute type 234 is unknown"); + attr->set("vendor", Element::create("DSL-Forum")); + EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, + "attribute type 234 in vendor 'DSL-Forum' is unknown"); + attr->set("vendor", Element::create("1234")); + EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, + "attribute type 234 in vendor '1234' is unknown"); // Set a type. attr = Element::createMap(); @@ -786,21 +857,43 @@ TEST_F(ConfigTest, attribute) { // Get the option to look at into. EXPECT_FALSE(srv->attributes_.empty()); + EXPECT_EQ(1, srv->attributes_.size()); + EXPECT_TRUE(srv->attributes_.getDef(1)); EXPECT_FALSE(srv->attributes_.getExpr(1)); EXPECT_EQ("", srv->attributes_.getTest(1)); - const Attributes& attrs = srv->attributes_.getAll(); - ASSERT_EQ(1, attrs.size()); - const ConstAttributePtr& first = *attrs.cbegin(); - ASSERT_TRUE(first); - EXPECT_EQ("User-Name='foobar'", first->toText()); + ConstAttributePtr got = srv->attributes_.get(1); + ASSERT_TRUE(got); + EXPECT_EQ("User-Name='foobar'", got->toText()); // Another way to check. - string expected = "[ { " + expected = "[ { " " \"name\": \"User-Name\", " " \"type\": 1, " " \"data\": \"foobar\" } ]"; runToElementTest(expected, srv->attributes_); + // Check with a vendor. + srv->attributes_.clear(); + attr = Element::createMap(); + attr->set("vendor", Element::create("DSL-Forum")); + attr->set("data", Element::create("foobar")); + attr->set("type", Element::create(1)); + EXPECT_NO_THROW(parser.parse(srv, attr)); + EXPECT_FALSE(srv->attributes_.empty()); + EXPECT_EQ(1, srv->attributes_.size()); + EXPECT_TRUE(srv->attributes_.getDef(1, 3561)); + EXPECT_FALSE(srv->attributes_.getExpr(1, 3561)); + EXPECT_EQ("", srv->attributes_.getTest(1, 3561)); + got = srv->attributes_.get(1, 3561); + ASSERT_TRUE(got); + EXPECT_EQ("Vendor-Specific=[3561]0x0108666F6F626172", got->toText()); + expected = "[ { " + " \"name\": \"Vendor-Specific\", " + " \"type\": 26, " + " \"vendor\": \"3561\", " + " \"vsa-raw\": \"0108666F6F626172\" } ]"; + runToElementTest(expected, srv->attributes_); + // Vendor-Specific (26) does not support textual data. srv->attributes_.clear(); attr = Element::createMap(); diff --git a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc index 9b16e7051d..436a02722c 100644 --- a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc +++ b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc @@ -47,7 +47,7 @@ public: /// @brief Destructor. virtual ~DictionaryTest() { AttrDefs::instance().clear(); - static_cast(remove(TEST_DICT)); + static_cast(remove(TEST_FILE)); } /// @brief Parse a line. @@ -95,10 +95,10 @@ public: } /// Name of a dictionary file used during tests. - static const char* TEST_DICT; + static const char* TEST_FILE; }; -const char* DictionaryTest::TEST_DICT = "test-dict"; +const char* DictionaryTest::TEST_FILE = "test-dictionary"; // Verifies standards definitions can be read from the dictionary. TEST_F(DictionaryTest, standard) { @@ -420,15 +420,15 @@ TEST_F(DictionaryTest, include) { // Verifies the $INCLUDE entry can't eat the stack. TEST_F(DictionaryTest, includeLimit) { - string include = "$INCLUDE " + string(TEST_DICT) + "\n"; - writeFile(TEST_DICT, include); + string include = "$INCLUDE " + string(TEST_FILE) + "\n"; + writeFile(TEST_FILE, include); string expected = "Too many nested $INCLUDE "; - expected += "at line 1 in dictionary 'test-dict', "; - expected += "at line 1 in dictionary 'test-dict', "; - expected += "at line 1 in dictionary 'test-dict', "; - expected += "at line 1 in dictionary 'test-dict', "; + expected += "at line 1 in dictionary 'test-dictionary', "; + expected += "at line 1 in dictionary 'test-dictionary', "; + expected += "at line 1 in dictionary 'test-dictionary', "; + expected += "at line 1 in dictionary 'test-dictionary', "; expected += "at line 1"; - EXPECT_THROW_MSG(parseLine(string("$INCLUDE ") + string(TEST_DICT)), + EXPECT_THROW_MSG(parseLine(string("$INCLUDE ") + string(TEST_FILE)), BadValue, expected); } -- 2.47.3