[#3860] Checkpoint: need more UTs and doc

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
 

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<int64_t>(vendor_str);
+            if ((val < numeric_limits<int32_t>::min()) ||
+                (val > numeric_limits<uint32_t>::max())) {
+                isc_throw(Unexpected, "not 32 bit " << vendor_str);
+            }
+            vendor = static_cast<uint32_t>(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<int64_t>(vendor_str);
+            if ((val < numeric_limits<int32_t>::min()) ||
+                (val > numeric_limits<uint32_t>::max())) {
+                isc_throw(Unexpected, "not 32 bit " << vendor_str);
+            }
+            value = static_cast<uint32_t>(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));

src/hooks/dhcp/radius/client_dictionary.h

@@ -292,8 +292,10 @@ public:
     /// 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 @@ public:
     /// 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 @@ protected:
     /// @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_;

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, "" },
+    { "data",   Element::string, "" },
-    { "expr", Element::string, "" },
+    { "expr",   Element::string, "" },
-    { "raw",  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<int64_t>(vendor_txt);
+                if ((val < numeric_limits<int32_t>::min()) ||
+                    (val > numeric_limits<uint32_t>::max())) {
+                    isc_throw(Unexpected, "not 32 bit " << vendor_txt);
+                }
+                vendor = static_cast<uint32_t>(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 '"
+            ostringstream msg;
-                      << name->stringValue() << "' is unknown");
+            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<uint8_t>(type->intValue());
         if (def && (def->type_ != attrib)) {
-            isc_throw(ConfigError, name->stringValue() << " attribute has "
+            ostringstream msg;
-                      << "type " << static_cast<unsigned>(def->type_)
+            msg << "'" << name->stringValue() << "' attribute";
-                      << ", not " << static_cast<unsigned>(attrib));
+            if (vendor != 0) {
+                msg << " in vendor '" << vendor_txt << "'";
+            }
+            msg << " has type " << static_cast<unsigned>(def->type_)
+                << ", not " << static_cast<unsigned>(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 "
+            ostringstream msg;
-                      << static_cast<unsigned>(attrib) << " is unknown");
+            msg << "attribute type " << static_cast<unsigned>(attrib);
+            if (vendor != 0) {
+                msg << " in vendor '" << vendor_txt << "'";
+            }
+            msg << " is unknown";
+            isc_throw(ConfigError, msg.str());
         }
     }
 

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.

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();

src/hooks/dhcp/radius/tests/dictionary_unittests.cc

@@ -53,23 +53,33 @@ public:
     /// @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<string>& lines, unsigned int depth = 0) {
+    void parseLines(const list<string>& 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,
+    EXPECT_THROW_MSG(parseLine("BEGIN-TLV my-vendor"), BadValue,
-                     "unknown dictionary entry 'BEGIN-VENDOR' at line 1");
+                     "unknown dictionary entry 'BEGIN-TLV' at line 1");
-    EXPECT_THROW_MSG(parseLine("END-VENDOR my-vendor"), BadValue,
+    EXPECT_THROW_MSG(parseLine("END-TLV my-vendor"), BadValue,
-                     "unknown dictionary entry 'END-VENDOR' at line 1");
+                     "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<string> 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<string> 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<string> begin0 = {
+        "BEGIN-VENDOR 0"
+    };
+    EXPECT_THROW_MSG(parseLines(begin0), BadValue,
+                     "0 is reserved at line 1");
+
+    // Positive using a name.
+    list<string> 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<string> 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<string> end_name = {
+        "END-VENDOR DSL-Forum"
+    };
+    EXPECT_NO_THROW_LOG(parseLines(end_name, 3561, 0));
+
+    // End using an integer.
+    list<string> end_int = {
+        "END-VENDOR 3561"
+        };
+    EXPECT_NO_THROW_LOG(parseLines(end_int, 3561, 0));
+
+    // Not matching.
+    list<string> 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<string> 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.