fix: Ensure GPO URLs contain the FQDN of the controller (#820)

cmd/adsysd/integration_tests/adsysctl_policy_test.go

@@ -979,6 +979,9 @@ func TestPolicyUpdate(t *testing.T) {
 				tc.backend = "sssd"
 			}
 
+			if tc.winbindMockBehavior == "" {
+				tc.winbindMockBehavior = "integration_tests"
+			}
 			t.Setenv("ADSYS_WBCLIENT_BEHAVIOR", tc.winbindMockBehavior)
 
 			// Create fake certmonger and cepces binaries for the certificate manager

cmd/adsysd/integration_tests/systemdaemons/system_daemons.py

@@ -111,7 +111,10 @@ def sssd_on_bus(bus: dbus.Bus):
         False)
     main_object.AddMethods("", [
         ("IsOnline", "", "b", "ret = True"),
-        ("ActiveServer", "s", "s", 'ret = "adc.example.com"'),
+        # In real environments this is the FQDN of a domain controller
+        # For testing purposes we need to match the value to the underlying SMB
+        # server running on localhost
+        ("ActiveServer", "s", "s", 'ret = "localhost:1446"'),
     ])
 
     main_object.AddObject(

cmd/adsysd/integration_tests/testdata/TestServiceStatus/golden/invalid_startup_time_leads_to_unknown_refresh_time

@@ -11,7 +11,7 @@ Active Directory:
   Configuration: testdata/sssd-configs/sssd.conf-example.com
   Cache: /tmp/sss_cache
   Domain: example.com
-  Server URL: ldap://adc.example.com
+  Server FQDN: localhost:1446
 
 Daemon:
   Timeout after 30s

cmd/adsysd/integration_tests/testdata/TestServiceStatus/golden/invalid_unit_refresh_time_leads_to_unknown_refresh_time

@@ -11,7 +11,7 @@ Active Directory:
   Configuration: testdata/sssd-configs/sssd.conf-example.com
   Cache: /tmp/sss_cache
   Domain: example.com
-  Server URL: ldap://adc.example.com
+  Server FQDN: localhost:1446
 
 Daemon:
   Timeout after 30s

cmd/adsysd/integration_tests/testdata/TestServiceStatus/golden/no_startup_time_leads_to_unknown_refresh_time

@@ -11,7 +11,7 @@ Active Directory:
   Configuration: testdata/sssd-configs/sssd.conf-example.com
   Cache: /tmp/sss_cache
   Domain: example.com
-  Server URL: ldap://adc.example.com
+  Server FQDN: localhost:1446
 
 Daemon:
   Timeout after 30s

cmd/adsysd/integration_tests/testdata/TestServiceStatus/golden/no_unit_refresh_time_leads_to_unknown_refresh_time

@@ -11,7 +11,7 @@ Active Directory:
   Configuration: testdata/sssd-configs/sssd.conf-example.com
   Cache: /tmp/sss_cache
   Domain: example.com
-  Server URL: ldap://adc.example.com
+  Server FQDN: localhost:1446
 
 Daemon:
   Timeout after 30s

cmd/adsysd/integration_tests/testdata/TestServiceStatus/golden/status_is_always_authorized

@@ -11,7 +11,7 @@ Active Directory:
   Configuration: testdata/sssd-configs/sssd.conf-example.com
   Cache: /tmp/sss_cache
   Domain: example.com
-  Server URL: ldap://adc.example.com
+  Server FQDN: localhost:1446
 
 Daemon:
   Timeout after 30s

cmd/adsysd/integration_tests/testdata/TestServiceStatus/golden/status_no_user_connected_and_no_machine

@@ -10,7 +10,7 @@ Active Directory:
   Configuration: testdata/sssd-configs/sssd.conf-example.com
   Cache: /tmp/sss_cache
   Domain: example.com
-  Server URL: ldap://adc.example.com
+  Server FQDN: localhost:1446
 
 Daemon:
   Timeout after 30s

cmd/adsysd/integration_tests/testdata/TestServiceStatus/golden/status_offline_cache

@@ -12,7 +12,7 @@ Active Directory:
   Cache: /tmp/sss_cache
   **Offline mode** using cached policies
   Domain: offline
-  Server URL: Unknown
+  Server FQDN: Unknown
 
 Daemon:
   Timeout after 30s

cmd/adsysd/integration_tests/testdata/TestServiceStatus/golden/status_on_user_connected_with_no_cache

@@ -11,7 +11,7 @@ Active Directory:
   Configuration: testdata/sssd-configs/sssd.conf-example.com
   Cache: /tmp/sss_cache
   Domain: example.com
-  Server URL: ldap://adc.example.com
+  Server FQDN: localhost:1446
 
 Daemon:
   Timeout after 30s

cmd/adsysd/integration_tests/testdata/TestServiceStatus/golden/status_with_empty_dynamic_ad_server

@@ -11,7 +11,7 @@ Active Directory:
   Configuration: testdata/sssd-configs/sssd.conf-online_no_active_server
   Cache: /tmp/sss_cache
   Domain: online_no_active_server
-  Server URL: Unknown
+  Server FQDN: Unknown
 
 Daemon:
   Timeout after 30s

cmd/adsysd/integration_tests/testdata/TestServiceStatus/golden/status_with_static_ad_server

@@ -11,7 +11,7 @@ Active Directory:
   Configuration: testdata/sssd-configs/sssd.conf-example.com_static-server
   Cache: /tmp/sss_cache
   Domain: example.com
-  Server URL: ldap://staticserver.example.com
+  Server FQDN: localhost:1446
 
 Daemon:
   Timeout after 30s

cmd/adsysd/integration_tests/testdata/TestServiceStatus/golden/status_with_users_and_machines

@@ -11,7 +11,7 @@ Active Directory:
   Configuration: testdata/sssd-configs/sssd.conf-example.com
   Cache: /tmp/sss_cache
   Domain: example.com
-  Server URL: ldap://adc.example.com
+  Server FQDN: localhost:1446
 
 Daemon:
   Timeout after 30s

cmd/adsysd/integration_tests/testdata/TestServiceStatus/golden/ubuntu_pro_subscription_is_not_active

@@ -17,7 +17,7 @@ Active Directory:
   Configuration: testdata/sssd-configs/sssd.conf-example.com
   Cache: /tmp/sss_cache
   Domain: example.com
-  Server URL: ldap://adc.example.com
+  Server FQDN: localhost:1446
 
 Daemon:
   Timeout after 30s

cmd/adsysd/integration_tests/testdata/sssd-configs/sssd.conf-example.com_static-server

@@ -3,4 +3,4 @@ domains = example.com
 
 [domain/example.com]
 ad_domain = example.com
-ad_server = staticserver.example.com
+ad_server = localhost:1446

internal/ad/ad.go

@@ -153,11 +153,11 @@ func New(ctx context.Context, configBackend backends.Backend, hostname string, o
 	}
 
 	domain := configBackend.Domain()
-	serverURL, err := configBackend.ServerURL(ctx)
+	serverFQDN, err := configBackend.ServerFQDN(ctx)
 	if err != nil && !errors.Is(err, backends.ErrNoActiveServer) {
-		return nil, fmt.Errorf(i18n.G("can't get current Server URL: %w"), err)
+		return nil, fmt.Errorf(i18n.G("can't get current Server FQDN: %w"), err)
 	}
-	log.Debugf(ctx, "Backend is SSSD. AD domain: %q, server from configuration: %q", domain, serverURL)
+	log.Debugf(ctx, "Backend is SSSD. AD domain: %q, server from configuration: %q", domain, serverFQDN)
 
 	return &AD{
 		hostname:         hostname,
@@ -232,15 +232,15 @@ func (ad *AD) GetPolicies(ctx context.Context, objectName string, objectClass Ob
 		return cachedPolicies, nil
 	}
 
-	// We need an AD LDAP url to connect to
-	adServerURL, err := ad.configBackend.ServerURL(ctx)
+	// We need an AD DC to connect to
+	adServerFQDN, err := ad.configBackend.ServerFQDN(ctx)
 	if err != nil {
-		return policies.Policies{}, fmt.Errorf(i18n.G("can't get current Server URL: %w"), err)
+		return policies.Policies{}, fmt.Errorf(i18n.G("can't get current Server FQDN: %w"), err)
 	}
 
 	// Otherwise, try fetching the GPO list from LDAP
 	args := append([]string{}, ad.gpoListCmd...) // Copy gpoListCmd to prevent data race
-	scriptArgs := []string{"--objectclass", string(objectClass), adServerURL, objectName}
+	scriptArgs := []string{"--objectclass", string(objectClass), adServerFQDN, objectName}
 	cmdArgs := append(args, scriptArgs...)
 	cmdCtx, cancel := context.WithTimeout(ctx, time.Second*10)
 	defer cancel()
@@ -605,12 +605,12 @@ func (ad *AD) GetInfo(ctx context.Context) (msg string) {
 		online = fmt.Sprint(i18n.G("**Offline mode** using cached policies\n"))
 	}
 	domain := ad.configBackend.Domain()
-	server, err := ad.configBackend.ServerURL(ctx)
+	server, err := ad.configBackend.ServerFQDN(ctx)
 	if err != nil {
 		server = "Unknown"
 	}
 
-	return fmt.Sprintf(i18n.G("%s\n%sDomain: %s\nServer URL: %s"), config, online, domain, server)
+	return fmt.Sprintf(i18n.G("%s\n%sDomain: %s\nServer FQDN: %s"), config, online, domain, server)
 }
 
 // NormalizeTargetName transforms the specified target to values adsys knows.

internal/ad/ad_test.go

@@ -29,20 +29,20 @@ func TestNew(t *testing.T) {
 	require.NoError(t, err, "Setup: failed to get hostname for tests.")
 
 	tests := map[string]struct {
-		sysvolCacheDirExists  bool
-		cacheDirRO            bool
-		runDirRO              bool
-		backendServerURLError error
+		sysvolCacheDirExists   bool
+		cacheDirRO             bool
+		runDirRO               bool
+		backendServerFQDNError error
 
 		wantErr bool
 	}{
 		"create KRB5 and Sysvol cache directory":                {},
-		"no active server in backend does not fail ad creation": {backendServerURLError: backends.ErrNoActiveServer},
+		"no active server in backend does not fail ad creation": {backendServerFQDNError: backends.ErrNoActiveServer},
 
-		"failed to create KRB5 cache directory":     {runDirRO: true, wantErr: true},
-		"failed to create Sysvol cache directory":   {cacheDirRO: true, wantErr: true},
-		"failed to create Policies cache directory": {sysvolCacheDirExists: true, cacheDirRO: true, wantErr: true},
-		"error on backend ServerURL random failure": {backendServerURLError: errors.New("Some failure on ServerURL"), wantErr: true},
+		"failed to create KRB5 cache directory":      {runDirRO: true, wantErr: true},
+		"failed to create Sysvol cache directory":    {cacheDirRO: true, wantErr: true},
+		"failed to create Policies cache directory":  {sysvolCacheDirExists: true, cacheDirRO: true, wantErr: true},
+		"error on backend ServerFQDN random failure": {backendServerFQDNError: errors.New("Some failure on ServerFQDN"), wantErr: true},
 	}
 	for name, tc := range tests {
 		tc := tc
@@ -61,7 +61,7 @@ func TestNew(t *testing.T) {
 				testutils.MakeReadOnly(t, cacheDir)
 			}
 
-			adc, err := ad.New(context.Background(), mock.Backend{ErrServerURL: tc.backendServerURLError}, hostname,
+			adc, err := ad.New(context.Background(), mock.Backend{ErrServerFQDN: tc.backendServerFQDNError}, hostname,
 				ad.WithRunDir(runDir),
 				ad.WithCacheDir(cacheDir))
 			if tc.wantErr {
@@ -480,12 +480,12 @@ func TestGetPolicies(t *testing.T) {
 			gpoListArgs: []string{"gpoonly.com", hostname + ":standard"},
 			wantErr:     true,
 		},
-		"Error on backend ServerURL call failed": {
+		"Error on backend ServerFQDN call failed": {
 			backend: mock.Backend{
 				Dom:    "gpoonly.com",
 				Online: true,
 				// This error is skipped by New(), but not by GetPolicies
-				ErrServerURL: backends.ErrNoActiveServer,
+				ErrServerFQDN: backends.ErrNoActiveServer,
 			},
 			gpoListArgs: []string{"gpoonly.com", "bob:standard"},
 			wantErr:     true,
@@ -558,7 +558,7 @@ func TestGetPolicies(t *testing.T) {
 				}
 			}
 			if tc.backend.ServURL == "" {
-				tc.backend.ServURL = "ldap://myserver." + tc.backend.Dom
+				tc.backend.ServURL = "myserver." + tc.backend.Dom
 			}
 			// we file in host_ccache to not have to reset it in every single test
 			if tc.backend.HostKrb5CCNamePath == "" {
@@ -696,7 +696,7 @@ func TestGetPoliciesOffline(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			tc.backend.ServURL = "ldap://myserver." + tc.backend.Dom
+			tc.backend.ServURL = "myserver." + tc.backend.Dom
 			tc.backend.HostKrb5CCNamePath = filepath.Join(t.TempDir(), "host_ccache")
 			testutils.CreatePath(t, tc.backend.HostKrb5CCNamePath)
 
@@ -848,7 +848,7 @@ func TestGetPoliciesWorkflows(t *testing.T) {
 
 			backend := mock.Backend{
 				Dom:                "assetsandgpo.com",
-				ServURL:            "ldap://UNUSED:1636/",
+				ServURL:            "UNUSED:1636",
 				HostKrb5CCNamePath: filepath.Join(t.TempDir(), "host_ccache"),
 				Online:             true,
 			}
@@ -1007,7 +1007,7 @@ func TestGetPoliciesConcurrently(t *testing.T) {
 
 			backend := mock.Backend{
 				Dom:                "assetsandgpo.com",
-				ServURL:            "ldap://UNUSED:1636/",
+				ServURL:            "UNUSED:1636",
 				HostKrb5CCNamePath: filepath.Join(t.TempDir(), "host_ccache"),
 				Online:             true,
 			}
@@ -1178,7 +1178,7 @@ func TestListUsers(t *testing.T) {
 				require.NoError(t, os.Remove(srcPath), "Setup: can’t remove krb5cc symlink target")
 			}
 
-			adc, err := ad.New(context.Background(), mock.Backend{Dom: "gpoonly.com", ServURL: "ldap://myserver.gpoonly.com"}, hostname,
+			adc, err := ad.New(context.Background(), mock.Backend{Dom: "gpoonly.com", ServURL: "myserver.gpoonly.com"}, hostname,
 				ad.WithCacheDir(cachedir), ad.WithRunDir(rundir))
 			require.NoError(t, err, "Setup: New should return no error")
 
@@ -1215,16 +1215,16 @@ func TestGetInfo(t *testing.T) {
 	require.NoError(t, err, "Setup: failed to get hostname for tests.")
 
 	tests := map[string]struct {
-		online       bool
-		errIsOnline  bool
-		ErrServerURL error
+		online        bool
+		errIsOnline   bool
+		ErrServerFQDN error
 	}{
 		"Info reported from backend, online":  {online: true},
 		"Info reported from backend, offline": {online: false},
 
 		"Report unknown state if IsOnline calls fail": {errIsOnline: true},
 		// This error is skipped by New(), but not by GetInfo
-		"Report unknown state if ServerURL calls fail": {ErrServerURL: backends.ErrNoActiveServer},
+		"Report unknown state if ServerFQDN calls fail": {ErrServerFQDN: backends.ErrNoActiveServer},
 	}
 
 	for name, tc := range tests {
@@ -1234,9 +1234,9 @@ func TestGetInfo(t *testing.T) {
 
 			adc, err := ad.New(context.Background(),
 				mock.Backend{
-					Dom: "example.com", ServURL: "ldap://myserver.example.com",
+					Dom: "example.com", ServURL: "myserver.example.com",
 					Online:      tc.online,
-					ErrIsOnline: tc.errIsOnline, ErrServerURL: tc.ErrServerURL},
+					ErrIsOnline: tc.errIsOnline, ErrServerFQDN: tc.ErrServerFQDN},
 				hostname,
 				ad.WithCacheDir(t.TempDir()), ad.WithRunDir(t.TempDir()))
 			require.NoError(t, err, "Setup: New should return no error")
@@ -1289,7 +1289,7 @@ func TestNormalizeTargetName(t *testing.T) {
 			t.Parallel()
 
 			adc, err := ad.New(context.Background(),
-				mock.Backend{Dom: tc.defaultDomainSuffix, ServURL: "ldap://myserver.gpoonly.com"}, // Dom is the default domain suffix in the mock
+				mock.Backend{Dom: tc.defaultDomainSuffix, ServURL: "myserver.gpoonly.com"}, // Dom is the default domain suffix in the mock
 				hostname,
 				ad.WithCacheDir(t.TempDir()), ad.WithRunDir(t.TempDir()))
 			require.NoError(t, err, "Setup: New should return no error")

internal/ad/adsys-gpolist

@@ -220,8 +220,9 @@ def get_gpos_for_dn(samdb, dn, token, sids, is_computer):
 
 def main():
     parser = argparse.ArgumentParser(description='List GPOs for a user or computer.')
-    parser.add_argument('url', metavar='URL', type=str,
-                        help='URL of the domain controller.')
+    parser.add_argument('fqdn', metavar='FQDN', type=str,
+                        help='FQDN of the domain controller (without ldap:// prefix). \
+                        e.g. dc.example.com')
     parser.add_argument('accountname', help='Name of the object to search for.')
     parser.add_argument('--objectclass', type=str,
                         choices=(ObjectClass.user, ObjectClass.computer), default=ObjectClass.user,
@@ -230,13 +231,14 @@ def main():
     args = parser.parse_args()
 
     accountname = args.accountname
+    fqdn = args.fqdn
 
     # Users don’t need @, as we already have the specific-domain ticket
     if args.objectclass == ObjectClass.user:
         accountname = accountname.split('@')[0]
 
     try:
-        samdb = connectLDAP(args.url)
+        samdb = connectLDAP("ldap://" + fqdn)
     except Exception as exc:
         # Could be a private _ldb.Error, check status
         if len(exc.args) > 1:
@@ -281,8 +283,17 @@ def main():
         return ReturnCode.GPO_FAILED
 
     for g in gpos:
-        print("%s\tsmb:%s" % (g[0], str(g[1]).replace("\\", "/")))
+        gpo_name = g[0]
+        gpo_path = parse_gpo_path(g[1], fqdn)
+        print("%s\t%s" % (gpo_name, gpo_path))
 
+def parse_gpo_path(gpo_path, dc_fqdn):
+    ''' Parse a GPO path to a SMB path with the appropriate DC FQDN '''
+    path = str(gpo_path).replace("\\", "/")
+    parts = path[2:].split("/")
+    parts[0] = dc_fqdn
+
+    return "smb://" +"/".join(parts)
 
 if __name__ == "__main__":
     exit(main())