diff --git a/server_utils/origin_posix.go b/server_utils/origin_posix.go index be4a69a8b..da27f9879 100644 --- a/server_utils/origin_posix.go +++ b/server_utils/origin_posix.go @@ -1,3 +1,5 @@ +//go:build !windows + /*************************************************************** * * Copyright (C) 2025, Pelican Project, Morgridge Institute for Research @@ -19,6 +21,12 @@ package server_utils import ( + "os" + "syscall" + + "github.com/pkg/errors" + + "github.com/pelicanplatform/pelican/config" "github.com/pelicanplatform/pelican/server_structs" ) @@ -36,3 +44,110 @@ func (o *PosixOrigin) validateStoragePrefix(prefix string) error { // the federation prefix. return validateFederationPrefix(prefix) } + +// validateExtra checks that the daemon user (xrootd user) has the necessary filesystem +// permissions to perform the operations specified by the export's capabilities. +// This prevents hard-to-diagnose runtime errors by catching permission mismatches at startup. +func (o *PosixOrigin) validateExtra(e *OriginExport, _ int) error { + return ValidatePosixPermissions(e.StoragePrefix, e.Capabilities, e.FederationPrefix) +} + +// ValidatePosixPermissions checks if the daemon user has the required filesystem permissions +// on the given path to support the specified capabilities. +func ValidatePosixPermissions(storagePath string, caps server_structs.Capabilities, federationPrefix string) error { + // Get XRootD daemon user info + uid, err := config.GetDaemonUID() + if err != nil { + return errors.Wrap(err, "failed to get XRootD daemon UID for permission validation") + } + gid, err := config.GetDaemonGID() + if err != nil { + return errors.Wrap(err, "failed to get XRootD daemon GID for permission validation") + } + username, err := config.GetDaemonUser() + if err != nil { + username = "username-unknown" + } + + // Check if the storage prefix exists + info, err := os.Stat(storagePath) + if err != nil { + if os.IsNotExist(err) { + return errors.Wrapf(ErrInvalidOriginConfig, + "storage prefix %q for export %q does not exist", + storagePath, federationPrefix) + } + return errors.Wrapf(err, "failed to stat storage prefix %q for export %q", + storagePath, federationPrefix) + } + + // The storage prefix should be a directory + if !info.IsDir() { + return errors.Wrapf(ErrInvalidOriginConfig, + "storage prefix %q for export %q is not a directory", + storagePath, federationPrefix) + } + + // Get the directory's owner and group + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return errors.Errorf("failed to get system stat info for %q", storagePath) + } + dirUID := int(stat.Uid) + dirGID := int(stat.Gid) + mode := info.Mode() + + // Determine which permission bits apply to XRootD daemon user + var canRead, canWrite, canExecute bool + if uid == dirUID { + // User is owner - check owner bits + canRead = mode&0400 != 0 + canWrite = mode&0200 != 0 + canExecute = mode&0100 != 0 + } else if gid == dirGID { + // User is in group - check group bits + canRead = mode&0040 != 0 + canWrite = mode&0020 != 0 + canExecute = mode&0010 != 0 + } else { + // User is neither owner nor in group - check others bits + canRead = mode&0004 != 0 + canWrite = mode&0002 != 0 + canExecute = mode&0001 != 0 + } + + // Helper to format permission error message + formatPermError := func(capability, requiredPerms string) error { + return errors.Wrapf(ErrInvalidOriginConfig, + "storage prefix %q for export %q requires %q permissions for the %q user (uid=%d, gid=%d) "+ + "to support the %q capability, but the current permissions are %q (owner uid=%d, gid=%d). "+ + "Please adjust the ownership or permissions of the directory", + storagePath, federationPrefix, requiredPerms, username, uid, gid, + capability, mode.Perm().String(), dirUID, dirGID) + } + + // Check permissions based on capabilities + + // Check reads (PublicReads or Reads) - needs read and execute + if caps.Reads || caps.PublicReads { + if !canRead || !canExecute { + return formatPermError("Reads/PublicReads", "read and execute (r-x)") + } + } + + // Check writes - needs write and execute + if caps.Writes { + if !canWrite || !canExecute { + return formatPermError("Writes", "write and execute (-wx)") + } + } + + // Check listings - needs read and execute + if caps.Listings { + if !canRead || !canExecute { + return formatPermError("Listings", "read and execute (r-x)") + } + } + + return nil +} diff --git a/server_utils/origin_test.go b/server_utils/origin_test.go index 811f43861..88f503416 100644 --- a/server_utils/origin_test.go +++ b/server_utils/origin_test.go @@ -112,11 +112,30 @@ func getTmpFile(t *testing.T) string { return tmpFile } +// getTmpDir returns a temp directory with 0777 permissions for test use. +// The 0777 permissions ensure the xrootd daemon user can access the directory +func getTmpDir(t *testing.T) string { + tmpDir := t.TempDir() + + // Set directory permissions to 777 so the xrootd daemon user can access it + err := os.Chmod(tmpDir, 0777) + if err != nil { + t.Fatalf("Failed to set directory permissions: %v", err) + } + + return tmpDir +} + func setup(t *testing.T, config string, shouldError bool) []OriginExport { viper.SetConfigType("yaml") // Use viper to read in the embedded config err := viper.ReadConfig(strings.NewReader(config)) require.NoError(t, err, "error reading config") + + // Check if this is a POSIX origin (default is posix if not specified) + storageType := viper.GetString("origin.storagetype") + isPosix := storageType == "" || storageType == "posix" + // Some keys need to be overridden because GetOriginExports validates things like filepaths by making // sure the file exists and is readable by the process. // Iterate through Origin.XXX keys and check for "SHOULD-OVERRIDE" in the value @@ -124,8 +143,30 @@ func setup(t *testing.T, config string, shouldError bool) []OriginExport { if strings.Contains(viper.GetString(key), "SHOULD-OVERRIDE-TEMPFILE") { tmpFile := getTmpFile(t) viper.Set(key, tmpFile) + } else if key == "origin.storageprefix" && isPosix { + // For POSIX origins, replace the storage prefix with a temp directory + // so the permission validation can succeed + tmpDir := getTmpDir(t) + viper.Set(key, tmpDir) + } else if key == "origin.exportvolumes" && isPosix { + // For POSIX origins, replace export volumes paths with temp directories + volumes := viper.GetStringSlice(key) + newVolumes := make([]string, len(volumes)) + for i, vol := range volumes { + // Parse the volume format "storagePrefix:federationPrefix" + parts := strings.SplitN(vol, ":", 2) + if len(parts) == 2 { + tmpDir := getTmpDir(t) + newVolumes[i] = tmpDir + ":" + parts[1] + } else { + tmpDir := getTmpDir(t) + newVolumes[i] = tmpDir + } + } + viper.Set(key, newVolumes) } else if key == "origin.exports" { // keys will be lowercased // We also need to override paths for any exports that define "SHOULD-OVERRIDE-TEMPFILE" + // and for POSIX origins, replace storage prefixes with temp directories exports := viper.Get(key).([]interface{}) for _, export := range exports { exportMap := export.(map[string]interface{}) @@ -133,6 +174,10 @@ func setup(t *testing.T, config string, shouldError bool) []OriginExport { if v == "SHOULD-OVERRIDE-TEMPFILE" { tmpFile := getTmpFile(t) exportMap[k] = tmpFile + } else if k == "storageprefix" && isPosix { + // For POSIX origins, replace storage prefixes with temp directories + // that have proper permissions for the daemon user + exportMap[k] = getTmpDir(t) } } } @@ -167,7 +212,8 @@ func TestGetExports(t *testing.T) { exports := setup(t, envVarMimicConfig, false) assert.Len(t, exports, 1, "expected 1 export") - assert.Equal(t, "/foo", exports[0].StoragePrefix, "expected /foo") + // Storage prefix is replaced with temp dir for permission validation tests + assert.True(t, strings.HasPrefix(exports[0].StoragePrefix, "/tmp/"), "expected storage prefix to be temp dir") assert.False(t, exports[0].Capabilities.Writes, "expected no writes") assert.True(t, exports[0].Capabilities.PublicReads, "expected public reads") @@ -179,35 +225,27 @@ func TestGetExports(t *testing.T) { defer ResetTestState() exports := setup(t, multiExportValidConfig, false) - expectedExport1 := OriginExport{ - StoragePrefix: "/test1", - FederationPrefix: "/first/namespace", - Capabilities: server_structs.Capabilities{ - Writes: true, - PublicReads: true, - Listings: true, - Reads: true, - DirectReads: true, - }, - IssuerUrls: []string{defaultIssuerUrl}, - } - - expectedExport2 := OriginExport{ - StoragePrefix: "/test2", - FederationPrefix: "/second/namespace", - Capabilities: server_structs.Capabilities{ - Writes: true, - PublicReads: false, - Listings: false, - Reads: false, - DirectReads: false, - }, - IssuerUrls: []string{defaultIssuerUrl}, - } - assert.Len(t, exports, 2, "expected 2 exports") - assert.Equal(t, expectedExport1, exports[0]) - assert.Equal(t, expectedExport2, exports[1]) + + // Check first export (storage prefix is temp dir) + assert.True(t, strings.HasPrefix(exports[0].StoragePrefix, "/tmp/"), "expected storage prefix to be temp dir") + assert.Equal(t, "/first/namespace", exports[0].FederationPrefix) + assert.Equal(t, []string{defaultIssuerUrl}, exports[0].IssuerUrls) + assert.True(t, exports[0].Capabilities.Writes) + assert.True(t, exports[0].Capabilities.PublicReads) + assert.True(t, exports[0].Capabilities.Listings) + assert.True(t, exports[0].Capabilities.Reads) + assert.True(t, exports[0].Capabilities.DirectReads) + + // Check second export (storage prefix is temp dir) + assert.True(t, strings.HasPrefix(exports[1].StoragePrefix, "/tmp/"), "expected storage prefix to be temp dir") + assert.Equal(t, "/second/namespace", exports[1].FederationPrefix) + assert.Equal(t, []string{defaultIssuerUrl}, exports[1].IssuerUrls) + assert.True(t, exports[1].Capabilities.Writes) + assert.False(t, exports[1].Capabilities.PublicReads) + assert.False(t, exports[1].Capabilities.Listings) + assert.False(t, exports[1].Capabilities.Reads) + assert.False(t, exports[1].Capabilities.DirectReads) }) t.Run("testTrailingSlashRemovalPosix", func(t *testing.T) { @@ -224,35 +262,27 @@ func TestGetExports(t *testing.T) { defer ResetTestState() exports := setup(t, exportVolumesValidConfig, false) - expectedExport1 := OriginExport{ - StoragePrefix: "/test1", - FederationPrefix: "/first/namespace", - Capabilities: server_structs.Capabilities{ - Writes: false, - PublicReads: false, - Listings: true, - Reads: true, - DirectReads: true, - }, - IssuerUrls: []string{defaultIssuerUrl}, - } - - expectedExport2 := OriginExport{ - StoragePrefix: "/test2", - FederationPrefix: "/second/namespace", - Capabilities: server_structs.Capabilities{ - Writes: false, - PublicReads: false, - Listings: true, - Reads: true, - DirectReads: true, - }, - IssuerUrls: []string{defaultIssuerUrl}, - } - assert.Len(t, exports, 2, "expected 2 exports") - assert.Equal(t, expectedExport1, exports[0]) - assert.Equal(t, expectedExport2, exports[1]) + + // Check first export (storage prefix is temp dir) + assert.True(t, strings.HasPrefix(exports[0].StoragePrefix, "/tmp/"), "expected storage prefix to be temp dir") + assert.Equal(t, "/first/namespace", exports[0].FederationPrefix) + assert.Equal(t, []string{defaultIssuerUrl}, exports[0].IssuerUrls) + assert.False(t, exports[0].Capabilities.Writes) + assert.False(t, exports[0].Capabilities.PublicReads) + assert.True(t, exports[0].Capabilities.Listings) + assert.True(t, exports[0].Capabilities.Reads) + assert.True(t, exports[0].Capabilities.DirectReads) + + // Check second export (storage prefix is temp dir) + assert.True(t, strings.HasPrefix(exports[1].StoragePrefix, "/tmp/"), "expected storage prefix to be temp dir") + assert.Equal(t, "/second/namespace", exports[1].FederationPrefix) + assert.Equal(t, []string{defaultIssuerUrl}, exports[1].IssuerUrls) + assert.False(t, exports[1].Capabilities.Writes) + assert.False(t, exports[1].Capabilities.PublicReads) + assert.True(t, exports[1].Capabilities.Listings) + assert.True(t, exports[1].Capabilities.Reads) + assert.True(t, exports[1].Capabilities.DirectReads) }) // When we have a single export volume, we also set a few viper variables that can be @@ -261,24 +291,21 @@ func TestGetExports(t *testing.T) { defer ResetTestState() exports := setup(t, exportSingleVolumeConfig, false) - expectedExport := OriginExport{ - StoragePrefix: "/test1", - FederationPrefix: "/first/namespace", - Capabilities: server_structs.Capabilities{ - Writes: true, - PublicReads: true, - Listings: false, - Reads: true, - DirectReads: false, - }, - IssuerUrls: []string{defaultIssuerUrl}, - } - assert.Len(t, exports, 1, "expected 1 export") - assert.Equal(t, expectedExport, exports[0]) + + // Check export (storage prefix is temp dir) + assert.True(t, strings.HasPrefix(exports[0].StoragePrefix, "/tmp/"), "expected storage prefix to be temp dir") + assert.Equal(t, "/first/namespace", exports[0].FederationPrefix) + assert.Equal(t, []string{defaultIssuerUrl}, exports[0].IssuerUrls) + assert.True(t, exports[0].Capabilities.Writes) + assert.True(t, exports[0].Capabilities.PublicReads) + assert.False(t, exports[0].Capabilities.Listings) + assert.True(t, exports[0].Capabilities.Reads) + assert.False(t, exports[0].Capabilities.DirectReads) // Now check that we properly set the other viper vars we should have - assert.Equal(t, "/test1", viper.GetString("Origin.StoragePrefix")) + // Note: StoragePrefix is a temp dir now + assert.True(t, strings.HasPrefix(viper.GetString("Origin.StoragePrefix"), "/tmp/"), "expected viper storage prefix to be temp dir") assert.Equal(t, "/first/namespace", viper.GetString("Origin.FederationPrefix")) assert.True(t, viper.GetBool("Origin.EnableReads")) assert.True(t, viper.GetBool("Origin.EnableWrites")) @@ -291,24 +318,22 @@ func TestGetExports(t *testing.T) { defer ResetTestState() exports := setup(t, singleExportBlockConfig, false) - expectedExport := OriginExport{ - StoragePrefix: "/test1", - FederationPrefix: "/first/namespace", - Capabilities: server_structs.Capabilities{ - Writes: false, - PublicReads: true, - Listings: false, - Reads: true, - DirectReads: true, - }, - // No issuer is populated because there are no namespaces requiring it - } - assert.Len(t, exports, 1, "expected 1 export") - assert.Equal(t, expectedExport, exports[0]) + + // Check export (storage prefix is temp dir) + assert.True(t, strings.HasPrefix(exports[0].StoragePrefix, "/tmp/"), "expected storage prefix to be temp dir") + assert.Equal(t, "/first/namespace", exports[0].FederationPrefix) + // No issuer is populated because there are no namespaces requiring it + assert.Nil(t, exports[0].IssuerUrls) + assert.False(t, exports[0].Capabilities.Writes) + assert.True(t, exports[0].Capabilities.PublicReads) + assert.False(t, exports[0].Capabilities.Listings) + assert.True(t, exports[0].Capabilities.Reads) + assert.True(t, exports[0].Capabilities.DirectReads) // Now check that we properly set the other viper vars we should have - assert.Equal(t, "/test1", viper.GetString("Origin.StoragePrefix")) + // Note: StoragePrefix is a temp dir now + assert.True(t, strings.HasPrefix(viper.GetString("Origin.StoragePrefix"), "/tmp/")) assert.Equal(t, "/first/namespace", viper.GetString("Origin.FederationPrefix")) assert.True(t, viper.GetBool("Origin.EnableReads")) assert.False(t, viper.GetBool("Origin.EnableWrites")) @@ -725,3 +750,182 @@ func TestFederationPrefixValidation(t *testing.T) { runFedPrefixTest(t, "/caches/example.org", false) runFedPrefixTest(t, "/valid/prefix", true) // Test valid prefix } + +// TestValidatePosixPermissions tests the filesystem permission validation for POSIX origins. +// These tests ensure that the origin correctly validates whether the XRootD daemon user has the +// necessary permissions to perform operations specified by the export's capabilities. +func TestValidatePosixPermissions(t *testing.T) { + // Test with non-existent path + t.Run("NonExistentPath", func(t *testing.T) { + caps := server_structs.Capabilities{Reads: true} + err := ValidatePosixPermissions("/nonexistent/path/that/does/not/exist", caps, "/test") + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidOriginConfig) + assert.Contains(t, err.Error(), "does not exist") + }) + + // Test with a file instead of directory + t.Run("FileInsteadOfDirectory", func(t *testing.T) { + tmpFile := t.TempDir() + "/testfile" + file, err := os.Create(tmpFile) + require.NoError(t, err) + file.Close() + + caps := server_structs.Capabilities{Reads: true} + err = ValidatePosixPermissions(tmpFile, caps, "/test") + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidOriginConfig) + assert.Contains(t, err.Error(), "is not a directory") + }) + + // Test with full permissions (rwx) - all capabilities should pass + // Note: We use 0777 because tests run as root but the daemon user (xrootd) is different, + // so we need "others" permissions to be rwx for the xrootd user to have access. + t.Run("FullPermissions", func(t *testing.T) { + tmpDir := t.TempDir() + err := os.Chmod(tmpDir, 0777) // rwx for everyone including "others" (daemon user) + require.NoError(t, err) + + // Test reads capability + caps := server_structs.Capabilities{Reads: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + assert.NoError(t, err) + + // Test public reads capability + caps = server_structs.Capabilities{PublicReads: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + assert.NoError(t, err) + + // Test writes capability + caps = server_structs.Capabilities{Writes: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + assert.NoError(t, err) + + // Test listings capability + caps = server_structs.Capabilities{Listings: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + assert.NoError(t, err) + + // Test all capabilities together + caps = server_structs.Capabilities{ + Reads: true, + PublicReads: true, + Writes: true, + Listings: true, + } + err = ValidatePosixPermissions(tmpDir, caps, "/test") + assert.NoError(t, err) + }) + + // Test with no capabilities - should always pass (nothing to validate) + t.Run("NoCapabilities", func(t *testing.T) { + tmpDir := t.TempDir() + // Even with restrictive permissions, no capabilities means no validation needed + err := os.Chmod(tmpDir, 0000) + require.NoError(t, err) + defer os.Chmod(tmpDir, 0755) // Restore for cleanup + + caps := server_structs.Capabilities{} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + assert.NoError(t, err) + }) + + // Test with read-only permissions (r-x) - writes should fail + t.Run("ReadOnlyPermissions", func(t *testing.T) { + tmpDir := t.TempDir() + err := os.Chmod(tmpDir, 0555) // r-x r-x r-x + require.NoError(t, err) + defer os.Chmod(tmpDir, 0755) // Restore for cleanup + + // Reads should pass + caps := server_structs.Capabilities{Reads: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + assert.NoError(t, err) + + // Listings should pass + caps = server_structs.Capabilities{Listings: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + assert.NoError(t, err) + + // Writes should fail + caps = server_structs.Capabilities{Writes: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidOriginConfig) + assert.Contains(t, err.Error(), "Writes") + assert.Contains(t, err.Error(), "write and execute") + }) + + // Test with write-only permissions (-wx) - reads and listings should fail + t.Run("WriteOnlyPermissions", func(t *testing.T) { + tmpDir := t.TempDir() + err := os.Chmod(tmpDir, 0333) // -wx -wx -wx + require.NoError(t, err) + defer os.Chmod(tmpDir, 0755) // Restore for cleanup + + // Writes should pass + caps := server_structs.Capabilities{Writes: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + assert.NoError(t, err) + + // Reads should fail + caps = server_structs.Capabilities{Reads: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidOriginConfig) + assert.Contains(t, err.Error(), "Reads") + + // Listings should fail + caps = server_structs.Capabilities{Listings: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidOriginConfig) + assert.Contains(t, err.Error(), "Listings") + }) + + // Test with no execute permission - all capabilities requiring traversal should fail + t.Run("NoExecutePermission", func(t *testing.T) { + tmpDir := t.TempDir() + err := os.Chmod(tmpDir, 0666) // rw- rw- rw- + require.NoError(t, err) + defer os.Chmod(tmpDir, 0755) // Restore for cleanup + + // Reads should fail (needs execute for directory traversal) + caps := server_structs.Capabilities{Reads: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidOriginConfig) + + // Writes should fail (needs execute for directory traversal) + caps = server_structs.Capabilities{Writes: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidOriginConfig) + + // Listings should fail (needs execute for directory traversal) + caps = server_structs.Capabilities{Listings: true} + err = ValidatePosixPermissions(tmpDir, caps, "/test") + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidOriginConfig) + }) + + // Test error message contains useful information + t.Run("ErrorMessageContent", func(t *testing.T) { + tmpDir := t.TempDir() + err := os.Chmod(tmpDir, 0000) // --- --- --- + require.NoError(t, err) + defer os.Chmod(tmpDir, 0755) // Restore for cleanup + + caps := server_structs.Capabilities{Reads: true} + err = ValidatePosixPermissions(tmpDir, caps, "/my/federation/prefix") + require.Error(t, err) + + errStr := err.Error() + // Check that the error message contains useful debugging information + assert.Contains(t, errStr, tmpDir, "error should contain the storage path") + assert.Contains(t, errStr, "/my/federation/prefix", "error should contain the federation prefix") + assert.Contains(t, errStr, "uid=", "error should contain UID info") + assert.Contains(t, errStr, "gid=", "error should contain GID info") + assert.Contains(t, errStr, "permissions", "error should mention permissions") + }) +} diff --git a/xrootd/authorization_test.go b/xrootd/authorization_test.go index 55db98157..6483603d5 100644 --- a/xrootd/authorization_test.go +++ b/xrootd/authorization_test.go @@ -377,25 +377,18 @@ func TestAuthPoliciesFromLine(t *testing.T) { } // Helper function for other tests here who call server_utils.GetOriginExports() internally. -// This function gets a temp dir for export StoragePrefixes, whose existence is validated -// by server_utils.GetOriginExports(). -func getTmpFile(t *testing.T) string { - tmpFile := t.TempDir() + "/tmpfile" - - // Create the file - file, err := os.Create(tmpFile) - if err != nil { - t.Fatalf("Failed to create temp file: %v", err) - } - file.Close() +// This function gets a temp dir for export StoragePrefixes, whose existence and permissions +// are validated by server_utils.GetOriginExports(). +func getTmpDir(t *testing.T) string { + tmpDir := t.TempDir() - // Set file permissions to 777 - err = os.Chmod(tmpFile, 0777) + // Set directory permissions to 777 so the daemon XRootD daemon user can access it + err := os.Chmod(tmpDir, 0777) if err != nil { - t.Fatalf("Failed to set file permissions: %v", err) + t.Fatalf("Failed to set directory permissions: %v", err) } - return tmpFile + return tmpDir } // Helper function for other tests here who call server_utils.GetOriginExports() internally. @@ -406,21 +399,23 @@ func setupExports(t *testing.T, config string) { err := viper.ReadConfig(strings.NewReader(config)) require.NoError(t, err, "error reading config") // Some keys need to be overridden because GetOriginExports validates things like filepaths by making - // sure the file exists and is readable by the process. + // sure the file exists and is readable by the process. For POSIX origins, we also need directories + // with proper permissions for the XRootD daemon user. // Iterate through Origin.XXX keys and check for "" in the value for _, key := range viper.AllKeys() { if strings.Contains(viper.GetString(key), "") { - tmpFile := getTmpFile(t) - viper.Set(key, tmpFile) + tmpDir := getTmpDir(t) + viper.Set(key, tmpDir) } else if key == "origin.exports" { // keys will be lowercased - // We also need to override paths for any exports that define "SHOULD-OVERRIDE-TEMPFILE" + // We also need to override paths for any exports that define "" exports := viper.Get(key).([]interface{}) for _, export := range exports { exportMap := export.(map[string]interface{}) for k, v := range exportMap { - if v == "" { - tmpFile := getTmpFile(t) - exportMap[k] = tmpFile + // Use strings.Contains because the value might have a leading "/" like "/" + if vStr, ok := v.(string); ok && strings.Contains(vStr, "") { + tmpDir := getTmpDir(t) + exportMap[k] = tmpDir } } } @@ -1087,7 +1082,7 @@ func TestMergeConfig(t *testing.T) { viper.Set(param.Origin_RunLocation.GetName(), dirname) viper.Set(param.Origin_Port.GetName(), 8443) - viper.Set(param.Origin_StoragePrefix.GetName(), "/") + viper.Set(param.Origin_StoragePrefix.GetName(), getTmpDir(t)) viper.Set(param.Origin_FederationPrefix.GetName(), "/") viper.Set("ConfigDir", dirname) // We don't inherit any defaults at this level of code -- in order to recognize @@ -1308,6 +1303,21 @@ func TestGenerateOriginIssuer(t *testing.T) { viper.SetConfigType("yaml") err := viper.MergeConfig(strings.NewReader(tc.yamlConfig)) require.NoError(t, err, "error reading config") + + // Replace storage prefixes with temp directories that have proper permissions + if exports, ok := viper.Get("origin.exports").([]interface{}); ok { + for _, export := range exports { + exportMap := export.(map[string]interface{}) + for k, v := range exportMap { + if vStr, ok := v.(string); ok && strings.Contains(vStr, "") { + tmpDir := getTmpDir(t) + exportMap[k] = tmpDir + } + } + } + viper.Set("origin.exports", exports) + } + err = config.InitServer(ctx, server_structs.OriginType) require.NoError(t, err) @@ -1570,7 +1580,7 @@ func TestGenerateFederationIssuer(t *testing.T) { viper.Set(param.Origin_EnablePublicReads.GetName(), tc.PublicReads) viper.Set(param.Origin_EnableListings.GetName(), false) viper.Set(param.Origin_EnableWrites.GetName(), false) - viper.Set(param.Origin_StoragePrefix.GetName(), "/does/not/matter") + viper.Set(param.Origin_StoragePrefix.GetName(), getTmpDir(t)) viper.Set(param.Origin_FederationPrefix.GetName(), "/foo/bar") viper.Set(param.TLSSkipVerify.GetName(), true) @@ -1604,7 +1614,9 @@ func TestWriteOriginScitokensConfig(t *testing.T) { viper.Set(param.Origin_RunLocation.GetName(), tmpDir) viper.Set(param.Origin_SelfTest.GetName(), true) viper.Set(param.Origin_FederationPrefix.GetName(), "/foo/bar") - viper.Set(param.Origin_StoragePrefix.GetName(), "/does/not/matter") + // Create storage dir with permissions for XRootD daemon user + storageDir := getTmpDir(t) + viper.Set(param.Origin_StoragePrefix.GetName(), storageDir) viper.Set(param.Server_Hostname.GetName(), "origin.example.com") viper.Set(param.Origin_StorageType.GetName(), string(server_structs.OriginStoragePosix)) diff --git a/xrootd/origin_test.go b/xrootd/origin_test.go index 5e1f9f7cd..06376244a 100644 --- a/xrootd/origin_test.go +++ b/xrootd/origin_test.go @@ -162,7 +162,8 @@ func TestOrigin(t *testing.T) { require.NoError(t, err) require.Len(t, ports, 2) - viper.Set(param.Origin_StoragePrefix.GetName(), t.TempDir()) + // Create temp dir with 0777 permissions so XRootD daemon user can write + viper.Set(param.Origin_StoragePrefix.GetName(), getTmpDir(t)) viper.Set(param.Origin_FederationPrefix.GetName(), "/test") viper.Set(param.Origin_StorageType.GetName(), "posix") // Disable functionality we're not using (and is difficult to make work on Mac) @@ -204,6 +205,19 @@ func TestMultiExportOrigin(t *testing.T) { err := viper.ReadConfig(strings.NewReader(multiExportOriginConfig)) require.NoError(t, err, "error reading config") + // Replace storage prefixes with temp directories that have proper permissions + // for the daemon user (xrootd) before calling GetOriginExports() + exports := viper.Get("origin.exports").([]interface{}) + for _, export := range exports { + exportMap := export.(map[string]interface{}) + for k, v := range exportMap { + if v == "/" { + exportMap[k] = getTmpDir(t) + } + } + } + viper.Set("origin.exports", exports) + // Get available, unique ports and pre-allocate for xrootd startup. ports, err := test_utils.GetUniqueAvailablePorts(2) require.NoError(t, err) @@ -224,16 +238,12 @@ func TestMultiExportOrigin(t *testing.T) { err = config.InitServer(ctx, server_structs.OriginType) require.NoError(t, err) - exports, err := server_utils.GetOriginExports() + originExports, err := server_utils.GetOriginExports() require.NoError(t, err) - require.Len(t, exports, 2) + require.Len(t, originExports, 2) - // Override the object store prefix to a temp directory issuerUrl, err := config.GetServerIssuerURL() require.NoError(t, err) - for i := range exports { - exports[i].StoragePrefix = t.TempDir() - } mockupCancel := originMockup(ctx, egrp, t) defer mockupCancel() @@ -400,11 +410,11 @@ func TestPosixOriginWithSentinel(t *testing.T) { defer server_utils.ResetTestState() - // Create a test temp dir, ensure it's readable by XRootD + // Create a test temp dir, ensure it's readable and writable by XRootD daemon user tmpPathPattern := "XRD-Tst_Orgn*" tmpPath, err := os.MkdirTemp("", tmpPathPattern) require.NoError(t, err) - err = os.Chmod(tmpPath, 0755) + err = os.Chmod(tmpPath, 0777) // 0777 to allow XRootD daemon user access require.NoError(t, err) ports, err := test_utils.GetUniqueAvailablePorts(2) diff --git a/xrootd/xrootd_config_test.go b/xrootd/xrootd_config_test.go index 2a62742fa..481889178 100644 --- a/xrootd/xrootd_config_test.go +++ b/xrootd/xrootd_config_test.go @@ -55,8 +55,8 @@ func setupXrootd(t *testing.T, ctx context.Context, server server_structs.Server viper.Set(param.Xrootd_RunLocation.GetName(), tmpDir) viper.Set(param.Cache_RunLocation.GetName(), tmpDir) viper.Set(param.Origin_RunLocation.GetName(), tmpDir) - viper.Set(param.Origin_StoragePrefix.GetName(), "/") - viper.Set(param.Origin_FederationPrefix.GetName(), "/") + viper.Set(param.Origin_StoragePrefix.GetName(), getTmpDir(t)) + viper.Set(param.Origin_FederationPrefix.GetName(), "/test") viper.Set(param.Server_IssuerUrl.GetName(), "https://my-xrootd.com:8444") test_utils.MockFederationRoot(t, nil, nil) @@ -295,7 +295,7 @@ func TestUpdateAuth(t *testing.T) { scitokensName := filepath.Join(configDirname, "scitokens.cfg") viper.Set(param.Xrootd_ScitokensConfig.GetName(), scitokensName) viper.Set(param.Origin_FederationPrefix.GetName(), "/test") - viper.Set(param.Origin_StoragePrefix.GetName(), "/") + viper.Set(param.Origin_StoragePrefix.GetName(), getTmpDir(t)) test_utils.MockFederationRoot(t, nil, nil)