From edb8d145070a9a51b6fa00ed69ec61503a531b02 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Nov 2016 10:18:16 -0800 Subject: [PATCH 01/11] Add ac address whitelist to domain server settings --- domain-server/resources/describe-settings.json | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/domain-server/resources/describe-settings.json b/domain-server/resources/describe-settings.json index e6663888b4..12e2c51a2c 100644 --- a/domain-server/resources/describe-settings.json +++ b/domain-server/resources/describe-settings.json @@ -388,6 +388,23 @@ "default": "", "advanced": false }, + { + "name": "ac_address_whitelist", + "label": "Assignment Client IP address Whitelist", + "type": "table", + "can_add_new_rows": true, + "help": "IP addresses of Assignment Clients that can connect to your domain server. Local ACs (localhost) are always permitted and do not need to be added here.", + "numbered": false, + "advanced": true, + "columns": [ + { + "name": "ip", + "label": "IP Address", + "type": "ip", + "can_set": true + } + ] + }, { "name": "standard_permissions", "type": "table", From 63cfbf55ca0478e72455b77269a2d59474d898a0 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Nov 2016 10:19:03 -0800 Subject: [PATCH 02/11] Add ip address whitelist processing to domain server --- domain-server/src/DomainServer.cpp | 36 ++++++++++++++++++++++++++++++ domain-server/src/DomainServer.h | 5 +++++ 2 files changed, 41 insertions(+) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index d13f9b883f..6766290440 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -158,6 +158,42 @@ DomainServer::DomainServer(int argc, char* argv[]) : qDebug() << "domain-server is running"; + static const SubnetMask LOCALHOST_MASK { QHostAddress("127.0.0.1"), 32 }; + + this->_acIPAddressWhitelist = { LOCALHOST_MASK }; + + _settingsManager.getWhitelistAssignmentClientAddresses(); + auto whitelist = _settingsManager.valueOrDefaultValueForKeyPath("security.ac_address_whitelist").toStringList(); + for (auto& mask : whitelist) { + auto maskParts = mask.trimmed().split("/"); + + if (maskParts.size() > 2) { + qDebug() << "Ignoring ip in whitelist, malformed: " << mask; + continue; + } + + // The default netmask is 32 if one has not been specified, which will + // match only the ip provided. + int netmask = 32; + + if (maskParts.size() == 2) { + bool ok; + netmask = maskParts[1].toInt(&ok); + if (!ok) { + qDebug() << "Ignoring ip in whitelist, bad netmask: " << mask; + continue; + } + } + + auto ip = QHostAddress(maskParts[0]); + + if (!ip.isNull()) { + qDebug() << "Adding AC whitelist IP: " << mask << " -> " << (ip.toString() + "/" + QString::number(netmask)); + _acIPAddressWhitelist.push_back({ ip , netmask }); + } else { + qDebug() << "Ignoring ip in whitelist, invalid ip: " << mask; + } + } } void DomainServer::parseCommandLine() { diff --git a/domain-server/src/DomainServer.h b/domain-server/src/DomainServer.h index c14ec5eee0..34c408b621 100644 --- a/domain-server/src/DomainServer.h +++ b/domain-server/src/DomainServer.h @@ -36,6 +36,9 @@ typedef QSharedPointer SharedAssignmentPointer; typedef QMultiHash TransactionHash; +using SubnetMask = QPair; +using SubnetMaskList = std::vector>; + class DomainServer : public QCoreApplication, public HTTPSRequestHandler { Q_OBJECT public: @@ -156,6 +159,8 @@ private: void setupGroupCacheRefresh(); + SubnetMaskList _acIPAddressWhitelist; + DomainGatekeeper _gatekeeper; HTTPManager _httpManager; From ca6a74d63d1fa02cc89921aaa00bb2630f080508 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Nov 2016 10:19:46 -0800 Subject: [PATCH 03/11] Add whitelist filtering to assignment requests --- domain-server/src/DomainServer.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 6766290440..722ffc6a74 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -1037,6 +1037,23 @@ void DomainServer::processRequestAssignmentPacket(QSharedPointergetSenderSockAddr().getAddress(); + + auto isHostAddressInSubnet = [&senderAddr](const SubnetMask& mask) -> bool { + return senderAddr.isInSubnet(mask); + }; + + auto it = find_if(_acIPAddressWhitelist.begin(), _acIPAddressWhitelist.end(), isHostAddressInSubnet); + if (it != _acIPAddressWhitelist.end()) { + auto maskString = it->first.toString() + "/" + QString::number(it->second); + qDebug() << "Received connection from whitelisted ip: " << senderAddr.toString() + << ", matches subnet mask: " << maskString; + } else { + qDebug() << "Received an assignment connect request from a disallowed ip address: " + << senderAddr; + return; + } + // Suppress these for Assignment::AgentType to once per 5 seconds static QElapsedTimer noisyMessageTimer; static bool wasNoisyTimerStarted = false; From e1466c0d47d85c071449176e2caa740675bda8c4 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Nov 2016 10:21:58 -0800 Subject: [PATCH 04/11] Fix NULL -> nullptr --- libraries/shared/src/HifiConfigVariantMap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/shared/src/HifiConfigVariantMap.cpp b/libraries/shared/src/HifiConfigVariantMap.cpp index 252079f182..a1be6db448 100644 --- a/libraries/shared/src/HifiConfigVariantMap.cpp +++ b/libraries/shared/src/HifiConfigVariantMap.cpp @@ -226,5 +226,5 @@ QVariant* valueForKeyPath(QVariantMap& variantMap, const QString& keyPath, bool shouldCreateIfMissing); } - return NULL; + return nullptr; } From d691aa3302053bce7b58efbcb37c38027c12ecc0 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Nov 2016 10:32:45 -0800 Subject: [PATCH 05/11] Update naming for subnet whitelist in ds --- domain-server/src/DomainServer.cpp | 12 ++++++------ domain-server/src/DomainServer.h | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 722ffc6a74..8e1cabe149 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -158,9 +158,9 @@ DomainServer::DomainServer(int argc, char* argv[]) : qDebug() << "domain-server is running"; - static const SubnetMask LOCALHOST_MASK { QHostAddress("127.0.0.1"), 32 }; + static const Subnet LOCALHOST { QHostAddress("127.0.0.1"), 32 }; - this->_acIPAddressWhitelist = { LOCALHOST_MASK }; + this->_acSubnetWhitelist = { LOCALHOST }; _settingsManager.getWhitelistAssignmentClientAddresses(); auto whitelist = _settingsManager.valueOrDefaultValueForKeyPath("security.ac_address_whitelist").toStringList(); @@ -189,7 +189,7 @@ DomainServer::DomainServer(int argc, char* argv[]) : if (!ip.isNull()) { qDebug() << "Adding AC whitelist IP: " << mask << " -> " << (ip.toString() + "/" + QString::number(netmask)); - _acIPAddressWhitelist.push_back({ ip , netmask }); + _acSubnetWhitelist.push_back({ ip , netmask }); } else { qDebug() << "Ignoring ip in whitelist, invalid ip: " << mask; } @@ -1039,12 +1039,12 @@ void DomainServer::processRequestAssignmentPacket(QSharedPointergetSenderSockAddr().getAddress(); - auto isHostAddressInSubnet = [&senderAddr](const SubnetMask& mask) -> bool { + auto isHostAddressInSubnet = [&senderAddr](const Subnet& mask) -> bool { return senderAddr.isInSubnet(mask); }; - auto it = find_if(_acIPAddressWhitelist.begin(), _acIPAddressWhitelist.end(), isHostAddressInSubnet); - if (it != _acIPAddressWhitelist.end()) { + auto it = find_if(_acSubnetWhitelist.begin(), _acSubnetWhitelist.end(), isHostAddressInSubnet); + if (it != _acSubnetWhitelist.end()) { auto maskString = it->first.toString() + "/" + QString::number(it->second); qDebug() << "Received connection from whitelisted ip: " << senderAddr.toString() << ", matches subnet mask: " << maskString; diff --git a/domain-server/src/DomainServer.h b/domain-server/src/DomainServer.h index 34c408b621..73135695eb 100644 --- a/domain-server/src/DomainServer.h +++ b/domain-server/src/DomainServer.h @@ -36,8 +36,8 @@ typedef QSharedPointer SharedAssignmentPointer; typedef QMultiHash TransactionHash; -using SubnetMask = QPair; -using SubnetMaskList = std::vector>; +using Subnet = QPair; +using SubnetList = std::vector; class DomainServer : public QCoreApplication, public HTTPSRequestHandler { Q_OBJECT @@ -159,7 +159,7 @@ private: void setupGroupCacheRefresh(); - SubnetMaskList _acIPAddressWhitelist; + SubnetList _acSubnetWhitelist; DomainGatekeeper _gatekeeper; From bd1bcaf1a426733c039c892fe16d60f3eb7f9ef5 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Nov 2016 10:38:56 -0800 Subject: [PATCH 06/11] Clean up subnet whitelist implementation --- domain-server/src/DomainServer.cpp | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 8e1cabe149..d8420c580c 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -158,17 +158,17 @@ DomainServer::DomainServer(int argc, char* argv[]) : qDebug() << "domain-server is running"; - static const Subnet LOCALHOST { QHostAddress("127.0.0.1"), 32 }; + static const QString AC_SUBNET_WHITELIST_SETTING_PATH = "security.ac_subnet_whitelist"; + static const Subnet LOCALHOST { QHostAddress("127.0.0.1"), 32 }; this->_acSubnetWhitelist = { LOCALHOST }; - _settingsManager.getWhitelistAssignmentClientAddresses(); - auto whitelist = _settingsManager.valueOrDefaultValueForKeyPath("security.ac_address_whitelist").toStringList(); - for (auto& mask : whitelist) { - auto maskParts = mask.trimmed().split("/"); + auto whitelist = _settingsManager.valueOrDefaultValueForKeyPath(AC_SUBNET_WHITELIST_SETTING_PATH).toStringList(); + for (auto& subnet : whitelist) { + auto netmaskParts = subnet.trimmed().split("/"); - if (maskParts.size() > 2) { - qDebug() << "Ignoring ip in whitelist, malformed: " << mask; + if (netmaskParts.size() > 2) { + qDebug() << "Ignoring subnet in whitelist, malformed: " << subnet; continue; } @@ -176,22 +176,22 @@ DomainServer::DomainServer(int argc, char* argv[]) : // match only the ip provided. int netmask = 32; - if (maskParts.size() == 2) { + if (netmaskParts.size() == 2) { bool ok; - netmask = maskParts[1].toInt(&ok); + netmask = netmaskParts[1].toInt(&ok); if (!ok) { - qDebug() << "Ignoring ip in whitelist, bad netmask: " << mask; + qDebug() << "Ignoring subnet in whitelist, bad netmask: " << subnet; continue; } } - auto ip = QHostAddress(maskParts[0]); + auto ip = QHostAddress(netmaskParts[0]); if (!ip.isNull()) { - qDebug() << "Adding AC whitelist IP: " << mask << " -> " << (ip.toString() + "/" + QString::number(netmask)); + qDebug() << "Adding AC whitelist subnet: " << subnet << " -> " << (ip.toString() + "/" + QString::number(netmask)); _acSubnetWhitelist.push_back({ ip , netmask }); } else { - qDebug() << "Ignoring ip in whitelist, invalid ip: " << mask; + qDebug() << "Ignoring subnet in whitelist, invalid ip portion: " << subnet; } } } From 969cbf4cb0160e4ee2e1b36cb8d3e513826d29df Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Nov 2016 10:46:36 -0800 Subject: [PATCH 07/11] Update domain server subnet whitelist description --- domain-server/resources/describe-settings.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/domain-server/resources/describe-settings.json b/domain-server/resources/describe-settings.json index 12e2c51a2c..a2a0c56648 100644 --- a/domain-server/resources/describe-settings.json +++ b/domain-server/resources/describe-settings.json @@ -389,11 +389,11 @@ "advanced": false }, { - "name": "ac_address_whitelist", + "name": "ac_subnet_whitelist", "label": "Assignment Client IP address Whitelist", "type": "table", "can_add_new_rows": true, - "help": "IP addresses of Assignment Clients that can connect to your domain server. Local ACs (localhost) are always permitted and do not need to be added here.", + "help": "The IP addresses or subnets of ACs that can connect to this server. You can specify an IP address or a subnet in CIDR notation ('A.B.C.D/E', Example: '10.0.0.0/24'). Local ACs (localhost) are always permitted and do not need to be added here.", "numbered": false, "advanced": true, "columns": [ From d522d03bd252316a9daac6590d3b0d08d0b4f236 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Nov 2016 11:29:52 -0800 Subject: [PATCH 08/11] Add repeated message suppression to request assignment packets --- domain-server/src/DomainServer.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index d8420c580c..e95379adb9 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -1045,12 +1045,16 @@ void DomainServer::processRequestAssignmentPacket(QSharedPointerfirst.toString() + "/" + QString::number(it->second); - qDebug() << "Received connection from whitelisted ip: " << senderAddr.toString() - << ", matches subnet mask: " << maskString; + qDebug() << "Received connection from whitelisted ip:" << senderAddr.toString() + << ", matches subnet mask:" << maskString; } else { - qDebug() << "Received an assignment connect request from a disallowed ip address: " - << senderAddr; + static QString repeatedMessage = LogHandler::getInstance().addRepeatedMessageRegex( + "Received an assignment connect request from a disallowed ip address: [^ ]+"); + qDebug() << "Received an assignment connect request from a disallowed ip address:" + << senderAddr.toString(); return; } From e177004d7178f7beef205f3bd3eb56e5c44d2805 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Nov 2016 14:23:16 -0800 Subject: [PATCH 09/11] Fix ds not restarting when changing ac whitelist --- domain-server/src/DomainServerSettingsManager.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/domain-server/src/DomainServerSettingsManager.cpp b/domain-server/src/DomainServerSettingsManager.cpp index c7944bbcad..f460ddd115 100644 --- a/domain-server/src/DomainServerSettingsManager.cpp +++ b/domain-server/src/DomainServerSettingsManager.cpp @@ -1077,6 +1077,9 @@ QJsonObject DomainServerSettingsManager::settingDescriptionFromGroup(const QJson } bool DomainServerSettingsManager::recurseJSONObjectAndOverwriteSettings(const QJsonObject& postedObject) { + static const QString SECURITY_ROOT_KEY = "security"; + static const QString AC_SUBNET_WHITELIST_KEY = "ac_subnet_whitelist"; + auto& settingsVariant = _configMap.getConfig(); bool needRestart = false; @@ -1127,7 +1130,7 @@ bool DomainServerSettingsManager::recurseJSONObjectAndOverwriteSettings(const QJ if (!matchingDescriptionObject.isEmpty()) { updateSetting(rootKey, rootValue, *thisMap, matchingDescriptionObject); - if (rootKey != "security") { + if (rootKey != SECURITY_ROOT_KEY) { needRestart = true; } } else { @@ -1143,7 +1146,7 @@ bool DomainServerSettingsManager::recurseJSONObjectAndOverwriteSettings(const QJ if (!matchingDescriptionObject.isEmpty()) { QJsonValue settingValue = rootValue.toObject()[settingKey]; updateSetting(settingKey, settingValue, *thisMap, matchingDescriptionObject); - if (rootKey != "security") { + if (rootKey != SECURITY_ROOT_KEY || settingKey == AC_SUBNET_WHITELIST_KEY) { needRestart = true; } } else { From fc1a7255f6d7cebbcd2074338c13d3ed8fd479d4 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Nov 2016 14:24:28 -0800 Subject: [PATCH 10/11] Remove unneeded 'this->' --- domain-server/src/DomainServer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index e95379adb9..73dfc3a277 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -161,7 +161,7 @@ DomainServer::DomainServer(int argc, char* argv[]) : static const QString AC_SUBNET_WHITELIST_SETTING_PATH = "security.ac_subnet_whitelist"; static const Subnet LOCALHOST { QHostAddress("127.0.0.1"), 32 }; - this->_acSubnetWhitelist = { LOCALHOST }; + _acSubnetWhitelist = { LOCALHOST }; auto whitelist = _settingsManager.valueOrDefaultValueForKeyPath(AC_SUBNET_WHITELIST_SETTING_PATH).toStringList(); for (auto& subnet : whitelist) { From 528b8e93f00c3a6ea5ea4b0627cbf7bdd5d4fe7d Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Nov 2016 14:33:34 -0800 Subject: [PATCH 11/11] Remove extraneous logging in DomainServer for whitelist --- domain-server/src/DomainServer.cpp | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 73dfc3a277..5208cb2326 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -1044,13 +1044,7 @@ void DomainServer::processRequestAssignmentPacket(QSharedPointerfirst.toString() + "/" + QString::number(it->second); - qDebug() << "Received connection from whitelisted ip:" << senderAddr.toString() - << ", matches subnet mask:" << maskString; - } else { + if (it == _acSubnetWhitelist.end()) { static QString repeatedMessage = LogHandler::getInstance().addRepeatedMessageRegex( "Received an assignment connect request from a disallowed ip address: [^ ]+"); qDebug() << "Received an assignment connect request from a disallowed ip address:"