From bc730569829fdb94d1d93e5e0cd6d18dfa9c8e09 Mon Sep 17 00:00:00 2001
From: Tomek Mrugalski <tomasz@isc.org>
Date: Thu, 5 Jan 2017 14:01:53 +0100
Subject: [PATCH] [5032] Duplicate mac-sources are no longer accepted.

---
 src/lib/dhcpsrv/cfg_mac_source.cc             | 12 +++++++++-
 src/lib/dhcpsrv/cfg_mac_source.h              |  8 +++----
 src/lib/dhcpsrv/parsers/dhcp_parsers.cc       |  3 +++
 .../dhcpsrv/tests/dhcp_parsers_unittest.cc    | 23 +++++++++++++++++++
 4 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/src/lib/dhcpsrv/cfg_mac_source.cc b/src/lib/dhcpsrv/cfg_mac_source.cc
index 4307d407c1..9cc1dbd35f 100644
--- a/src/lib/dhcpsrv/cfg_mac_source.cc
+++ b/src/lib/dhcpsrv/cfg_mac_source.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2015,2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -47,6 +47,16 @@ uint32_t CfgMACSource::MACSourceFromText(const std::string& name) {
     isc_throw(BadValue, "Can't convert '" << name << "' to any known MAC source.");
 }
 
+void CfgMACSource::add(uint32_t source) {
+    for (CfgMACSources::const_iterator it = mac_sources_.begin();
+         it != mac_sources_.end(); ++it) {
+        if (*it == source) {
+            isc_throw(InvalidParameter, "mac-source paramter " << source
+                      << "' specified twice.");
+        }
+    }
+    mac_sources_.push_back(source);
+}
 
 };
 };
diff --git a/src/lib/dhcpsrv/cfg_mac_source.h b/src/lib/dhcpsrv/cfg_mac_source.h
index cddfceedfb..11ff2870c0 100644
--- a/src/lib/dhcpsrv/cfg_mac_source.h
+++ b/src/lib/dhcpsrv/cfg_mac_source.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2015,2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -52,10 +52,8 @@ class CfgMACSource {
     /// @param source MAC source (see constants in Pkt::HWADDR_SOURCE_*)
     ///
     /// Specified source is being added to the mac_sources_ array.
-    /// @todo implement add(string) version of this method.
-    void add(uint32_t source) {
-        mac_sources_.push_back(source);
-    }
+    /// @throw InvalidParameter if such a source is already defined.
+    void add(uint32_t source);
 
     /// @brief Provides access to the configure MAC/Hardware address sources.
     ///
diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc
index c2e0389d80..3ff2634b0e 100644
--- a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc
+++ b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc
@@ -193,6 +193,9 @@ MACSourcesListConfigParser::parse(CfgMACSource& mac_sources, ConstElementPtr val
             source = CfgMACSource::MACSourceFromText(source_str);
             mac_sources.add(source);
             ++cnt;
+        } catch (const InvalidParameter& ex) {
+            isc_throw(DhcpConfigError, "The mac-sources value '" << source_str
+                      << "' was specified twice (" << value->getPosition() << ")");
         } catch (const std::exception& ex) {
             isc_throw(DhcpConfigError, "Failed to convert '"
                       << source_str << "' to any recognized MAC source:"
diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
index 90e89f3958..4cdc8bea2a 100644
--- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
+++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
@@ -292,6 +292,29 @@ TEST_F(DhcpParserTest, MacSourcesBogus) {
     EXPECT_THROW(parser.parse(sources, values), DhcpConfigError);
 }
 
+/// Verifies the code that properly catches duplicate entries
+/// in mac-sources definition.
+TEST_F(DhcpParserTest, MacSourcesDuplicate) {
+
+    // That's an equivalent of the following snippet:
+    // "mac-sources: [ \"duid\", \"ipv6\" ]";
+    ElementPtr values = Element::createList();
+    values->add(Element::create("ipv6-link-local"));
+    values->add(Element::create("duid"));
+    values->add(Element::create("duid"));
+    values->add(Element::create("duid"));
+
+    // Let's grab server configuration from CfgMgr
+    SrvConfigPtr cfg = CfgMgr::instance().getStagingCfg();
+    ASSERT_TRUE(cfg);
+    CfgMACSource& sources = cfg->getMACSources();
+
+    // This should parse the configuration and check that it throws.
+    MACSourcesListConfigParser parser;
+    EXPECT_THROW(parser.parse(sources, values), DhcpConfigError);
+}
+
+
 /// @brief Test Fixture class which provides basic structure for testing
 /// configuration parsing.  This is essentially the same structure provided
 /// by dhcp servers.
-- 
2.47.3