From cb424a2659bb5476ce4f01eda9da41279b30eb79 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Wed, 20 Aug 2025 16:02:46 +0200 Subject: [PATCH 01/13] [#3860] Added $INCLUDE --- src/hooks/dhcp/radius/client_dictionary.cc | 24 +++++-- src/hooks/dhcp/radius/client_dictionary.h | 15 +++-- .../dhcp/radius/tests/dictionary_unittests.cc | 63 +++++++++++++++++-- 3 files changed, 87 insertions(+), 15 deletions(-) diff --git a/src/hooks/dhcp/radius/client_dictionary.cc b/src/hooks/dhcp/radius/client_dictionary.cc index c5c4aab92a..c247437ef0 100644 --- a/src/hooks/dhcp/radius/client_dictionary.cc +++ b/src/hooks/dhcp/radius/client_dictionary.cc @@ -180,7 +180,7 @@ AttrDefs::add(IntCstDefPtr def) { } void -AttrDefs::parseLine(const string& line) { +AttrDefs::parseLine(const string& line, unsigned int depth) { // Ignore empty lines. if (line.empty()) { return; @@ -196,6 +196,14 @@ AttrDefs::parseLine(const string& line) { if (tokens.empty()) { return; } + // $INCLUDE include. + if (tokens[0] == "$INCLUDE") { + if (tokens.size() != 2) { + isc_throw(Unexpected, "expected 2 tokens, got " << tokens.size()); + } + readDictionary(tokens[1], depth + 1); + return; + } // Attribute definition. if (tokens[0] == "ATTRIBUTE") { if (tokens.size() != 4) { @@ -255,7 +263,10 @@ AttrDefs::parseLine(const string& line) { } void -AttrDefs::readDictionary(const string& path) { +AttrDefs::readDictionary(const string& path, unsigned depth) { + if (depth >= 5) { + isc_throw(BadValue, "Too many nested $INCLUDE"); + } ifstream ifs(path); if (!ifs.is_open()) { isc_throw(BadValue, "can't open dictionary '" << path << "': " @@ -265,23 +276,24 @@ AttrDefs::readDictionary(const string& path) { isc_throw(BadValue, "bad dictionary '" << path << "'"); } try { - readDictionary(ifs); + readDictionary(ifs, depth); ifs.close(); } catch (const exception& ex) { ifs.close(); - isc_throw(BadValue, ex.what() << " in dictionary '" << path << "'"); + isc_throw(BadValue, ex.what() << " in dictionary '" << path << "'" + << (depth > 0 ? "," : "")); } } void -AttrDefs::readDictionary(istream& is) { +AttrDefs::readDictionary(istream& is, unsigned int depth) { size_t lines = 0; string line; try { while (is.good()) { ++lines; getline(is, line); - parseLine(line); + parseLine(line, depth); } if (!is.eof()) { isc_throw(BadValue, "I/O error: " << strerror(errno)); diff --git a/src/hooks/dhcp/radius/client_dictionary.h b/src/hooks/dhcp/radius/client_dictionary.h index c949cc5dcc..c3b16e8edc 100644 --- a/src/hooks/dhcp/radius/client_dictionary.h +++ b/src/hooks/dhcp/radius/client_dictionary.h @@ -223,18 +223,22 @@ class AttrDefs : public boost::noncopyable { /// @brief Read a dictionary from a file. /// /// Fills attribute and integer constant definition tables from - /// a dictionary file. + /// a dictionary file. Recursion depth is initialized to 0, + /// incremented by includes and limited to 5. /// /// @param path dictionary file path. - void readDictionary(const std::string& path); + /// @param depth recursion depth. + void readDictionary(const std::string& path, unsigned int depth = 0); /// @brief Read a dictionary from an input stream. /// /// Fills attribute and integer constant definition tables from - /// a dictionary input stream. + /// a dictionary input stream. Recursion depth is initialized to 0, + /// incremented by includes and limited to 5. /// /// @param is input stream. - void readDictionary(std::istream& is); + /// @param depth recursion depth. + void readDictionary(std::istream& is, unsigned int depth = 0); /// @brief Check if a list of standard attribute definitions /// are available and correct. @@ -255,7 +259,8 @@ class AttrDefs : public boost::noncopyable { /// @brief Parse a dictionary line. /// /// @param line line to parse. - void parseLine(const std::string& line); + /// @param depth recursion depth. + void parseLine(const std::string& line, unsigned int depth); /// @brief Attribute definition container. AttrDefContainer container_; diff --git a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc index addfa28ae2..dc39ea9cb2 100644 --- a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc +++ b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc @@ -47,29 +47,49 @@ class DictionaryTest : public ::testing::Test { /// @brief Destructor. virtual ~DictionaryTest() { AttrDefs::instance().clear(); + static_cast(remove(TEST_DICT)); } /// @brief Parse a line. /// /// @param line line to parse. - void parseLine(const string& line) { + /// @param depth recursion depth. + void parseLine(const string& line, unsigned int depth = 0) { istringstream is(line + "\n"); - AttrDefs::instance().readDictionary(is); + AttrDefs::instance().readDictionary(is, depth); } /// @brief Parse a list of lines. /// /// @param lines list of lines. - void parseLines(const list& lines) { + /// @param depth recursion depth. + void parseLines(const list& lines, unsigned int depth = 0) { string content; for (auto const& line : lines) { content += line + "\n"; } istringstream is(content); - AttrDefs::instance().readDictionary(is); + AttrDefs::instance().readDictionary(is, depth); } + + /// @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(); + } + + /// Name of a dictionary file used during tests. + static const char* TEST_DICT; }; +const char* DictionaryTest::TEST_DICT = "test-dict"; + // Verifies standards definitions can be read from the dictionary. TEST_F(DictionaryTest, standard) { ASSERT_NO_THROW_LOG(AttrDefs::instance().readDictionary(TEST_DICTIONARY)); @@ -213,6 +233,41 @@ TEST_F(DictionaryTest, hookAttributes) { checkStandardDefs(RadiusConfigParser::USED_STANDARD_ATTR_DEFS)); } +// Verifies the $INCLUDE entry. +TEST_F(DictionaryTest, include) { + list include; + include.push_back("# Including the dictonary"); + include.push_back(string("$INCLUDE ") + string(TEST_DICTIONARY)); + include.push_back("# Dictionary included"); + // include.push_back("VALUE Vendor-Specific ISC 2495"); + include.push_back("VALUE ARAP-Security ISC 2495"); + EXPECT_NO_THROW_LOG(parseLines(include)); + EXPECT_NO_THROW_LOG(AttrDefs::instance(). + checkStandardDefs(RadiusConfigParser::USED_STANDARD_ATTR_DEFS)); + // auto isc = AttrDefs::instance().getByName(PW_VENDOR_SPECIFIC, "ISC"); + auto isc = AttrDefs::instance().getByName(PW_ARAP_SECURITY, "ISC"); + ASSERT_TRUE(isc); + EXPECT_EQ(2495, isc->value_); + + // max depth is 5. + EXPECT_THROW_MSG(parseLines(include, 4), BadValue, + "Too many nested $INCLUDE at line 2"); +} + +// 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 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"; + EXPECT_THROW_MSG(parseLine(string("$INCLUDE ") + string(TEST_DICT)), + BadValue, expected); +} + namespace { // RAII device freeing the glob buffer when going out of scope. From 438f7b8fcd7f521271862ce64bc3229d4d2952da Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Wed, 20 Aug 2025 22:15:20 +0200 Subject: [PATCH 02/13] [#3860] Added VENDOR --- doc/sphinx/arm/ext-radius.rst | 2 +- src/hooks/dhcp/radius/client_dictionary.cc | 28 +++++++++ src/hooks/dhcp/radius/client_dictionary.h | 2 + src/hooks/dhcp/radius/data/dictionary | 5 ++ .../dhcp/radius/tests/dictionary_unittests.cc | 59 +++++++++++++++++-- 5 files changed, 89 insertions(+), 7 deletions(-) diff --git a/doc/sphinx/arm/ext-radius.rst b/doc/sphinx/arm/ext-radius.rst index 7e4d038a74..ca1c2b4c65 100644 --- a/doc/sphinx/arm/ext-radius.rst +++ b/doc/sphinx/arm/ext-radius.rst @@ -550,7 +550,7 @@ RADIUS dictionary. There are differences: - Yes - - No + - since Kea 3.1.1 * - Support for Vendor Attributes diff --git a/src/hooks/dhcp/radius/client_dictionary.cc b/src/hooks/dhcp/radius/client_dictionary.cc index c247437ef0..01f7b9b641 100644 --- a/src/hooks/dhcp/radius/client_dictionary.cc +++ b/src/hooks/dhcp/radius/client_dictionary.cc @@ -172,6 +172,12 @@ AttrDefs::add(IntCstDefPtr def) { // Duplicate: ignore. return; } + if (def->type_ == PW_VENDOR_SPECIFIC) { + // Vendor id special case. + isc_throw(BadValue, "Illegal vendor id redefinition of '" + << def->name_ << "' value " << (*it)->value_ + << " by " << def->value_); + } isc_throw(BadValue, "Illegal integer constant redefinition of '" << def->name_ << "' for attribute '" << getName(def->type_) << "' value " << (*it)->value_ << " by " << def->value_); @@ -259,6 +265,28 @@ AttrDefs::parseLine(const string& line, unsigned int depth) { add(def); return; } + // Vendor id definition. + if (tokens[0] == "VENDOR") { + if (tokens.size() != 3) { + isc_throw(Unexpected, "expected 3 tokens, got " << tokens.size()); + } + const string& name = tokens[1]; + const string& value_str = tokens[2]; + uint32_t value = 0; + try { + int64_t val = boost::lexical_cast(value_str); + if ((val < numeric_limits::min()) || + (val > numeric_limits::max())) { + isc_throw(Unexpected, "not 32 bit " << value_str); + } + value = static_cast(val); + } catch (...) { + isc_throw(Unexpected, "can't parse integer value " << value_str); + } + IntCstDefPtr def(new IntCstDef(PW_VENDOR_SPECIFIC, name, value)); + add(def); + return; + } isc_throw(Unexpected, "unknown dictionary entry '" << tokens[0] << "'"); } diff --git a/src/hooks/dhcp/radius/client_dictionary.h b/src/hooks/dhcp/radius/client_dictionary.h index c3b16e8edc..21ee11b9b1 100644 --- a/src/hooks/dhcp/radius/client_dictionary.h +++ b/src/hooks/dhcp/radius/client_dictionary.h @@ -79,6 +79,8 @@ typedef boost::shared_ptr AttrDefPtr; typedef std::list AttrDefList; /// @brief RADIUS integer constant definitions. +/// +/// Include vendor ids with Vendor-Specific attribute. class IntCstDef { public: diff --git a/src/hooks/dhcp/radius/data/dictionary b/src/hooks/dhcp/radius/data/dictionary index 6e252bc2df..f06dde01b3 100644 --- a/src/hooks/dhcp/radius/data/dictionary +++ b/src/hooks/dhcp/radius/data/dictionary @@ -252,3 +252,8 @@ VALUE Acct-Terminate-Cause Service-Unavailable 15 VALUE Acct-Terminate-Cause Callback 16 VALUE Acct-Terminate-Cause User-Error 17 VALUE Acct-Terminate-Cause Host-Request 18 + +# Examples + +#$INCLUDE foobar +#VENDOR DSL-Forum 3561 diff --git a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc index dc39ea9cb2..2fbd014bc9 100644 --- a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc +++ b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc @@ -131,8 +131,20 @@ TEST_F(DictionaryTest, parseLine) { "expected 4 tokens, got 3 at line 1"); EXPECT_THROW_MSG(parseLine("VALUE My-Attribute My-Value 1"), BadValue, "unknown attribute 'My-Attribute' at line 1"); - EXPECT_THROW_MSG(parseLine("VENDOR my-vendor 4417"), BadValue, - "unknown dictionary entry 'VENDOR' at line 1"); + + EXPECT_THROW_MSG(parseLine("$INCLUDE"), BadValue, + "expected 2 tokens, got 1 at line 1"); + EXPECT_THROW_MSG(parseLine("$INCLUDE foo bar"), BadValue, + "expected 2 tokens, got 3 at line 1"); + EXPECT_THROW_MSG(parseLine("VENDOR my-vendor"), BadValue, + "expected 3 tokens, got 2 at line 1"); + EXPECT_THROW_MSG(parseLine("VENDOR my-vendor 44 17"), BadValue, + "expected 3 tokens, got 4 at line 1"); + + EXPECT_THROW_MSG(parseLine("BEGIN-VENDOR my-vendor"), BadValue, + "unknown dictionary entry 'BEGIN-VENDOR' at line 1"); + EXPECT_THROW_MSG(parseLine("END-VENDOR my-vendor"), BadValue, + "unknown dictionary entry 'END-VENDOR' at line 1"); } // Verifies sequences attribute of (re)definitions. @@ -216,12 +228,49 @@ TEST_F(DictionaryTest, integerConstant) { EXPECT_THROW_MSG(parseLines(new_value), BadValue, expected); } +// Verifies vendor id definitions. +TEST_F(DictionaryTest, vendorId) { + // Value must be an integer. + list not_integer_val = { + "VENDOR My-Value Non-Integer" + }; + EXPECT_THROW_MSG(parseLines(not_integer_val), BadValue, + "can't parse integer value Non-Integer at line 1"); + + // Positive case. + list positive = { + "VENDOR ISC 2495" + }; + EXPECT_NO_THROW_LOG(parseLines(positive)); + + // Redefine the same vendor id. + list same = { + "VENDOR ISC 2495", + "VENDOR ISC 2495" + }; + EXPECT_NO_THROW_LOG(parseLines(same)); + + // Redefine with a different value is not allowed. + list new_value = { + "VENDOR ISC 2495", + "VENDOR ISC 24950", + }; + string expected = "Illegal vendor id redefinition of "; + expected += "'ISC' value 2495 by 24950 at line 2"; + EXPECT_THROW_MSG(parseLines(new_value), BadValue, expected); +} + // Verifies errors from bad dictionary files. TEST_F(DictionaryTest, badFile) { string expected = "can't open dictionary '/does-not-exist': "; expected += "No such file or directory"; EXPECT_THROW_MSG(AttrDefs::instance().readDictionary("/does-not-exist"), BadValue, expected); + list bad_include = { + "$INCLUDE /does-not-exist" + }; + expected += " at line 1"; + EXPECT_THROW_MSG(parseLines(bad_include), BadValue, expected); } // Definitions of Standard attributes used by the hook. @@ -239,13 +288,11 @@ TEST_F(DictionaryTest, include) { include.push_back("# Including the dictonary"); include.push_back(string("$INCLUDE ") + string(TEST_DICTIONARY)); include.push_back("# Dictionary included"); - // include.push_back("VALUE Vendor-Specific ISC 2495"); - include.push_back("VALUE ARAP-Security ISC 2495"); + include.push_back("VENDOR ISC 2495"); EXPECT_NO_THROW_LOG(parseLines(include)); EXPECT_NO_THROW_LOG(AttrDefs::instance(). checkStandardDefs(RadiusConfigParser::USED_STANDARD_ATTR_DEFS)); - // auto isc = AttrDefs::instance().getByName(PW_VENDOR_SPECIFIC, "ISC"); - auto isc = AttrDefs::instance().getByName(PW_ARAP_SECURITY, "ISC"); + auto isc = AttrDefs::instance().getByName(PW_VENDOR_SPECIFIC, "ISC"); ASSERT_TRUE(isc); EXPECT_EQ(2495, isc->value_); From ab8bffed18ced2556d7e9d629d819239daf72799 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Thu, 21 Aug 2025 01:00:46 +0200 Subject: [PATCH 03/13] [#3860] Checkpoint: doing vsa --- src/hooks/dhcp/radius/client_attribute.cc | 114 ++++++++++++- src/hooks/dhcp/radius/client_attribute.h | 157 +++++++++++++++++- src/hooks/dhcp/radius/client_dictionary.cc | 8 + src/hooks/dhcp/radius/client_dictionary.h | 3 +- .../dhcp/radius/tests/attribute_unittests.cc | 29 +++- .../dhcp/radius/tests/dictionary_unittests.cc | 13 ++ 6 files changed, 310 insertions(+), 14 deletions(-) diff --git a/src/hooks/dhcp/radius/client_attribute.cc b/src/hooks/dhcp/radius/client_attribute.cc index 403f283d16..41aac45726 100644 --- a/src/hooks/dhcp/radius/client_attribute.cc +++ b/src/hooks/dhcp/radius/client_attribute.cc @@ -80,6 +80,8 @@ Attribute::fromText(const AttrDefPtr& def, const string& value) { return (AttrIpv6Addr::fromText(def->type_, value)); case PW_TYPE_IPV6PREFIX: return (AttrIpv6Prefix::fromText(def->type_, value)); + case PW_TYPE_VSA: + return (AttrVSA::fromText(def->type_, value)); default: // Impossible case. isc_throw(OutOfRange, "unknown value type " @@ -131,6 +133,8 @@ Attribute::fromBytes(const AttrDefPtr& def, const vector& value) { return (AttrIpv6Addr::fromBytes(def->type_, value)); case PW_TYPE_IPV6PREFIX: return (AttrIpv6Prefix::fromBytes(def->type_, value)); + case PW_TYPE_VSA: + return (AttrVSA::fromBytes(def->type_, value)); default: // Impossible case. isc_throw(OutOfRange, "unknown value type " @@ -177,13 +181,13 @@ Attribute::fromIpv6Prefix(const uint8_t type, const uint8_t len, string Attribute::toString() const { - isc_throw(TypeError, "the attribute value type must be string, not " + isc_throw(TypeError, "the attribute value type must be string or vsa, not " << attrValueTypeToText(getValueType())); } vector Attribute::toBinary() const { - isc_throw(TypeError, "the attribute value type must be string, not " + isc_throw(TypeError, "the attribute value type must be string or vsa, not " << attrValueTypeToText(getValueType())); } @@ -217,6 +221,18 @@ Attribute::toIpv6PrefixLen() const { << attrValueTypeToText(getValueType())); } +uint32_t +Attribute::toVendorId() const { + isc_throw(TypeError, "the attribute value type must be vsa, not " + << attrValueTypeToText(getValueType())); +} + +void +Attribute::setVendorId(const uint32_t vendor) { + isc_throw(TypeError, "the attribute value type must be vsa, not " + << attrValueTypeToText(getValueType())); +} + AttrString::AttrString(const uint8_t type, const vector& value) : Attribute(type), value_() { if (value.empty()) { @@ -612,6 +628,100 @@ AttrIpv6Prefix::toElement() const { return (output); } +AttrVSA::AttrVSA(const uint8_t type, const int32_t vendor, + const vector& value) + : Attribute(type), vendor_(vendor), value_() { + if (value.empty()) { + isc_throw(BadValue, "value is empty"); + } + if (value.size() > MAX_VSA_DATA_LEN) { + isc_throw(BadValue, "value is too large " << value.size() + << " > " << MAX_VSA_DATA_LEN); + } + value_.resize(value.size()); + memmove(&value_[0], &value[0], value_.size()); +} + +AttributePtr +AttrVSA::fromText(const uint8_t type, const string& repr) { + isc_throw(NotImplemented, "Can't decode VSA from text"); +} + +AttributePtr +AttrVSA::fromBytes(const uint8_t type, const vector& bytes) { + if (bytes.empty()) { + isc_throw(BadValue, "empty attribute value"); + } + if (bytes.size() < 5) { + isc_throw(BadValue, "value is too small " << bytes.size() << " < 5"); + } else if (bytes.size() > MAX_STRING_LEN) { + isc_throw(BadValue, "value is too large " << bytes.size() + << " > " << MAX_STRING_LEN); + } + uint32_t vendor = bytes[0] << 24; + vendor |= bytes[1] << 16; + vendor |= bytes[2] << 8; + vendor |= bytes[3]; + vector value; + value.resize(bytes.size() - 4); + if (value.size() > 0) { + memmove(&value[0], &bytes[4], value.size()); + } + return (AttributePtr(new AttrVSA(type, vendor, value))); +} + +string +AttrVSA::toText(size_t indent) const { + isc_throw(NotImplemented, "Can't encode VSA into text"); +} + +std::vector +AttrVSA::toBytes() const { + vector output; + output.resize(2 + getValueLen()); + output[0] = getType(); + output[1] = 2 + getValueLen(); + output[2] = (vendor_ & 0xff000000U) >> 24; + output[3] = (vendor_ & 0xff0000U) >> 16; + output[4] = (vendor_ & 0xff00U) >> 8; + output[5] = vendor_ & 0xffU; + if (output.size() > 6) { + memmove(&output[6], &value_[0], output.size() - 6); + } + return (output); +} + +std::vector +AttrVSA::toBinary() const { + vector binary; + binary.resize(getValueLen() - 4); + if (binary.size() > 0) { + memmove(&binary[0], &value_[0], binary.size()); + } + return (binary); +} + +ElementPtr +AttrVSA::toElement() const { + ElementPtr output = Element::createMap(); + AttrDefPtr def = AttrDefs::instance().getByType(getType()); + if (def) { + output->set("name", Element::create(def->name_)); + } + output->set("type", Element::create(static_cast(getType()))); + ostringstream vendor; + vendor << vendor_; + output->set("vendor", Element::create(vendor.str())); + vector binary; + binary.resize(value_.size()); + if (binary.size() > 0) { + memmove(&binary[0], value_.c_str(), binary.size()); + } + string raw = encode::encodeHex(binary); + output->set("vsa-raw", Element::create(raw)); + return (output); +} + void Attributes::add(const ConstAttributePtr& attr) { if (!attr) { diff --git a/src/hooks/dhcp/radius/client_attribute.h b/src/hooks/dhcp/radius/client_attribute.h index 5e7d642f23..ac13cfbd79 100644 --- a/src/hooks/dhcp/radius/client_attribute.h +++ b/src/hooks/dhcp/radius/client_attribute.h @@ -28,6 +28,9 @@ namespace radius { /// @brief Maximum string size. static constexpr size_t MAX_STRING_LEN = 253; +/// @brief Maximum vsa data size. +static constexpr size_t MAX_VSA_DATA_LEN = MAX_STRING_LEN - 4; + /// @brief Type error. using isc::data::TypeError; @@ -159,6 +162,28 @@ class Attribute : public data::CfgToElement { const uint8_t len, const asiolink::IOAddress& value); + /// @brief From Vendor ID and string data with type. + /// + /// @note Requires the type to be of the Vendor Specific attribute (26). + /// + /// @param type type of attribute. + /// @param vendor vendor id. + /// @param value vsa data. + static AttributePtr fromVSA(const uint8_t type, + const uint32_t vendor, + const std::string& value); + + /// @brief From Vendor ID and binary data with type. + /// + /// @note Requires the type to be of the Vendor Specific attribute (26). + /// + /// @param type type of attribute. + /// @param vendor vendor id. + /// @param value vsa data. + static AttributePtr fromVSA(const uint8_t type, + const uint32_t vendor, + const std::vector& value); + /// Generic get methods. /// @brief Value length. @@ -184,13 +209,13 @@ class Attribute : public data::CfgToElement { /// @brief To string. /// /// @return the string value. - /// @throw TypeError if the attribute is not a string one. + /// @throw TypeError if the attribute is not a string or vsa one. virtual std::string toString() const; /// @brief To binary. /// /// @return the string value as a binary. - /// @throw TypeError if the attribute is not a string one. + /// @throw TypeError if the attribute is not a string or vsa one. virtual std::vector toBinary() const; /// @brief To integer. @@ -223,6 +248,20 @@ class Attribute : public data::CfgToElement { /// @throw TypeError if the attribute is not an ipv6prefix one. virtual uint8_t toIpv6PrefixLen() const; + /// @brief To vendor id. + /// + /// @return the vendor id. + /// @throw TypeError if the attribute is not a vsa one. + virtual uint32_t toVendorId() const; + + /// Generic set methods. + + /// @brief Set vendor id. + /// + /// @param vendor vendor id. + /// @throw TypeError if the attribute is not a vsa one. + virtual void setVendorId(const uint32_t vendor); + /// @brief Type. const uint8_t type_; }; @@ -233,6 +272,7 @@ class Attribute : public data::CfgToElement { /// @brief RADIUS attribute holding strings. class AttrString : public Attribute { protected: + /// @brief Constructor. /// /// @param type attribute type. @@ -648,6 +688,119 @@ class AttrIpv6Prefix : public Attribute { asiolink::IOAddress value_; }; +/// @brief RADIUS attribute holding vsa. +class AttrVSA : public Attribute { +protected: + + /// @brief Constructor. + /// + /// @param type attribute type. + /// @param vendor vendor id. + /// @param value string vsa data. + AttrVSA(const uint8_t type, const int32_t vendor, const std::string& value) + : Attribute(type), vendor_(vendor), value_(value) { + if (value.empty()) { + isc_throw(BadValue, "value is empty"); + } + if (value.size() > MAX_VSA_DATA_LEN) { + isc_throw(BadValue, "value is too large " << value.size() + << " > " << MAX_VSA_DATA_LEN); + } + } + + /// @brief Constructor. + /// + /// @param type attribute type. + /// @param vendor vendor id. + /// @param value binary vsa data. + AttrVSA(const uint8_t type, const int32_t vendor, + const std::vector& value); + + /// @brief From text. + /// + /// @param type attribute type. + /// @param repr value representation. + /// @return pointer to the attribute or null. + static AttributePtr fromText(const uint8_t type, const std::string& repr); + + /// @brief From bytes. + /// + /// @param type attribute type. + /// @param bytes binary value. + /// @return pointer to the attribute or null. + static AttributePtr fromBytes(const uint8_t type, + const std::vector& bytes); + + /// Make Attribute a friend class. + friend class Attribute; + +public: + + /// @brief Get value type. + /// + /// @return the value type. + virtual AttrValueType getValueType() const override { + return (PW_TYPE_VSA); + } + + /// @brief Value length. + /// + /// @return Value length. + virtual size_t getValueLen() const override { + return (4 + value_.size()); + } + + /// @brief Returns text representation of the attribute. + /// + /// @param indent number of spaces before printing text. + /// @return string with text representation. + virtual std::string toText(size_t indent = 0) const override; + + /// @brief To bytes. + /// + /// @return binary representation. + virtual std::vector toBytes() const override; + + /// @brief To string. + /// + /// @return the string value. + virtual std::string toString() const override { + return (value_); + } + + /// @brief To binary. + /// + /// @return the string value as a binary. + virtual std::vector toBinary() const override; + + /// @brief To vendor id. + /// + /// @return the vendor id. + virtual uint32_t toVendorId() const override { + return (vendor_); + } + + /// @brief Set vendor id. + /// + /// @param vendor vendor id. + virtual void setVendorId(const uint32_t vendor) override { + vendor_ = vendor; + } + + /// @brief Unparse attribute. + /// + /// @return a pointer to unparsed attribute. + virtual data::ElementPtr toElement() const override; + +private: + /// @brief Vendor id. + uint32_t vendor_; + + /// @brief Value. + std::string value_; +}; + + /// @brief Collection of attributes. class Attributes : public data::CfgToElement { public: diff --git a/src/hooks/dhcp/radius/client_dictionary.cc b/src/hooks/dhcp/radius/client_dictionary.cc index 01f7b9b641..988ab2f2ff 100644 --- a/src/hooks/dhcp/radius/client_dictionary.cc +++ b/src/hooks/dhcp/radius/client_dictionary.cc @@ -34,6 +34,8 @@ attrValueTypeToText(const AttrValueType value) { return ("ipv6addr"); case PW_TYPE_IPV6PREFIX: return ("ipv6prefix"); + case PW_TYPE_VSA: + return ("vsa"); default: // Impossible case. return ("unknown?"); @@ -52,6 +54,8 @@ textToAttrValueType(const string& name) { return (PW_TYPE_IPV6ADDR); } else if (name == "ipv6prefix") { return (PW_TYPE_IPV6PREFIX); + } else if (name == "vsa") { + return (PW_TYPE_VSA); } else { isc_throw(OutOfRange, "unknown AttrValueType name " << name); } @@ -230,6 +234,10 @@ AttrDefs::parseLine(const string& line, unsigned int depth) { isc_throw(Unexpected, "can't parse attribute type " << type_str); } AttrValueType value_type = textToAttrValueType(tokens[3]); + if ((value_type == PW_TYPE_VSA) && (type != PW_VENDOR_SPECIFIC)) { + isc_throw(BadValue, "only Vendor-Specific (26) attribute can " + << "have the vsa data type"); + } AttrDefPtr def(new AttrDef(type, name, value_type)); add(def); return; diff --git a/src/hooks/dhcp/radius/client_dictionary.h b/src/hooks/dhcp/radius/client_dictionary.h index 21ee11b9b1..4b6824faee 100644 --- a/src/hooks/dhcp/radius/client_dictionary.h +++ b/src/hooks/dhcp/radius/client_dictionary.h @@ -31,7 +31,8 @@ enum AttrValueType { PW_TYPE_INTEGER, PW_TYPE_IPADDR, PW_TYPE_IPV6ADDR, - PW_TYPE_IPV6PREFIX + PW_TYPE_IPV6PREFIX, + PW_TYPE_VSA }; /// @brief AttrValueType value -> name function. diff --git a/src/hooks/dhcp/radius/tests/attribute_unittests.cc b/src/hooks/dhcp/radius/tests/attribute_unittests.cc index 2d72b05c51..1bc3e32c90 100644 --- a/src/hooks/dhcp/radius/tests/attribute_unittests.cc +++ b/src/hooks/dhcp/radius/tests/attribute_unittests.cc @@ -175,6 +175,8 @@ TEST_F(AttributeTest, attrString) { "the attribute value type must be ipv6prefix, not string"); EXPECT_THROW_MSG(attr->toIpv6PrefixLen(), TypeError, "the attribute value type must be ipv6prefix, not string"); + EXPECT_THROW_MSG(attr->toVendorId(), TypeError, + "the attribute value type must be vsa, not string"); } // Verifies raw string attribute. @@ -268,9 +270,9 @@ TEST_F(AttributeTest, attrInt) { << def_bytes->toText() << " != " << attr->toText(); EXPECT_THROW_MSG(attr->toString(), TypeError, - "the attribute value type must be string, not integer"); + "the attribute value type must be string or vsa, not integer"); EXPECT_THROW_MSG(attr->toBinary(), TypeError, - "the attribute value type must be string, not integer"); + "the attribute value type must be string or vsa, not integer"); EXPECT_THROW_MSG(attr->toIpAddr(), TypeError, "the attribute value type must be ipaddr, not integer"); EXPECT_THROW_MSG(attr->toIpv6Addr(), TypeError, @@ -278,7 +280,10 @@ TEST_F(AttributeTest, attrInt) { EXPECT_THROW_MSG(attr->toIpv6Prefix(), TypeError, "the attribute value type must be ipv6prefix, not integer"); EXPECT_THROW_MSG(attr->toIpv6PrefixLen(), TypeError, - "the attribute value type must be ipv6prefix, not integer"); + "the attribute value type must be ipv6prefix, not integer" +); + EXPECT_THROW_MSG(attr->toVendorId(), TypeError, + "the attribute value type must be vsa, not integer"); } // Verifies IP address attribute. @@ -338,9 +343,9 @@ TEST_F(AttributeTest, attrIpAddr) { << def_bytes->toText() << " != " << attr->toText(); EXPECT_THROW_MSG(attr->toString(), TypeError, - "the attribute value type must be string, not ipaddr"); + "the attribute value type must be string or vsa, not ipaddr"); EXPECT_THROW_MSG(attr->toBinary(), TypeError, - "the attribute value type must be string, not ipaddr"); + "the attribute value type must be string or vsa, not ipaddr"); EXPECT_THROW_MSG(attr->toInt(), TypeError, "the attribute value type must be integer, not ipaddr"); EXPECT_THROW_MSG(attr->toIpv6Addr(), TypeError, @@ -349,6 +354,8 @@ TEST_F(AttributeTest, attrIpAddr) { "the attribute value type must be ipv6prefix, not ipaddr"); EXPECT_THROW_MSG(attr->toIpv6PrefixLen(), TypeError, "the attribute value type must be ipv6prefix, not ipaddr"); + EXPECT_THROW_MSG(attr->toVendorId(), TypeError, + "the attribute value type must be vsa, not ipaddr"); } // Verifies IPv6 address attribute. @@ -415,9 +422,9 @@ TEST_F(AttributeTest, attrIpv6Addr) { << def_bytes->toText() << " != " << attr->toText(); EXPECT_THROW_MSG(attr->toString(), TypeError, - "the attribute value type must be string, not ipv6addr"); + "the attribute value type must be string or vsa, not ipv6addr"); EXPECT_THROW_MSG(attr->toBinary(), TypeError, - "the attribute value type must be string, not ipv6addr"); + "the attribute value type must be string or vsa, not ipv6addr"); EXPECT_THROW_MSG(attr->toInt(), TypeError, "the attribute value type must be integer, not ipv6addr"); EXPECT_THROW_MSG(attr->toIpAddr(), TypeError, @@ -426,6 +433,8 @@ TEST_F(AttributeTest, attrIpv6Addr) { "the attribute value type must be ipv6prefix, not ipv6addr"); EXPECT_THROW_MSG(attr->toIpv6PrefixLen(), TypeError, "the attribute value type must be ipv6prefix, not ipv6addr"); + EXPECT_THROW_MSG(attr->toVendorId(), TypeError, + "the attribute value type must be vsa, not ipv6addr"); } // Verifies IPv6 prefix attribute. @@ -524,15 +533,17 @@ TEST_F(AttributeTest, attrIpv6Prefix) { << def_bytes->toText() << " != " << attr->toText(); EXPECT_THROW_MSG(attr->toString(), TypeError, - "the attribute value type must be string, not ipv6prefix"); + "the attribute value type must be string or vsa, not ipv6prefix"); EXPECT_THROW_MSG(attr->toBinary(), TypeError, - "the attribute value type must be string, not ipv6prefix"); + "the attribute value type must be string or vsa, not ipv6prefix"); EXPECT_THROW_MSG(attr->toInt(), TypeError, "the attribute value type must be integer, not ipv6prefix"); EXPECT_THROW_MSG(attr->toIpAddr(), TypeError, "the attribute value type must be ipaddr, not ipv6prefix"); EXPECT_THROW_MSG(attr->toIpv6Addr(), TypeError, "the attribute value type must be ipv6addr, not ipv6prefix"); + EXPECT_THROW_MSG(attr->toVendorId(), TypeError, + "the attribute value type must be vsa, not ipv6prefix"); } // Verifies basic methods for attribute collection. diff --git a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc index 2fbd014bc9..a0589478d3 100644 --- a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc +++ b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc @@ -180,6 +180,19 @@ TEST_F(DictionaryTest, parseLines) { expected = "Illegal attribute redefinition of 'Service-Type' "; expected += "type 6 value type integer by 6 string at line 2"; EXPECT_THROW_MSG(parseLines(new_value_type), BadValue, expected); + + // Only the attribute 26 (Vendor-Specific) can have the vsa data type. + list bad_vsa = { + "ATTRIBUTE Attr126 126 vsa" + }; + expected = "only Vendor-Specific (26) attribute can have "; + expected += "the vsa data type at line 1"; + EXPECT_THROW_MSG(parseLines(bad_vsa), BadValue, expected); + + list vsa = { + "ATTRIBUTE Attr26 26 vsa" + }; + EXPECT_NO_THROW_LOG(parseLines(vsa)); } // Verifies integer constant definitions. From f73fc806742c0441d45ab870e0840f711985738c Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Thu, 21 Aug 2025 09:39:23 +0200 Subject: [PATCH 04/13] [#3860] More vsa --- src/hooks/dhcp/radius/client_attribute.cc | 106 ++++----- src/hooks/dhcp/radius/client_attribute.h | 53 ++--- src/hooks/dhcp/radius/client_dictionary.cc | 3 + src/hooks/dhcp/radius/data/dictionary | 2 +- src/hooks/dhcp/radius/radius_parsers.cc | 1 + .../dhcp/radius/tests/access_unittests.cc | 12 +- .../dhcp/radius/tests/accounting_unittests.cc | 108 ++++----- src/hooks/dhcp/radius/tests/attribute_test.cc | 3 + .../dhcp/radius/tests/attribute_unittests.cc | 214 +++++++++++------- .../dhcp/radius/tests/config_unittests.cc | 15 +- .../dhcp/radius/tests/dictionary_unittests.cc | 2 + .../dhcp/radius/tests/request_unittests.h | 8 +- 12 files changed, 281 insertions(+), 246 deletions(-) diff --git a/src/hooks/dhcp/radius/client_attribute.cc b/src/hooks/dhcp/radius/client_attribute.cc index 41aac45726..7840bd4b97 100644 --- a/src/hooks/dhcp/radius/client_attribute.cc +++ b/src/hooks/dhcp/radius/client_attribute.cc @@ -24,36 +24,6 @@ using namespace std; namespace isc { namespace radius { -AttributePtr -Attribute::fromText(const string& repr) { - if (repr.empty()) { - isc_throw(BadValue, "empty text attribute"); - } - string trimed = str::trim(repr); - if (trimed.empty()) { - isc_throw(BadValue, "blank text attribute '" << repr << "'"); - } - size_t equal = trimed.find('='); - if (equal == string::npos) { - isc_throw(BadValue, "can't find '=' in text attribute '" - << repr << "'"); - } - string name = str::trim(trimed.substr(0, equal)); - if (name.empty()) { - isc_throw(BadValue, "empty attribute name in '" << repr << "'"); - } - string value = str::trim(trimed.substr(equal + 1)); - if (value.empty()) { - isc_throw(BadValue, "empty attribute value in '" << repr << "'"); - } - AttrDefPtr def = AttrDefs::instance().getByName(name); - if (!def) { - isc_throw(NotFound, "can't find attribute definition for '" - << name << "'"); - } - return (Attribute::fromText(def, value)); -} - AttributePtr Attribute::fromText(const AttrDefPtr& def, const string& value) { if (!def) { @@ -81,7 +51,7 @@ Attribute::fromText(const AttrDefPtr& def, const string& value) { case PW_TYPE_IPV6PREFIX: return (AttrIpv6Prefix::fromText(def->type_, value)); case PW_TYPE_VSA: - return (AttrVSA::fromText(def->type_, value)); + return (AttrVsa::fromText(def->type_, value)); default: // Impossible case. isc_throw(OutOfRange, "unknown value type " @@ -134,7 +104,7 @@ Attribute::fromBytes(const AttrDefPtr& def, const vector& value) { case PW_TYPE_IPV6PREFIX: return (AttrIpv6Prefix::fromBytes(def->type_, value)); case PW_TYPE_VSA: - return (AttrVSA::fromBytes(def->type_, value)); + return (AttrVsa::fromBytes(def->type_, value)); default: // Impossible case. isc_throw(OutOfRange, "unknown value type " @@ -179,15 +149,27 @@ Attribute::fromIpv6Prefix(const uint8_t type, const uint8_t len, return (AttributePtr(new AttrIpv6Prefix(type, len, value))); } +AttributePtr +Attribute::fromVsa(const uint8_t type, const uint32_t vendor, + const std::string& value) { + return (AttributePtr(new AttrVsa(type, vendor, value))); +} + +AttributePtr +Attribute::fromVsa(const uint8_t type, const uint32_t vendor, + const std::vector& value) { + return (AttributePtr(new AttrVsa(type, vendor, value))); +} + string Attribute::toString() const { - isc_throw(TypeError, "the attribute value type must be string or vsa, not " + isc_throw(TypeError, "the attribute value type must be string, not " << attrValueTypeToText(getValueType())); } vector Attribute::toBinary() const { - isc_throw(TypeError, "the attribute value type must be string or vsa, not " + isc_throw(TypeError, "the attribute value type must be string, not " << attrValueTypeToText(getValueType())); } @@ -227,8 +209,8 @@ Attribute::toVendorId() const { << attrValueTypeToText(getValueType())); } -void -Attribute::setVendorId(const uint32_t vendor) { +string +Attribute::toVsaData() const { isc_throw(TypeError, "the attribute value type must be vsa, not " << attrValueTypeToText(getValueType())); } @@ -276,7 +258,17 @@ AttrString::toText(size_t indent) const { for (size_t i = 0; i < indent; i++) { output << " "; } - output << AttrDefs::instance().getName(getType()) << '=' << value_; + output << AttrDefs::instance().getName(getType()) << '='; + if (str::isPrintable(value_)) { + output << "'" << value_ << "'"; + } else { + vector binary; + binary.resize(value_.size()); + if (binary.size() > 0) { + memmove(&binary[0], value_.c_str(), binary.size()); + } + output << "0x" << encode::encodeHex(binary); + } return (output.str()); } @@ -628,7 +620,7 @@ AttrIpv6Prefix::toElement() const { return (output); } -AttrVSA::AttrVSA(const uint8_t type, const int32_t vendor, +AttrVsa::AttrVsa(const uint8_t type, const uint32_t vendor, const vector& value) : Attribute(type), vendor_(vendor), value_() { if (value.empty()) { @@ -643,12 +635,12 @@ AttrVSA::AttrVSA(const uint8_t type, const int32_t vendor, } AttributePtr -AttrVSA::fromText(const uint8_t type, const string& repr) { - isc_throw(NotImplemented, "Can't decode VSA from text"); +AttrVsa::fromText(const uint8_t type, const string& repr) { + isc_throw(NotImplemented, "Can't decode vsa from text"); } AttributePtr -AttrVSA::fromBytes(const uint8_t type, const vector& bytes) { +AttrVsa::fromBytes(const uint8_t type, const vector& bytes) { if (bytes.empty()) { isc_throw(BadValue, "empty attribute value"); } @@ -667,16 +659,28 @@ AttrVSA::fromBytes(const uint8_t type, const vector& bytes) { if (value.size() > 0) { memmove(&value[0], &bytes[4], value.size()); } - return (AttributePtr(new AttrVSA(type, vendor, value))); + return (AttributePtr(new AttrVsa(type, vendor, value))); } string -AttrVSA::toText(size_t indent) const { - isc_throw(NotImplemented, "Can't encode VSA into text"); +AttrVsa::toText(size_t indent) const { + ostringstream output; + for (size_t i = 0; i < indent; i++) { + output << " "; + } + output << AttrDefs::instance().getName(getType()) << "=[" + << vendor_ << "]"; + vector binary; + binary.resize(value_.size()); + if (binary.size() > 0) { + memmove(&binary[0], value_.c_str(), binary.size()); + } + output << "0x" << encode::encodeHex(binary); + return (output.str()); } std::vector -AttrVSA::toBytes() const { +AttrVsa::toBytes() const { vector output; output.resize(2 + getValueLen()); output[0] = getType(); @@ -691,18 +695,8 @@ AttrVSA::toBytes() const { return (output); } -std::vector -AttrVSA::toBinary() const { - vector binary; - binary.resize(getValueLen() - 4); - if (binary.size() > 0) { - memmove(&binary[0], &value_[0], binary.size()); - } - return (binary); -} - ElementPtr -AttrVSA::toElement() const { +AttrVsa::toElement() const { ElementPtr output = Element::createMap(); AttrDefPtr def = AttrDefs::instance().getByType(getType()); if (def) { diff --git a/src/hooks/dhcp/radius/client_attribute.h b/src/hooks/dhcp/radius/client_attribute.h index ac13cfbd79..d98ee9d601 100644 --- a/src/hooks/dhcp/radius/client_attribute.h +++ b/src/hooks/dhcp/radius/client_attribute.h @@ -69,13 +69,6 @@ class Attribute : public data::CfgToElement { /// Generic factories. - /// @brief From text. - /// - /// @param repr name=value representation. - /// @return pointer to the attribute. - /// @throw NotFound if the definition can't be found. - static AttributePtr fromText(const std::string& repr); - /// @brief From bytes (wire format). /// /// @param bytes binary attribute. @@ -164,12 +157,12 @@ class Attribute : public data::CfgToElement { /// @brief From Vendor ID and string data with type. /// - /// @note Requires the type to be of the Vendor Specific attribute (26). + /// @note Requires the type to be of a standard vsa attribute. /// /// @param type type of attribute. /// @param vendor vendor id. /// @param value vsa data. - static AttributePtr fromVSA(const uint8_t type, + static AttributePtr fromVsa(const uint8_t type, const uint32_t vendor, const std::string& value); @@ -180,7 +173,7 @@ class Attribute : public data::CfgToElement { /// @param type type of attribute. /// @param vendor vendor id. /// @param value vsa data. - static AttributePtr fromVSA(const uint8_t type, + static AttributePtr fromVsa(const uint8_t type, const uint32_t vendor, const std::vector& value); @@ -209,13 +202,13 @@ class Attribute : public data::CfgToElement { /// @brief To string. /// /// @return the string value. - /// @throw TypeError if the attribute is not a string or vsa one. + /// @throw TypeError if the attribute is not a string one. virtual std::string toString() const; /// @brief To binary. /// /// @return the string value as a binary. - /// @throw TypeError if the attribute is not a string or vsa one. + /// @throw TypeError if the attribute is not a string one. virtual std::vector toBinary() const; /// @brief To integer. @@ -254,13 +247,11 @@ class Attribute : public data::CfgToElement { /// @throw TypeError if the attribute is not a vsa one. virtual uint32_t toVendorId() const; - /// Generic set methods. - - /// @brief Set vendor id. + /// @brief To vsa data. /// - /// @param vendor vendor id. + /// @return the vsa data. /// @throw TypeError if the attribute is not a vsa one. - virtual void setVendorId(const uint32_t vendor); + virtual std::string toVsaData() const; /// @brief Type. const uint8_t type_; @@ -689,7 +680,7 @@ class AttrIpv6Prefix : public Attribute { }; /// @brief RADIUS attribute holding vsa. -class AttrVSA : public Attribute { +class AttrVsa : public Attribute { protected: /// @brief Constructor. @@ -697,7 +688,8 @@ class AttrVSA : public Attribute { /// @param type attribute type. /// @param vendor vendor id. /// @param value string vsa data. - AttrVSA(const uint8_t type, const int32_t vendor, const std::string& value) + AttrVsa(const uint8_t type, const uint32_t vendor, + const std::string& value) : Attribute(type), vendor_(vendor), value_(value) { if (value.empty()) { isc_throw(BadValue, "value is empty"); @@ -713,7 +705,7 @@ class AttrVSA : public Attribute { /// @param type attribute type. /// @param vendor vendor id. /// @param value binary vsa data. - AttrVSA(const uint8_t type, const int32_t vendor, + AttrVsa(const uint8_t type, const uint32_t vendor, const std::vector& value); /// @brief From text. @@ -721,6 +713,7 @@ class AttrVSA : public Attribute { /// @param type attribute type. /// @param repr value representation. /// @return pointer to the attribute or null. + /// @throw NotImplemented static AttributePtr fromText(const uint8_t type, const std::string& repr); /// @brief From bytes. @@ -761,18 +754,6 @@ class AttrVSA : public Attribute { /// @return binary representation. virtual std::vector toBytes() const override; - /// @brief To string. - /// - /// @return the string value. - virtual std::string toString() const override { - return (value_); - } - - /// @brief To binary. - /// - /// @return the string value as a binary. - virtual std::vector toBinary() const override; - /// @brief To vendor id. /// /// @return the vendor id. @@ -780,11 +761,11 @@ class AttrVSA : public Attribute { return (vendor_); } - /// @brief Set vendor id. + /// @brief To vsa data. /// - /// @param vendor vendor id. - virtual void setVendorId(const uint32_t vendor) override { - vendor_ = vendor; + /// @return the vsa data. + virtual std::string toVsaData() const override { + return (value_); } /// @brief Unparse attribute. diff --git a/src/hooks/dhcp/radius/client_dictionary.cc b/src/hooks/dhcp/radius/client_dictionary.cc index 988ab2f2ff..479db62e0d 100644 --- a/src/hooks/dhcp/radius/client_dictionary.cc +++ b/src/hooks/dhcp/radius/client_dictionary.cc @@ -291,6 +291,9 @@ AttrDefs::parseLine(const string& line, unsigned int depth) { } catch (...) { isc_throw(Unexpected, "can't parse integer value " << value_str); } + if (value == 0) { + isc_throw(Unexpected, "0 is reserved"); + } IntCstDefPtr def(new IntCstDef(PW_VENDOR_SPECIFIC, name, value)); add(def); return; diff --git a/src/hooks/dhcp/radius/data/dictionary b/src/hooks/dhcp/radius/data/dictionary index f06dde01b3..94ed4716bc 100644 --- a/src/hooks/dhcp/radius/data/dictionary +++ b/src/hooks/dhcp/radius/data/dictionary @@ -26,7 +26,7 @@ ATTRIBUTE Framed-Route 22 string ATTRIBUTE Framed-IPX-Network 23 ipaddr ATTRIBUTE State 24 string ATTRIBUTE Class 25 string -ATTRIBUTE Vendor-Specific 26 string +ATTRIBUTE Vendor-Specific 26 vsa ATTRIBUTE Session-Timeout 27 integer ATTRIBUTE Idle-Timeout 28 integer ATTRIBUTE Termination-Action 29 integer diff --git a/src/hooks/dhcp/radius/radius_parsers.cc b/src/hooks/dhcp/radius/radius_parsers.cc index 1e98f413db..20c89d1053 100644 --- a/src/hooks/dhcp/radius/radius_parsers.cc +++ b/src/hooks/dhcp/radius/radius_parsers.cc @@ -74,6 +74,7 @@ const AttrDefList RadiusConfigParser::USED_STANDARD_ATTR_DEFS = { { PW_FRAMED_IP_ADDRESS, "Framed-IP-Address", PW_TYPE_IPADDR }, { PW_REPLY_MESSAGE, "Reply-Message", PW_TYPE_STRING }, { PW_CLASS, "Class", PW_TYPE_STRING }, + { PW_VENDOR_SPECIFIC, "Vendor-Specific", PW_TYPE_VSA }, { PW_CALLING_STATION_ID, "Calling-Station-Id", PW_TYPE_STRING }, { PW_ACCT_STATUS_TYPE, "Acct-Status-Type", PW_TYPE_INTEGER }, { PW_ACCT_DELAY_TIME, "Acct-Delay-Time", PW_TYPE_INTEGER }, diff --git a/src/hooks/dhcp/radius/tests/access_unittests.cc b/src/hooks/dhcp/radius/tests/access_unittests.cc index a42da6cde8..d97fe16f14 100644 --- a/src/hooks/dhcp/radius/tests/access_unittests.cc +++ b/src/hooks/dhcp/radius/tests/access_unittests.cc @@ -1755,12 +1755,12 @@ TEST_F(AccessTest, buildHWAddr4) { EXPECT_LE(2, handler->env_.send_attrs_->size()); ConstAttributePtr user_name = handler->env_.send_attrs_->get(PW_USER_NAME); ASSERT_TRUE(user_name); - string expected = "User-Name=" + text; + string expected = "User-Name='" + text + "'"; EXPECT_EQ(expected, user_name->toText()); ConstAttributePtr calling_station_id = handler->env_.send_attrs_->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=20:e5:2a:b8:15:14", + EXPECT_EQ("Calling-Station-Id='20:e5:2a:b8:15:14'", calling_station_id->toText()); } @@ -1783,12 +1783,12 @@ TEST_F(AccessTest, buildHWAddr6) { EXPECT_LE(2, handler->env_.send_attrs_->size()); ConstAttributePtr user_name = handler->env_.send_attrs_->get(PW_USER_NAME); ASSERT_TRUE(user_name); - string expected = "User-Name=" + text; + string expected = "User-Name='" + text + "'"; EXPECT_EQ(expected, user_name->toText()); ConstAttributePtr calling_station_id = handler->env_.send_attrs_->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=08:00:27:58:f1:e8", + EXPECT_EQ("Calling-Station-Id='08:00:27:58:f1:e8'", calling_station_id->toText()); } @@ -1809,7 +1809,7 @@ TEST_F(AccessTest, buildCanonHWAddr4) { ConstAttributePtr calling_station_id = handler->env_.send_attrs_->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=20-e5-2a-b8-15-14", + EXPECT_EQ("Calling-Station-Id='20-e5-2a-b8-15-14'", calling_station_id->toText()); } @@ -1830,7 +1830,7 @@ TEST_F(AccessTest, buildCanonHWAddr6) { ConstAttributePtr calling_station_id = handler->env_.send_attrs_->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=08-00-27-58-f1-e8", + EXPECT_EQ("Calling-Station-Id='08-00-27-58-f1-e8'", calling_station_id->toText()); } diff --git a/src/hooks/dhcp/radius/tests/accounting_unittests.cc b/src/hooks/dhcp/radius/tests/accounting_unittests.cc index 2521a200de..9af3040647 100644 --- a/src/hooks/dhcp/radius/tests/accounting_unittests.cc +++ b/src/hooks/dhcp/radius/tests/accounting_unittests.cc @@ -743,10 +743,10 @@ void AccountingTest::testBuildAcctLease4() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); ConstAttributePtr calling_station_id = attrs->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=20:e5:2a:b8:15:14", + EXPECT_EQ("Calling-Station-Id='20:e5:2a:b8:15:14'", calling_station_id->toText()); ConstAttributePtr framed_ip_address = attrs->get(PW_FRAMED_IP_ADDRESS); ASSERT_TRUE(framed_ip_address); @@ -782,7 +782,7 @@ void AccountingTest::testBuildAcctLease4canon() { ASSERT_LE(5, attrs->size()); ConstAttributePtr calling_station_id = attrs->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=20-e5-2a-b8-15-14", + EXPECT_EQ("Calling-Station-Id='20-e5-2a-b8-15-14'", calling_station_id->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -805,7 +805,7 @@ void AccountingTest::testBuildAcctLease4noDuid() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -827,7 +827,7 @@ void AccountingTest::testBuildAcctLease4noPop0() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -849,7 +849,7 @@ void AccountingTest::testBuildAcctLease4notPrintable() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -873,7 +873,7 @@ void AccountingTest::testBuildAcctLease4Duid() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=05:06:07:08:09", user_name->toText()); + EXPECT_EQ("User-Name='05:06:07:08:09'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -898,7 +898,7 @@ void AccountingTest::testBuildAcctLease4DuidPrintable() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=Foobar", user_name->toText()); + EXPECT_EQ("User-Name='Foobar'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -920,7 +920,7 @@ void AccountingTest::testBuildAcctLease4Pop0() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -942,7 +942,7 @@ void AccountingTest::testBuildAcctLease4Pop0Printable() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=Foobar", user_name->toText()); + EXPECT_EQ("User-Name='Foobar'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -962,7 +962,7 @@ void AccountingTest::testBuildAcctLease4noClientId() { ASSERT_LE(4, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=20:e5:2a:b8:15:14", user_name->toText()); + EXPECT_EQ("User-Name='20:e5:2a:b8:15:14'", user_name->toText()); ConstAttributePtr calling_station_id = attrs->get(PW_CALLING_STATION_ID); EXPECT_FALSE(calling_station_id); EXPECT_EQ(0, attrs->count(PW_CLASS)); @@ -1166,7 +1166,7 @@ void AccountingTest::testBuildAcctLease4ClassClientID() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that buildAcct on IPv4/HwAddr can get the Class from host cache. @@ -1200,7 +1200,7 @@ void AccountingTest::testBuildAcctLease4ClassHwAddr() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that buildAcct on IPv4/DUID can get the Class from host cache. @@ -1235,7 +1235,7 @@ void AccountingTest::testBuildAcctLease4ClassDuid() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that buildAcct on IPv4/Flex can get the Class from host cache. @@ -1268,7 +1268,7 @@ void AccountingTest::testBuildAcctLease4ClassFlex() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that buildAcct on IPv6 lease works. @@ -1292,10 +1292,10 @@ void AccountingTest::testBuildAcctLease6() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); ConstAttributePtr calling_station_id = attrs->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=20:e5:2a:b8:15:14", + EXPECT_EQ("Calling-Station-Id='20:e5:2a:b8:15:14'", calling_station_id->toText()); ConstAttributePtr framed_ip_address = attrs->get(PW_FRAMED_IPV6_ADDRESS); ASSERT_TRUE(framed_ip_address); @@ -1335,10 +1335,10 @@ void AccountingTest::testBuildAcctLease6prefix() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); ConstAttributePtr calling_station_id = attrs->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=20:e5:2a:b8:15:14", + EXPECT_EQ("Calling-Station-Id='20:e5:2a:b8:15:14'", calling_station_id->toText()); ConstAttributePtr delegated_prefix = attrs->get(PW_DELEGATED_IPV6_PREFIX); ASSERT_TRUE(delegated_prefix); @@ -1376,7 +1376,7 @@ void AccountingTest::testBuildAcctLease6canon() { ASSERT_LE(5, attrs->size()); ConstAttributePtr calling_station_id = attrs->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=20-e5-2a-b8-15-14", + EXPECT_EQ("Calling-Station-Id='20-e5-2a-b8-15-14'", calling_station_id->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -1400,7 +1400,7 @@ void AccountingTest::testBuildAcctLease6noPop0() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); } /// Verify that buildAcct on IPv6 lease works with not printable duid. @@ -1422,7 +1422,7 @@ void AccountingTest::testBuildAcctLease6notPrintable() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -1445,7 +1445,7 @@ void AccountingTest::testBuildAcctLease6Pop0() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -1468,7 +1468,7 @@ void AccountingTest::testBuildAcctLease6Printable() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=Foobar", user_name->toText()); + EXPECT_EQ("User-Name='Foobar'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -1491,7 +1491,7 @@ void AccountingTest::testBuildAcctLease6Pop0Printable() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=Foobar", user_name->toText()); + EXPECT_EQ("User-Name='Foobar'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -1701,7 +1701,7 @@ void AccountingTest::testBuildAcctLease6ClassDUID() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that buildAcct on IPv6/HwAddr can get the Class from host cache. @@ -1736,7 +1736,7 @@ void AccountingTest::testBuildAcctLease6ClassHwAddr() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that buildAcct on IPv6/Flex can get the Class from host cache. @@ -1770,7 +1770,7 @@ void AccountingTest::testBuildAcctLease6ClassFlex() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that buildAcct4 works. @@ -1794,10 +1794,10 @@ void AccountingTest::testBuildAcct4() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); ConstAttributePtr calling_station_id = attrs->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=20:e5:2a:b8:15:14", + EXPECT_EQ("Calling-Station-Id='20:e5:2a:b8:15:14'", calling_station_id->toText()); ConstAttributePtr framed_ip_address = attrs->get(PW_FRAMED_IP_ADDRESS); ASSERT_TRUE(framed_ip_address); @@ -1872,7 +1872,7 @@ void AccountingTest::testBuildAcct4canon() { ASSERT_LE(5, attrs->size()); ConstAttributePtr calling_station_id = attrs->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=20-e5-2a-b8-15-14", + EXPECT_EQ("Calling-Station-Id='20-e5-2a-b8-15-14'", calling_station_id->toText()); ConstAttributePtr framed_ip_address = attrs->get(PW_FRAMED_IP_ADDRESS); EXPECT_EQ("Framed-IP-Address=192.0.2.1", framed_ip_address->toText()); @@ -1897,7 +1897,7 @@ void AccountingTest::testBuildAcct4noPop0() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -1919,7 +1919,7 @@ void AccountingTest::testBuildAcct4notPrintable() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -1941,7 +1941,7 @@ void AccountingTest::testBuildAcct4Pop0() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -1963,7 +1963,7 @@ void AccountingTest::testBuildAcct4Printable() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=Foobar", user_name->toText()); + EXPECT_EQ("User-Name='Foobar'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -1985,7 +1985,7 @@ void AccountingTest::testBuildAcct4Pop0Printable() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=Foobar", user_name->toText()); + EXPECT_EQ("User-Name='Foobar'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -2029,7 +2029,7 @@ void AccountingTest::testBuildAcct4noClientId() { ASSERT_LE(4, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=20:e5:2a:b8:15:14", user_name->toText()); + EXPECT_EQ("User-Name='20:e5:2a:b8:15:14'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -2050,7 +2050,7 @@ void AccountingTest::testBuildAcct4noClientIdcanon() { ASSERT_LE(4, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=20-e5-2a-b8-15-14", user_name->toText()); + EXPECT_EQ("User-Name='20-e5-2a-b8-15-14'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -2132,7 +2132,7 @@ void AccountingTest::testBuildAcct4ClassClientID() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that buildAcct4 on HwAddr can get the Class from host cache. @@ -2167,7 +2167,7 @@ void AccountingTest::testBuildAcct4ClassHwAddr() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that buildAcct4 on Flex can get the Class from host cache. @@ -2202,7 +2202,7 @@ void AccountingTest::testBuildAcct4ClassFlex() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that buildAcct6 works. @@ -2226,10 +2226,10 @@ void AccountingTest::testBuildAcct6() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); ConstAttributePtr calling_station_id = attrs->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=20:e5:2a:b8:15:14", + EXPECT_EQ("Calling-Station-Id='20:e5:2a:b8:15:14'", calling_station_id->toText()); ConstAttributePtr framed_ip_address = attrs->get(PW_FRAMED_IPV6_ADDRESS); ASSERT_TRUE(framed_ip_address); @@ -2270,10 +2270,10 @@ void AccountingTest::testBuildAcct6prefix() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); ConstAttributePtr calling_station_id = attrs->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=20:e5:2a:b8:15:14", + EXPECT_EQ("Calling-Station-Id='20:e5:2a:b8:15:14'", calling_station_id->toText()); ConstAttributePtr delegated_prefix = attrs->get(PW_DELEGATED_IPV6_PREFIX); ASSERT_TRUE(delegated_prefix); @@ -2464,7 +2464,7 @@ void AccountingTest::testBuildAcct6canon() { ASSERT_LE(5, attrs->size()); ConstAttributePtr calling_station_id = attrs->get(PW_CALLING_STATION_ID); ASSERT_TRUE(calling_station_id); - EXPECT_EQ("Calling-Station-Id=20-e5-2a-b8-15-14", + EXPECT_EQ("Calling-Station-Id='20-e5-2a-b8-15-14'", calling_station_id->toText()); ConstAttributePtr framed_ip_address = attrs->get(PW_FRAMED_IPV6_ADDRESS); EXPECT_EQ("Framed-IPv6-Address=2001:db8::1235", @@ -2491,7 +2491,7 @@ void AccountingTest::testBuildAcct6noPop0() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -2514,7 +2514,7 @@ void AccountingTest::testBuildAcct6notPrintable() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -2537,7 +2537,7 @@ void AccountingTest::testBuildAcct6Pop0() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=01:02:03:04", user_name->toText()); + EXPECT_EQ("User-Name='01:02:03:04'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -2560,7 +2560,7 @@ void AccountingTest::testBuildAcct6Printable() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=Foobar", user_name->toText()); + EXPECT_EQ("User-Name='Foobar'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -2583,7 +2583,7 @@ void AccountingTest::testBuildAcct6Pop0Printable() { ASSERT_LE(5, attrs->size()); ConstAttributePtr user_name = attrs->get(PW_USER_NAME); ASSERT_TRUE(user_name); - EXPECT_EQ("User-Name=Foobar", user_name->toText()); + EXPECT_EQ("User-Name='Foobar'", user_name->toText()); EXPECT_EQ(0, attrs->count(PW_CLASS)); } @@ -2690,7 +2690,7 @@ void AccountingTest::testBuildAcct6ClassDUID() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that buildAcct6 on HwAddr can get the Class from host cache. @@ -2724,7 +2724,7 @@ void AccountingTest::testBuildAcct6ClassHwAddr() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that buildAcct6 on Flex can get the Class from host cache. @@ -2758,7 +2758,7 @@ void AccountingTest::testBuildAcct6ClassFlex() { ASSERT_LE(6, attrs->size()); ConstAttributePtr cclass = attrs->get(PW_CLASS); ASSERT_TRUE(cclass); - EXPECT_EQ("Class=foobar", cclass->toText()); + EXPECT_EQ("Class='foobar'", cclass->toText()); } /// Verify that lease4_select hook returns for fake allocations. diff --git a/src/hooks/dhcp/radius/tests/attribute_test.cc b/src/hooks/dhcp/radius/tests/attribute_test.cc index 927e391af4..dc3e0c45f4 100644 --- a/src/hooks/dhcp/radius/tests/attribute_test.cc +++ b/src/hooks/dhcp/radius/tests/attribute_test.cc @@ -35,6 +35,9 @@ AttributeTest::compare(ConstAttributePtr first, ConstAttributePtr second) { } else if (first->getValueType() == PW_TYPE_IPV6PREFIX) { return ((first->toIpv6Prefix() == second->toIpv6Prefix()) && (first->toIpv6PrefixLen() == second->toIpv6PrefixLen())); + } else if (first->getValueType() == PW_TYPE_VSA) { + return ((first->toVendorId() == second->toVendorId()) && + (first->toVsaData() == second->toVsaData())); } // Should not happen... return (false); diff --git a/src/hooks/dhcp/radius/tests/attribute_unittests.cc b/src/hooks/dhcp/radius/tests/attribute_unittests.cc index 1bc3e32c90..e7f51066dd 100644 --- a/src/hooks/dhcp/radius/tests/attribute_unittests.cc +++ b/src/hooks/dhcp/radius/tests/attribute_unittests.cc @@ -28,22 +28,6 @@ using namespace isc::test; namespace { -// Verify error cases for generic factory fromText. -TEST_F(AttributeTest, fromText) { - EXPECT_THROW_MSG(Attribute::fromText(""), BadValue, - "empty text attribute"); - EXPECT_THROW_MSG(Attribute::fromText(" "), BadValue, - "blank text attribute ' '"); - EXPECT_THROW_MSG(Attribute::fromText("foo-bar"), BadValue, - "can't find '=' in text attribute 'foo-bar'"); - EXPECT_THROW_MSG(Attribute::fromText("=bar"), BadValue, - "empty attribute name in '=bar'"); - EXPECT_THROW_MSG(Attribute::fromText("foo="), BadValue, - "empty attribute value in 'foo='"); - EXPECT_THROW_MSG(Attribute::fromText("Foo-Bar=1"), NotFound, - "can't find attribute definition for 'Foo-Bar'"); -} - // Verify error cases for factory fromText with definition. TEST_F(AttributeTest, defFromText) { AttrDefPtr def; @@ -107,18 +91,13 @@ TEST_F(AttributeTest, attrString) { string to_string; EXPECT_NO_THROW_LOG(to_string = attr->toString()); EXPECT_EQ("foobar", to_string); - EXPECT_EQ("User-Name=foobar", attr->toText()); + EXPECT_EQ("User-Name='foobar'", attr->toText()); vector binary = { 1, 8, 0x66, 0x6f, 0x6f, 0x62, 0x61, 0x72 }; EXPECT_EQ(binary, attr->toBytes()); string expected = "{ \"type\": 1, \"name\": \"User-Name\", "; expected += " \"data\": \"foobar\" }"; runToElementTest(expected, *attr); - AttributePtr from_text = Attribute::fromText("User-Name=foobar"); - ASSERT_TRUE(from_text); - EXPECT_TRUE(compare(from_text, attr)) - << from_text->toText() << " != " << attr->toText(); - AttributePtr from_bytes = Attribute::fromBytes(binary); ASSERT_TRUE(from_bytes); EXPECT_TRUE(compare(from_bytes, attr)) @@ -177,6 +156,8 @@ TEST_F(AttributeTest, attrString) { "the attribute value type must be ipv6prefix, not string"); EXPECT_THROW_MSG(attr->toVendorId(), TypeError, "the attribute value type must be vsa, not string"); + EXPECT_THROW_MSG(attr->toVsaData(), TypeError, + "the attribute value type must be vsa, not string"); } // Verifies raw string attribute. @@ -190,18 +171,13 @@ TEST_F(AttributeTest, rawAttrString) { string to_string; EXPECT_NO_THROW_LOG(to_string = attr->toString()); EXPECT_EQ("\x01\x02\x03", to_string); - EXPECT_EQ("User-Name=\x01\x02\x03", attr->toText()); + EXPECT_EQ("User-Name=0x010203", attr->toText()); vector binary = { 1, 5, 1, 2, 3 }; EXPECT_EQ(binary, attr->toBytes()); string expected = "{ \"type\": 1, \"name\": \"User-Name\", "; expected += " \"raw\": \"010203\" }"; runToElementTest(expected, *attr); - AttributePtr from_text = Attribute::fromText("User-Name=\x01\x02\x03"); - ASSERT_TRUE(from_text); - EXPECT_TRUE(compare(from_text, attr)) - << from_text->toText() << " != " << attr->toText(); - AttributePtr from_bytes = Attribute::fromBytes(binary); ASSERT_TRUE(from_bytes); EXPECT_TRUE(compare(from_bytes, attr)) @@ -232,19 +208,6 @@ TEST_F(AttributeTest, attrInt) { expected += " \"data\": \"15\" }"; runToElementTest(expected, *attr); - AttributePtr from_text = Attribute::fromText("NAS-Port-Type=Ethernet"); - ASSERT_TRUE(from_text); - EXPECT_TRUE(compare(from_text, attr)) - << from_text->toText() << " != " << attr->toText(); - - AttributePtr attr2; - ASSERT_NO_THROW(attr2 = Attribute::fromInt(61, 1155)); - ASSERT_TRUE(attr2); - AttributePtr from_text2 = Attribute::fromText("NAS-Port-Type=1155"); - ASSERT_TRUE(from_text2); - EXPECT_TRUE(compare(from_text2, attr2)) - << from_text2->toText() << " != " << attr2->toText(); - AttributePtr from_bytes = Attribute::fromBytes(binary); ASSERT_TRUE(from_bytes); EXPECT_TRUE(compare(from_bytes, attr)) @@ -270,9 +233,9 @@ TEST_F(AttributeTest, attrInt) { << def_bytes->toText() << " != " << attr->toText(); EXPECT_THROW_MSG(attr->toString(), TypeError, - "the attribute value type must be string or vsa, not integer"); + "the attribute value type must be string, not integer"); EXPECT_THROW_MSG(attr->toBinary(), TypeError, - "the attribute value type must be string or vsa, not integer"); + "the attribute value type must be string, not integer"); EXPECT_THROW_MSG(attr->toIpAddr(), TypeError, "the attribute value type must be ipaddr, not integer"); EXPECT_THROW_MSG(attr->toIpv6Addr(), TypeError, @@ -284,6 +247,8 @@ TEST_F(AttributeTest, attrInt) { ); EXPECT_THROW_MSG(attr->toVendorId(), TypeError, "the attribute value type must be vsa, not integer"); + EXPECT_THROW_MSG(attr->toVsaData(), TypeError, + "the attribute value type must be vsa, not integer"); } // Verifies IP address attribute. @@ -310,11 +275,6 @@ TEST_F(AttributeTest, attrIpAddr) { expected += " \"data\": \"192.0.2.1\" }"; runToElementTest(expected, *attr); - AttributePtr from_text = Attribute::fromText("Framed-IP-Address=192.0.2.1"); - ASSERT_TRUE(from_text); - EXPECT_TRUE(compare(from_text, attr)) - << from_text->toText() << " != " << attr->toText(); - EXPECT_THROW_MSG(Attribute::fromIpAddr(8, IOAddress("2001:db8::1235")), BadValue, "not v4 address 2001:db8::1235"); @@ -343,9 +303,9 @@ TEST_F(AttributeTest, attrIpAddr) { << def_bytes->toText() << " != " << attr->toText(); EXPECT_THROW_MSG(attr->toString(), TypeError, - "the attribute value type must be string or vsa, not ipaddr"); + "the attribute value type must be string, not ipaddr"); EXPECT_THROW_MSG(attr->toBinary(), TypeError, - "the attribute value type must be string or vsa, not ipaddr"); + "the attribute value type must be string, not ipaddr"); EXPECT_THROW_MSG(attr->toInt(), TypeError, "the attribute value type must be integer, not ipaddr"); EXPECT_THROW_MSG(attr->toIpv6Addr(), TypeError, @@ -356,6 +316,8 @@ TEST_F(AttributeTest, attrIpAddr) { "the attribute value type must be ipv6prefix, not ipaddr"); EXPECT_THROW_MSG(attr->toVendorId(), TypeError, "the attribute value type must be vsa, not ipaddr"); + EXPECT_THROW_MSG(attr->toVsaData(), TypeError, + "the attribute value type must be vsa, not ipaddr"); } // Verifies IPv6 address attribute. @@ -384,19 +346,13 @@ TEST_F(AttributeTest, attrIpv6Addr) { expected += " \"data\": \"2001:db8::1235\" }"; runToElementTest(expected, *attr); - AttributePtr from_text = - Attribute::fromText("Framed-IPv6-Address=2001:db8::1235"); - ASSERT_TRUE(from_text); - EXPECT_TRUE(compare(from_text, attr)) - << from_text->toText() << " != " << attr->toText(); - EXPECT_THROW_MSG(Attribute::fromIpv6Addr(168, IOAddress("192.0.2.1")), BadValue, "not v6 address 192.0.2.1"); AttributePtr def_text = Attribute::fromText(def, "2001:db8::1235"); - ASSERT_TRUE(from_text); - EXPECT_TRUE(compare(from_text, attr)) - << from_text->toText() << " != " << attr->toText(); + ASSERT_TRUE(def_text); + EXPECT_TRUE(compare(def_text, attr)) + << def_text->toText() << " != " << attr->toText(); AttributePtr from_bytes = Attribute::fromBytes(binary); ASSERT_TRUE(from_bytes); @@ -422,9 +378,9 @@ TEST_F(AttributeTest, attrIpv6Addr) { << def_bytes->toText() << " != " << attr->toText(); EXPECT_THROW_MSG(attr->toString(), TypeError, - "the attribute value type must be string or vsa, not ipv6addr"); + "the attribute value type must be string, not ipv6addr"); EXPECT_THROW_MSG(attr->toBinary(), TypeError, - "the attribute value type must be string or vsa, not ipv6addr"); + "the attribute value type must be string, not ipv6addr"); EXPECT_THROW_MSG(attr->toInt(), TypeError, "the attribute value type must be integer, not ipv6addr"); EXPECT_THROW_MSG(attr->toIpAddr(), TypeError, @@ -435,6 +391,8 @@ TEST_F(AttributeTest, attrIpv6Addr) { "the attribute value type must be ipv6prefix, not ipv6addr"); EXPECT_THROW_MSG(attr->toVendorId(), TypeError, "the attribute value type must be vsa, not ipv6addr"); + EXPECT_THROW_MSG(attr->toVsaData(), TypeError, + "the attribute value type must be vsa, not ipv6addr"); } // Verifies IPv6 prefix attribute. @@ -466,28 +424,11 @@ TEST_F(AttributeTest, attrIpv6Prefix) { expected += " \"data\": \"2001:db8::1235/128\" }"; runToElementTest(expected, *attr); - AttributePtr from_text; - EXPECT_NO_THROW_LOG(from_text = Attribute::fromText("Delegated-IPv6-Prefix=2001:db8::1235/128")); - ASSERT_TRUE(from_text); - EXPECT_TRUE(compare(from_text, attr)) - << from_text->toText() << " != " << attr->toText(); - - EXPECT_THROW_MSG(Attribute::fromText("Delegated-IPv6-Prefix=192.0.2.1/32"), - BadValue, "not v6 address 192.0.2.1"); - AttributePtr def_text; EXPECT_NO_THROW_LOG(def_text = Attribute::fromText(def, "2001:db8::1235/128")); - ASSERT_TRUE(from_text); - EXPECT_TRUE(compare(from_text, attr)) - << from_text->toText() << " != " << attr->toText(); - - string long_prefix = "Delegated-IPv6-Prefix=2001:db8::1235/300"; - EXPECT_THROW_MSG(Attribute::fromText(long_prefix), BadValue, - "not 8 bit prefix length 2001:db8::1235/300"); - - long_prefix = "Delegated-IPv6-Prefix=2001:db8::1235/129"; - EXPECT_THROW_MSG(Attribute::fromText(long_prefix), BadValue, - "too long prefix 129"); + ASSERT_TRUE(def_text); + EXPECT_TRUE(compare(def_text, attr)) + << def_text->toText() << " != " << attr->toText(); AttributePtr from_bytes; EXPECT_NO_THROW_LOG(from_bytes = Attribute::fromBytes(binary)); @@ -533,9 +474,9 @@ TEST_F(AttributeTest, attrIpv6Prefix) { << def_bytes->toText() << " != " << attr->toText(); EXPECT_THROW_MSG(attr->toString(), TypeError, - "the attribute value type must be string or vsa, not ipv6prefix"); + "the attribute value type must be string, not ipv6prefix"); EXPECT_THROW_MSG(attr->toBinary(), TypeError, - "the attribute value type must be string or vsa, not ipv6prefix"); + "the attribute value type must be string, not ipv6prefix"); EXPECT_THROW_MSG(attr->toInt(), TypeError, "the attribute value type must be integer, not ipv6prefix"); EXPECT_THROW_MSG(attr->toIpAddr(), TypeError, @@ -544,6 +485,109 @@ TEST_F(AttributeTest, attrIpv6Prefix) { "the attribute value type must be ipv6addr, not ipv6prefix"); EXPECT_THROW_MSG(attr->toVendorId(), TypeError, "the attribute value type must be vsa, not ipv6prefix"); + EXPECT_THROW_MSG(attr->toVsaData(), TypeError, + "the attribute value type must be vsa, not ipv6prefix"); +} + +// Verifies vsa attribute. +TEST_F(AttributeTest, attrVsa) { + // Using Vector-Specific (26) *only* vsa attribute. + AttrDefPtr def = AttrDefs::instance().getByType(PW_VENDOR_SPECIFIC); + ASSERT_TRUE(def); + EXPECT_EQ(26, def->type_); + EXPECT_EQ(PW_TYPE_VSA, def->value_type_); + + AttributePtr attr; + ASSERT_NO_THROW(attr = Attribute::fromVsa(PW_VENDOR_SPECIFIC, + 1234, "foobar")); + ASSERT_TRUE(attr); + + EXPECT_EQ(26, attr->getType()); + EXPECT_EQ(PW_TYPE_VSA, attr->getValueType()); + uint32_t vendor = 0; + ASSERT_NO_THROW(vendor = attr->toVendorId()); + EXPECT_EQ(1234, vendor); + EXPECT_EQ("Vendor-Specific=[1234]0x666F6F626172", attr->toText()); + vector binary = { 26, 12, 0, 0, 0x04, 0xd2, + 0x66, 0x6f, 0x6f, 0x62, 0x61, 0x72 }; + EXPECT_EQ(binary, attr->toBytes()); + string expected = "{ \"type\": 26, \"name\": \"Vendor-Specific\", "; + expected += " \"vendor\": \"1234\", \"vsa-raw\": \"666F6F626172\" }"; + runToElementTest(expected, *attr); + + AttributePtr from_bytes = Attribute::fromBytes(binary); + ASSERT_TRUE(from_bytes); + EXPECT_TRUE(compare(from_bytes, attr)) + << from_bytes->toText() << " != " << attr->toText(); + + vector value = { 0x66, 0x6f, 0x6f, 0x62, 0x61, 0x72 }; + AttributePtr def_value = Attribute::fromVsa(PW_VENDOR_SPECIFIC, + 1234, value); + ASSERT_TRUE(def_value); + EXPECT_TRUE(compare(def_value, attr)) + << def_value->toText() << " != " << attr->toText(); + + vector bytes = { 0, 0, 0x04, 0xd2, + 0x66, 0x6f, 0x6f, 0x62, 0x61, 0x72 }; + AttributePtr def_bytes = Attribute::fromBytes(def, bytes); + ASSERT_TRUE(def_bytes); + EXPECT_TRUE(compare(def_bytes, attr)) + << def_bytes->toText() << " != " << attr->toText(); + + EXPECT_THROW_MSG(Attribute::fromVsa(PW_VENDOR_SPECIFIC, 1234, ""), + BadValue, "value is empty"); + + vector empty; + EXPECT_THROW_MSG(Attribute::fromVsa(PW_VENDOR_SPECIFIC, 1234, empty), + BadValue, "value is empty"); + + vector small = { 26, 6, 0, 0, 0x4, 0xd2 }; + EXPECT_THROW_MSG(Attribute::fromBytes(small), BadValue, + "value is too small 4 < 5"); + + vector small2 = { 0, 0, 0x4, 0xd2 }; + EXPECT_THROW_MSG(Attribute::fromBytes(def, small2), BadValue, + "value is too small 4 < 5"); + + string big_text(MAX_VSA_DATA_LEN, 'x'); + EXPECT_NO_THROW_LOG(Attribute::fromVsa(PW_VENDOR_SPECIFIC, + 1234, big_text)); + + vector big_binary(MAX_VSA_DATA_LEN, 0x78); + EXPECT_NO_THROW_LOG(Attribute::fromVsa(PW_VENDOR_SPECIFIC, + 1234, big_binary)); + + vector big_value(MAX_STRING_LEN, 0x87); + EXPECT_NO_THROW_LOG(Attribute::fromBytes(def, big_value)); + + string too_big_text(MAX_VSA_DATA_LEN + 1, 'x'); + EXPECT_THROW_MSG(Attribute::fromVsa(PW_VENDOR_SPECIFIC, 1234, + too_big_text), BadValue, + "value is too large 250 > 249"); + + vector too_big_binary(MAX_VSA_DATA_LEN + 1, 0x87); + EXPECT_THROW_MSG(Attribute::fromVsa(PW_VENDOR_SPECIFIC, 1234, + too_big_binary), BadValue, + "value is too large 250 > 249"); + + vector too_bigvalue(MAX_STRING_LEN + 1, 0x87); + EXPECT_THROW_MSG(Attribute::fromBytes(def, too_bigvalue), BadValue, + "value is too large 254 > 253"); + + EXPECT_THROW_MSG(attr->toString(), TypeError, + "the attribute value type must be string, not vsa"); + EXPECT_THROW_MSG(attr->toBinary(), TypeError, + "the attribute value type must be string, not vsa"); + EXPECT_THROW_MSG(attr->toInt(), TypeError, + "the attribute value type must be integer, not vsa"); + EXPECT_THROW_MSG(attr->toIpAddr(), TypeError, + "the attribute value type must be ipaddr, not vsa"); + EXPECT_THROW_MSG(attr->toIpv6Addr(), TypeError, + "the attribute value type must be ipv6addr, not vsa"); + EXPECT_THROW_MSG(attr->toIpv6Prefix(), TypeError, + "the attribute value type must be ipv6prefix, not vsa"); + EXPECT_THROW_MSG(attr->toIpv6PrefixLen(), TypeError, + "the attribute value type must be ipv6prefix, not vsa"); } // Verifies basic methods for attribute collection. @@ -604,8 +648,8 @@ TEST_F(AttributeTest, attributesAddDel) { attr.reset(); // toText. - string expected = "User-Name=foobar,\nUser-Name=foo,\n"; - expected += "Service-Type=20,\nUser-Name=bar"; + string expected = "User-Name='foobar',\nUser-Name='foo',\n"; + expected += "Service-Type=20,\nUser-Name='bar'"; string got; ASSERT_NO_THROW(got = attrs.toText()); EXPECT_EQ(expected, got) << expected << "\n" << got << "\n"; diff --git a/src/hooks/dhcp/radius/tests/config_unittests.cc b/src/hooks/dhcp/radius/tests/config_unittests.cc index f1caf080d8..d060aa21bc 100644 --- a/src/hooks/dhcp/radius/tests/config_unittests.cc +++ b/src/hooks/dhcp/radius/tests/config_unittests.cc @@ -792,7 +792,7 @@ TEST_F(ConfigTest, attribute) { ASSERT_EQ(1, attrs.size()); const ConstAttributePtr& first = *attrs.cbegin(); ASSERT_TRUE(first); - EXPECT_EQ("User-Name=foobar", first->toText()); + EXPECT_EQ("User-Name='foobar'", first->toText()); // Another way to check. string expected = "[ { " @@ -801,6 +801,15 @@ TEST_F(ConfigTest, attribute) { " \"data\": \"foobar\" } ]"; runToElementTest(expected, srv->attributes_); + // Vendor-Specific (26) does not support textual data. + srv->attributes_.clear(); + attr = Element::createMap(); + attr->set("data", Element::create("foobar")); + attr->set("type", Element::create(26)); + expected = "can't create Vendor-Specific attribute from [foobar]: "; + expected += "Can't decode vsa from text"); + EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, expected); + // One of expr, data, raw srv->attributes_.clear(); attr = Element::createMap(); @@ -845,9 +854,7 @@ TEST_F(ConfigTest, attribute) { EXPECT_EQ("", srv->attributes_.getTest(1)); const ConstAttributePtr& firstr = srv->attributes_.get(1); ASSERT_TRUE(firstr); - expected = "User-Name=f\x01\x02"; - expected += "bar"; - EXPECT_EQ(expected, firstr->toText()); + EXPECT_EQ("User-Name=0x660102626172", firstr->toText()); expected = "[ { " " \"name\": \"User-Name\", " " \"type\": 1, " diff --git a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc index a0589478d3..9cbacbe61e 100644 --- a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc +++ b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc @@ -140,6 +140,8 @@ TEST_F(DictionaryTest, parseLine) { "expected 3 tokens, got 2 at line 1"); EXPECT_THROW_MSG(parseLine("VENDOR my-vendor 44 17"), BadValue, "expected 3 tokens, got 4 at line 1"); + EXPECT_THROW_MSG(parseLine("VENDOR my-vendor 0"), BadValue, + "0 is reserved at line 1"); EXPECT_THROW_MSG(parseLine("BEGIN-VENDOR my-vendor"), BadValue, "unknown dictionary entry 'BEGIN-VENDOR' at line 1"); diff --git a/src/hooks/dhcp/radius/tests/request_unittests.h b/src/hooks/dhcp/radius/tests/request_unittests.h index 05aafd88cb..981a3b6dc5 100644 --- a/src/hooks/dhcp/radius/tests/request_unittests.h +++ b/src/hooks/dhcp/radius/tests/request_unittests.h @@ -743,7 +743,7 @@ TEST_F(RequestTest, accept) { ASSERT_EQ(1, received_attributes_->count(1)); const ConstAttributePtr& attr = received_attributes_->get(1); ASSERT_TRUE(attr); - EXPECT_EQ("User-Name=user", attr->toText()); + EXPECT_EQ("User-Name='user'", attr->toText()); } /// Verify what happens with Accounting-Response response. @@ -938,7 +938,7 @@ TEST_F(RequestTest, badAccept) { ASSERT_EQ(1, received_attributes_->count(1)); const ConstAttributePtr& attr = received_attributes_->get(1); ASSERT_TRUE(attr); - EXPECT_EQ("User-Name=user", attr->toText()); + EXPECT_EQ("User-Name='user'", attr->toText()); } /// Verify what happens with bad Accounting-Response response. @@ -1326,7 +1326,7 @@ TEST_F(RequestTest, reject) { ASSERT_EQ(1, received_attributes_->count(1)); const ConstAttributePtr& attr = received_attributes_->get(1); ASSERT_TRUE(attr); - EXPECT_EQ("User-Name=user", attr->toText()); + EXPECT_EQ("User-Name='user'", attr->toText()); } /// Verify what happens with a backup authentication server. @@ -1427,7 +1427,7 @@ TEST_F(RequestTest, accept2) { ASSERT_EQ(1, received_attributes_->count(1)); const ConstAttributePtr& attr = received_attributes_->get(1); ASSERT_TRUE(attr); - EXPECT_EQ("User-Name=user", attr->toText()); + EXPECT_EQ("User-Name='user'", attr->toText()); } /// Verify what happens with a backup accounting server. From 854d7e14fbb7e9d74ddc6cd2057c2839a41963cb Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Thu, 21 Aug 2025 10:57:23 +0200 Subject: [PATCH 05/13] [#3860] Checkpoint vendor in def --- src/hooks/dhcp/radius/client_dictionary.cc | 57 +++++++----- src/hooks/dhcp/radius/client_dictionary.h | 90 ++++++++++++++++--- .../dhcp/radius/tests/config_unittests.cc | 2 +- 3 files changed, 110 insertions(+), 39 deletions(-) diff --git a/src/hooks/dhcp/radius/client_dictionary.cc b/src/hooks/dhcp/radius/client_dictionary.cc index 479db62e0d..92839e62f1 100644 --- a/src/hooks/dhcp/radius/client_dictionary.cc +++ b/src/hooks/dhcp/radius/client_dictionary.cc @@ -68,9 +68,9 @@ AttrDefs::instance() { } AttrDefPtr -AttrDefs::getByType(const uint8_t type) const { +AttrDefs::getByType(const uint8_t type, const uint32_t vendor) const { auto const& idx = container_.get<0>(); - auto it = idx.find(type); + auto it = idx.find(boost::make_tuple(vendor, type)); if (it != idx.end()) { return (*it); } @@ -78,15 +78,15 @@ AttrDefs::getByType(const uint8_t type) const { } AttrDefPtr -AttrDefs::getByName(const string& name) const { +AttrDefs::getByName(const string& name, const uint32_t vendor) const { auto const& idx = container_.get<1>(); - auto it = idx.find(name); + auto it = idx.find(boost::make_tuple(vendor, name)); if (it != idx.end()) { return (*it); } - auto alias = aliases_.find(name); + auto alias = aliases_.find(boost::make_tuple(vendor, name)); if (alias != aliases_.end()) { - auto ita = idx.find(alias->second); + auto ita = idx.find(boost::make_tuple(vendor, alias->name_)); if (ita != idx.end()) { return (*ita); } @@ -100,42 +100,51 @@ AttrDefs::add(AttrDefPtr def) { return; } auto& idx1 = container_.get<1>(); - auto it1 = idx1.find(def->name_); + auto it1 = idx1.find(boost::make_tuple(def->vendor_, def->name_)); if (it1 != idx1.end()) { if ((def->type_ == (*it1)->type_) && (def->value_type_ == (*it1)->value_type_)) { // Duplicate: ignore. return; } - isc_throw(BadValue, "Illegal attribute redefinition of '" << def->name_ - << "' type " << static_cast((*it1)->type_) - << " value type " << attrValueTypeToText((*it1)->value_type_) - << " by " << static_cast(def->type_) - << " " << attrValueTypeToText(def->value_type_)); + ostringstream msg; + msg << "Illegal attribute redefinition of '" << def->name_ << "'"; + if (def->vendor_ != 0) { + msg << " vendor " << def->vendor_; + } + msg << " type " << static_cast((*it1)->type_) + << " value type " << attrValueTypeToText((*it1)->value_type_) + << " by " << static_cast(def->type_) + << " " << attrValueTypeToText(def->value_type_); + isc_throw(BadValue, msg.str()); } auto& idx0 = container_.get<0>(); - auto it0 = idx0.find(def->type_); + auto it0 = idx0.find(boost::make_tuple(def->vendor_, def->type_)); if (it0 != idx0.end()) { if (def->value_type_ == (*it0)->value_type_) { // Alias. - auto p = pair(def->name_, (*it0)->name_); - static_cast(aliases_.insert(p)); + AttrDefAlias alias(def->name_, (*it0)->name_, def->vendor_); + static_cast(aliases_.insert(alias)); return; } - isc_throw(BadValue, "Illegal attribute redefinition of '" - << (*it0)->name_ << "' type " - << static_cast((*it0)->type_) << " value type " - << attrValueTypeToText((*it0)->value_type_) - << " by '" << def->name_ << "' " - << static_cast(def->type_) << " " - << attrValueTypeToText(def->value_type_)); + ostringstream msg; + msg << "Illegal attribute redefinition of '" << (*it0)->name_ << "'"; + if (def->vendor_ != 0) { + msg << " vendor " << def->vendor_; + } + msg << " type " << static_cast((*it0)->type_) + << " value type " << attrValueTypeToText((*it0)->value_type_) + << " by '" << def->name_ << "' " + << static_cast(def->type_) << " " + << attrValueTypeToText(def->value_type_); + isc_throw(BadValue, msg.str()); } static_cast(container_.insert(def)); } string -AttrDefs::getName(const uint8_t type) const { - AttrDefPtr def = getByType(type); +AttrDefs::getName(const uint8_t type, const uint32_t vendor) const { + AttrDefPtr def = getByType(type, vendor); if (def) { return (def->name_); } diff --git a/src/hooks/dhcp/radius/client_dictionary.h b/src/hooks/dhcp/radius/client_dictionary.h index 4b6824faee..8af9df0f56 100644 --- a/src/hooks/dhcp/radius/client_dictionary.h +++ b/src/hooks/dhcp/radius/client_dictionary.h @@ -58,9 +58,10 @@ class AttrDef { /// @param type attribute type. /// @param name attribute name. /// @param value_type attribute value type. + /// @param vendor vendor id (default 0). AttrDef(const uint8_t type, const std::string& name, - const AttrValueType value_type) - : type_(type), name_(name), value_type_(value_type) { + const AttrValueType value_type, const uint32_t vendor = 0) + : type_(type), name_(name), value_type_(value_type), vendor_(vendor) { } /// @brief type. @@ -71,6 +72,9 @@ class AttrDef { /// @brief value_type. const AttrValueType value_type_; + + /// @brief vendor id (default 0). + const uint32_t vendor_; }; /// @brief Shared pointers to Attribute definition. @@ -79,6 +83,30 @@ typedef boost::shared_ptr AttrDefPtr; /// @brief List of Attribute definitions. typedef std::list AttrDefList; +/// @brief RADIUS attribute aliases. +class AttrDefAlias { +public: + + /// @brief Constructor. + /// + /// @param alias attribute alias name. + /// @param name attribute name. + /// @param vendor vendor id (default 0). + AttrDefAlias(const std::string& alias, const std::string& name, + const uint32_t vendor = 0) + : alias_(alias), name_(name), vendor_(vendor) { + } + + /// @brief alias. + const std::string alias_; + + /// @brief name. + const std::string name_; + + /// @brief vendor id (default 0). + const uint32_t vendor_; +}; + /// @brief RADIUS integer constant definitions. /// /// Include vendor ids with Vendor-Specific attribute. @@ -118,23 +146,53 @@ class AttrDefs : public boost::noncopyable { AttrDefPtr, // Start specification of indexes here. boost::multi_index::indexed_by< - // Hash index for by type. + // Hash index for by vendor and type. boost::multi_index::hashed_unique< - boost::multi_index::member< - AttrDef, const uint8_t, &AttrDef::type_ + boost::multi_index::composite_key< + AttrDef, + boost::multi_index::member< + AttrDef, const uint32_t, &AttrDef::vendor_ + >, + boost::multi_index::member< + AttrDef, const uint8_t, &AttrDef::type_ + > > >, - // Hash index for by name. + // Hash index for by vendor and name. boost::multi_index::hashed_unique< - boost::multi_index::member< - AttrDef, const std::string, &AttrDef::name_ + boost::multi_index::composite_key< + AttrDef, + boost::multi_index::member< + AttrDef, const uint32_t, &AttrDef::vendor_ + >, + boost::multi_index::member< + AttrDef, const std::string, &AttrDef::name_ + > > > > > AttrDefContainer; /// @brief Type of the alias table (alias -> standard name map). - typedef std::unordered_map AttrDefAliases; + typedef boost::multi_index_container< + // This container stores aliases. + AttrDefAlias, + // Start specification of indexes here. + boost::multi_index::indexed_by< + // Hash index for by vendor and alias. + boost::multi_index::hashed_unique< + boost::multi_index::composite_key< + AttrDefAlias, + boost::multi_index::member< + AttrDefAlias, const uint32_t, &AttrDefAlias::vendor_ + >, + boost::multi_index::member< + AttrDefAlias, const std::string, &AttrDefAlias::alias_ + > + > + > + > + > AttrDefAliases; /// @brief Type of the integer constant definition container. typedef boost::multi_index_container< @@ -176,17 +234,20 @@ class AttrDefs : public boost::noncopyable { /// @return the single instance. static AttrDefs& instance(); - /// @brief Get attribute definition by type. + /// @brief Get attribute definition by type and vendor. /// /// @param type type to look for. + /// @param vendor vendor id to look for (default 0). /// @return pointer to the attribute definition or null. - AttrDefPtr getByType(const uint8_t type) const; + AttrDefPtr getByType(const uint8_t type, const uint32_t vendor = 0) const; - /// @brief Get attribute definition by name. + /// @brief Get attribute definition by name and vendor. /// /// @param name name to look for. + /// @param vendor vendor id to look for (default 0). /// @return pointer to the attribute definition or null. - AttrDefPtr getByName(const std::string& name) const; + AttrDefPtr getByName(const std::string& name, + const uint32_t vendor = 0) const; /// @brief Add (or replace) an attribute definition. /// @@ -203,7 +264,8 @@ class AttrDefs : public boost::noncopyable { /// @brief Get attribute name. /// /// @param type type to look for. - std::string getName(const uint8_t type) const; + /// @param vendor vendor id to look for (default 0). + std::string getName(const uint8_t type, const uint32_t vendor = 0) const; /// @brief Get integer constant definition by attribute type and name. /// diff --git a/src/hooks/dhcp/radius/tests/config_unittests.cc b/src/hooks/dhcp/radius/tests/config_unittests.cc index d060aa21bc..2eb232db37 100644 --- a/src/hooks/dhcp/radius/tests/config_unittests.cc +++ b/src/hooks/dhcp/radius/tests/config_unittests.cc @@ -807,7 +807,7 @@ TEST_F(ConfigTest, attribute) { attr->set("data", Element::create("foobar")); attr->set("type", Element::create(26)); expected = "can't create Vendor-Specific attribute from [foobar]: "; - expected += "Can't decode vsa from text"); + expected += "Can't decode vsa from text"; EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, expected); // One of expr, data, raw From de7139a94b45f399d91940656fb1323ce8f0b554 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Thu, 21 Aug 2025 20:18:58 +0200 Subject: [PATCH 06/13] [#3860] More vendor --- src/hooks/dhcp/radius/cfg_attribute.cc | 51 +++++++------- src/hooks/dhcp/radius/cfg_attribute.h | 64 +++++++++++++++--- src/hooks/dhcp/radius/client_attribute.cc | 34 +++++++++- src/hooks/dhcp/radius/client_attribute.h | 34 ++++++++-- .../dhcp/radius/tests/attribute_unittests.cc | 32 +++++++++ .../dhcp/radius/tests/dictionary_unittests.cc | 66 +++++++++++++++++++ 6 files changed, 239 insertions(+), 42 deletions(-) diff --git a/src/hooks/dhcp/radius/cfg_attribute.cc b/src/hooks/dhcp/radius/cfg_attribute.cc index 81a855d462..54bd2c814d 100644 --- a/src/hooks/dhcp/radius/cfg_attribute.cc +++ b/src/hooks/dhcp/radius/cfg_attribute.cc @@ -28,13 +28,12 @@ CfgAttributes::add(const AttrDefPtr& def, if (!def) { isc_throw(BadValue, "no attribute definition"); } - container_.insert(pair - (def->type_, AttributeValue(def, attr, expr, test))); + container_.insert(AttributeValue(def, attr, expr, test)); } bool -CfgAttributes::del(const uint8_t type) { - auto it = container_.find(type); +CfgAttributes::del(const uint8_t type, const uint32_t vendor) { + auto it = container_.find(boost::make_tuple(vendor, type)); if (it != container_.end()) { container_.erase(it); return (true); @@ -43,46 +42,46 @@ CfgAttributes::del(const uint8_t type) { } AttrDefPtr -CfgAttributes::getDef(const uint8_t type) const { - auto it = container_.find(type); +CfgAttributes::getDef(const uint8_t type, const uint32_t vendor) const { + auto it = container_.find(boost::make_tuple(vendor, type)); if (it == container_.end()) { return (AttrDefPtr()); } - return (it->second.def_); + return (it->def_); } ConstAttributePtr -CfgAttributes::get(const uint8_t type) const { - auto it = container_.find(type); +CfgAttributes::get(const uint8_t type, const uint32_t vendor) const { + auto it = container_.find(boost::make_tuple(vendor, type)); if (it == container_.end()) { return (AttributePtr()); } - return (it->second.attr_); + return (it->attr_); } ExpressionPtr -CfgAttributes::getExpr(const uint8_t type) const { - auto it = container_.find(type); +CfgAttributes::getExpr(const uint8_t type, const uint32_t vendor) const { + auto it = container_.find(boost::make_tuple(vendor, type)); if (it == container_.end()) { return (ExpressionPtr()); } - return (it->second.expr_); + return (it->expr_); } string -CfgAttributes::getTest(const uint8_t type) const { - auto it = container_.find(type); +CfgAttributes::getTest(const uint8_t type, const uint32_t vendor) const { + auto it = container_.find(boost::make_tuple(vendor, type)); if (it == container_.end()) { return (""); } - return (it->second.test_); + return (it->test_); } Attributes CfgAttributes::getAll() const { Attributes attrs; for (auto const& it : container_) { - attrs.add(it.second.attr_); + attrs.add(it.attr_); } return (attrs); } @@ -91,16 +90,16 @@ Attributes CfgAttributes::getEvalAll(Pkt& pkt) { Attributes attrs; for (auto const& it : container_) { - const ExpressionPtr& match_expr = it.second.expr_; + const ExpressionPtr& match_expr = it.expr_; if (!match_expr) { - attrs.add(it.second.attr_); + attrs.add(it.attr_); continue; } string value = evaluateString(*match_expr, pkt); if (value.empty()) { continue; } - AttrDefPtr def = it.second.def_; + AttrDefPtr def = it.def_; if (!def) { continue; } @@ -117,18 +116,18 @@ ElementPtr CfgAttributes::toElement() const { ElementPtr result = Element::createList(); for (auto const& it : container_) { - AttrDefPtr def = it.second.def_; + AttrDefPtr def = it.def_; if (!def) { continue; } ElementPtr map; - if (!it.second.test_.empty()) { + if (!it.test_.empty()) { map = Element::createMap(); - map->set("type", Element::create(static_cast(it.first))); - map->set("expr", Element::create(it.second.test_)); + map->set("type", Element::create(static_cast(def->type_))); + map->set("expr", Element::create(it.test_)); map->set("name", Element::create(def->name_)); - } else if (it.second.attr_) { - map = it.second.attr_->toElement(); + } else if (it.attr_) { + map = it.attr_->toElement(); } result->add(map); } diff --git a/src/hooks/dhcp/radius/cfg_attribute.h b/src/hooks/dhcp/radius/cfg_attribute.h index c8dd89c988..d97b6d6731 100644 --- a/src/hooks/dhcp/radius/cfg_attribute.h +++ b/src/hooks/dhcp/radius/cfg_attribute.h @@ -14,6 +14,10 @@ #include #include #include +#include +#include +#include +#include #include #include #include @@ -81,8 +85,9 @@ class CfgAttributes : public data::CfgToElement { /// Deletes only the first occurrence. /// /// @param type type of the attribute to delete. + /// @param vendor vendor id (default 0). /// @return true if found. - bool del(const uint8_t type); + bool del(const uint8_t type, const uint32_t vendor = 0); /// @brief Clear the container. void clear() { @@ -92,34 +97,41 @@ class CfgAttributes : public data::CfgToElement { /// @brief Counts instance of the attribute in the configuration. /// /// @param type type of the attribute to count. - /// @return count of attributes with the given type in the configuration. - size_t count(const uint8_t type) const { - return (container_.count(type)); + /// @param vendor vendor id (default 0). + /// @return count of attributes with the given type and vendor + /// in the configuration. + size_t count(const uint8_t type, const uint32_t vendor = 0) const { + return (container_.count(boost::make_tuple(vendor, type))); } /// @brief Returns the definition of an attribute. /// /// @param type type of the attribute. + /// @param vendor vendor id (default 0). /// @return the definition of the attribute. - AttrDefPtr getDef(const uint8_t type) const; + AttrDefPtr getDef(const uint8_t type, const uint32_t vendor = 0) const; /// @brief Get instance of the attribute in the configuration. /// /// @param type type of the attribute to retrieve. + /// @param vendor vendor id (default 0). /// @return the first instance if exists. - ConstAttributePtr get(const uint8_t type) const; + ConstAttributePtr get(const uint8_t type, const uint32_t vendor = 0) const; /// @brief Returns the expression of an attribute. /// /// @param type type of the attribute. + /// @param vendor vendor id (default 0). /// @return the expression of the attribute. - dhcp::ExpressionPtr getExpr(const uint8_t type) const; + dhcp::ExpressionPtr getExpr(const uint8_t type, + const uint32_t vendor = 0) const; /// @brief Returns the text expression of an attribute. /// /// @param type type of the attribute. + /// @param vendor vendor id (default 0). /// @return the text expression of the attribute. - std::string getTest(const uint8_t type) const; + std::string getTest(const uint8_t type, const uint32_t vendor = 0) const; /// @brief Get all attributes in the configuration. /// @@ -182,10 +194,44 @@ class CfgAttributes : public data::CfgToElement { /// @brief Original expression. std::string test_; + + /// @brief Return the type. + uint8_t getType() const { + if (!def_) { + isc_throw(BadValue, "no attribute definition"); + } + return (def_->type_); + } + + /// @brief Return the vendor. + uint32_t getVendor() const { + if (!def_) { + isc_throw(BadValue, "no attribute definition"); + } + return (def_->vendor_); + } }; /// @brief The container. - std::multimap container_; + boost::multi_index_container< + // This container stores AttributeValue objects. + AttributeValue, + // Start specification of indexes here. + boost::multi_index::indexed_by< + // Hash index for by vendor and type. + boost::multi_index::hashed_non_unique< + boost::multi_index::composite_key< + AttributeValue, + boost::multi_index::const_mem_fun< + AttributeValue, uint32_t, &AttributeValue::getVendor + >, + boost::multi_index::const_mem_fun< + AttributeValue, uint8_t, &AttributeValue::getType + > + > + > + > + > container_; }; } // end of namespace isc::radius diff --git a/src/hooks/dhcp/radius/client_attribute.cc b/src/hooks/dhcp/radius/client_attribute.cc index 7840bd4b97..bb77f5e646 100644 --- a/src/hooks/dhcp/radius/client_attribute.cc +++ b/src/hooks/dhcp/radius/client_attribute.cc @@ -32,6 +32,17 @@ Attribute::fromText(const AttrDefPtr& def, const string& value) { if (value.empty()) { isc_throw(BadValue, "empty attribute value"); } + AttributePtr attr = fromText0(def, value); + if (def->vendor_ == 0) { + return (attr); + } + // Encapsulate into a Vendor-Specific attribute. + const vector vsa_data = attr->toBytes(); + return (fromVsa(PW_VENDOR_SPECIFIC, def->vendor_, vsa_data)); +} + +AttributePtr +Attribute::fromText0(const AttrDefPtr& def, const string& value) { switch (static_cast(def->value_type_)) { case PW_TYPE_STRING: return (AttrString::fromText(def->type_, value)); @@ -92,6 +103,17 @@ Attribute::fromBytes(const AttrDefPtr& def, const vector& value) { if (value.empty()) { isc_throw(BadValue, "empty attribute value"); } + AttributePtr attr = fromBytes0(def, value); + if (def->vendor_ == 0) { + return (attr); + } + // Encapsulate into a Vendor-Specific attribute. + const vector vsa_data = attr->toBytes(); + return (fromVsa(PW_VENDOR_SPECIFIC, def->vendor_, vsa_data)); +} + +AttributePtr +Attribute::fromBytes0(const AttrDefPtr& def, const vector& value) { switch (static_cast(def->value_type_)) { case PW_TYPE_STRING: return (AttrString::fromBytes(def->type_, value)); @@ -209,7 +231,7 @@ Attribute::toVendorId() const { << attrValueTypeToText(getValueType())); } -string +std::vector Attribute::toVsaData() const { isc_throw(TypeError, "the attribute value type must be vsa, not " << attrValueTypeToText(getValueType())); @@ -695,6 +717,16 @@ AttrVsa::toBytes() const { return (output); } +std::vector +AttrVsa::toVsaData() const { + vector binary; + binary.resize(value_.size()); + if (binary.size() > 0) { + memmove(&binary[0], &value_[0], binary.size()); + } + return (binary); +} + ElementPtr AttrVsa::toElement() const { ElementPtr output = Element::createMap(); diff --git a/src/hooks/dhcp/radius/client_attribute.h b/src/hooks/dhcp/radius/client_attribute.h index d98ee9d601..db00b2e2e5 100644 --- a/src/hooks/dhcp/radius/client_attribute.h +++ b/src/hooks/dhcp/radius/client_attribute.h @@ -78,7 +78,9 @@ class Attribute : public data::CfgToElement { /// From definition generic factories. - /// @brief From text with definition. + /// @brief From text with definition (handle vendor). + /// + /// Handle Vendor-Specific encapsulation. /// /// @param def pointer to attribute definition. /// @param value textual value. @@ -87,7 +89,9 @@ class Attribute : public data::CfgToElement { static AttributePtr fromText(const AttrDefPtr& def, const std::string& value); - /// @brief From bytes with definition. + /// @brief From bytes with definition (handle vendor). + /// + /// Handle Vendor-Specific encapsulation. /// /// @param def pointer to attribute definition. /// @param value binary value. @@ -251,10 +255,30 @@ class Attribute : public data::CfgToElement { /// /// @return the vsa data. /// @throw TypeError if the attribute is not a vsa one. - virtual std::string toVsaData() const; + virtual std::vector toVsaData() const; /// @brief Type. const uint8_t type_; + +private: + + /// @brief From text with definition. + /// + /// @param def pointer to attribute definition. + /// @param value textual value. + /// @return pointer to the attribute. + /// @throw BadValue on errors. + static AttributePtr fromText0(const AttrDefPtr& def, + const std::string& value); + + /// @brief From bytes with definition. + /// + /// @param def pointer to attribute definition. + /// @param value binary value. + /// @return pointer to the attribute. + /// @throw BadValue on errors. + static AttributePtr fromBytes0(const AttrDefPtr& def, + const std::vector& value); }; /// @brief RADIUS attribute derived classes: do not use them directly @@ -764,9 +788,7 @@ class AttrVsa : public Attribute { /// @brief To vsa data. /// /// @return the vsa data. - virtual std::string toVsaData() const override { - return (value_); - } + virtual std::vector toVsaData() const override; /// @brief Unparse attribute. /// diff --git a/src/hooks/dhcp/radius/tests/attribute_unittests.cc b/src/hooks/dhcp/radius/tests/attribute_unittests.cc index e7f51066dd..ee97ea0d56 100644 --- a/src/hooks/dhcp/radius/tests/attribute_unittests.cc +++ b/src/hooks/dhcp/radius/tests/attribute_unittests.cc @@ -590,6 +590,38 @@ TEST_F(AttributeTest, attrVsa) { "the attribute value type must be ipv6prefix, not vsa"); } +// Verifies vendor fromText. +TEST_F(AttributeTest, vendorFromText) { + // Using DSL-Forum (3561) Agent-Circuit-Id (1), + AttrDefPtr def(new AttrDef(1, "Agent-Circuit-Id", PW_TYPE_STRING, 3561)); + AttributePtr attr; + ASSERT_NO_THROW(attr = Attribute::fromText(def, "foobar")); + ASSERT_TRUE(attr); + EXPECT_EQ(PW_VENDOR_SPECIFIC, attr->getType()); + EXPECT_EQ(PW_TYPE_VSA, attr->getValueType()); + EXPECT_EQ(3561, attr->toVendorId()); + EXPECT_EQ(12, attr->getValueLen()); + vector vsa_data = { 1, 8, 0x66, 0x6f, 0x6f, 0x62, 0x61, 0x72 }; + EXPECT_EQ(vsa_data, attr->toVsaData()); +} + +// Verifies vendor fromBytes. +TEST_F(AttributeTest, vendorFromBytes) { + // Using DSL-Forum (3561) Access-Loop-Encapsulation (144), + AttrDefPtr def(new AttrDef(144, "Access-Loop-Encapsulation", + PW_TYPE_STRING, 3561)); + AttributePtr attr; + vector value = { 2, 0, 0 }; + ASSERT_NO_THROW(attr = Attribute::fromBytes(def, value)); + ASSERT_TRUE(attr); + EXPECT_EQ(PW_VENDOR_SPECIFIC, attr->getType()); + EXPECT_EQ(PW_TYPE_VSA, attr->getValueType()); + EXPECT_EQ(3561, attr->toVendorId()); + EXPECT_EQ(9, attr->getValueLen()); + vector vsa_data = { 144, 5, 2, 0, 0 }; + EXPECT_EQ(vsa_data, attr->toVsaData()); +} + // Verifies basic methods for attribute collection. TEST_F(AttributeTest, attributesBasic) { Attributes attrs; diff --git a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc index 9cbacbe61e..6d2115e39d 100644 --- a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc +++ b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc @@ -370,6 +370,7 @@ TEST_F(AttributeTest, attrDefs) { EXPECT_EQ(1, def->type_); EXPECT_EQ("User-Name", def->name_); EXPECT_EQ(PW_TYPE_STRING, def->value_type_); + EXPECT_EQ(0, def->vendor_); def.reset(); // Type 0 is reserved. @@ -377,18 +378,28 @@ TEST_F(AttributeTest, attrDefs) { EXPECT_FALSE(def); def.reset(); + // Only vendor 0 i.e. no vendor was populated. + ASSERT_NO_THROW(def = AttrDefs::instance().getByType(1, 1234)); + EXPECT_FALSE(def); + def.reset(); + // getByName. ASSERT_NO_THROW(def = AttrDefs::instance().getByName("User-Name")); ASSERT_TRUE(def); EXPECT_EQ(1, def->type_); EXPECT_EQ("User-Name", def->name_); EXPECT_EQ(PW_TYPE_STRING, def->value_type_); + EXPECT_EQ(0, def->vendor_); def.reset(); ASSERT_NO_THROW(def = AttrDefs::instance().getByName("Does-not-exist")); EXPECT_FALSE(def); def.reset(); + ASSERT_NO_THROW(def = AttrDefs::instance().getByName("User-Name", 1234)); + EXPECT_FALSE(def); + def.reset(); + // getName. string name; ASSERT_NO_THROW(name = AttrDefs::instance().getName(1)); @@ -396,6 +407,10 @@ TEST_F(AttributeTest, attrDefs) { name.clear(); ASSERT_NO_THROW(name = AttrDefs::instance().getName(252)); EXPECT_EQ("Attribute-252", name); + name.clear(); + ASSERT_NO_THROW(name = AttrDefs::instance().getName(1, 1234)); + EXPECT_EQ("Attribute-1", name); + name.clear(); // add (new). AttrDefPtr def1(new AttrDef(252, "Foo-Bar", PW_TYPE_IPADDR)); @@ -405,12 +420,14 @@ TEST_F(AttributeTest, attrDefs) { EXPECT_EQ(252, def->type_); EXPECT_EQ("Foo-Bar", def->name_); EXPECT_EQ(PW_TYPE_IPADDR, def->value_type_); + EXPECT_EQ(0, def->vendor_); def.reset(); ASSERT_NO_THROW(def = AttrDefs::instance().getByName("Foo-Bar")); ASSERT_TRUE(def); EXPECT_EQ(252, def->type_); EXPECT_EQ("Foo-Bar", def->name_); EXPECT_EQ(PW_TYPE_IPADDR, def->value_type_); + EXPECT_EQ(0, def->vendor_); def.reset(); // add (alias). @@ -421,12 +438,37 @@ TEST_F(AttributeTest, attrDefs) { ASSERT_TRUE(got); EXPECT_EQ(18, got->type_); EXPECT_EQ(PW_TYPE_STRING, got->value_type_); + EXPECT_EQ(0, got->vendor_); + def.reset(); + + // add (vendor). + AttrDefPtr defv(new AttrDef(1, "Agent-Circuit-Id", PW_TYPE_STRING, 3561)); + ASSERT_NO_THROW(AttrDefs::instance().add(defv)); + ASSERT_NO_THROW(def = AttrDefs::instance().getByType(1, 3561)); + ASSERT_TRUE(def); + EXPECT_EQ(1, def->type_); + EXPECT_EQ("Agent-Circuit-Id", def->name_); + EXPECT_EQ(PW_TYPE_STRING, def->value_type_); + EXPECT_EQ(3561, def->vendor_); + def.reset(); + ASSERT_NO_THROW(def = + AttrDefs::instance().getByName("Agent-Circuit-Id", 3561)); + ASSERT_TRUE(def); + EXPECT_EQ(1, def->type_); + EXPECT_EQ("Agent-Circuit-Id", def->name_); + EXPECT_EQ(PW_TYPE_STRING, def->value_type_); + EXPECT_EQ(3561, def->vendor_); + def.reset(); + ASSERT_NO_THROW(name = AttrDefs::instance().getName(1, 3561)); + EXPECT_EQ("Agent-Circuit-Id", name); + name.clear(); // add (change type). ASSERT_NO_THROW(def = AttrDefs::instance().getByName("User-Password")); ASSERT_TRUE(def); EXPECT_EQ(2, def->type_); EXPECT_EQ(PW_TYPE_STRING, def->value_type_); + EXPECT_EQ(0, def->vendor_); AttrDefPtr def3(new AttrDef(17, "User-Password", PW_TYPE_STRING)); string expected = "Illegal attribute redefinition of "; expected += "'User-Password' type 2 value type string by 17 string"; @@ -444,11 +486,35 @@ TEST_F(AttributeTest, attrDefs) { expected += "type 2 value type string by 'Password' 2 integer"; EXPECT_THROW_MSG(AttrDefs::instance().add(def5), BadValue, expected); + // Same with vendor. + ASSERT_NO_THROW(def = + AttrDefs::instance().getByName("Agent-Circuit-Id", 3561)); + ASSERT_TRUE(def); + EXPECT_EQ(1, def->type_); + EXPECT_EQ(PW_TYPE_STRING, def->value_type_); + EXPECT_EQ(3561, def->vendor_); + AttrDefPtr def3v(new AttrDef(2, "Agent-Circuit-Id", PW_TYPE_STRING, 3561)); + expected = "Illegal attribute redefinition of 'Agent-Circuit-Id' "; + expected += "vendor 3561 type 1 value type string by 2 string"; + EXPECT_THROW_MSG(AttrDefs::instance().add(def3v), BadValue, expected); + AttrDefPtr def4v(new AttrDef(1, "Agent-Circuit-Id", PW_TYPE_INTEGER, 3561)); + expected = "Illegal attribute redefinition of 'Agent-Circuit-Id' "; + expected += "vendor 3561 type 1 value type string by 1 integer"; + EXPECT_THROW_MSG(AttrDefs::instance().add(def4v), BadValue, expected); + AttrDefPtr def5v(new AttrDef(1, "Agent-Remote-Id", PW_TYPE_INTEGER, 3561)); + expected = "Illegal attribute redefinition of 'Agent-Circuit-Id' "; + expected += "vendor 3561 type 1 value type string by "; + expected += "'Agent-Remote-Id' 1 integer"; + EXPECT_THROW_MSG(AttrDefs::instance().add(def5v), BadValue, expected); + // clear. ASSERT_NO_THROW(AttrDefs::instance().clear()); ASSERT_NO_THROW(def = AttrDefs::instance().getByType(1)); EXPECT_FALSE(def); def.reset(); + ASSERT_NO_THROW(def = AttrDefs::instance().getByType(1, 3561)); + EXPECT_FALSE(def); + def.reset(); ASSERT_NO_THROW(def = AttrDefs::instance().getByName("Foo-Bar")); EXPECT_FALSE(def); } From 2c89d8c9365e9ffdbce22674ffbc4a2abb2924af Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Thu, 21 Aug 2025 23:35:31 +0200 Subject: [PATCH 07/13] [#3860] Checkpoint: need more UTs and doc --- doc/sphinx/arm/ext-radius.rst | 4 +- src/hooks/dhcp/radius/client_dictionary.cc | 85 ++++++++++-- src/hooks/dhcp/radius/client_dictionary.h | 12 +- src/hooks/dhcp/radius/radius_parsers.cc | 69 ++++++++-- src/hooks/dhcp/radius/tests/attribute_test.h | 4 +- .../dhcp/radius/tests/config_unittests.cc | 2 +- .../dhcp/radius/tests/dictionary_unittests.cc | 130 ++++++++++++++++-- 7 files changed, 265 insertions(+), 41 deletions(-) diff --git a/doc/sphinx/arm/ext-radius.rst b/doc/sphinx/arm/ext-radius.rst index ca1c2b4c65..cd8f939f91 100644 --- a/doc/sphinx/arm/ext-radius.rst +++ b/doc/sphinx/arm/ext-radius.rst @@ -550,13 +550,13 @@ RADIUS dictionary. There are differences: - Yes - - since Kea 3.1.1 + - since Kea 3.1.2 * - Support for Vendor Attributes - Yes - - No + - since Kea 3.1.2 * - Attribute Names and Attribute Values diff --git a/src/hooks/dhcp/radius/client_dictionary.cc b/src/hooks/dhcp/radius/client_dictionary.cc index 92839e62f1..4a48c27b4c 100644 --- a/src/hooks/dhcp/radius/client_dictionary.cc +++ b/src/hooks/dhcp/radius/client_dictionary.cc @@ -199,7 +199,7 @@ AttrDefs::add(IntCstDefPtr def) { } void -AttrDefs::parseLine(const string& line, unsigned int depth) { +AttrDefs::parseLine(const string& line, uint32_t& vendor, unsigned int depth) { // Ignore empty lines. if (line.empty()) { return; @@ -220,7 +220,7 @@ AttrDefs::parseLine(const string& line, unsigned int depth) { if (tokens.size() != 2) { isc_throw(Unexpected, "expected 2 tokens, got " << tokens.size()); } - readDictionary(tokens[1], depth + 1); + readDictionary(tokens[1], vendor, depth + 1); return; } // Attribute definition. @@ -243,11 +243,12 @@ AttrDefs::parseLine(const string& line, unsigned int depth) { isc_throw(Unexpected, "can't parse attribute type " << type_str); } AttrValueType value_type = textToAttrValueType(tokens[3]); - if ((value_type == PW_TYPE_VSA) && (type != PW_VENDOR_SPECIFIC)) { + if ((value_type == PW_TYPE_VSA) && + ((vendor != 0) || (type != PW_VENDOR_SPECIFIC))) { isc_throw(BadValue, "only Vendor-Specific (26) attribute can " << "have the vsa data type"); } - AttrDefPtr def(new AttrDef(type, name, value_type)); + AttrDefPtr def(new AttrDef(type, name, value_type, vendor)); add(def); return; } @@ -307,11 +308,79 @@ AttrDefs::parseLine(const string& line, unsigned int depth) { add(def); return; } + // Begin vendor attribute definitions. + if (tokens[0] == "BEGIN-VENDOR") { + if (vendor != 0) { + isc_throw(Unexpected, "unsupported embedded begin vendor, " + << vendor << " is still open"); + } + if (tokens.size() != 2) { + isc_throw(Unexpected, "expected 2 tokens, got " << tokens.size()); + } + const string& vendor_str = tokens[1]; + IntCstDefPtr vendor_cst = getByName(PW_VENDOR_SPECIFIC, vendor_str); + if (vendor_cst) { + vendor = vendor_cst->value_; + return; + } + try { + int64_t val = boost::lexical_cast(vendor_str); + if ((val < numeric_limits::min()) || + (val > numeric_limits::max())) { + isc_throw(Unexpected, "not 32 bit " << vendor_str); + } + vendor = static_cast(val); + } catch (...) { + isc_throw(Unexpected, "can't parse integer value " << vendor_str); + } + if (vendor == 0) { + isc_throw(Unexpected, "0 is reserved"); + } + return; + } + // End vendor attribute definitions. + if (tokens[0] == "END-VENDOR") { + if (vendor == 0) { + isc_throw(Unexpected, "no matching begin vendor"); + } + if (tokens.size() != 2) { + isc_throw(Unexpected, "expected 2 tokens, got " << tokens.size()); + } + const string& vendor_str = tokens[1]; + IntCstDefPtr vendor_cst = getByName(PW_VENDOR_SPECIFIC, vendor_str); + if (vendor_cst) { + if (vendor_cst->value_ == vendor) { + vendor = 0; + return; + } else { + isc_throw(Unexpected, "begin vendor " << vendor + << " and end vendor " << vendor_cst->value_ + << " do not match"); + } + } + uint32_t value; + try { + int64_t val = boost::lexical_cast(vendor_str); + if ((val < numeric_limits::min()) || + (val > numeric_limits::max())) { + isc_throw(Unexpected, "not 32 bit " << vendor_str); + } + value = static_cast(val); + } catch (...) { + isc_throw(Unexpected, "can't parse integer value " << vendor_str); + } + if (vendor == value) { + vendor = 0; + return; + } + isc_throw(Unexpected, "begin vendor " << vendor + << " and end vendor " << value << " do not match"); + } isc_throw(Unexpected, "unknown dictionary entry '" << tokens[0] << "'"); } void -AttrDefs::readDictionary(const string& path, unsigned depth) { +AttrDefs::readDictionary(const string& path, uint32_t& vendor, unsigned depth) { if (depth >= 5) { isc_throw(BadValue, "Too many nested $INCLUDE"); } @@ -324,7 +393,7 @@ AttrDefs::readDictionary(const string& path, unsigned depth) { isc_throw(BadValue, "bad dictionary '" << path << "'"); } try { - readDictionary(ifs, depth); + readDictionary(ifs, vendor, depth); ifs.close(); } catch (const exception& ex) { ifs.close(); @@ -334,14 +403,14 @@ AttrDefs::readDictionary(const string& path, unsigned depth) { } void -AttrDefs::readDictionary(istream& is, unsigned int depth) { +AttrDefs::readDictionary(istream& is, uint32_t& vendor, unsigned int depth) { size_t lines = 0; string line; try { while (is.good()) { ++lines; getline(is, line); - parseLine(line, depth); + parseLine(line, vendor, depth); } if (!is.eof()) { isc_throw(BadValue, "I/O error: " << strerror(errno)); diff --git a/src/hooks/dhcp/radius/client_dictionary.h b/src/hooks/dhcp/radius/client_dictionary.h index 8af9df0f56..854fef8990 100644 --- a/src/hooks/dhcp/radius/client_dictionary.h +++ b/src/hooks/dhcp/radius/client_dictionary.h @@ -292,8 +292,10 @@ class AttrDefs : public boost::noncopyable { /// incremented by includes and limited to 5. /// /// @param path dictionary file path. + /// @param vendor reference to the current vendor id. /// @param depth recursion depth. - void readDictionary(const std::string& path, unsigned int depth = 0); + void readDictionary(const std::string& path, uint32_t& vendor, + unsigned int depth = 0); /// @brief Read a dictionary from an input stream. /// @@ -302,8 +304,10 @@ class AttrDefs : public boost::noncopyable { /// incremented by includes and limited to 5. /// /// @param is input stream. + /// @param vendor reference to the current vendor id. /// @param depth recursion depth. - void readDictionary(std::istream& is, unsigned int depth = 0); + void readDictionary(std::istream& is, uint32_t& vendor, + unsigned int depth = 0); /// @brief Check if a list of standard attribute definitions /// are available and correct. @@ -324,8 +328,10 @@ class AttrDefs : public boost::noncopyable { /// @brief Parse a dictionary line. /// /// @param line line to parse. + /// @param vendor reference to the current vendor id. /// @param depth recursion depth. - void parseLine(const std::string& line, unsigned int depth); + void parseLine(const std::string& line, uint32_t& vendor, + unsigned int depth); /// @brief Attribute definition container. AttrDefContainer container_; diff --git a/src/hooks/dhcp/radius/radius_parsers.cc b/src/hooks/dhcp/radius/radius_parsers.cc index 20c89d1053..261a8fd448 100644 --- a/src/hooks/dhcp/radius/radius_parsers.cc +++ b/src/hooks/dhcp/radius/radius_parsers.cc @@ -87,9 +87,10 @@ const AttrDefList RadiusConfigParser::USED_STANDARD_ATTR_DEFS = { /// @brief Defaults for Radius attribute configuration. const SimpleDefaults RadiusAttributeParser::ATTRIBUTE_DEFAULTS = { - { "data", Element::string, "" }, - { "expr", Element::string, "" }, - { "raw", Element::string, "" } + { "data", Element::string, "" }, + { "expr", Element::string, "" }, + { "raw", Element::string, "" }, + { "vendor", Element::string, "" } }; void @@ -106,12 +107,17 @@ RadiusConfigParser::parse(ElementPtr& config) { // Read the dictionary if (!AttrDefs::instance().getByType(1)) { + uint32_t vendor = 0; try { - AttrDefs::instance().readDictionary(riref.dictionary_); + AttrDefs::instance().readDictionary(riref.dictionary_, vendor); } catch (const exception& ex) { isc_throw(BadValue, "can't read radius dictionary: " << ex.what()); } + if (vendor != 0) { + isc_throw(BadValue, "vendor definitions were not properly " + << "closed: vendor " << vendor << " is still open"); + } } // Check it. @@ -511,16 +517,43 @@ RadiusAttributeParser::parse(const RadiusServicePtr& service, // Set defaults. setDefaults(attr, ATTRIBUTE_DEFAULTS); + // vendor. + uint32_t vendor = 0; + const string& vendor_txt = getString(attr, "vendor"); + if (!vendor_txt.empty()) { + IntCstDefPtr vendor_cst = + AttrDefs::instance().getByName(PW_VENDOR_SPECIFIC, vendor_txt); + if (vendor_cst) { + vendor = vendor_cst->value_; + } else { + try { + int64_t val = boost::lexical_cast(vendor_txt); + if ((val < numeric_limits::min()) || + (val > numeric_limits::max())) { + isc_throw(Unexpected, "not 32 bit " << vendor_txt); + } + vendor = static_cast(val); + } catch (...) { + isc_throw(ConfigError, "can't parse vendor " << vendor_txt); + } + } + } + // name. const ConstElementPtr& name = attr->get("name"); if (name) { if (name->stringValue().empty()) { isc_throw(ConfigError, "attribute name is empty"); } - def = AttrDefs::instance().getByName(name->stringValue()); + def = AttrDefs::instance().getByName(name->stringValue(), vendor); if (!def) { - isc_throw(ConfigError, "attribute '" - << name->stringValue() << "' is unknown"); + ostringstream msg; + msg << "attribute '" << name->stringValue() << "'"; + if (vendor != 0) { + msg << " in vendor '" << vendor_txt << "'"; + } + msg << " is unknown"; + isc_throw(ConfigError, msg.str()); } } @@ -533,16 +566,26 @@ RadiusAttributeParser::parse(const RadiusServicePtr& service, } uint8_t attrib = static_cast(type->intValue()); if (def && (def->type_ != attrib)) { - isc_throw(ConfigError, name->stringValue() << " attribute has " - << "type " << static_cast(def->type_) - << ", not " << static_cast(attrib)); + ostringstream msg; + msg << "'" << name->stringValue() << "' attribute"; + if (vendor != 0) { + msg << " in vendor '" << vendor_txt << "'"; + } + msg << " has type " << static_cast(def->type_) + << ", not " << static_cast(attrib); + isc_throw(ConfigError, msg.str()); } if (!def) { - def = AttrDefs::instance().getByType(attrib); + def = AttrDefs::instance().getByType(attrib, vendor); } if (!def) { - isc_throw(ConfigError, "attribute type " - << static_cast(attrib) << " is unknown"); + ostringstream msg; + msg << "attribute type " << static_cast(attrib); + if (vendor != 0) { + msg << " in vendor '" << vendor_txt << "'"; + } + msg << " is unknown"; + isc_throw(ConfigError, msg.str()); } } diff --git a/src/hooks/dhcp/radius/tests/attribute_test.h b/src/hooks/dhcp/radius/tests/attribute_test.h index c75d14a173..537fcb0cfb 100644 --- a/src/hooks/dhcp/radius/tests/attribute_test.h +++ b/src/hooks/dhcp/radius/tests/attribute_test.h @@ -19,7 +19,9 @@ class AttributeTest : public ::testing::Test { public: /// @brief Constructor. AttributeTest() { - AttrDefs::instance().readDictionary(TEST_DICTIONARY); + uint32_t vendor = 0; + AttrDefs::instance().readDictionary(TEST_DICTIONARY, vendor); + EXPECT_EQ(0, vendor); } /// @brief Destructor. diff --git a/src/hooks/dhcp/radius/tests/config_unittests.cc b/src/hooks/dhcp/radius/tests/config_unittests.cc index 2eb232db37..4753d208e4 100644 --- a/src/hooks/dhcp/radius/tests/config_unittests.cc +++ b/src/hooks/dhcp/radius/tests/config_unittests.cc @@ -756,7 +756,7 @@ TEST_F(ConfigTest, attribute) { 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"); + "'User-Name' attribute has type 1, not 123"); // Type must be between 0 and 255. attr = Element::createMap(); diff --git a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc index 6d2115e39d..9b16e7051d 100644 --- a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc +++ b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc @@ -53,23 +53,33 @@ class DictionaryTest : public ::testing::Test { /// @brief Parse a line. /// /// @param line line to parse. + /// @param before vendor id on input (default to 0). + /// @param after expected vendor id on output (default to 0). /// @param depth recursion depth. - void parseLine(const string& line, unsigned int depth = 0) { + void parseLine(const string& line, uint32_t before = 0, + uint32_t after = 0, unsigned int depth = 0) { istringstream is(line + "\n"); - AttrDefs::instance().readDictionary(is, depth); + uint32_t vendor = before; + AttrDefs::instance().readDictionary(is, vendor, depth); + EXPECT_EQ(after, vendor); } /// @brief Parse a list of lines. /// /// @param lines list of lines. + /// @param before vendor id on input (default to 0). + /// @param after expected vendor id on output (default to 0). /// @param depth recursion depth. - void parseLines(const list& lines, unsigned int depth = 0) { + void parseLines(const list& lines, uint32_t before = 0, + uint32_t after = 0, unsigned int depth = 0) { string content; for (auto const& line : lines) { content += line + "\n"; } istringstream is(content); - AttrDefs::instance().readDictionary(is, depth); + uint32_t vendor = before; + AttrDefs::instance().readDictionary(is, vendor, depth); + EXPECT_EQ(after, vendor); } /// @brief writes specified content to a file. @@ -92,7 +102,10 @@ const char* DictionaryTest::TEST_DICT = "test-dict"; // Verifies standards definitions can be read from the dictionary. TEST_F(DictionaryTest, standard) { - ASSERT_NO_THROW_LOG(AttrDefs::instance().readDictionary(TEST_DICTIONARY)); + uint32_t vendor = 0; + ASSERT_NO_THROW_LOG(AttrDefs::instance().readDictionary(TEST_DICTIONARY, + vendor)); + EXPECT_EQ(0, vendor); } // Verifies parseLine internal routine. @@ -142,11 +155,19 @@ TEST_F(DictionaryTest, parseLine) { "expected 3 tokens, got 4 at line 1"); EXPECT_THROW_MSG(parseLine("VENDOR my-vendor 0"), BadValue, "0 is reserved at line 1"); + EXPECT_THROW_MSG(parseLine("BEGIN-VENDOR"), BadValue, + "expected 2 tokens, got 1 at line 1"); + EXPECT_THROW_MSG(parseLine("BEGIN-VENDOR my-vendor 1"), BadValue, + "expected 2 tokens, got 3 at line 1"); + EXPECT_THROW_MSG(parseLine("END-VENDOR", 1), BadValue, + "expected 2 tokens, got 1 at line 1"); + EXPECT_THROW_MSG(parseLine("END-VENDOR my-vendor 1", 1), BadValue, + "expected 2 tokens, got 3 at line 1"); - EXPECT_THROW_MSG(parseLine("BEGIN-VENDOR my-vendor"), BadValue, - "unknown dictionary entry 'BEGIN-VENDOR' at line 1"); - EXPECT_THROW_MSG(parseLine("END-VENDOR my-vendor"), BadValue, - "unknown dictionary entry 'END-VENDOR' at line 1"); + EXPECT_THROW_MSG(parseLine("BEGIN-TLV my-vendor"), BadValue, + "unknown dictionary entry 'BEGIN-TLV' at line 1"); + EXPECT_THROW_MSG(parseLine("END-TLV my-vendor"), BadValue, + "unknown dictionary entry 'END-TLV' at line 1"); } // Verifies sequences attribute of (re)definitions. @@ -275,12 +296,90 @@ TEST_F(DictionaryTest, vendorId) { EXPECT_THROW_MSG(parseLines(new_value), BadValue, expected); } +// Verifies begin and end vendor entries. +TEST_F(DictionaryTest, beginEndVendor) { + // Begin already open. + list begin_unknown = { + "BEGIN-VENDOR foo" + }; + string expected = "unsupported embedded begin vendor, "; + expected += "1 is still open at line 1"; + EXPECT_THROW_MSG(parseLines(begin_unknown, 1), BadValue, expected); + // Value must be a known name or integer. + EXPECT_THROW_MSG(parseLines(begin_unknown), BadValue, + "can't parse integer value foo at line 1"); + // End not yet open. + list end_unknown = { + "END-VENDOR foo" + }; + EXPECT_THROW_MSG(parseLines(end_unknown), BadValue, + "no matching begin vendor at line 1"); + EXPECT_THROW_MSG(parseLines(end_unknown, 1), BadValue, + "can't parse integer value foo at line 1"); + // 0 is reserved. + list begin0 = { + "BEGIN-VENDOR 0" + }; + EXPECT_THROW_MSG(parseLines(begin0), BadValue, + "0 is reserved at line 1"); + + // Positive using a name. + list positive = { + "VENDOR DSL-Forum 3561", + "BEGIN-VENDOR DSL-Forum", + "ATTRIBUTE Agent-Circuit-Id 1 string" + }; + EXPECT_NO_THROW_LOG(parseLines(positive, 0, 3561)); + auto aci = AttrDefs::instance().getByName("Agent-Circuit-Id", 3561); + ASSERT_TRUE(aci); + EXPECT_EQ(1, aci->type_); + EXPECT_EQ(PW_TYPE_STRING, aci->value_type_); + EXPECT_EQ("Agent-Circuit-Id", aci->name_); + EXPECT_EQ(3561, aci->vendor_); + + // Positive using an integer. + list positive_n = { + "BEGIN-VENDOR 3561", + "ATTRIBUTE Actual-Data-Rate-Upstream 129 integer" + }; + EXPECT_NO_THROW_LOG(parseLines(positive_n, 0, 3561)); + auto adru = AttrDefs::instance().getByType(129, 3561); + ASSERT_TRUE(adru); + EXPECT_EQ(129, adru->type_); + EXPECT_EQ(PW_TYPE_INTEGER, adru->value_type_); + EXPECT_EQ("Actual-Data-Rate-Upstream", adru->name_); + EXPECT_EQ(3561, adru->vendor_); + + // End using a name. + list end_name = { + "END-VENDOR DSL-Forum" + }; + EXPECT_NO_THROW_LOG(parseLines(end_name, 3561, 0)); + + // End using an integer. + list end_int = { + "END-VENDOR 3561" + }; + EXPECT_NO_THROW_LOG(parseLines(end_int, 3561, 0)); + + // Not matching. + list no_match = { + "BEGIN-VENDOR 1234", + "END-VENDOR 2345" + }; + expected = "begin vendor 1234 and end vendor 2345 do not match at line 2"; + EXPECT_THROW_MSG(parseLines(no_match), BadValue, expected); +} + // Verifies errors from bad dictionary files. TEST_F(DictionaryTest, badFile) { string expected = "can't open dictionary '/does-not-exist': "; expected += "No such file or directory"; - EXPECT_THROW_MSG(AttrDefs::instance().readDictionary("/does-not-exist"), + uint32_t vendor = 0; + EXPECT_THROW_MSG(AttrDefs::instance().readDictionary("/does-not-exist", + vendor), BadValue, expected); + EXPECT_EQ(0, vendor); list bad_include = { "$INCLUDE /does-not-exist" }; @@ -292,7 +391,10 @@ TEST_F(DictionaryTest, badFile) { // Verifies that the dictionary correctly defines used standard attributes. TEST_F(DictionaryTest, hookAttributes) { - ASSERT_NO_THROW_LOG(AttrDefs::instance().readDictionary(TEST_DICTIONARY)); + uint32_t vendor = 0; + ASSERT_NO_THROW_LOG(AttrDefs::instance().readDictionary(TEST_DICTIONARY, + vendor)); + EXPECT_EQ(0, vendor); EXPECT_NO_THROW_LOG(AttrDefs::instance(). checkStandardDefs(RadiusConfigParser::USED_STANDARD_ATTR_DEFS)); } @@ -312,7 +414,7 @@ TEST_F(DictionaryTest, include) { EXPECT_EQ(2495, isc->value_); // max depth is 5. - EXPECT_THROW_MSG(parseLines(include, 4), BadValue, + EXPECT_THROW_MSG(parseLines(include, 0, 0, 4), BadValue, "Too many nested $INCLUDE at line 2"); } @@ -353,11 +455,13 @@ TEST_F(DictionaryTest, DISABLED_readDictionaries) { const string path_regex("/usr/share/freeradius/*"); Glob g(path_regex); AttrDefs& defs(AttrDefs::instance()); + uint32_t vendor = 0; for (size_t i = 0; i < g.glob_buffer_.gl_pathc; ++i) { const string file_name(g.glob_buffer_.gl_pathv[i]); SCOPED_TRACE(file_name); - EXPECT_NO_THROW_LOG(defs.readDictionary(file_name)); + EXPECT_NO_THROW_LOG(defs.readDictionary(file_name, vendor)); } + EXPECT_EQ(0, vendor); } // Verifies attribute definitions. From da9602146c4d5567344922e508e79fb530161f89 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Fri, 22 Aug 2025 12:26:05 +0200 Subject: [PATCH 08/13] [#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 @@ class Attribute : public data::CfgToElement { /// @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 @@ class Attribute : public data::CfgToElement { /// @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 @@ class Attribute : public data::CfgToElement { /// @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 @@ class Attribute : public data::CfgToElement { /// @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 @@ class AttrVsa : public Attribute { /// @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 @@ class Attributes : public data::CfgToElement { /// @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 @@ class ConfigTest : public radius::test::AttributeTest { 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 @@ class DictionaryTest : public ::testing::Test { /// @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 @@ class DictionaryTest : public ::testing::Test { } /// 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); } From 5f2e5a1e5cee30c4cdd92ade1e953eda4db1bb3c Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Fri, 22 Aug 2025 19:53:21 +0200 Subject: [PATCH 09/13] [#3860] Last checkpoint before vendor VALUE --- src/hooks/dhcp/radius/cfg_attribute.cc | 11 +++- .../dhcp/radius/tests/config_unittests.cc | 62 ++++++++++++++++++- 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/hooks/dhcp/radius/cfg_attribute.cc b/src/hooks/dhcp/radius/cfg_attribute.cc index 54bd2c814d..4b80258fa5 100644 --- a/src/hooks/dhcp/radius/cfg_attribute.cc +++ b/src/hooks/dhcp/radius/cfg_attribute.cc @@ -121,13 +121,18 @@ CfgAttributes::toElement() const { continue; } ElementPtr map; - if (!it.test_.empty()) { + if (it.attr_) { + map = it.attr_->toElement(); + } else if (!it.test_.empty()) { map = Element::createMap(); map->set("type", Element::create(static_cast(def->type_))); map->set("expr", Element::create(it.test_)); map->set("name", Element::create(def->name_)); - } else if (it.attr_) { - map = it.attr_->toElement(); + if (def->vendor_ != 0) { + ostringstream vendor; + vendor << def->vendor_; + map->set("vendor", Element::create(vendor.str())); + } } result->add(map); } diff --git a/src/hooks/dhcp/radius/tests/config_unittests.cc b/src/hooks/dhcp/radius/tests/config_unittests.cc index 1a9f85943c..ea03e69795 100644 --- a/src/hooks/dhcp/radius/tests/config_unittests.cc +++ b/src/hooks/dhcp/radius/tests/config_unittests.cc @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -954,6 +955,28 @@ TEST_F(ConfigTest, attribute) { " \"raw\": \"660102626172\" } ]"; runToElementTest(expected, srv->attributes_); + // Try the raw version of DSL-Forum (3561) Agent-Circuit-Id (1). + srv->attributes_.clear(); + attr = Element::createMap(); + attr->set("vendor", Element::create("DSL-Forum")); + attr->set("raw", Element::create("660102626172")); + 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]0x0108660102626172", got->toText()); + expected = "[ { " + " \"name\": \"Vendor-Specific\", " + " \"type\": 26, " + " \"vendor\": \"3561\", " + " \"vsa-raw\": \"0108660102626172\" } ]"; + runToElementTest(expected, srv->attributes_); + // Check with expr. srv->attributes_.clear(); attr = Element::createMap(); @@ -979,7 +1002,7 @@ TEST_F(ConfigTest, attribute) { EXPECT_NO_THROW(parser.parse(srv, attr)); EXPECT_EQ(1, srv->attributes_.size()); EXPECT_TRUE(srv->attributes_.getAll().empty()); - const ExpressionPtr& expr = srv->attributes_.getExpr(1); + ExpressionPtr expr = srv->attributes_.getExpr(1); ASSERT_TRUE(expr); ASSERT_EQ(1, expr->size()); TokenPtr token = (*expr)[0]; @@ -994,6 +1017,43 @@ TEST_F(ConfigTest, attribute) { " \"type\": 1, " " \"expr\": \"'foobar'\" } ]"; runToElementTest(expected, srv->attributes_); + + // Try with vendor. + srv->attributes_.clear(); + attr = Element::createMap(); + attr->set("vendor", Element::create("3561")); + attr->set("type", Element::create(1)); + attr->set("expr", Element::create("'foobar'")); + EXPECT_NO_THROW(parser.parse(srv, attr)); + EXPECT_EQ(1, srv->attributes_.size()); + EXPECT_TRUE(srv->attributes_.getDef(1, 3561)); + EXPECT_FALSE(srv->attributes_.get(1, 3561)); + EXPECT_TRUE(srv->attributes_.getAll().empty()); + expr = srv->attributes_.getExpr(1, 3561); + ASSERT_TRUE(expr); + ASSERT_EQ(1, expr->size()); + token = (*expr)[0]; + tokstr = boost::dynamic_pointer_cast(token); + EXPECT_TRUE(tokstr); + EXPECT_EQ("'foobar'", srv->attributes_.getTest(1, 3561)); + expected = "[ { " + " \"name\": \"Agent-Circuit-Id\", " + " \"type\": 1, " + " \"vendor\": \"3561\", " + " \"expr\": \"'foobar'\" } ]"; + runToElementTest(expected, srv->attributes_); + + // Evaluate. + Pkt4Ptr query(new Pkt4(DHCPDISCOVER, 12345)); + Attributes evaluated; + EXPECT_NO_THROW_LOG(evaluated = srv->attributes_.getEvalAll(*query)); + EXPECT_EQ(1, evaluated.size()); + expected = "[ { " + " \"name\": \"Vendor-Specific\", " + " \"type\": 26, " + " \"vendor\": \"3561\", " + " \"vsa-raw\": \"0108666F6F626172\" } ]"; + runToElementTest(expected, evaluated); } // Verify checkAttributes sanity check. From 40fdb61e72dd9948781cbe28d12ccc9ee2ea3da3 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Fri, 22 Aug 2025 20:37:21 +0200 Subject: [PATCH 10/13] [#3860] Added vendor VALUE in dicts --- src/hooks/dhcp/radius/client_dictionary.cc | 48 ++++++++++++------- src/hooks/dhcp/radius/client_dictionary.h | 26 +++++++--- .../dhcp/radius/tests/dictionary_unittests.cc | 19 ++++++++ 3 files changed, 70 insertions(+), 23 deletions(-) diff --git a/src/hooks/dhcp/radius/client_dictionary.cc b/src/hooks/dhcp/radius/client_dictionary.cc index 2706111460..ab288ad0fb 100644 --- a/src/hooks/dhcp/radius/client_dictionary.cc +++ b/src/hooks/dhcp/radius/client_dictionary.cc @@ -154,9 +154,10 @@ AttrDefs::getName(const uint8_t type, const uint32_t vendor) const { } IntCstDefPtr -AttrDefs::getByName(const uint8_t type, const string& name) const { +AttrDefs::getByName(const uint8_t type, const string& name, + const uint32_t vendor) const { auto const& idx = ic_container_.get<0>(); - auto it = idx.find(boost::make_tuple(type, name)); + auto it = idx.find(boost::make_tuple(vendor, type, name)); if (it != idx.end()) { return (*it); } @@ -164,9 +165,10 @@ AttrDefs::getByName(const uint8_t type, const string& name) const { } IntCstDefPtr -AttrDefs::getByValue(const uint8_t type, const uint32_t value) const { +AttrDefs::getByValue(const uint8_t type, const uint32_t value, + const uint32_t vendor) const { auto const& idx = ic_container_.get<1>(); - auto it = idx.find(boost::make_tuple(type, value)); + auto it = idx.find(boost::make_tuple(vendor, type, value)); if (it != idx.end()) { return (*it); } @@ -179,7 +181,7 @@ AttrDefs::add(IntCstDefPtr def) { return; } auto& idx = ic_container_.get<0>(); - auto it = idx.find(boost::make_tuple(def->type_, def->name_)); + auto it = idx.find(boost::make_tuple(def->vendor_, def->type_, def->name_)); if (it != idx.end()) { if (def->value_ == (*it)->value_) { // Duplicate: ignore. @@ -191,9 +193,15 @@ AttrDefs::add(IntCstDefPtr def) { << def->name_ << "' value " << (*it)->value_ << " by " << def->value_); } - isc_throw(BadValue, "Illegal integer constant redefinition of '" - << def->name_ << "' for attribute '" << getName(def->type_) - << "' value " << (*it)->value_ << " by " << def->value_); + ostringstream msg; + msg << "Illegal integer constant redefinition of '" + << def->name_ << "' for attribute '" + << getName(def->type_, def->vendor_) << "'"; + if (def->vendor_ != 0) { + msg << " in vendor " << def->vendor_; + } + msg << " value " << (*it)->value_ << " by " << def->value_; + isc_throw(BadValue, msg.str()); } static_cast(ic_container_.insert(def)); } @@ -254,21 +262,27 @@ 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/*, vendor*/); + AttrDefPtr attr = getByName(attr_str, vendor); if (!attr) { - isc_throw(Unexpected, "unknown attribute '" << attr_str << "'"); + ostringstream msg; + msg << "unknown attribute '" << attr_str << "'"; + if (vendor != 0) { + msg << " in vendor " << vendor; + } + isc_throw(Unexpected, msg.str()); } if (attr->value_type_ != PW_TYPE_INTEGER) { - isc_throw(Unexpected, "attribute '" << attr_str - << "' is not an integer attribute"); + ostringstream msg; + msg << "attribute '" << attr_str << "'"; + if (vendor != 0) { + msg << " in vendor " << vendor; + } + msg << " is not an integer attribute"; + isc_throw(Unexpected, msg.str()); } const string& name = tokens[2]; const string& value_str = tokens[3]; @@ -283,7 +297,7 @@ AttrDefs::parseLine(const string& line, uint32_t& vendor, unsigned int depth) { } catch (...) { isc_throw(Unexpected, "can't parse integer value " << value_str); } - IntCstDefPtr def(new IntCstDef(attr->type_, name, value)); + IntCstDefPtr def(new IntCstDef(attr->type_, name, value, vendor)); add(def); return; } diff --git a/src/hooks/dhcp/radius/client_dictionary.h b/src/hooks/dhcp/radius/client_dictionary.h index 854fef8990..cec8986a26 100644 --- a/src/hooks/dhcp/radius/client_dictionary.h +++ b/src/hooks/dhcp/radius/client_dictionary.h @@ -118,9 +118,10 @@ class IntCstDef { /// @param type attribute type. /// @param name integer constant name. /// @param value integer constant value. + /// @param vendor vendor id (default 0). IntCstDef(const uint8_t type, const std::string& name, - const uint32_t value) - : type_(type), name_(name), value_(value) { + const uint32_t value, const uint32_t vendor = 0) + : type_(type), name_(name), value_(value), vendor_(vendor) { } /// @brief attribute type. @@ -131,6 +132,9 @@ class IntCstDef { /// @brief value. const uint32_t value_; + + /// @brief vendor id (default 0). + const uint32_t vendor_; }; /// @brief Shared pointers to Integer constant definition. @@ -200,10 +204,13 @@ class AttrDefs : public boost::noncopyable { IntCstDefPtr, // Start specification of indexes here. boost::multi_index::indexed_by< - // Hash index for by type and name. + // Hash index for by vendor, type and name. boost::multi_index::hashed_unique< boost::multi_index::composite_key< IntCstDef, + boost::multi_index::member< + IntCstDef, const uint32_t, &IntCstDef::vendor_ + >, boost::multi_index::member< IntCstDef, const uint8_t, &IntCstDef::type_ >, @@ -212,10 +219,13 @@ class AttrDefs : public boost::noncopyable { > > >, - // Hash index for by type and value. + // Hash index for by vendor, type and value. boost::multi_index::hashed_unique< boost::multi_index::composite_key< IntCstDef, + boost::multi_index::member< + IntCstDef, const uint32_t, &IntCstDef::vendor_ + >, boost::multi_index::member< IntCstDef, const uint8_t, &IntCstDef::type_ >, @@ -271,15 +281,19 @@ class AttrDefs : public boost::noncopyable { /// /// @param type attribute type. /// @param name name to look for. + /// @param vendor vendor id to look for (default 0). /// @return pointer to the integer constant definition or null. - IntCstDefPtr getByName(const uint8_t type, const std::string& name) const; + IntCstDefPtr getByName(const uint8_t type, const std::string& name, + const uint32_t vendor = 0) const; /// @brief Get integer constant definition by attribute type and value. /// /// @param type attribute type. /// @param value value to look for. + /// @param vendor vendor id to look for (default 0). /// @return pointer to the integer constant definition or null. - IntCstDefPtr getByValue(const uint8_t type, const uint32_t value) const; + IntCstDefPtr getByValue(const uint8_t type, const uint32_t value, + const uint32_t vendor = 0) const; /// @brief Add (or replace) an integer constant definition. /// diff --git a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc index 436a02722c..b2cc58ec7a 100644 --- a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc +++ b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc @@ -144,6 +144,9 @@ TEST_F(DictionaryTest, parseLine) { "expected 4 tokens, got 3 at line 1"); EXPECT_THROW_MSG(parseLine("VALUE My-Attribute My-Value 1"), BadValue, "unknown attribute 'My-Attribute' at line 1"); + EXPECT_THROW_MSG(parseLine("VALUE My-Attribute My-Value 1", 99), + BadValue, + "unknown attribute 'My-Attribute' in vendor 99 at line 1"); EXPECT_THROW_MSG(parseLine("$INCLUDE"), BadValue, "expected 2 tokens, got 1 at line 1"); @@ -229,6 +232,18 @@ TEST_F(DictionaryTest, integerConstant) { expected += " at line 2"; EXPECT_THROW_MSG(parseLines(not_integer_attr), BadValue, expected); + // Same with a vendor attribute. + list not_integer_attrv = { + "VENDOR DSL-Forum 3561", + "BEGIN-VENDOR DSL-Forum", + "ATTRIBUTE Agent-Circuit-Id 1 string", + "VALUE Agent-Circuit-Id My-Value 1", + "END-VENDOR DSL-Forum" + }; + expected = "attribute 'Agent-Circuit-Id' in vendor 3561"; + expected += " is not an integer attribute at line 4"; + EXPECT_THROW_MSG(parseLines(not_integer_attrv), BadValue, expected); + // Value must be an integer. list not_integer_val = { "ATTRIBUTE Acct-Status-Type 40 integer", @@ -262,6 +277,10 @@ TEST_F(DictionaryTest, integerConstant) { expected += "'Start' for attribute 'Acct-Status-Type' value 1 by 2"; expected += " at line 3"; EXPECT_THROW_MSG(parseLines(new_value), BadValue, expected); + expected = "Illegal integer constant redefinition of "; + expected += "'Start' for attribute 'Acct-Status-Type' in vendor 1234 "; + expected += "value 1 by 2 at line 3"; + EXPECT_THROW_MSG(parseLines(new_value, 1234, 1234), BadValue, expected); } // Verifies vendor id definitions. From 5d3b7dadaacacbd89e1499104a4d5811b7651fb7 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Fri, 22 Aug 2025 23:38:26 +0200 Subject: [PATCH 11/13] [#3860] Last updates including doc --- .../3860-radius-vendor-attributes | 5 +++++ doc/sphinx/arm/ext-radius.rst | 17 +++++++++++++++++ src/hooks/dhcp/radius/client_attribute.cc | 3 ++- .../dhcp/radius/tests/attribute_unittests.cc | 16 +++++++++++++++- 4 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 changelog_unreleased/3860-radius-vendor-attributes diff --git a/changelog_unreleased/3860-radius-vendor-attributes b/changelog_unreleased/3860-radius-vendor-attributes new file mode 100644 index 0000000000..df1de76549 --- /dev/null +++ b/changelog_unreleased/3860-radius-vendor-attributes @@ -0,0 +1,5 @@ +[func] fdupont + Added support of RADIUS vendor attributes and integer + translations to the RADIUS hook library for compatibility + with previous versions using the FreeRADIUS client library. + (Gitlab #2860) diff --git a/doc/sphinx/arm/ext-radius.rst b/doc/sphinx/arm/ext-radius.rst index cd8f939f91..6d32aae0a9 100644 --- a/doc/sphinx/arm/ext-radius.rst +++ b/doc/sphinx/arm/ext-radius.rst @@ -226,9 +226,20 @@ At the service level, three sections can be configured: - ``expr`` - is the last of the three ways to specify the attribute content. It specifies an evaluation expression on the DHCP query packet. + - ``vendor`` - since Kea 3.1.2 is the vendor id of the attribute. + It allways contents a string with the vendor name or an integer litteral. Attributes are supported only for the access service. +.. note:: + + Vendor-Specific attribute can be specified in two ways: using a ``raw`` + value which must include the vendor and the vsa data, note that the ``data`` + value is no longer supported sine Kea 3.1.2, and the ``expr`` value + is evaluated to the content of the attribute. The second way was added + by 3.1.2 and allows to specify a vendor attribute which is automatically + embedded into a Vendor-Specific attribute. + - The ``peer-updates`` boolean flag (default ``true``) controls whether lease updates coming from an active High-Availability (HA) partner should result in an accounting request. This may be desirable to remove duplicates if HA @@ -570,6 +581,12 @@ RADIUS dictionary. There are differences: - Must have an associated attribute definition in the dictionary. + * - Attribute and Integer Value name spaces + + - flat name spaces allowing duplicates. + + - since Kea 3.1.2 different name spaces per vendor. + * - Reply-Message Presence in the Kea Logs - Only as part of the aggregated list of attributes in ``RADIUS_AUTHENTICATION_ACCEPTED``, ``RADIUS_ACCESS_CACHE_INSERT``, ``RADIUS_ACCESS_CACHE_GET`` log messages. diff --git a/src/hooks/dhcp/radius/client_attribute.cc b/src/hooks/dhcp/radius/client_attribute.cc index bb77f5e646..b011488be8 100644 --- a/src/hooks/dhcp/radius/client_attribute.cc +++ b/src/hooks/dhcp/radius/client_attribute.cc @@ -49,7 +49,8 @@ Attribute::fromText0(const AttrDefPtr& def, const string& value) { case PW_TYPE_INTEGER: if (!isdigit(value[0])) { IntCstDefPtr ic_def = - AttrDefs::instance().getByName(def->type_, value); + AttrDefs::instance().getByName(def->type_, value, + def->vendor_); if (ic_def) { return (fromInt(def->type_, ic_def->value_)); } diff --git a/src/hooks/dhcp/radius/tests/attribute_unittests.cc b/src/hooks/dhcp/radius/tests/attribute_unittests.cc index ee97ea0d56..b93c47e1dd 100644 --- a/src/hooks/dhcp/radius/tests/attribute_unittests.cc +++ b/src/hooks/dhcp/radius/tests/attribute_unittests.cc @@ -184,7 +184,7 @@ TEST_F(AttributeTest, rawAttrString) { << from_bytes->toText() << " != " << attr->toText(); } -// Verifies integer string attribute. +// Verifies integer attribute. TEST_F(AttributeTest, attrInt) { // Using NAS-Port-Type (61) integer attribute. AttrDefPtr def = AttrDefs::instance().getByType(PW_NAS_PORT_TYPE); @@ -251,6 +251,20 @@ TEST_F(AttributeTest, attrInt) { "the attribute value type must be vsa, not integer"); } +// Verifies vendor integer attribute. +TEST_F(AttributeTest, vendorAttrInt) { + // Attibute. + AttrDefPtr def(new AttrDef(1, "My-Int", PW_TYPE_INTEGER, 2495)); + ASSERT_NO_THROW(AttrDefs::instance().add(def)); + // Integer constant. + IntCstDefPtr cst(new IntCstDef(1, "My-Cst", 144, 2495)); + ASSERT_NO_THROW(AttrDefs::instance().add(cst)); + AttributePtr attr; + ASSERT_NO_THROW(attr = Attribute::fromText(def, "My-Cst")); + ASSERT_TRUE(attr); + EXPECT_EQ("Vendor-Specific=[2495]0x010600000090", attr->toText()); +} + // Verifies IP address attribute. TEST_F(AttributeTest, attrIpAddr) { // Using Framed-IP-Address (8) IP address attribute. From 1385f1e625df49ab33a3436643fe3cea59a85dad Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Fri, 22 Aug 2025 23:40:11 +0200 Subject: [PATCH 12/13] [#3860] Spelling --- src/hooks/dhcp/radius/tests/attribute_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hooks/dhcp/radius/tests/attribute_unittests.cc b/src/hooks/dhcp/radius/tests/attribute_unittests.cc index b93c47e1dd..02e9c3b48e 100644 --- a/src/hooks/dhcp/radius/tests/attribute_unittests.cc +++ b/src/hooks/dhcp/radius/tests/attribute_unittests.cc @@ -253,7 +253,7 @@ TEST_F(AttributeTest, attrInt) { // Verifies vendor integer attribute. TEST_F(AttributeTest, vendorAttrInt) { - // Attibute. + // Attribute. AttrDefPtr def(new AttrDef(1, "My-Int", PW_TYPE_INTEGER, 2495)); ASSERT_NO_THROW(AttrDefs::instance().add(def)); // Integer constant. From 6cfac24668fd9e51cba09e036b9e1b42c2fa4d80 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Sat, 23 Aug 2025 09:47:39 +0200 Subject: [PATCH 13/13] [#3860] Added includes in ChangeLog --- changelog_unreleased/3860-radius-vendor-attributes | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/changelog_unreleased/3860-radius-vendor-attributes b/changelog_unreleased/3860-radius-vendor-attributes index df1de76549..5eeb67fcbe 100644 --- a/changelog_unreleased/3860-radius-vendor-attributes +++ b/changelog_unreleased/3860-radius-vendor-attributes @@ -1,5 +1,6 @@ [func] fdupont - Added support of RADIUS vendor attributes and integer - translations to the RADIUS hook library for compatibility - with previous versions using the FreeRADIUS client library. + Added support of RADIUS dictionary includes, vendor + attributes and integer translations to the RADIUS hook + library for compatibility with previous versions using + the FreeRADIUS client library. (Gitlab #2860)