authpolicy: Fix for setting fake domain SID.
In the latest Samba versions (4.8.6+), "net gpo list" expects
domain SID to be set, which is s associated with a workgroup
in secrets.tdb.
The previous code was setting domain SID only once in authpolicy
daemon lifetime associating it with the first workgroup for which
MaybeSetFakeDomainSid() was called. Therefore, it was failing if
workgroup was different for user and device accounts.
This fix is making sure setdomainsid is called for every account
workgroup.
BUG=chromium:1142823
TEST=emerge-${BOARD} samba && test_that -b ${BOARD} ${IP_OF_DUT} enterprise_AuthPolicyDaemonServer.auth
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2375331
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Tested-by: Tomasz Dobrowolski <tomdobro@chromium.org>
Commit-Queue: Tomasz Dobrowolski <tomdobro@chromium.org>
Auto-Submit: Tomasz Dobrowolski <tomdobro@chromium.org>
(cherry picked from commit 8740cfe406559812616da54247a4a07e1059a01b)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2414372
Reviewed-by: Tomasz Dobrowolski <tomdobro@chromium.org>
Change-Id: Ib203964102f5d61c8f091352a2cbb36a8a8c4570
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2512204
diff --git a/authpolicy/authpolicy_test.cc b/authpolicy/authpolicy_test.cc
index f075844..dd68fe8 100644
--- a/authpolicy/authpolicy_test.cc
+++ b/authpolicy/authpolicy_test.cc
@@ -117,7 +117,6 @@
constexpr char kCacheTestMachineDcName[] = "cache_test_machine_dc_name";
// See stub_net_main for the strings. Don't bother to make those constants.
-constexpr char kDefaultWorkgroup[] = "WOKGROUP";
constexpr char kDefaultKdcIp[] = "111.222.33.2";
constexpr char kDefaultDcName[] = "DCNAME.EXAMPLE.COM";
@@ -1797,6 +1796,35 @@
EXPECT_FALSE(user_affiliation_marker_set_);
}
+// Successful device and user policy fetch with empty policy for different
+// workgroups.
+TEST_F(AuthPolicyTest, DeviceAndUserPolicyFetchDifferentWorkgroupsSucceeds) {
+ // The workgroup is associated with the realm. Pass secondary workgroup realm,
+ // so `HandleWorkgroup()` imitating "net ads workgroup" in stub will fetch the
+ // secondary workgroup associated with it.
+ JoinDomainRequest request;
+ request.set_machine_name(kMachineName);
+ request.set_machine_domain(kSecondaryWorkgroupRealm);
+ request.set_user_principal_name(kUserPrincipal);
+ EXPECT_EQ(ERROR_NONE, JoinEx(request, MakePasswordFd()));
+ MarkDeviceAsLocked();
+ validate_device_policy_ = &CheckDevicePolicyEmpty;
+ FetchAndValidateDevicePolicy(ERROR_NONE);
+
+ // Validate device workgroup.
+ SmbConf device_smb_conf;
+ ReadSmbConf(paths_->Get(Path::DEVICE_SMB_CONF), &device_smb_conf);
+ EXPECT_EQ(kSecondaryWorkgroup, device_smb_conf.workgroup);
+
+ validate_user_policy_ = &CheckUserPolicyEmpty;
+ FetchAndValidateUserPolicy(DefaultAuth(), ERROR_NONE);
+
+ // Validate user workgroup.
+ SmbConf user_smb_conf;
+ ReadSmbConf(paths_->Get(Path::USER_SMB_CONF), &user_smb_conf);
+ EXPECT_EQ(kDefaultWorkgroup, user_smb_conf.workgroup);
+}
+
// Successful user policy fetch with actual data.
TEST_F(AuthPolicyTest, UserPolicyFetchSucceedsWithData) {
// Write a preg file with all basic data types. The file is picked up by
diff --git a/authpolicy/samba_interface.cc b/authpolicy/samba_interface.cc
index 9ef005f..0817b03 100644
--- a/authpolicy/samba_interface.cc
+++ b/authpolicy/samba_interface.cc
@@ -56,7 +56,7 @@
"\tclient ldap sasl wrapping = sign\n";
// Fake domain SID to work around issue in Samba-4.8.6+, see
-// MaybeSetFakeDomainSid().
+// `MaybeSetFakeDomainSid()`.
constexpr char kFakeDomainSid[] = "S-1-5-21-0000000000-0000000000-00000000";
constexpr int kFileMode_rwr = base::FILE_PERMISSION_READ_BY_USER |
@@ -994,6 +994,8 @@
return error;
// Download device GPOs with user policy data.
+ // Note that device account data was updated by calling `AcquireDeviceTgt()`
+ // above.
error = GetGpos(GpoSource::MACHINE, PolicyScope::USER, &gpo_file_paths);
if (error != ERROR_NONE)
return error;
@@ -1027,6 +1029,7 @@
return error;
// Download device GPOs with device policy data.
+ // Note that account data was updated by calling `AcquireDeviceTgt()` above.
std::vector<base::FilePath> gpo_file_paths;
error = GetGpos(GpoSource::MACHINE, PolicyScope::MACHINE, &gpo_file_paths);
if (error != ERROR_NONE)
@@ -1400,6 +1403,13 @@
return error;
}
+ // Set fake domain SID if it was not set for this account workgroup yet.
+ // This is workaround for Samba 4.8.6+, see `MaybeSetFakeDomainSid()`
+ // description.
+ error = MaybeSetFakeDomainSid(*account);
+ if (error != ERROR_NONE)
+ return error;
+
// Query the key distribution center IP and server time and store them in
// |account|->kdc_ip and |account|->server_time, respectively.
error = UpdateKdcIpAndServerTime(account);
@@ -1787,11 +1797,6 @@
<< " GPO list for "
<< (source == GpoSource::USER ? "user" : "device") << " account";
- // Work around issue in Samba 4.8.6+.
- ErrorType error = MaybeSetFakeDomainSid();
- if (error != ERROR_NONE)
- return error;
-
const AccountData& account = GetAccount(source);
const TgtManager& tgt_manager = GetTgtManager(source);
authpolicy::ProcessExecutor net_cmd(
@@ -2271,20 +2276,17 @@
AnonymizeRealm(user_realm, kUserRealmPlaceholder);
}
-ErrorType SambaInterface::MaybeSetFakeDomainSid() {
- if (fake_domain_sid_was_set_)
+ErrorType SambaInterface::MaybeSetFakeDomainSid(const AccountData& account) {
+ // Don't set twice for the same workgroup.
+ if (fake_domain_sid_was_set_for_workgroup_.find(account.workgroup) !=
+ fake_domain_sid_was_set_for_workgroup_.end())
return ERROR_NONE;
- // Make sure config is up-to-date.
- ErrorType error = UpdateAccountData(&device_account_);
- if (error != ERROR_NONE)
- return error;
-
// Reuse the NET_ADS_SECCOMP filter for simplicity, even though it's not a net
// ads command.
ProcessExecutor net_cmd({paths_->Get(Path::NET), "setdomainsid",
kFakeDomainSid, kConfigParam,
- paths_->Get(Path::DEVICE_SMB_CONF), kDebugParam,
+ paths_->Get(account.smb_conf_path), kDebugParam,
flags_.net_log_level()});
if (!jail_helper_.SetupJailAndRun(&net_cmd, Path::NET_ADS_SECCOMP,
TIMER_NONE)) {
@@ -2293,7 +2295,8 @@
return ERROR_LOCAL_IO;
}
- fake_domain_sid_was_set_ = true;
+ // Mark as set for this workgroup.
+ fake_domain_sid_was_set_for_workgroup_.insert(account.workgroup);
return ERROR_NONE;
}
diff --git a/authpolicy/samba_interface.h b/authpolicy/samba_interface.h
index a0aafdb..637b476 100644
--- a/authpolicy/samba_interface.h
+++ b/authpolicy/samba_interface.h
@@ -7,6 +7,7 @@
#include <map>
#include <memory>
+#include <set>
#include <string>
#include <vector>
@@ -431,15 +432,17 @@
// Similar to SetUser, but sets user_account_.realm.
void SetUserRealm(const std::string& user_realm);
- // Calls net setdomainsid S-1-5-21-0000000000-0000000000-00000000. This is a
- // workaround for Samba 4.8.6+, which expects a domain SID to exist in
- // add_builtin_guests() (Samba code). The SID is stored in Samba's,
- // secrets.tdb, which authpolicyd places in /tmp/authpolicyd/samba/private,
- // so it is wiped whenever the daemon is restarted and the SID is lost.
- // However, the SID is not really needed for our purposes, so we set a fake
- // SID here.
- // Earlies out if set successfully before.
- ErrorType MaybeSetFakeDomainSid() WARN_UNUSED_RESULT;
+ // Calls net setdomainsid S-1-5-21-0000000000-0000000000-00000000 if it was
+ // not set for the account workgroup yet. This is a workaround for
+ // Samba 4.8.6+, which expects a domain SID to exist in add_builtin_guests()
+ // (Samba code). Without a SID 'net ads gpo list' fails with "Failed to check
+ // for local Guests membership (NT_STATUS_INVALID_PARAMETER_MIX)". The SID is
+ // stored in Samba's, secrets.tdb as a key/value store with key as a
+ // workgroup, which authpolicyd places in /tmp/authpolicyd/samba/private, so
+ // it is wiped whenever the daemon is restarted and the SID is lost. However,
+ // the SID is not really needed for our purposes, so we set a fake SID here.
+ ErrorType MaybeSetFakeDomainSid(const AccountData& account)
+ WARN_UNUSED_RESULT;
// Sets machine name and realm on the device account and the tgt manager.
void InitDeviceAccount(const std::string& netbios_name,
@@ -562,8 +565,9 @@
// Whether device policy has been fetched or loaded from disk on startup.
bool has_device_policy_ = false;
- // Whether a fake domain SID was set to work around Samba issue.
- bool fake_domain_sid_was_set_ = false;
+ // Whether a fake domain SID was set for a given workgroup to work around
+ // Samba issue.
+ std::set<std::string> fake_domain_sid_was_set_for_workgroup_;
// For testing only. Used/consumed during Initialize().
std::unique_ptr<policy::DevicePolicyImpl> device_policy_impl_for_testing;
diff --git a/authpolicy/stub_common.cc b/authpolicy/stub_common.cc
index 401eca5..9a73913 100644
--- a/authpolicy/stub_common.cc
+++ b/authpolicy/stub_common.cc
@@ -23,8 +23,12 @@
const int kExitCodeError = 1;
const int kExitCodeUnspecifiedError = 255;
+const char kDefaultWorkgroup[] = "WORKGROUP1";
+const char kSecondaryWorkgroup[] = "WORKGROUP2";
+
const char kUserRealm[] = "REALM.EXAMPLE.COM";
const char kMachineRealm[] = "DEVICES.EXAMPLE.COM";
+const char kSecondaryWorkgroupRealm[] = "SECONDARY.EXAMPLE.COM";
const char kUserName[] = "user";
const char kUserPrincipal[] = "user@REALM.EXAMPLE.COM";
diff --git a/authpolicy/stub_common.h b/authpolicy/stub_common.h
index 6e204de..2f8d0a7 100644
--- a/authpolicy/stub_common.h
+++ b/authpolicy/stub_common.h
@@ -19,10 +19,17 @@
extern const int kExitCodeError;
extern const int kExitCodeUnspecifiedError;
+// Workgroups.
+extern const char kDefaultWorkgroup[];
+extern const char kSecondaryWorkgroup[];
+
// Realms.
extern const char kUserRealm[];
extern const char kMachineRealm[];
+// Realm for secondary workgroup.
+extern const char kSecondaryWorkgroupRealm[];
+
// Default, valid user name.
extern const char kUserName[];
// Default, valid user principal.
diff --git a/authpolicy/stub_net_main.cc b/authpolicy/stub_net_main.cc
index 60ef6ed..fc38bc0 100644
--- a/authpolicy/stub_net_main.cc
+++ b/authpolicy/stub_net_main.cc
@@ -11,6 +11,7 @@
#include <base/files/file_path.h>
#include <base/files/file_util.h>
#include <base/logging.h>
+#include <base/strings/strcat.h>
#include <base/strings/string_split.h>
#include <base/strings/string_util.h>
#include <base/strings/stringprintf.h>
@@ -29,7 +30,9 @@
const char kStateDir[] = "state";
const char kSambaDir[] = "samba";
const char kKrb5CCUser[] = "krb5cc_user";
-const char kFakeDomainSidMarker[] = "fake_domain_sid_marker";
+
+// Prefix for the fake domain sid marker file "fake_domain_sid_<workgroup>".
+const char kFakeDomainSidMarkerPrefix[] = "fake_domain_sid_";
// Various stub error messages.
const char kSmbConfArgMissingError[] =
@@ -323,6 +326,17 @@
return krb5cc_data;
}
+// Reads the smb.conf file at |smb_conf_path| and extracts the string value
+// associated with given |setting|.
+std::string GetStringValueFromSmbConf(const std::string& smb_conf_path,
+ const std::string& setting) {
+ std::string smb_conf;
+ CHECK(base::ReadFileToString(base::FilePath(smb_conf_path), &smb_conf));
+ std::string value;
+ CHECK(FindToken(smb_conf, '=', setting, &value));
+ return value;
+}
+
// Reads the smb.conf file at |smb_conf_path| and extracts the netbios name.
std::string GetMachineNameFromSmbConf(const std::string& smb_conf_path) {
// We need the device smb.conf here, the user smb.conf doesn't contain the
@@ -330,12 +344,7 @@
std::string device_smb_conf_path = smb_conf_path;
base::ReplaceFirstSubstringAfterOffset(&device_smb_conf_path, 0, kSmbConfUser,
kSmbConfDevice);
- std::string smb_conf;
- CHECK(
- base::ReadFileToString(base::FilePath(device_smb_conf_path), &smb_conf));
- std::string machine_name;
- CHECK(FindToken(smb_conf, '=', "netbios name", &machine_name));
- return machine_name;
+ return GetStringValueFromSmbConf(device_smb_conf_path, "netbios name");
}
// Returns different stub net ads search results depending on |object_guid|.
@@ -406,9 +415,12 @@
}
// Returns the path of a marker file to check whether "net setdomainsid" has
-// been called.
+// been called for a given workgroup stored in a config file at |smb_conf_path|.
base::FilePath GetDomainSidMarkerPath(const std::string& smb_conf_path) {
- return base::FilePath(smb_conf_path).DirName().Append(kFakeDomainSidMarker);
+ return base::FilePath(smb_conf_path)
+ .DirName()
+ .Append(kFakeDomainSidMarkerPrefix +
+ GetStringValueFromSmbConf(smb_conf_path, "workgroup"));
}
// Fakes setting a "net setdomainsid" call by writing a marker file.
@@ -438,7 +450,13 @@
return kExitCodeError;
}
- WriteOutput("Workgroup: WOKGROUP", "");
+ // Select workgroup based on realm.
+ std::string workgroup = (GetStringValueFromSmbConf(smb_conf_path, "realm") ==
+ kSecondaryWorkgroupRealm)
+ ? kSecondaryWorkgroup
+ : kDefaultWorkgroup;
+
+ WriteOutput(base::StrCat({"Workgroup: ", workgroup}), "");
return kExitCodeOk;
}
@@ -678,7 +696,7 @@
// fakes the behavior of Samba 4.10.7, which requires the domain sid to be set
// for that command.
int HandleSetDomainSid(const std::string& smb_conf_path) {
- // net setdomainsid should be called at most once.
+ // net setdomainsid should be called at most once for the same workgroup.
if (IsFakeDomainSidSet(smb_conf_path))
return kExitCodeError;
SetFakeDomainSid(smb_conf_path);
