[#4070] Addressed comments

diff --git a/changelog_unreleased/4070-add-basic-auth-user-file-option-to-high-availability-parameters b/changelog_unreleased/4070-add-basic-auth-user-file-option-to-high-availability-parameters
index 21398549df1f9f235a1a2cd6fc94bdeb76a3cfed..736a9c0c4c07de696ba25fbed5c55daa02fbbd78 100644 (file)
--- a/changelog_unreleased/4070-add-basic-auth-user-file-option-to-high-availability-parameters
+++ b/changelog_unreleased/4070-add-basic-auth-user-file-option-to-high-availability-parameters
@@ -1,6 +1,6 @@
-[func]         fdupont
+[sec]          fdupont
        Added the "basic-auth-user-file" parameter to the HA
-       hook library to provide the user ID which is part of
-       the secret used by the basic HTTP auth from a file
-       instead in clear in the configuration.
+       hook library. This allows the basic HTTP auth user
+       ID to be read from a file rather than specified as
+       clear text in the configuration.
        (Gitlab #4070)

diff --git a/doc/sphinx/arm/hooks-ha.rst b/doc/sphinx/arm/hooks-ha.rst
index bfeda44ba3c414b005fa4dcd6194f76925be6a62..e1ec8e5c6469a38c87ab945815c2d9fc3e8c206e 100644 (file)
--- a/doc/sphinx/arm/hooks-ha.rst
+++ b/doc/sphinx/arm/hooks-ha.rst
@@ -915,7 +915,7 @@ list:
    not specified or specified as an empty string, no authentication header is
    added to HTTP transactions. It must not contain the colon (:) character.
 
--  ``basic-auth-user-file`` - is an alternatibe to ``basic-auth-user``:
+-  ``basic-auth-user-file`` - is an alternative to ``basic-auth-user``:
    instead of presenting the user ID in the configuration file it is specified
    in the file indicated by this parameter.
 

diff --git a/src/hooks/dhcp/high_availability/tests/ha_config_unittest.cc b/src/hooks/dhcp/high_availability/tests/ha_config_unittest.cc
index 0ccb6861c455113c702e0235c48a1bf4d3af55f7..fb8970207554e0f6a58a4564a595be9d5642c5fb 100644 (file)
--- a/src/hooks/dhcp/high_availability/tests/ha_config_unittest.cc
+++ b/src/hooks/dhcp/high_availability/tests/ha_config_unittest.cc
@@ -1368,6 +1368,132 @@ TEST_F(HAConfigTest, invalidUser) {
         "user 'foo:bar' must not contain a ':' in peer 'server2'");
 }
 
+// Test that only one of basic-auth-password and basic-auth-password-file
+// is allowed
+TEST_F(HAConfigTest, twoBasicAuthPassword) {
+    std::string expected = "only one of basic-auth-password and ";
+    expected += "basic-auth-password-file parameter can be ";
+    expected += "configured in peer 'server2'";
+    testInvalidConfig(
+        "["
+        "    {"
+        "        \"this-server-name\": \"server1\","
+        "        \"mode\": \"load-balancing\","
+        "        \"peers\": ["
+        "            {"
+        "                \"name\": \"server1\","
+        "                \"url\": \"http://127.0.0.1:8080/\","
+        "                \"role\": \"primary\","
+        "                \"auto-failover\": false"
+        "            },"
+        "            {"
+        "                \"name\": \"server2\","
+        "                \"url\": \"http://127.0.0.1:8080/\","
+        "                \"basic-auth-user\": \"foobar\","
+        "                \"basic-auth-password\": \"foobar\","
+        "                \"basic-auth-password-file\": \"foobar\","
+        "                \"role\": \"secondary\","
+        "                \"auto-failover\": true"
+        "            }"
+        "        ]"
+        "    }"
+        "]",
+        expected);
+}
+
+// Test that invalid basic-auth-password-file is refused.
+TEST_F(HAConfigTest, invalidBasicAuthPasswordFile) {
+    std::string expected = "bad password file in peer 'server2': ";
+    expected += "Expected a file at path '/does/not/exist'";
+    testInvalidConfig(
+        "["
+        "    {"
+        "        \"this-server-name\": \"server1\","
+        "        \"mode\": \"load-balancing\","
+        "        \"peers\": ["
+        "            {"
+        "                \"name\": \"server1\","
+        "                \"url\": \"http://127.0.0.1:8080/\","
+        "                \"role\": \"primary\","
+        "                \"auto-failover\": false"
+        "            },"
+        "            {"
+        "                \"name\": \"server2\","
+        "                \"url\": \"http://127.0.0.1:8080/\","
+        "                \"basic-auth-user\": \"foobar\","
+        "                \"basic-auth-password-file\": \"/does/not/exist\","
+        "                \"role\": \"secondary\","
+        "                \"auto-failover\": true"
+        "            }"
+        "        ]"
+        "    }"
+        "]",
+        expected);
+}
+
+// Test that only one of basic-auth-user and basic-auth-user-file
+// is allowed
+TEST_F(HAConfigTest, twoBasicAuthUser) {
+    std::string expected = "only one of basic-auth-user and ";
+    expected += "basic-auth-user-file parameter can be ";
+    expected += "configured in peer 'server2'";
+    testInvalidConfig(
+        "["
+        "    {"
+        "        \"this-server-name\": \"server1\","
+        "        \"mode\": \"load-balancing\","
+        "        \"peers\": ["
+        "            {"
+        "                \"name\": \"server1\","
+        "                \"url\": \"http://127.0.0.1:8080/\","
+        "                \"role\": \"primary\","
+        "                \"auto-failover\": false"
+        "            },"
+        "            {"
+        "                \"name\": \"server2\","
+        "                \"url\": \"http://127.0.0.1:8080/\","
+        "                \"basic-auth-user\": \"foobar\","
+        "                \"basic-auth-user-file\": \"foobar\","
+        "                \"basic-auth-password\": \"foobar\","
+        "                \"role\": \"secondary\","
+        "                \"auto-failover\": true"
+        "            }"
+        "        ]"
+        "    }"
+        "]",
+        expected);
+}
+
+// Test that invalid basic-auth-user-file is refused.
+TEST_F(HAConfigTest, invalidBasicAuthUserFile) {
+    std::string expected = "bad user file in peer 'server2': ";
+    expected += "Expected a file at path '/does/not/exist'";
+    testInvalidConfig(
+        "["
+        "    {"
+        "        \"this-server-name\": \"server1\","
+        "        \"mode\": \"load-balancing\","
+        "        \"peers\": ["
+        "            {"
+        "                \"name\": \"server1\","
+        "                \"url\": \"http://127.0.0.1:8080/\","
+        "                \"role\": \"primary\","
+        "                \"auto-failover\": false"
+        "            },"
+        "            {"
+        "                \"name\": \"server2\","
+        "                \"url\": \"http://127.0.0.1:8080/\","
+        "                \"basic-auth-user-file\": \"/does/not/exist\","
+        "                \"basic-auth-password\": \"foobar\","
+        "                \"role\": \"secondary\","
+        "                \"auto-failover\": true"
+        "            }"
+        "        ]"
+        "    }"
+        "]",
+        expected);
+}
+
 // Test that setting delayed-updates-limit is not allowed in hot-standby mode.
 TEST_F(HAConfigTest, hotStandbyDelayedUpdatesLimit) {
     testInvalidConfig(
@@ -2110,5 +2236,4 @@ TEST_F(HAConfigTest, getSubnetServerNameInvalid) {
     EXPECT_THROW(HAConfig::getSubnetServerName(subnet6), BadValue);
 }
 
-
 }  // namespace