From 98734632a44c7bd55c63a6125614ffd623ff9753 Mon Sep 17 00:00:00 2001 From: Keith James Date: Thu, 2 Jan 2025 12:58:27 +0000 Subject: [PATCH 1/4] Fix access for federated users with the same name as a local user Ensure that when a data obect has "foo#localZone:read" in its ACL, it allows read operations by "foo#localZone" and "foo" (implied "localZone"), but not by "foo#remoteZone". To do this we need to distinguish in the IsReadableByUser function between the local zone (the zone to which the Sqyrrl server is logged in) and the home zone of the user being tested. In practice the local/login zone and the home zone of the iRODS user that wants access are very likely to be the same. However, conflating the two zones makes the IsReadableByUser function confusing. --- server/handlers.go | 11 ++++-- server/irods.go | 29 ++++++++------- server/irods_test.go | 85 ++++++++++++++++++++++++++++++++------------ 3 files changed, 87 insertions(+), 38 deletions(-) diff --git a/server/handlers.go b/server/handlers.go index 45ecaec..d2c722b 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -338,15 +338,20 @@ func HandleIRODSGet(server *SqyrrlServer) http.Handler { return } - userZone := server.iRODSAccount.ClientZone + localZone := server.iRODSAccount.ClientZone var isReadable bool if server.isAuthenticated(r) { + // The username obtained from the email address does not include the iRODS + // zone. We use the local zone to which the Sqyrrl server is connected as + // the user's zone. userName := iRODSUsernameFromEmail(logger, server.getSessionUserEmail(r)) + userZone := localZone + logger.Debug().Str("user", userName).Msg("User is authenticated") - isReadable, err = IsReadableByUser(logger, rodsFs, userName, userZone, objPath) + isReadable, err = IsReadableByUser(logger, rodsFs, localZone, userName, userZone, objPath) if err != nil { logger.Err(err).Msg("Failed to check if the object is readable") writeErrorResponse(logger, w, http.StatusInternalServerError) @@ -364,7 +369,7 @@ func HandleIRODSGet(server *SqyrrlServer) http.Handler { } } else { logger.Debug().Msg("User is not authenticated") - isReadable, err = IsPublicReadable(logger, rodsFs, userZone, objPath) + isReadable, err = IsPublicReadable(logger, rodsFs, localZone, objPath) if err != nil { logger.Err(err).Msg("Failed to check if the object is public readable") writeErrorResponse(logger, w, http.StatusInternalServerError) diff --git a/server/irods.go b/server/irods.go index 17f899b..92e4f3d 100644 --- a/server/irods.go +++ b/server/irods.go @@ -190,17 +190,20 @@ func NewIRODSAccount(logger zerolog.Logger, manager *config.ICommandsEnvironment } // IsReadableByUser checks if the data object at the given path is readable by the -// given user in the zone hosting the file. +// given user in the zone the Sqyrrl server is logged into. // // If iRODS is federated, there may be multiple zones, each with their own users. -// The zone argument is the zone of user whose read permission is to be checked, -// which is normally the current zone. This is consulted only if the ACL user zone is -// empty. +// +// The localZone argument is the zone that Sqyrrl is logged into. This is consulted +// only if an access permission in the data object's ACL has an empty user zone, in which +// case the local zone is assumed. +// +// The userZone argument is the zone of the user whose read permission is to be checked. // // A file is considered readable if the user has read access or is in a group that has // read access. func IsReadableByUser(logger zerolog.Logger, filesystem *ifs.FileSystem, - userName string, userZone string, rodsPath string) (_ bool, err error) { + localZone string, userName string, userZone string, rodsPath string) (_ bool, err error) { var acl []*types.IRODSAccess if acl, err = filesystem.ListACLs(rodsPath); err != nil { @@ -231,11 +234,11 @@ func IsReadableByUser(logger zerolog.Logger, filesystem *ifs.FileSystem, for _, ac := range acl { // ACL user zone may be empty if it refers to the local zone - var effectiveUserZone string + var acUserZone string if ac.UserZone != "" { - effectiveUserZone = ac.UserZone + acUserZone = ac.UserZone } else { - effectiveUserZone = userZone + acUserZone = localZone } hasRead := ac.AccessLevel == types.IRODSAccessLevelReadObject @@ -243,13 +246,13 @@ func IsReadableByUser(logger zerolog.Logger, filesystem *ifs.FileSystem, // There is permission directly for the user if ac.UserType == types.IRODSUserRodsUser || ac.UserType == types.IRODSUserRodsAdmin { - if effectiveUserZone == userZone && ac.UserName == userName && (hasRead || hasOwn) { + if acUserZone == userZone && ac.UserName == userName && (hasRead || hasOwn) { logger.Trace(). Str("path", rodsPath). Str("user", userName). Str("zone", userZone). - Str("effective_zone", effectiveUserZone). Str("ac_user", ac.UserName). + Str("ac_zone", acUserZone). Str("ac_level", string(ac.AccessLevel)). Bool("read", hasRead). Bool("own", hasOwn). @@ -263,7 +266,7 @@ func IsReadableByUser(logger zerolog.Logger, filesystem *ifs.FileSystem, Str("user", userName). Str("zone", userZone). Str("ac_user", ac.UserName). - Str("effective_zone", effectiveUserZone). + Str("ac_zone", acUserZone). Str("ac_level", string(ac.AccessLevel)). Bool("read", hasRead). Bool("own", hasOwn). @@ -275,13 +278,13 @@ func IsReadableByUser(logger zerolog.Logger, filesystem *ifs.FileSystem, // Check if user in the group of this AC (ac.UserName is the name of the AC's group, unfortunately) _, userInGroup := userGroupLookup[ac.UserName] - if effectiveUserZone == userZone && userInGroup && (hasRead || hasOwn) { + if acUserZone == userZone && userInGroup && (hasRead || hasOwn) { logger.Trace(). Str("path", rodsPath). Str("user", userName). Str("zone", userZone). - Str("effective_zone", effectiveUserZone). Str("ac_user", ac.UserName). + Str("ac_zone", acUserZone). Str("ac_level", string(ac.AccessLevel)). Bool("read", hasRead). Bool("own", hasOwn). diff --git a/server/irods_test.go b/server/irods_test.go index 797677a..e0c28e6 100644 --- a/server/irods_test.go +++ b/server/irods_test.go @@ -34,13 +34,15 @@ import ( var _ = Describe("iRODS functions", func() { var conn *connection.IRODSConnection - var zone string + var localZone, userZone string var workColl string var testFile, localPath, remotePath string var err error BeforeEach(func(ctx SpecContext) { - zone = "testZone" + localZone = "testZone" + userZone = localZone + workColl = TmpRodsPath(rootColl, "iRODSGetHandler") err = irodsFS.MakeDir(workColl, true) Expect(err).NotTo(HaveOccurred()) @@ -67,8 +69,8 @@ var _ = Describe("iRODS functions", func() { When("a non-existent path is given", func() { It("should return a FileNotFoundError", func() { - _, err := server.IsReadableByUser(suiteLogger, irodsFS, userInPublic, - zone, "/no/such/path") + _, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userInPublic, userZone, "/no/such/path") Expect(err).To(HaveOccurred()) Expect(err).To(MatchError(&types.FileNotFoundError{})) }) @@ -76,8 +78,8 @@ var _ = Describe("iRODS functions", func() { When("a non-existent user is given", func() { It("should return false", func() { - readable, err := server.IsReadableByUser(suiteLogger, irodsFS, "no_such_user", - zone, remotePath) + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + "no_such_user", userZone, remotePath) Expect(err).NotTo(HaveOccurred()) Expect(readable).To(BeFalse()) }) @@ -108,21 +110,31 @@ var _ = Describe("iRODS functions", func() { When("the user is in the public group", func() { It("should return false", func() { - readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userInPublic, - zone, remotePath) + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userInPublic, userZone, remotePath) Expect(err).NotTo(HaveOccurred()) Expect(readable).To(BeFalse()) }) + + When("the user is in a different zone", func() { + It("should return false", func() { + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userInPublic, "anotherZone", remotePath) + Expect(err).NotTo(HaveOccurred()) + Expect(readable).To(BeFalse()) + }) + }) }) When("the user is not in the public group", func() { It("should return false", func() { - readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userNotInPublic, - zone, remotePath) + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userNotInPublic, userZone, remotePath) Expect(err).NotTo(HaveOccurred()) Expect(readable).To(BeFalse()) }) }) + }) When("the data object has read permissions for the public group", func() { @@ -134,21 +146,31 @@ var _ = Describe("iRODS functions", func() { When("the user is in the public group", func() { It("should return true", func() { - readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userInPublic, - zone, remotePath) + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userInPublic, userZone, remotePath) Expect(err).NotTo(HaveOccurred()) Expect(readable).To(BeTrue()) }) + + When("the user is in a different zone", func() { + It("should return false", func() { + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userInPublic, "anotherZone", remotePath) + Expect(err).NotTo(HaveOccurred()) + Expect(readable).To(BeFalse()) + }) + }) }) When("the user is not in the public group", func() { It("should return false", func() { - readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userNotInPublic, - zone, remotePath) + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userNotInPublic, userZone, remotePath) Expect(err).NotTo(HaveOccurred()) Expect(readable).To(BeFalse()) }) }) + }) When("the data object has own permissions for the public group", func() { @@ -160,21 +182,31 @@ var _ = Describe("iRODS functions", func() { When("the user is in the public group", func() { It("should return true", func() { - readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userInPublic, - zone, remotePath) + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userInPublic, userZone, remotePath) Expect(err).NotTo(HaveOccurred()) Expect(readable).To(BeTrue()) }) + + When("the user is in a different zone", func() { + It("should return false", func() { + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userInPublic, "anotherZone", remotePath) + Expect(err).NotTo(HaveOccurred()) + Expect(readable).To(BeFalse()) + }) + }) }) When("the user is not in the public group", func() { It("should return false", func() { - readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userNotInPublic, - zone, remotePath) + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userNotInPublic, userZone, remotePath) Expect(err).NotTo(HaveOccurred()) Expect(readable).To(BeFalse()) }) }) + }) When("the data object has read permissions for several groups", func() { @@ -188,17 +220,26 @@ var _ = Describe("iRODS functions", func() { When("the user is in one of the groups", func() { It("should return true", func() { - readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userInOthers, - zone, remotePath) + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userInOthers, userZone, remotePath) Expect(err).NotTo(HaveOccurred()) Expect(readable).To(BeTrue()) }) + + When("the user is in a different zone", func() { + It("should return false", func() { + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userInOthers, "anotherZone", remotePath) + Expect(err).NotTo(HaveOccurred()) + Expect(readable).To(BeFalse()) + }) + }) }) When("the user is not in one of the groups", func() { It("should return false", func() { - readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userNotInPublic, - zone, remotePath) + readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone, + userNotInPublic, userZone, remotePath) Expect(err).NotTo(HaveOccurred()) Expect(readable).To(BeFalse()) }) From dded76024fa042151ed4b3d260e042d21c4dab01 Mon Sep 17 00:00:00 2001 From: Keith James Date: Thu, 2 Jan 2025 14:30:16 +0000 Subject: [PATCH 2/4] Add target to ensure that Ginkgo is up-to-date --- .github/workflows/run-tests.yml | 5 ----- Makefile | 9 ++++++--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index f227ffc..5acb8b8 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -54,11 +54,6 @@ jobs: go-version-file: "go.mod" cache: true - - name: "Install test runner" - run: | - go install -mod=mod github.com/onsi/ginkgo/v2/ginkgo - go get github.com/onsi/gomega/... - - name: "Configure iRODS clients" run: | mkdir -p "$HOME/.irods" diff --git a/Makefile b/Makefile index 0b0f092..960e3dd 100644 --- a/Makefile +++ b/Makefile @@ -8,7 +8,7 @@ export GOARCH := $(shell go env GOARCH) CGO_ENABLED := 1 -.PHONY: build build-linux build-darwin build-windows check clean coverage install lint test +.PHONY: build build-linux build-darwin build-windows check clean coverage install lint test-install test all: build @@ -37,10 +37,13 @@ lint: check: test -test: +test-install: + go install github.com/onsi/ginkgo/v2/ginkgo + +test: test-install ginkgo -r --race -coverage: +coverage: test-install ginkgo -r --cover -coverprofile=coverage.out dist: build From 29cccc697f93bae21f159e98b6887d3411ef9a31 Mon Sep 17 00:00:00 2001 From: Keith James Date: Thu, 2 Jan 2025 14:52:42 +0000 Subject: [PATCH 3/4] Add a check that the expected local iRODS user exists --- server/irods.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/server/irods.go b/server/irods.go index 92e4f3d..b1fbbea 100644 --- a/server/irods.go +++ b/server/irods.go @@ -205,6 +205,21 @@ func NewIRODSAccount(logger zerolog.Logger, manager *config.ICommandsEnvironment func IsReadableByUser(logger zerolog.Logger, filesystem *ifs.FileSystem, localZone string, userName string, userZone string, rodsPath string) (_ bool, err error) { var acl []*types.IRODSAccess + var users []*types.IRODSUser + + localUserExists := false + if users, err = filesystem.ListUsers(); err != nil { + return false, err + } + for _, user := range users { + if user.Name == userName && user.Zone == userZone { + localUserExists = true + break + } + } + if !localUserExists { + return false, nil + } if acl, err = filesystem.ListACLs(rodsPath); err != nil { return false, err From 89ebeb2352c7fe31071a32cb7a1c25dbb4cccdac Mon Sep 17 00:00:00 2001 From: Keith James Date: Tue, 7 Jan 2025 13:45:29 +0000 Subject: [PATCH 4/4] Add a TLS cipher to allow connection to older iRODS servers --- server/irods.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/server/irods.go b/server/irods.go index b1fbbea..d24b413 100644 --- a/server/irods.go +++ b/server/irods.go @@ -18,6 +18,7 @@ package server import ( + "crypto/tls" "fmt" "net/http" "os" @@ -141,6 +142,14 @@ func NewIRODSAccount(logger zerolog.Logger, manager *config.ICommandsEnvironment return nil, err } + var tlsConfig *tls.Config + if tlsConfig, err = account.SSLConfiguration.GetTLSConfig(account.Host); err != nil { + logger.Err(err).Msg("Failed to update TLS configuration") + return nil, err + } + // Add a cipher suite that is compatible with older iRODS servers (4.2.7) + tlsConfig.CipherSuites = append(tlsConfig.CipherSuites, tls.TLS_RSA_WITH_AES_256_CBC_SHA) + if password != "" { if err = InitIRODS(logger, manager, password); err != nil { logger.Err(err). @@ -218,6 +227,11 @@ func IsReadableByUser(logger zerolog.Logger, filesystem *ifs.FileSystem, } } if !localUserExists { + logger.Warn(). + Str("path", rodsPath). + Str("user", userName). + Str("zone", userZone). + Msg("iRODS user not found") return false, nil }