Skip to content

Commit

Permalink
[CrOS VPN] New case for blocking user from configuring VPN
Browse files Browse the repository at this point in the history
This is the C++ follow-up to http://crrev/c/4891265.

Adds a Chrome-sid C++ check to prohibit users from creating
a VPN configuration when the admin activates always on VPN
for all user traffic [1], and prohibits users from manually
disconnecting from the VPN [2].

[1] https://screenshot.googleplex.com/4umCzz9bG6GjVGc
[2] https://screenshot.googleplex.com/4KLUdLUtPsvNMDT

Bug: b/297580117
Test: chromeos_unittest *CrosNetworkConfig*
Change-Id: I27ffcd419bfca7c06ab8bbc2eaaa985df2eaa02f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4903599
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Reviewed-by: Chad Duffin <chadduffin@chromium.org>
Commit-Queue: Regan Hsu <hsuregan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1211283}
  • Loading branch information
Regan Hsu authored and Chromium LUCI CQ committed Oct 18, 2023
1 parent 7ae0e9e commit d842189
Show file tree
Hide file tree
Showing 12 changed files with 146 additions and 0 deletions.
2 changes: 2 additions & 0 deletions chromeos/ash/components/network/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ config("network_config") {
component("network") {
configs += [ ":network_config" ]
deps = [
"//ash/components/arc:prefs",
"//ash/constants",
"//base",
"//chromeos/ash/components/dbus/hermes",
Expand Down Expand Up @@ -282,6 +283,7 @@ source_set("unit_tests") {
deps = [
":network",
":test_support",
"//ash/components/arc:prefs",
"//ash/constants",
"//base",
"//base:i18n",
Expand Down
4 changes: 4 additions & 0 deletions chromeos/ash/components/network/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,11 @@ specific_include_rules = {
"ui_proxy_config_service_unittest.cc": [
"+components/sync_preferences/testing_pref_service_syncable.h"
],
"managed_network_configuration_handler_impl.cc": [
"+ash/components/arc/arc_prefs.h"
],
"managed_network_configuration_handler_unittest.cc": [
"+ash/components/arc/arc_prefs.h",
"+components/sync_preferences/testing_pref_service_syncable.h"
],
"cellular_policy_handler_unittest.cc": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,12 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) ManagedNetworkConfigurationHandler {
// enabled.
virtual bool UserCreatedNetworkConfigurationsAreEphemeral() const = 0;

// Return true if the following user prefs exist and meet the following
// conditions: `arc::prefs::kAlwaysOnVpnPackage` is non-empty,
// `arc::prefs::kAlwaysOnVpnLockdown` is true, and `prefs::kVpnConfigAllowed`
// is false.
virtual bool IsProhibitedFromConfiguringVpn() const = 0;

// Returns the value for the AllowTextMessages policy.
virtual PolicyTextMessageSuppressionState GetAllowTextMessages() const = 0;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
#include <utility>
#include <vector>

#include "ash/components/arc/arc_prefs.h"
#include "ash/constants/ash_features.h"
#include "ash/constants/ash_pref_names.h"
#include "base/containers/contains.h"
#include "base/functional/bind.h"
#include "base/functional/callback_helpers.h"
Expand Down Expand Up @@ -702,6 +704,11 @@ void ManagedNetworkConfigurationHandlerImpl::set_ui_proxy_config_service(
ui_proxy_config_service_ = ui_proxy_config_service;
}

void ManagedNetworkConfigurationHandlerImpl::set_user_prefs(
PrefService* user_prefs) {
user_prefs_ = user_prefs;
}

void ManagedNetworkConfigurationHandlerImpl::OnProfileAdded(
const NetworkProfile& profile) {
VLOG(1) << "Adding profile: " << profile.ToDebugString();
Expand Down Expand Up @@ -1058,6 +1065,25 @@ bool ManagedNetworkConfigurationHandlerImpl::
.value_or(false);
}

bool ManagedNetworkConfigurationHandlerImpl::IsProhibitedFromConfiguringVpn()
const {
if (!user_prefs_ ||
!user_prefs_->FindPreference(arc::prefs::kAlwaysOnVpnPackage) ||
!user_prefs_->FindPreference(arc::prefs::kAlwaysOnVpnLockdown) ||
!user_prefs_->FindPreference(prefs::kVpnConfigAllowed)) {
return false;
}

// When an admin Activate Always ON VPN for all user traffic with an Android
// VPN, arc::prefs::kAlwaysOnVpnPackage will be non empty, and
// arc::prefs::kAlwaysOnVpnLockdown will be true. If additionally, the admin
// prohibits users from disconnecting from a VPN manually,
// prefs::kVpnConfigAllowed becomes false. See go/test-cros-vpn-policies.
return !user_prefs_->GetString(arc::prefs::kAlwaysOnVpnPackage).empty() &&
user_prefs_->GetBoolean(arc::prefs::kAlwaysOnVpnLockdown) &&
!user_prefs_->GetBoolean(prefs::kVpnConfigAllowed);
}

std::vector<std::string>
ManagedNetworkConfigurationHandlerImpl::GetBlockedHexSSIDs() const {
const base::Value::List* blocked_value =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
#include "chromeos/ash/components/network/policy_applicator.h"
#include "chromeos/ash/components/network/profile_policies.h"
#include "chromeos/ash/components/network/text_message_suppression_state.h"
#include "components/prefs/pref_service.h"

class PrefService;

namespace base {
class Value;
Expand Down Expand Up @@ -155,6 +158,8 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) ManagedNetworkConfigurationHandlerImpl
bool AllowOnlyPolicyWiFiToConnect() const override;
bool AllowOnlyPolicyWiFiToConnectIfAvailable() const override;
bool AllowOnlyPolicyNetworksToAutoconnect() const override;
bool IsProhibitedFromConfiguringVpn() const override;

bool RecommendedValuesAreEphemeral() const override;
bool UserCreatedNetworkConfigurationsAreEphemeral() const override;
std::vector<std::string> GetBlockedHexSSIDs() const override;
Expand Down Expand Up @@ -336,6 +341,8 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) ManagedNetworkConfigurationHandlerImpl
void set_ui_proxy_config_service(
UIProxyConfigService* ui_proxy_config_service);

void set_user_prefs(PrefService* user_prefs);

// Returns the device policy GlobalNetworkConfiguration boolean value under
// `key` or `absl::nullopt` if such a value doesn't exist or is not of type
// BOOLEAN.
Expand Down Expand Up @@ -370,6 +377,9 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) ManagedNetworkConfigurationHandlerImpl
raw_ptr<HotspotController, DanglingUntriaged | ExperimentalAsh>
hotspot_controller_ = nullptr;

// Initialized to null and set once SetUserPrefs() is called.
raw_ptr<PrefService, ExperimentalAsh> user_prefs_ = nullptr;

UserToPolicyApplicationInfo policy_application_info_map_;

base::ObserverList<NetworkPolicyObserver, true>::Unchecked observers_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
#include <string>
#include <utility>

#include "ash/components/arc/arc_prefs.h"
#include "ash/constants/ash_features.h"
#include "ash/constants/ash_pref_names.h"
#include "base/functional/bind.h"
#include "base/functional/callback_helpers.h"
#include "base/logging.h"
Expand Down Expand Up @@ -237,6 +239,7 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test {
network_handler->hotspot_controller());
managed_network_configuration_handler_->set_ui_proxy_config_service(
ui_proxy_config_service_.get());
managed_network_configuration_handler_->set_user_prefs(&user_prefs_);
managed_network_configuration_handler_->AddObserver(&policy_observer_);
cellular_policy_handler_->Init(
cellular_esim_profile_handler_.get(), cellular_esim_installer_.get(),
Expand Down Expand Up @@ -402,6 +405,17 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test {
CellularConnectionHandler::kWaitingForAutoConnectTimeout);
}

void SetArcAlwaysOnUserPrefs(std::string package_name,
bool lockdown,
bool vpn_configured_allowed = false) {
user_prefs_.SetUserPref(arc::prefs::kAlwaysOnVpnPackage,
base::Value(package_name));
user_prefs_.SetUserPref(arc::prefs::kAlwaysOnVpnLockdown,
base::Value(lockdown));
user_prefs_.SetUserPref(prefs::kVpnConfigAllowed,
base::Value(vpn_configured_allowed));
}

ProhibitedTechnologiesHandler* prohibited_technologies_handler() {
return prohibited_technologies_handler_.get();
}
Expand Down Expand Up @@ -2394,4 +2408,24 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, ActiveProxySettingsPreference) {
ASSERT_EQ(*policy_after_pref, "Direct");
}

TEST_F(ManagedNetworkConfigurationHandlerTest, IsProhibitedFromConfiguringVpn) {
arc::prefs::RegisterProfilePrefs(user_prefs_.registry());
user_prefs_.registry()->RegisterBooleanPref(prefs::kVpnConfigAllowed, true);

for (const std::string& package_name : {"", "package_name"}) {
for (const bool lockdown : {true, false}) {
for (const bool vpn_configure_allowed : {true, false}) {
SetArcAlwaysOnUserPrefs(package_name, lockdown, vpn_configure_allowed);
if (package_name.empty() || !lockdown || vpn_configure_allowed) {
EXPECT_FALSE(managed_network_configuration_handler_
->IsProhibitedFromConfiguringVpn());
continue;
}
EXPECT_TRUE(managed_network_configuration_handler_
->IsProhibitedFromConfiguringVpn());
}
}
}
}

} // namespace ash
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) MockManagedNetworkConfigurationHandler
MOCK_CONST_METHOD0(AllowOnlyPolicyWiFiToConnect, bool());
MOCK_CONST_METHOD0(AllowOnlyPolicyWiFiToConnectIfAvailable, bool());
MOCK_CONST_METHOD0(AllowOnlyPolicyNetworksToAutoconnect, bool());
MOCK_CONST_METHOD0(IsProhibitedFromConfiguringVpn, bool());
MOCK_CONST_METHOD0(RecommendedValuesAreEphemeral, bool());
MOCK_CONST_METHOD0(UserCreatedNetworkConfigurationsAreEphemeral, bool());
MOCK_CONST_METHOD0(GetAllowTextMessages, PolicyTextMessageSuppressionState());
Expand Down
3 changes: 3 additions & 0 deletions chromeos/ash/components/network/network_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,8 @@ void NetworkHandler::InitializePrefServices(
network_profile_handler_.get()));
managed_network_configuration_handler_->set_ui_proxy_config_service(
ui_proxy_config_service_.get());
managed_network_configuration_handler_->set_user_prefs(
logged_in_profile_prefs);
network_metadata_store_.reset(new NetworkMetadataStore(
network_configuration_handler_.get(), network_connection_handler_.get(),
network_state_handler_.get(),
Expand Down Expand Up @@ -297,6 +299,7 @@ void NetworkHandler::ShutdownPrefServices() {
cellular_esim_profile_handler_->SetDevicePrefs(nullptr);
managed_cellular_pref_handler_->SetDevicePrefs(nullptr);
ui_proxy_config_service_.reset();
managed_network_configuration_handler_->set_user_prefs(nullptr);
hidden_network_handler_->SetNetworkMetadataStore(nullptr);
if (features::IsSuppressTextMessagesEnabled()) {
text_message_provider_->SetNetworkMetadataStore(nullptr);
Expand Down
1 change: 1 addition & 0 deletions chromeos/ash/services/network_config/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ source_set("unit_tests") {
deps = [
":network_config",
":test_support",
"//ash/components/arc:prefs",
"//ash/constants",
"//base",
"//base/test:test_support",
Expand Down
1 change: 1 addition & 0 deletions chromeos/ash/services/network_config/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ include_rules = [

specific_include_rules = {
"cros_network_config_unittest.cc": [
"+ash/components/arc/arc_prefs.h",
"+components/proxy_config",
"+components/sync_preferences/testing_pref_service_syncable.h",
"+third_party/re2"
Expand Down
9 changes: 9 additions & 0 deletions chromeos/ash/services/network_config/cros_network_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ const char kErrorAccessToSharedConfig[] = "Error.CannotChangeSharedConfig";
const char kErrorInvalidONCConfiguration[] = "Error.InvalidONCConfiguration";
const char kErrorNetworkUnavailable[] = "Error.NetworkUnavailable";
const char kErrorNotReady[] = "Error.NotReady";
const char kErrorUserIsProhibitedFromConfiguringVpn[] =
"Error.UserIsProhibitedFromConfiguringVpn";

// Default traffic counter reset day.
const int kDefaultResetDay = 1;
Expand Down Expand Up @@ -2654,6 +2656,13 @@ void CrosNetworkConfig::ConfigureNetwork(mojom::ConfigPropertiesPtr properties,
return;
}

if (properties->type_config->is_vpn() &&
network_configuration_handler_->IsProhibitedFromConfiguringVpn()) {
std::move(callback).Run(/*guid=*/absl::nullopt,
kErrorUserIsProhibitedFromConfiguringVpn);
return;
}

absl::optional<base::Value::Dict> onc =
GetOncFromConfigProperties(properties.get(), /*guid=*/absl::nullopt);
if (!onc) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@

#include <tuple>

#include "ash/components/arc/arc_prefs.h"
#include "ash/constants/ash_features.h"
#include "ash/constants/ash_pref_names.h"
#include "base/containers/contains.h"
#include "base/functional/bind.h"
#include "base/functional/callback_helpers.h"
Expand Down Expand Up @@ -212,6 +214,21 @@ std::string CreateApnShillDict() {
return test_apn_data.AsApnShillDict();
}

mojom::ConfigPropertiesPtr CreateFakeVpnConfig(std::string name,
std::string host,
mojom::VpnType type) {
auto vpn = mojom::VPNConfigProperties::New();
vpn->host = host;
vpn->type = mojom::VpnTypeConfig::New();
vpn->type->value = type;

auto config = mojom::ConfigProperties::New();
config->name = name;
config->type_config =
mojom::NetworkTypeConfigProperties::NewVpn(std::move(vpn));
return config;
}

bool OncApnHasId(const base::Value::Dict& apn) {
if (const std::string* id = apn.FindString(::onc::cellular_apn::kId)) {
return re2::RE2::FullMatch(*id, kApnIdRegex);
Expand Down Expand Up @@ -776,6 +793,17 @@ class CrosNetworkConfigTest : public testing::Test {
base::RunLoop().RunUntilIdle();
}

void SetArcAlwaysOnUserPrefs(std::string package_name,
bool lockdown,
bool vpn_configured_allowed = false) {
user_prefs_.SetUserPref(arc::prefs::kAlwaysOnVpnPackage,
base::Value(package_name));
user_prefs_.SetUserPref(arc::prefs::kAlwaysOnVpnLockdown,
base::Value(lockdown));
user_prefs_.SetUserPref(prefs::kVpnConfigAllowed,
base::Value(vpn_configured_allowed));
}

std::vector<std::string> GetSupportedVpnTypes() {
std::vector<std::string> result;
base::RunLoop run_loop;
Expand Down Expand Up @@ -4011,6 +4039,27 @@ TEST_F(CrosNetworkConfigTest, SetAlwaysOnVpn) {
shill::kAlwaysOnVpnServiceProperty));
}

TEST_F(CrosNetworkConfigTest, IsProhibitedFromConfiguringVpn) {
arc::prefs::RegisterProfilePrefs(user_prefs_.registry());
user_prefs_.registry()->RegisterBooleanPref(prefs::kVpnConfigAllowed, true);

for (const std::string& package_name : {"", "package_name"}) {
for (const bool lockdown : {true, false}) {
for (const bool vpn_configure_allowed : {true, false}) {
SetArcAlwaysOnUserPrefs(package_name, lockdown, vpn_configure_allowed);
const std::string guid = ConfigureNetwork(
CreateFakeVpnConfig("name", "host", mojom::VpnType::kArc),
/*shared=*/true);
if (package_name.empty() || !lockdown || vpn_configure_allowed) {
EXPECT_FALSE(guid.empty());
continue;
}
EXPECT_TRUE(guid.empty());
}
}
}
}

TEST_F(CrosNetworkConfigTest, RequestTrafficCountersWithIntegerType) {
base::Value::List traffic_counters;

Expand Down

0 comments on commit d842189

Please sign in to comment.