diff --git a/opencloud/pkg/init/init.go b/opencloud/pkg/init/init.go index 4e50d396c5..07380b004a 100644 --- a/opencloud/pkg/init/init.go +++ b/opencloud/pkg/init/init.go @@ -104,7 +104,7 @@ func CreateConfig(insecure, forceOverwrite, diff bool, configPath, adminPassword return fmt.Errorf("could not generate random secret for urlSigningSecret: %s", err) } } - authGuestJWTSecret = oldCfg.AuthGuest.TokenManager.JWTSecret + authGuestJWTSecret = oldCfg.AuthGuest.JWT.Secret if authGuestJWTSecret == "" { authGuestJWTSecret, err = generators.GenerateRandomPassword(passwordLength) if err != nil { @@ -226,7 +226,7 @@ func CreateConfig(insecure, forceOverwrite, diff bool, configPath, adminPassword }, AuthGuest: AuthGuest{ ServiceAccount: serviceAccount, - TokenManager: TokenManager{JWTSecret: authGuestJWTSecret}, + JWT: AuthGuestJWT{Secret: authGuestJWTSecret}, }, Users: UsersAndGroupsService{ Drivers: LdapBasedService{ diff --git a/opencloud/pkg/init/structs.go b/opencloud/pkg/init/structs.go index eec147250b..b0f63d84f2 100644 --- a/opencloud/pkg/init/structs.go +++ b/opencloud/pkg/init/structs.go @@ -57,7 +57,12 @@ type Activitylog struct { // AuthGuest is the configuration for the auth-guest service type AuthGuest struct { ServiceAccount ServiceAccount `yaml:"service_account"` - TokenManager TokenManager `yaml:"token_manager"` + JWT AuthGuestJWT `yaml:"jwt"` +} + +// AuthGuestJWT is the configuration for the guest session tokens +type AuthGuestJWT struct { + Secret string `yaml:"secret"` } // App is the configuration for the collaboration service diff --git a/services/auth-guest/README.md b/services/auth-guest/README.md index 1e763f7dbf..ea6c7326ff 100644 --- a/services/auth-guest/README.md +++ b/services/auth-guest/README.md @@ -41,7 +41,7 @@ sequenceDiagram Redeem->>+Reva: Get Share Reva->>-Redeem: Share Note right of Redeem: Validate Share, Mark Token used - Redeem->>-Web: Set Cookie, return shareid + Redeem->>-Web: Set Cookie, return shareid Note right of Web: HTTP only cookie with signed JWT (JWT lifetime 24h) Web->>+Proxy: "/graph/me/drives/sharedWithMe" Proxy->>+Reva: validate token extracted from JWT @@ -91,7 +91,8 @@ the event consumer, set `AUTH_GUEST_HTTP_DISABLED=true`. Relevant options: -- `AUTH_GUEST_JWT_SECRET` — secret used to sign session tokens. +- `AUTH_GUEST_SESSION_JWT_SECRET` — secret used to sign guest session tokens. + It must differ from `OC_JWT_SECRET`. - `AUTH_GUEST_JWT_COOKIE_NAME`, `AUTH_GUEST_JWT_TTL` — session cookie name and lifetime. - `AUTH_GUEST_TOKENS_STORAGE_ROOT` — where guest link token records are stored. diff --git a/services/auth-guest/pkg/command/server.go b/services/auth-guest/pkg/command/server.go index 204ec6b290..9139031b79 100644 --- a/services/auth-guest/pkg/command/server.go +++ b/services/auth-guest/pkg/command/server.go @@ -19,6 +19,7 @@ import ( "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/config" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/config/parser" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/metrics" + "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/revaconfig" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/server/debug" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/server/http" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/authguest" @@ -26,6 +27,7 @@ import ( "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/jwt" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/storage" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/token" + "github.com/opencloud-eu/reva/v2/cmd/revad/runtime" "github.com/opencloud-eu/reva/v2/pkg/events" "github.com/opencloud-eu/reva/v2/pkg/events/stream" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" @@ -77,7 +79,7 @@ func Server(cfg *config.Config) *cobra.Command { tokenSvc := token.NewTokenService() store := storage.NewFileManager(cfg.Storage.RootDirectory) - jwtService := jwt.NewJwtService(cfg.TokenManager.JWTSecret, cfg.JWT.TTL) + jwtService := jwt.NewJwtService(cfg.JWT.Secret, cfg.JWT.TTL) authGuest := authguest.NewAuthGuestService(tokenSvc, store, authguest.GatewaySelector(gatewaySelector), @@ -143,7 +145,18 @@ func Server(cfg *config.Config) *cobra.Command { } else { logger.Info().Msg("event listening disabled, not starting event service") } - + { + //FIXME: Does this need to be optional? Similar to cfg.HTTP.Disabled? + // run the appropriate reva servers based on the config + rCfg := revaconfig.GuestLinksConfigFromStruct(cfg) + if rServer := runtime.NewDrivenGRPCServerWithOptions(rCfg, + runtime.WithLogger(&logger.Logger), + runtime.WithRegistry(registry.GetRegistry()), + runtime.WithTraceProvider(tracerProvider), + ); rServer != nil { + gr.Add(runner.NewRevaServiceRunner(cfg.Service.Name+".rgrpc", rServer)) + } + } { debugServer, err := debug.Server( debug.Logger(logger), @@ -157,6 +170,10 @@ func Server(cfg *config.Config) *cobra.Command { gr.Add(runner.NewGolangHttpServerRunner(cfg.Service.Name+".debug", debugServer)) } + grpcSvc := registry.BuildGRPCService(cfg.GRPC.Namespace+"."+cfg.Service.Name, cfg.GRPC.Protocol, cfg.GRPC.Addr, version.GetString()) + if err := registry.RegisterService(ctx, logger, grpcSvc, cfg.Debug.Addr); err != nil { + logger.Fatal().Err(err).Msg("failed to register the grpc service") + } grResults := gr.Run(ctx) diff --git a/services/auth-guest/pkg/config/config.go b/services/auth-guest/pkg/config/config.go index 1e24494681..9d3292ad84 100644 --- a/services/auth-guest/pkg/config/config.go +++ b/services/auth-guest/pkg/config/config.go @@ -25,6 +25,7 @@ type Config struct { RevaGateway string `yaml:"reva_gateway" env:"OC_REVA_GATEWAY" desc:"CS3 gateway used to look up user metadata" introductionVersion:"%%NEXT%%"` GRPCClientTLS *shared.GRPCClientTLS `yaml:"grpc_client_tls"` + GRPC GRPCConfig `yaml:"grpc"` HTTP HTTP `yaml:"http"` Storage Storage `yaml:"storage"` TokenManager *TokenManager `yaml:"token_manager"` @@ -73,6 +74,14 @@ type HTTP struct { TLS shared.HTTPServiceTLS `yaml:"tls"` } +// GRPCConfig defines the GRPC configuration +type GRPCConfig struct { + Addr string `yaml:"addr" env:"AUTH_GUEST_GRPC_ADDR" desc:"The bind address of the GRPC service." introductionVersion:"%%NEXT%%"` + TLS *shared.GRPCServiceTLS `yaml:"tls"` + Namespace string `yaml:"-"` + Protocol string `yaml:"protocol" env:"OC_GRPC_PROTOCOL;AUTH_GUEST_GRPC_PROTOCOL" desc:"The transport protocol of the GRPC service." introductionVersion:"%%NEXT%%"` +} + // Storage defines the configuration for the token storage. type Storage struct { RootDirectory string `yaml:"root_directory" env:"AUTH_GUEST_TOKENS_STORAGE_ROOT" desc:"The directory where the guest share tokens are stored. If not defined, the root directory derives from $OC_BASE_DATA_PATH/auth-guest." introductionVersion:"%%NEXT%%"` @@ -80,11 +89,12 @@ type Storage struct { // TokenManager is the config for using the reva token manager type TokenManager struct { - JWTSecret string `yaml:"jwt_secret" env:"AUTH_GUEST_JWT_SECRET" desc:"The secret to mint and validate jwt tokens." introductionVersion:"%%NEXT%%"` + JWTSecret string `yaml:"jwt_secret" env:"OC_JWT_SECRET;AUTH_GUEST_JWT_SECRET" desc:"The secret to mint and validate jwt tokens." introductionVersion:"%%NEXT%%"` } // JWT defines the configuration for guest session tokens. type JWT struct { + Secret string `yaml:"secret" env:"AUTH_GUEST_SESSION_JWT_SECRET" desc:"The secret used to sign and validate guest session tokens. It must differ from OC_JWT_SECRET." introductionVersion:"%%NEXT%%" mask:"password"` CookieName string `yaml:"cookie_name" env:"AUTH_GUEST_JWT_COOKIE_NAME" desc:"The name of the session cookie set when a guest token is redeemed." introductionVersion:"%%NEXT%%"` TTL time.Duration `yaml:"ttl" env:"AUTH_GUEST_JWT_TTL" desc:"The lifetime of a redeemed guest session token." introductionVersion:"%%NEXT%%"` } diff --git a/services/auth-guest/pkg/config/defaults/defaultconfig.go b/services/auth-guest/pkg/config/defaults/defaultconfig.go index 52e2632c6e..880e62d019 100644 --- a/services/auth-guest/pkg/config/defaults/defaultconfig.go +++ b/services/auth-guest/pkg/config/defaults/defaultconfig.go @@ -40,6 +40,11 @@ func DefaultConfig() *config.Config { EnableTLS: false, }, RevaGateway: shared.DefaultRevaConfig().Address, + GRPC: config.GRPCConfig{ + Addr: "127.0.0.1:9268", + Namespace: "eu.opencloud.api", + Protocol: "tcp", + }, HTTP: config.HTTP{ Addr: "127.0.0.1:9266", Root: "/graph", @@ -70,7 +75,15 @@ func EnsureDefaults(cfg *config.Config) { cfg.GRPCClientTLS = structs.CopyOrZeroValue(cfg.Commons.GRPCClientTLS) } - if cfg.TokenManager == nil { + if cfg.GRPC.TLS == nil && cfg.Commons != nil { + cfg.GRPC.TLS = structs.CopyOrZeroValue(cfg.Commons.GRPCServiceTLS) + } + + if cfg.TokenManager == nil && cfg.Commons != nil && cfg.Commons.TokenManager != nil { + cfg.TokenManager = &config.TokenManager{ + JWTSecret: cfg.Commons.TokenManager.JWTSecret, + } + } else if cfg.TokenManager == nil { cfg.TokenManager = &config.TokenManager{} } diff --git a/services/auth-guest/pkg/config/parser/parse.go b/services/auth-guest/pkg/config/parser/parse.go index b2add76af3..38b2d27192 100644 --- a/services/auth-guest/pkg/config/parser/parse.go +++ b/services/auth-guest/pkg/config/parser/parse.go @@ -5,8 +5,10 @@ package parser import ( "errors" + "fmt" occfg "github.com/opencloud-eu/opencloud/pkg/config" + ocdefaults "github.com/opencloud-eu/opencloud/pkg/config/defaults" "github.com/opencloud-eu/opencloud/pkg/shared" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/config" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/config/defaults" @@ -41,5 +43,17 @@ func Validate(cfg *config.Config) error { if cfg.TokenManager == nil || cfg.TokenManager.JWTSecret == "" { return shared.MissingJWTTokenError(cfg.Service.Name) } + if cfg.JWT.Secret == "" { + return fmt.Errorf("the guest session secret has not been set properly in your config for %s. "+ + "Make sure your %s config contains the proper values "+ + "(e.g. by using 'opencloud init --diff' and applying the patch or setting a value manually in "+ + "the config/corresponding environment variable AUTH_GUEST_SESSION_JWT_SECRET)", + cfg.Service.Name, ocdefaults.BaseConfigPath()) + } + // The guest session token and the reva access token are both HS256 JWTs. Signing them + // with the same key would make them interchangeable. + if cfg.JWT.Secret == cfg.TokenManager.JWTSecret { + return fmt.Errorf("the guest session secret (AUTH_GUEST_SESSION_JWT_SECRET) of %s must differ from the jwt secret (OC_JWT_SECRET)", cfg.Service.Name) + } return nil } diff --git a/services/auth-guest/pkg/config/parser/parse_test.go b/services/auth-guest/pkg/config/parser/parse_test.go new file mode 100644 index 0000000000..1852027318 --- /dev/null +++ b/services/auth-guest/pkg/config/parser/parse_test.go @@ -0,0 +1,38 @@ +// Copyright 2026 OpenCloud GmbH +// SPDX-License-Identifier: Apache-2.0 + +package parser + +import ( + "testing" + + "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/config" +) + +func TestValidate(t *testing.T) { + tests := []struct { + name string + jwtSecret string + sessionSecret string + wantErr bool + }{ + {name: "distinct secrets", jwtSecret: "reva-secret", sessionSecret: "session-secret"}, + {name: "missing jwt secret", sessionSecret: "session-secret", wantErr: true}, + {name: "missing session secret", jwtSecret: "reva-secret", wantErr: true}, + {name: "shared secret", jwtSecret: "same-secret", sessionSecret: "same-secret", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := &config.Config{ + TokenManager: &config.TokenManager{JWTSecret: tt.jwtSecret}, + JWT: config.JWT{Secret: tt.sessionSecret}, + } + + err := Validate(cfg) + if (err != nil) != tt.wantErr { + t.Fatalf("Validate() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/services/auth-guest/pkg/revaconfig/config.go b/services/auth-guest/pkg/revaconfig/config.go new file mode 100644 index 0000000000..4cb0ae745b --- /dev/null +++ b/services/auth-guest/pkg/revaconfig/config.go @@ -0,0 +1,46 @@ +package revaconfig + +import ( + "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/config" +) + +// GuestLinksConfigFromStruct will adapt an OpenCloud config struct into a reva mapstructure to start a reva service. +func GuestLinksConfigFromStruct(cfg *config.Config) map[string]any { + rcfg := map[string]any{ + "shared": map[string]any{ + "jwt_secret": cfg.TokenManager.JWTSecret, + "gatewaysvc": cfg.RevaGateway, + "grpc_client_options": cfg.GRPCClientTLS, + "multi_tenant_enabled": cfg.Commons.MultiTenantEnabled, + }, + "grpc": map[string]any{ + "network": cfg.GRPC.Protocol, + "address": cfg.GRPC.Addr, + "tls_settings": map[string]any{ + "enabled": cfg.GRPC.TLS.Enabled, + "certificate": cfg.GRPC.TLS.Cert, + "key": cfg.GRPC.TLS.Key, + }, + "services": map[string]any{ + "authprovider": map[string]any{ + "auth_manager": "guestlinks", + "auth_managers": map[string]any{ + "guestlinks": map[string]any{ + "gateway_addr": cfg.RevaGateway, + "jwt_secret": cfg.JWT.Secret, + "service_account_id": cfg.ServiceAccount.ServiceAccountID, + "service_account_secret": cfg.ServiceAccount.ServiceAccountSecret, + }, + }, + }, + }, + "interceptors": map[string]any{ + "prometheus": map[string]any{ + "namespace": "opencloud", + "subsystem": "auth_guest", + }, + }, + }, + } + return rcfg +} diff --git a/services/auth-guest/pkg/server/http/errors.go b/services/auth-guest/pkg/server/http/errors.go index 869d0f6234..61952c91ee 100644 --- a/services/auth-guest/pkg/server/http/errors.go +++ b/services/auth-guest/pkg/server/http/errors.go @@ -9,7 +9,6 @@ import ( "net/http" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/authguest" - "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/storage" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/token" ) @@ -39,10 +38,6 @@ func writeRedeemError(w http.ResponseWriter, err error) { status, errorType = http.StatusUnauthorized, "tokenExpired" case errors.Is(re.ErrorType, token.ErrInvalidToken): status, errorType = http.StatusUnauthorized, "tokenInvalid" - case errors.Is(re.ErrorType, storage.ErrNotFound): - status, errorType = http.StatusNotFound, "tokenNotFound" - case errors.Is(re.ErrorType, storage.ErrInvalidHash): - status, errorType = http.StatusUnauthorized, "tokenInvalid" case errors.Is(re.ErrorType, authguest.ErrAlreadyRedeemed): status, errorType = http.StatusConflict, "tokenAlreadyRedeemed" case errors.Is(re.ErrorType, authguest.ErrShareNotFound): diff --git a/services/auth-guest/pkg/server/http/redeem.go b/services/auth-guest/pkg/server/http/redeem.go index 09bd63c5d0..c900431d02 100644 --- a/services/auth-guest/pkg/server/http/redeem.go +++ b/services/auth-guest/pkg/server/http/redeem.go @@ -12,6 +12,10 @@ import ( "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/authguest" ) +// maxRedeemBodySize limits the body of the unauthenticated redeem request. A +// token is about 90 bytes, so this leaves plenty of room for the JSON wrapping. +const maxRedeemBodySize = 4 << 10 + // RedeemRequest is the request body for token redemption. type RedeemRequest struct { Token string `json:"token"` @@ -24,6 +28,8 @@ type redeemResponse struct { // RedeemHandler validates the token submitted to the redeem endpoint. func RedeemHandler(log log.Logger, s authguest.AuthGuest, cfg *config.Config) func(w http.ResponseWriter, r *http.Request) { return func(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, maxRedeemBodySize) + var req RedeemRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { log.Debug().Err(err).Msg("request body is malformed") diff --git a/services/auth-guest/pkg/server/http/redeem_test.go b/services/auth-guest/pkg/server/http/redeem_test.go index 28a9320601..a4a4f1206a 100644 --- a/services/auth-guest/pkg/server/http/redeem_test.go +++ b/services/auth-guest/pkg/server/http/redeem_test.go @@ -15,7 +15,6 @@ import ( "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/config" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/authguest" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/authguest/mocks" - "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/storage" "github.com/opencloud-eu/opencloud/services/auth-guest/pkg/service/token" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -83,12 +82,6 @@ func TestRedeemHandlerErrorMapping(t *testing.T) { wantStatus: http.StatusUnauthorized, wantType: "tokenInvalid", }, - { - name: "token not found", - err: &authguest.RedeemError{ErrorType: storage.ErrNotFound}, - wantStatus: http.StatusNotFound, - wantType: "tokenNotFound", - }, { name: "token already redeemed", err: &authguest.RedeemError{ErrorType: authguest.ErrAlreadyRedeemed}, @@ -130,6 +123,17 @@ func TestRedeemHandlerErrorMapping(t *testing.T) { } } +func TestRedeemHandlerBodyTooLarge(t *testing.T) { + svcMock := mocks.NewAuthGuest(t) + + body := `{"token":"` + strings.Repeat("a", maxRedeemBodySize) + `"}` + rr := httptest.NewRecorder() + newRedeemHandler(t, svcMock)(rr, httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body))) + + assert.Equal(t, http.StatusBadRequest, rr.Code) + svcMock.AssertNotCalled(t, "Redeem", mock.Anything, mock.Anything) +} + func TestRedeemHandlerMalformedBody(t *testing.T) { svcMock := mocks.NewAuthGuest(t) diff --git a/services/auth-guest/pkg/service/authguest/service.go b/services/auth-guest/pkg/service/authguest/service.go index c33e016871..f7ae0be9a3 100644 --- a/services/auth-guest/pkg/service/authguest/service.go +++ b/services/auth-guest/pkg/service/authguest/service.go @@ -134,19 +134,26 @@ func (s *AuthGuestService) CleanupShare(shareID string) error { } // VerifyToken validates a token and returns its stored record. +// +// Until the secret has been verified, all failures are reported as +// token.ErrInvalidToken without a share id, so that a caller holding only part +// of a token learns neither the share id nor whether a record exists. func (s *AuthGuestService) verifyToken(tokenString string) (*storage.Record, error) { tok, err := s.tokenSvc.Parse(tokenString) if err != nil { - return nil, &RedeemError{ErrorType: err} + return nil, &RedeemError{ErrorType: token.ErrInvalidToken} } rec, err := s.store.Get(tok.ShareIDHash) - if err != nil { - return nil, &RedeemError{ErrorType: err} + switch { + case errors.Is(err, storage.ErrNotFound), errors.Is(err, storage.ErrInvalidHash): + return nil, &RedeemError{ErrorType: token.ErrInvalidToken} + case err != nil: + return nil, err } if err := s.tokenSvc.Verify(*tok, rec.SecretHash); err != nil { - return nil, &RedeemError{ErrorType: err, ShareID: rec.ShareID} + return nil, &RedeemError{ErrorType: token.ErrInvalidToken} } if !rec.Expiry.IsZero() && rec.Expiry.Before(time.Now()) { diff --git a/services/auth-guest/pkg/service/authguest/service_test.go b/services/auth-guest/pkg/service/authguest/service_test.go index df033dc596..03e1e20847 100644 --- a/services/auth-guest/pkg/service/authguest/service_test.go +++ b/services/auth-guest/pkg/service/authguest/service_test.go @@ -5,6 +5,8 @@ package authguest import ( "context" + "errors" + "strings" "testing" "time" @@ -158,6 +160,86 @@ func TestVerifyToken(t *testing.T) { } } +func TestVerifyTokenDoesNotDiscloseShare(t *testing.T) { + tok, rec := newToken(t) + // flip the last character of the secret + last := "x" + if strings.HasSuffix(tok, last) { + last = "y" + } + tamperedSecret := tok[:len(tok)-1] + last + + tests := []struct { + name string + token string + setup func(store *storagemocks.Manager) + }{ + { + name: "malformed token", + token: "not-a-token", + }, + { + name: "unknown record", + token: tok, + setup: func(store *storagemocks.Manager) { + store.On("Get", rec.ShareIDHash).Return(storage.Record{}, storage.ErrNotFound) + }, + }, + { + name: "invalid hash", + token: "v1.ab.secret", + setup: func(store *storagemocks.Manager) { + store.On("Get", "ab").Return(storage.Record{}, storage.ErrInvalidHash) + }, + }, + { + name: "wrong secret", + token: tamperedSecret, + setup: func(store *storagemocks.Manager) { + store.On("Get", rec.ShareIDHash).Return(rec, nil) + }, + }, + { + name: "wrong secret on expired and redeemed record", + token: tamperedSecret, + setup: func(store *storagemocks.Manager) { + r := rec + r.Expiry = time.Now().Add(-time.Hour) + r.Redeemed = true + store.On("Get", rec.ShareIDHash).Return(r, nil) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := storagemocks.NewManager(t) + if tt.setup != nil { + tt.setup(store) + } + s := NewAuthGuestService(token.NewTokenService(), store) + + _, err := s.verifyToken(tt.token) + var re *RedeemError + require.ErrorAs(t, err, &re) + assert.ErrorIs(t, re.ErrorType, token.ErrInvalidToken) + assert.Empty(t, re.ShareID) + }) + } +} + +func TestVerifyTokenStorageFailure(t *testing.T) { + tok, rec := newToken(t) + store := storagemocks.NewManager(t) + store.On("Get", rec.ShareIDHash).Return(storage.Record{}, errors.New("disk on fire")) + s := NewAuthGuestService(token.NewTokenService(), store) + + _, err := s.verifyToken(tok) + require.Error(t, err) + var re *RedeemError + assert.False(t, errors.As(err, &re), "storage failures must surface as internal errors") +} + func TestValidateShare(t *testing.T) { share := &collaboration.Share{Id: &collaboration.ShareId{OpaqueId: testShareID}} notExpiredShare := &collaboration.Share{ diff --git a/services/auth-guest/pkg/service/token/token.go b/services/auth-guest/pkg/service/token/token.go index e997845a62..5f0a240265 100644 --- a/services/auth-guest/pkg/service/token/token.go +++ b/services/auth-guest/pkg/service/token/token.go @@ -6,6 +6,7 @@ package token import ( "crypto/rand" "crypto/sha256" + "crypto/subtle" "encoding/base64" "errors" "fmt" @@ -65,7 +66,8 @@ func (s *TokenService) Parse(encoded string) (*Token, error) { } func (s *TokenService) Verify(candidate Token, storedSecretHash string) error { - if candidate.ShareIDHash == "" || candidate.secret == "" || candidate.SecretHash() != storedSecretHash { + if candidate.ShareIDHash == "" || candidate.secret == "" || + subtle.ConstantTimeCompare([]byte(candidate.SecretHash()), []byte(storedSecretHash)) != 1 { return ErrInvalidToken } diff --git a/services/gateway/pkg/config/config.go b/services/gateway/pkg/config/config.go index 62ab551046..6697a46bb1 100644 --- a/services/gateway/pkg/config/config.go +++ b/services/gateway/pkg/config/config.go @@ -37,6 +37,7 @@ type Config struct { AuthBearerEndpoint string `yaml:"auth_bearer_endpoint" env:"GATEWAY_AUTH_BEARER_ENDPOINT" desc:"The endpoint of the auth-bearer service. Can take a service name or a gRPC URI with the dns, kubernetes or unix protocol." introductionVersion:"1.0.0"` AuthMachineEndpoint string `yaml:"auth_machine_endpoint" env:"GATEWAY_AUTH_MACHINE_ENDPOINT" desc:"The endpoint of the auth-machine service. Can take a service name or a gRPC URI with the dns, kubernetes or unix protocol." introductionVersion:"1.0.0"` AuthServiceEndpoint string `yaml:"auth_service_endpoint" env:"GATEWAY_AUTH_SERVICE_ENDPOINT" desc:"The endpoint of the auth-service service. Can take a service name or a gRPC URI with the dns, kubernetes or unix protocol." introductionVersion:"1.0.0"` + AuthGuestLinkEndpoint string `yaml:"auth_guestlinks_endpoint" env:"GATEWAY_AUTH_GUESTLINKS_ENDPOINT" desc:"The endpoint of the auth-guestlinks service. Can take a service name or a gRPC URI with the dns, kubernetes or unix protocol." introductionVersion:"%%NEXT%%"` StoragePublicLinkEndpoint string `yaml:"storage_public_link_endpoint" env:"GATEWAY_STORAGE_PUBLIC_LINK_ENDPOINT" desc:"The endpoint of the storage-publiclink service. Can take a service name or a gRPC URI with the dns, kubernetes or unix protocol." introductionVersion:"1.0.0"` StorageUsersEndpoint string `yaml:"storage_users_endpoint" env:"GATEWAY_STORAGE_USERS_ENDPOINT" desc:"The endpoint of the storage-users service. Can take a service name or a gRPC URI with the dns, kubernetes or unix protocol." introductionVersion:"1.0.0"` StorageSharesEndpoint string `yaml:"storage_shares_endpoint" env:"GATEWAY_STORAGE_SHARES_ENDPOINT" desc:"The endpoint of the storage-shares service. Can take a service name or a gRPC URI with the dns, kubernetes or unix protocol." introductionVersion:"1.0.0"` diff --git a/services/gateway/pkg/config/defaults/defaultconfig.go b/services/gateway/pkg/config/defaults/defaultconfig.go index a6cc070046..95f536c73f 100644 --- a/services/gateway/pkg/config/defaults/defaultconfig.go +++ b/services/gateway/pkg/config/defaults/defaultconfig.go @@ -53,6 +53,7 @@ func DefaultConfig() *config.Config { AppRegistryEndpoint: "eu.opencloud.api.app-registry", AuthAppEndpoint: "eu.opencloud.api.auth-app", AuthBasicEndpoint: "eu.opencloud.api.auth-basic", + AuthGuestLinkEndpoint: "eu.opencloud.api.auth-guest", AuthMachineEndpoint: "eu.opencloud.api.auth-machine", AuthServiceEndpoint: "eu.opencloud.api.auth-service", GroupsEndpoint: "eu.opencloud.api.groups", diff --git a/services/gateway/pkg/revaconfig/config.go b/services/gateway/pkg/revaconfig/config.go index 331b658abd..79cb8be761 100644 --- a/services/gateway/pkg/revaconfig/config.go +++ b/services/gateway/pkg/revaconfig/config.go @@ -92,6 +92,7 @@ func GatewayConfigFromStruct(cfg *config.Config, logger log.Logger) map[string]a "publicshares": cfg.StoragePublicLinkEndpoint, "serviceaccounts": cfg.AuthServiceEndpoint, "ocmshares": cfg.OCMEndpoint, + "guestlinks": cfg.AuthGuestLinkEndpoint, }, }, }, diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index f0912f8fa9..4beba8f79f 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -332,6 +332,7 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config, } var authenticators []middleware.Authenticator + if cfg.EnableBasicAuth { logger.Warn().Msg("basic auth enabled, use only for testing or development") authenticators = append(authenticators, middleware.BasicAuthenticator{ @@ -352,6 +353,17 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config, return alice.Chain{}, err } authenticators = append(authenticators, oidcAuthenticator) + + if cfg.Commons != nil && cfg.Commons.EnableGuestLinks { + authenticators = append(authenticators, &middleware.GuestLinkAuthenticator{ + Logger: logger, + RevaGatewaySelector: gatewaySelector, + Config: middleware.GuestLinkAuthConfig{ + CookieName: cfg.GuestLinkAuth.CookieName, + }, + }) + } + authenticators = append(authenticators, middleware.PublicShareAuthenticator{ Logger: logger, RevaGatewaySelector: gatewaySelector, diff --git a/services/proxy/pkg/config/config.go b/services/proxy/pkg/config/config.go index 8ecf17dca7..838cf48595 100644 --- a/services/proxy/pkg/config/config.go +++ b/services/proxy/pkg/config/config.go @@ -49,6 +49,7 @@ type Config struct { CSPConfigFileLocation string `yaml:"csp_config_file_location" env:"PROXY_CSP_CONFIG_FILE_LOCATION" desc:"The location of the CSP configuration file." introductionVersion:"1.0.0"` CSPConfigFileOverrideLocation string `yaml:"csp_config_file_override_location" env:"PROXY_CSP_CONFIG_FILE_OVERRIDE_LOCATION" desc:"The location of the CSP configuration file override." introductionVersion:"4.0.0"` Events Events `yaml:"events"` + GuestLinkAuth GuestLinkAuth `yaml:"guest_link_auth"` Context context.Context `json:"-" yaml:"-"` } @@ -244,3 +245,8 @@ type Events struct { AuthUsername string `yaml:"username" env:"OC_EVENTS_AUTH_USERNAME;PROXY_EVENTS_AUTH_USERNAME" desc:"The username to authenticate with the events broker. The events broker is the OpenCloud service which receives and delivers events between the services." introductionVersion:"1.0.0"` AuthPassword string `yaml:"password" env:"OC_EVENTS_AUTH_PASSWORD;PROXY_EVENTS_AUTH_PASSWORD" desc:"The password to authenticate with the events broker. The events broker is the OpenCloud service which receives and delivers events between the services." introductionVersion:"1.0.0"` } + +// GuestLinkAuth configures the guest-link cookie authentication flow. +type GuestLinkAuth struct { + CookieName string `yaml:"cookie_name" env:"PROXY_GUEST_LINK_COOKIE_NAME" desc:"The name of the guest-session cookie. Defaults to __Host-opencloud-guest." introductionVersion:"%%NEXT%%"` +} diff --git a/services/proxy/pkg/config/defaults/defaultconfig.go b/services/proxy/pkg/config/defaults/defaultconfig.go index 457e150197..6399c35a5f 100644 --- a/services/proxy/pkg/config/defaults/defaultconfig.go +++ b/services/proxy/pkg/config/defaults/defaultconfig.go @@ -115,6 +115,9 @@ func DefaultConfig() *config.Config { AuthMiddleware: config.AuthMiddleware{ AllowAppAuth: true, }, + GuestLinkAuth: config.GuestLinkAuth{ + CookieName: "__Host-oc_guest_session", + }, } } diff --git a/services/proxy/pkg/config/parser/parse_test.go b/services/proxy/pkg/config/parser/parse_test.go index b955ac6c6d..4ef3694ddc 100644 --- a/services/proxy/pkg/config/parser/parse_test.go +++ b/services/proxy/pkg/config/parser/parse_test.go @@ -113,3 +113,17 @@ func validProxyConfig() *config.Config { cfg.Commons = &shared.Commons{URLSigningSecret: "test-url-secret"} return cfg } + +func TestGuestLinkAuthConfig(t *testing.T) { + t.Run("feature disabled by default", func(t *testing.T) { + cfg := defaults.FullDefaultConfig() + require.Equal(t, "__Host-oc_guest_session", cfg.GuestLinkAuth.CookieName) + }) + + t.Run("custom cookie name is accepted", func(t *testing.T) { + cfg := validProxyConfig() + cfg.GuestLinkAuth.CookieName = "custom-guest-cookie" + require.NoError(t, parser.Validate(cfg)) + require.Equal(t, "custom-guest-cookie", cfg.GuestLinkAuth.CookieName) + }) +} diff --git a/services/proxy/pkg/middleware/app_auth.go b/services/proxy/pkg/middleware/app_auth.go index 166c75191e..08f44dc89f 100644 --- a/services/proxy/pkg/middleware/app_auth.go +++ b/services/proxy/pkg/middleware/app_auth.go @@ -19,21 +19,21 @@ type AppAuthAuthenticator struct { } // Authenticate implements the authenticator interface to authenticate requests via app auth. -func (m AppAuthAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { +func (m AppAuthAuthenticator) Authenticate(r *http.Request) AuthenticationResult { if isPublicPath(r.URL.Path) { // The authentication of public path requests is handled by another authenticator. // Since we can't guarantee the order of execution of the authenticators, we better // implement an early return here for paths we can't authenticate in this authenticator. - return nil, false + return NotApplicable() } username, password, ok := r.BasicAuth() if !ok { - return nil, false + return NotApplicable() } next, err := m.RevaGatewaySelector.Next() if err != nil { - return nil, false + return AuthenticationErrorResult(err) } authenticateResponse, err := next.Authenticate(r.Context(), &gateway.AuthenticateRequest{ @@ -42,17 +42,17 @@ func (m AppAuthAuthenticator) Authenticate(r *http.Request) (*http.Request, bool ClientSecret: password, }) if err != nil { - return nil, false + return AuthenticationErrorResult(err) } if authenticateResponse.GetStatus().GetCode() != cs3rpc.Code_CODE_OK { m.Logger.Debug().Str("msg", authenticateResponse.GetStatus().GetMessage()).Str("clientid", username).Msg("app auth failed") - return nil, false + return Failed() } user := authenticateResponse.GetUser() if user, err = m.UserRoleAssigner.ApplyUserRole(r.Context(), user); err != nil { m.Logger.Error().Err(err).Str("clientid", username).Msg("app auth: failed to load user roles") - return nil, false + return FailedWithErr(err) } ctx := revactx.ContextSetUser(r.Context(), user) @@ -60,5 +60,5 @@ func (m AppAuthAuthenticator) Authenticate(r *http.Request) (*http.Request, bool r = r.WithContext(ctx) - return r, true + return Succeeded(r) } diff --git a/services/proxy/pkg/middleware/app_auth_test.go b/services/proxy/pkg/middleware/app_auth_test.go index 0b6d9242a4..94d2e76fda 100644 --- a/services/proxy/pkg/middleware/app_auth_test.go +++ b/services/proxy/pkg/middleware/app_auth_test.go @@ -53,14 +53,14 @@ var _ = Describe("Authenticating requests", Label("AppAuthAuthenticator"), func( req := httptest.NewRequest(http.MethodGet, "http://example.com/example/path", http.NoBody) req.SetBasicAuth("test-user", "AppPassword") - req2, valid := authenticator.Authenticate(req) + result := authenticator.Authenticate(req) - Expect(valid).To(Equal(true)) - Expect(req2).ToNot(BeNil()) - user, ok := revactx.ContextGetUser(req2.Context()) + Expect(result.State).To(Equal(AuthenticationSucceeded)) + Expect(result.Request).ToNot(BeNil()) + user, ok := revactx.ContextGetUser(result.Request.Context()) Expect(ok).To(BeTrue()) Expect(user).ToNot(BeNil()) - token, ok := revactx.ContextGetToken(req2.Context()) + token, ok := revactx.ContextGetToken(result.Request.Context()) Expect(ok).To(BeTrue()) Expect(token).To(Equal("reva-token")) }) @@ -71,10 +71,9 @@ var _ = Describe("Authenticating requests", Label("AppAuthAuthenticator"), func( req := httptest.NewRequest(http.MethodGet, "http://example.com/example/path", http.NoBody) req.SetBasicAuth("test-user", "WrongAppPassword") - req2, valid := authenticator.Authenticate(req) + result := authenticator.Authenticate(req) - Expect(valid).To(Equal(false)) - Expect(req2).To(BeNil()) + Expect(result.State).To(Equal(AuthenticationFailed)) }) }) }) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index b314b0c670..b5dc7d5326 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -1,6 +1,8 @@ package middleware import ( + "encoding/json" + "encoding/xml" "fmt" "io" "net/http" @@ -42,12 +44,92 @@ const ( WwwAuthenticate = "Www-Authenticate" ) +// AuthenticationState represents the outcome of an authentication attempt. +type AuthenticationState int + +const ( + // AuthenticationNotApplicable means the authenticator does not apply to this request + // (e.g., no relevant credentials present, or not the right path). + AuthenticationNotApplicable AuthenticationState = iota + // AuthenticationFailed means the authenticator found applicable credentials but + // they were rejected (e.g., wrong password, invalid token). + AuthenticationFailed + // AuthenticationSucceeded means authentication was successful and the request + // has been augmented with identity information. + AuthenticationSucceeded + // AuthenticationError means authentication encountered an internal/dependency failure + // (e.g., backend unavailable, transport error). + AuthenticationError +) + +// AuthenticationResult represents the typed result of an authentication attempt. +type AuthenticationResult struct { + Request *http.Request + State AuthenticationState + Err error + Terminal bool + ErrorDetails ErrorDetails + // CookiesToClear lists cookie names that should be cleared on terminal failures. + CookiesToClear []string +} + +// ErrorDetails carries optional structured data for terminal failures. +type ErrorDetails interface { + isErrorDetails() +} + +// GuestSessionExpiredDetails carries data needed to render an expired-session response. +type GuestSessionExpiredDetails struct { + PermissionID string + IsDAV bool +} + +func (g GuestSessionExpiredDetails) isErrorDetails() {} + +// NotApplicable returns a result indicating the authenticator does not apply to the request. +func NotApplicable() AuthenticationResult { + return AuthenticationResult{State: AuthenticationNotApplicable} +} + +// Failed returns a result indicating credentials were present but rejected. +func Failed() AuthenticationResult { + return AuthenticationResult{State: AuthenticationFailed} +} + +// FailedWithErr returns a result indicating credentials were present but rejected, +// with an associated error for logging/classification. +func FailedWithErr(err error) AuthenticationResult { + return AuthenticationResult{State: AuthenticationFailed, Err: err} +} + +// TerminalFailed returns a result indicating credentials were present but rejected, +// and no further authenticators should be tried. +func TerminalFailed() AuthenticationResult { + return AuthenticationResult{State: AuthenticationFailed, Terminal: true} +} + +// TerminalFailedWithErr returns a result indicating credentials were present but rejected, +// with an associated error for logging/classification, and no further authenticators should be tried. +func TerminalFailedWithErr(err error) AuthenticationResult { + return AuthenticationResult{State: AuthenticationFailed, Err: err, Terminal: true} +} + +// Succeeded returns a result indicating successful authentication with the augmented request. +func Succeeded(r *http.Request) AuthenticationResult { + return AuthenticationResult{Request: r, State: AuthenticationSucceeded} +} + +// AuthenticationErrorResult returns a result indicating an internal/dependency failure. +func AuthenticationErrorResult(err error) AuthenticationResult { + return AuthenticationResult{State: AuthenticationError, Err: err} +} + // Authenticator is the common interface implemented by all request authenticators. type Authenticator interface { // Authenticate is used to authenticate incoming HTTP requests. - // The Authenticator may augment the request with user info or anything related to the - // authentication and return the augmented request. - Authenticate(*http.Request) (*http.Request, bool) + // The Authenticator returns a typed result indicating whether the request was + // authenticated, not applicable, failed, or encountered an error. + Authenticate(*http.Request) AuthenticationResult } type authenticationChallengeSuppressor interface { @@ -81,15 +163,35 @@ func Authentication(auths []Authenticator, opts ...Option) func(next http.Handle } suppressAuthenticationChallenge := false + var terminalResult AuthenticationResult for _, a := range auths { - if req, ok := a.Authenticate(r); ok { + result := a.Authenticate(r) + if result.State == AuthenticationSucceeded { span.End() - next.ServeHTTP(w, req) + next.ServeHTTP(w, result.Request) return } if suppressor, ok := a.(authenticationChallengeSuppressor); ok && suppressor.SuppressAuthenticationChallenge(r) { suppressAuthenticationChallenge = true } + if result.Terminal && result.State != AuthenticationSucceeded { + terminalResult = result + break + } + } + + // Handle terminal failures with error details (e.g., expired guest sessions). + if terminalResult.State != AuthenticationNotApplicable { + // Clear any cookies specified in the terminal result. + for _, cookieName := range terminalResult.CookiesToClear { + http.SetCookie(w, clearGuestCookie(cookieName)) + } + if renderErr := renderTerminalFailure(w, r, terminalResult); renderErr != nil { + options.Logger.Error().Err(renderErr).Str("authenticator", "terminal_failure_renderer").Msg("Failed to render terminal failure response") + w.WriteHeader(http.StatusInternalServerError) + return + } + return } if !suppressAuthenticationChallenge && !isPublicPath(r.URL.Path) { @@ -225,3 +327,88 @@ func getTraceProvider(o Options) trace.TracerProvider { } return trace.NewNoopTracerProvider() } + +// renderTerminalFailure renders the appropriate response for a terminal authentication failure. +// Returns nil on success, or an error if rendering fails. +func renderTerminalFailure(w http.ResponseWriter, r *http.Request, result AuthenticationResult) error { + // Check for structured error details (e.g., expired guest sessions). + if result.ErrorDetails != nil { + return renderErrorDetails(w, r, result.ErrorDetails) + } + + // Default: generic 401 without challenges. + if !webdav.IsWebdavRequest(r) { + w.WriteHeader(http.StatusUnauthorized) + return nil + } + + w.Header().Set("Content-Type", "application/xml; charset=utf-8") + w.WriteHeader(http.StatusUnauthorized) + return webdav.Encode(w, webdav.Exception{ + Code: webdav.SabredavNotAuthenticated, + Message: "Authentication error", + }) +} + +// renderErrorDetails renders a response based on structured error details. +func renderErrorDetails(w http.ResponseWriter, r *http.Request, details ErrorDetails) error { + switch d := details.(type) { + case GuestSessionExpiredDetails: + if webdav.IsWebdavRequest(r) { + return renderDAVExpired(w, d.PermissionID) + } + return renderJSONExpired(w, d.PermissionID) + default: + w.WriteHeader(http.StatusUnauthorized) + return nil + } +} + +// renderJSONExpired writes a structured JSON response for expired guest sessions. +func renderJSONExpired(w http.ResponseWriter, permissionID string) error { + resp := struct { + ErrorType string `json:"error_type"` + Message string `json:"message"` + PermissionID string `json:"permissionId"` + }{ + ErrorType: "session_expired", + Message: "Your session has expired.", + PermissionID: permissionID, + } + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + return json.NewEncoder(w).Encode(resp) +} + +// renderDAVExpired writes a SabreDAV-compatible XML response for expired guest sessions. +func renderDAVExpired(w http.ResponseWriter, permissionID string) error { + w.Header().Set("Content-Type", "application/xml; charset=utf-8") + w.WriteHeader(http.StatusUnauthorized) + return webdav.EncodeXML(w, davExpiredResponse{ + XmlnsD: "DAV", + XmlnsS: "http://sabredav.org/ns", + Exception: "Sabre\\DAV\\Exception\\NotAuthenticated", + Message: "Guest session has expired.", + Details: davExpiredDetails{ + XmlnsOpenCloud: "http://opencloud.org/ns", + ErrorType: "session_expired", + ShareID: permissionID, + }, + }) +} + +type davExpiredResponse struct { + XMLName xml.Name `xml:"d:error"` + XmlnsD string `xml:"xmlns:d,attr"` + XmlnsS string `xml:"xmlns:s,attr"` + Exception string `xml:"s:Exception"` + Message string `xml:"s:Message"` + Details davExpiredDetails `xml:"opencloud:details"` +} + +type davExpiredDetails struct { + XmlnsOpenCloud string `xml:"xmlns:opencloud,attr"` + ErrorType string `xml:"opencloud:error_type"` + ShareID string `xml:"opencloud:share_id"` +} diff --git a/services/proxy/pkg/middleware/basic_auth.go b/services/proxy/pkg/middleware/basic_auth.go index bc9e1222f4..ee91646778 100644 --- a/services/proxy/pkg/middleware/basic_auth.go +++ b/services/proxy/pkg/middleware/basic_auth.go @@ -17,17 +17,17 @@ type BasicAuthenticator struct { } // Authenticate implements the authenticator interface to authenticate requests via basic auth. -func (m BasicAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { +func (m BasicAuthenticator) Authenticate(r *http.Request) AuthenticationResult { if isPublicPath(r.URL.Path) && isPublicWithShareToken(r) { // The authentication of public path requests is handled by another authenticator. // Since we can't guarantee the order of execution of the authenticators, we better // implement an early return here for paths we can't authenticate in this authenticator. - return nil, false + return NotApplicable() } login, password, ok := r.BasicAuth() if !ok { - return nil, false + return NotApplicable() } user, _, err := m.UserProvider.Authenticate(r.Context(), login, password) @@ -37,7 +37,7 @@ func (m BasicAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) Str("authenticator", "basic"). Str("path", r.URL.Path). Msg("failed to authenticate request") - return nil, false + return FailedWithErr(err) } // fake oidc claims @@ -58,5 +58,5 @@ func (m BasicAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) Str("authenticator", "basic"). Str("path", r.URL.Path). Msg("successfully authenticated request") - return r.WithContext(oidc.NewContext(r.Context(), claims)), true + return Succeeded(r.WithContext(oidc.NewContext(r.Context(), claims))) } diff --git a/services/proxy/pkg/middleware/basic_auth_test.go b/services/proxy/pkg/middleware/basic_auth_test.go index 054c3e1362..e4e87b6628 100644 --- a/services/proxy/pkg/middleware/basic_auth_test.go +++ b/services/proxy/pkg/middleware/basic_auth_test.go @@ -44,19 +44,19 @@ var _ = Describe("Authenticating requests", Label("BasicAuthenticator"), func() req := httptest.NewRequest(http.MethodGet, "http://example.com/example/path", http.NoBody) req.SetBasicAuth("testuser", "testpassword") - req2, valid := authenticator.Authenticate(req) + result := authenticator.Authenticate(req) - Expect(valid).To(Equal(true)) - Expect(req2).ToNot(BeNil()) + Expect(result.State).To(Equal(AuthenticationSucceeded)) + Expect(result.Request).ToNot(BeNil()) }) It("adds claims to the request context", func() { req := httptest.NewRequest(http.MethodGet, "http://example.com/example/path", http.NoBody) req.SetBasicAuth("testuser", "testpassword") - req2, valid := authenticator.Authenticate(req) - Expect(valid).To(Equal(true)) + result := authenticator.Authenticate(req) + Expect(result.State).To(Equal(AuthenticationSucceeded)) - claims := oidc.FromContext(req2.Context()) + claims := oidc.FromContext(result.Request.Context()) Expect(claims).ToNot(BeNil()) Expect(claims[oidc.Iss]).To(Equal("IdpId")) Expect(claims[oidc.PreferredUsername]).To(Equal("testuser")) diff --git a/services/proxy/pkg/middleware/create_home.go b/services/proxy/pkg/middleware/create_home.go index 9979367367..ecdc4df459 100644 --- a/services/proxy/pkg/middleware/create_home.go +++ b/services/proxy/pkg/middleware/create_home.go @@ -3,6 +3,7 @@ package middleware import ( "fmt" "net/http" + "slices" "strconv" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" @@ -68,7 +69,8 @@ func (m createHome) ServeHTTP(w http.ResponseWriter, req *http.Request) { createHomeReq := &provider.CreateHomeRequest{} u, ok := revactx.ContextGetUser(ctx) if ok { - if u.GetId().GetType() == userv1beta1.UserType_USER_TYPE_LIGHTWEIGHT || u.GetId().GetType() == userv1beta1.UserType_USER_TYPE_SERVICE { + userType := u.GetId().GetType() + if slices.Contains([]userv1beta1.UserType{userv1beta1.UserType_USER_TYPE_LIGHTWEIGHT, userv1beta1.UserType_USER_TYPE_SERVICE, userv1beta1.UserType_USER_TYPE_GUEST}, userType) { next() return } diff --git a/services/proxy/pkg/middleware/guest_link_auth.go b/services/proxy/pkg/middleware/guest_link_auth.go new file mode 100644 index 0000000000..31b5d4edaa --- /dev/null +++ b/services/proxy/pkg/middleware/guest_link_auth.go @@ -0,0 +1,276 @@ +package middleware + +import ( + "encoding/json" + "errors" + "net/http" + "strings" + "time" + + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + rpcv1beta1 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" + typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/webdav" + revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" + "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" + "go.opentelemetry.io/otel/trace/noop" +) + +const ( + spanAttrOutcome = "guest_auth.outcome" + + guestAuthOutcomeSuccess = "success" + guestAuthOutcomeExpired = "expired" + guestAuthOutcomeInvalid = "invalid" + guestAuthOutcomeUnavailable = "unavailable" + guestAuthOutcomeInternal = "internal" +) + +var ( + guestLinkPathPrefixes = []string{ + "/graph/v1beta1/me/drive/sharedWithMe", + "/dav/", + "/remote.php/dav/", + "/webdav/", + "/remote.php/webdav/", + } + + // unixEpoch is the zero time used for cookie expiration. + unixEpoch = time.Unix(0, 0) + + // maxShareIDLength is the maximum allowed length for a share ID in InnerError. + maxShareIDLength = 512 +) + +// guestLinkError represents the JSON error payload carried in Status.InnerError +// when a guest session has expired. +type guestLinkError struct { + Type string `json:"type"` + Reason string `json:"reason"` + ShareID string `json:"share_id"` +} + +// GuestLinkAuthConfig holds the guest-link authentication configuration. +type GuestLinkAuthConfig struct { + CookieName string +} + +// GuestLinkAuthenticator authenticates requests using a guest-session cookie. +type GuestLinkAuthenticator struct { + Logger log.Logger + RevaGatewaySelector pool.Selectable[gateway.GatewayAPIClient] + Config GuestLinkAuthConfig + Tracer trace.Tracer +} + +// guestLinkCookieName returns the configured cookie name, defaulting to __Host-opencloud-guest. +func guestLinkCookieName(cfg GuestLinkAuthConfig) string { + return cfg.CookieName +} + +// isGuestLinkPath returns whether the request path falls within a supported guest-link prefix. +func isGuestLinkPath(path string) bool { + for _, prefix := range guestLinkPathPrefixes { + if strings.HasPrefix(path, prefix) { + return true + } + } + return false +} + +// readGuestCookie reads the guest-session cookie from the request. +// Returns empty string if the cookie is missing or empty. +func readGuestCookie(r *http.Request, name string) (string, error) { + cookie, err := r.Cookie(name) + if err != nil { + return "", nil // Missing cookie is not an error; it means not applicable + } + return cookie.Value, nil +} + +// clearGuestCookie returns a cookie that instructs the browser to delete the guest-session cookie. +func clearGuestCookie(name string) *http.Cookie { + return &http.Cookie{ + Name: name, + Value: "", + Path: "/", + Domain: "", + Secure: true, + HttpOnly: true, + SameSite: http.SameSiteStrictMode, + Expires: unixEpoch, + MaxAge: -1, + } +} + +// SuppressAuthenticationChallenge prevents other authentication mechanisms from challenging guest-cookie requests. +func (m *GuestLinkAuthenticator) SuppressAuthenticationChallenge(req *http.Request) bool { + name := guestLinkCookieName(m.Config) + cookie, err := req.Cookie(name) + if err != nil { + return false + } + return cookie.Value != "" +} + +// Authenticate implements the Authenticator interface for guest-link cookie authentication. +func (m *GuestLinkAuthenticator) Authenticate(r *http.Request) AuthenticationResult { + _, span := m.tracer().Start(r.Context(), "guest_link_auth") + defer span.End() + + // Check applicability: enabled, cookie present, path allowed. + name := guestLinkCookieName(m.Config) + + rawCookie, err := readGuestCookie(r, name) + if err != nil || rawCookie == "" { + return NotApplicable() + } + + if !isGuestLinkPath(r.URL.Path) { + return NotApplicable() + } + + // Call Reva gateway Authenticate API. + gatewayClient, err := m.RevaGatewaySelector.Next() + if err != nil { + m.Logger.Error().Err(err).Str("authenticator", "guest_link").Msg("Failed to select gateway client") + span.SetAttributes(attribute.String(spanAttrOutcome, guestAuthOutcomeUnavailable)) + return AuthenticationErrorResult(err) + } + + authReq := &gateway.AuthenticateRequest{ + Type: "guestlinks", + ClientId: "", + ClientSecret: rawCookie, + } + + authResp, err := gatewayClient.Authenticate(r.Context(), authReq) + if err != nil { + m.Logger.Error().Err(err).Str("authenticator", "guest_link").Msg("Gateway Authenticate call failed") + span.SetAttributes(attribute.String(spanAttrOutcome, guestAuthOutcomeUnavailable)) + return AuthenticationErrorResult(err) + } + + switch authResp.GetStatus().GetCode() { + case rpcv1beta1.Code_CODE_OK: + return m.handleOK(r, authResp, span) + case rpcv1beta1.Code_CODE_UNAUTHENTICATED: + return m.handleUnauthenticated(r, authResp, span) + case rpcv1beta1.Code_CODE_UNAVAILABLE: + span.SetAttributes(attribute.String(spanAttrOutcome, guestAuthOutcomeUnavailable)) + m.Logger.Debug().Str("authenticator", "guest_link").Str("message", authResp.GetStatus().GetMessage()).Msg("Guest auth unavailable") + return AuthenticationErrorResult(errors.New(authResp.GetStatus().GetMessage())) + default: + span.SetAttributes(attribute.String(spanAttrOutcome, guestAuthOutcomeInternal)) + m.Logger.Error().Int32("status_code", int32(authResp.GetStatus().GetCode())).Str("authenticator", "guest_link").Msg("Unexpected guest auth status") + return AuthenticationErrorResult(errors.New("unexpected authentication status")) + } +} + +func (m *GuestLinkAuthenticator) handleOK(r *http.Request, authResp *gateway.AuthenticateResponse, span trace.Span) AuthenticationResult { + token := authResp.GetToken() + user := authResp.GetUser() + + if token == "" { + m.Logger.Error().Str("authenticator", "guest_link").Msg("Guest auth: CODE_OK but missing token") + span.SetAttributes(attribute.String(spanAttrOutcome, guestAuthOutcomeInternal)) + return AuthenticationErrorResult(errors.New("CODE_OK but missing token")) + } + + if user == nil { + m.Logger.Error().Str("authenticator", "guest_link").Msg("Guest auth: CODE_OK but missing user") + span.SetAttributes(attribute.String(spanAttrOutcome, guestAuthOutcomeInternal)) + return AuthenticationErrorResult(errors.New("CODE_OK but missing user")) + } + + ctx := revactx.ContextSetUser(r.Context(), user) + ctx = revactx.ContextSetToken(ctx, token) + r = r.WithContext(ctx) + r.Header.Set(revactx.TokenHeader, token) + + span.SetAttributes( + attribute.String("guest_auth.outcome", guestAuthOutcomeSuccess), + attribute.String("user.id", user.GetId().GetOpaqueId()), + ) + span.SetAttributes(attribute.String(spanAttrOutcome, guestAuthOutcomeSuccess)) + + m.Logger.Debug().Str("user.id", user.GetId().GetOpaqueId()).Str("authenticator", "guest_link").Msg("Guest auth succeeded") + + return Succeeded(r) +} + +func (m *GuestLinkAuthenticator) handleUnauthenticated(r *http.Request, authResp *gateway.AuthenticateResponse, span trace.Span) AuthenticationResult { + name := guestLinkCookieName(m.Config) + + innerError := authResp.GetStatus().GetInnerError() + expired, shareID := m.parseInnerError(innerError) + + if expired { + m.Logger.Debug().Str("share_id_hash", shareID).Str("authenticator", "guest_link").Msg("Guest session expired") + span.SetAttributes(attribute.String(spanAttrOutcome, guestAuthOutcomeExpired)) + return AuthenticationResult{ + State: AuthenticationFailed, + Terminal: true, + CookiesToClear: []string{name}, + ErrorDetails: GuestSessionExpiredDetails{ + PermissionID: shareID, + IsDAV: webdav.IsWebdavRequest(r), + }, + } + } + + // Generic unauthenticated failure. + span.SetAttributes(attribute.String(spanAttrOutcome, guestAuthOutcomeInvalid)) + m.Logger.Debug().Str("authenticator", "guest_link").Msg("Guest auth: invalid credentials") + + return AuthenticationResult{ + State: AuthenticationFailed, + Terminal: true, + CookiesToClear: []string{name}, + } +} + +func (m *GuestLinkAuthenticator) parseInnerError(innerError *typesv1beta1.OpaqueEntry) (bool, string) { + if innerError == nil { + return false, "" + } + + value := innerError.Value + if len(value) == 0 { + return false, "" + } + + decoder := innerError.Decoder + if decoder != "json" { + return false, "" + } + + var err guestLinkError + if err := json.Unmarshal(value, &err); err != nil { + return false, "" + } + + if err.Type != "opencloud_guest_link_error" { + return false, "" + } + + if err.Reason != "session_expired" { + return false, "" + } + + if len(err.ShareID) == 0 || len(err.ShareID) > maxShareIDLength { + return false, "" + } + + return true, err.ShareID +} + +func (m *GuestLinkAuthenticator) tracer() trace.Tracer { + if m.Tracer != nil { + return m.Tracer + } + return noop.NewTracerProvider().Tracer("proxy.middleware.guest_link") +} diff --git a/services/proxy/pkg/middleware/guest_link_auth_test.go b/services/proxy/pkg/middleware/guest_link_auth_test.go new file mode 100644 index 0000000000..714f456aa8 --- /dev/null +++ b/services/proxy/pkg/middleware/guest_link_auth_test.go @@ -0,0 +1,549 @@ +package middleware + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + gatewayv1beta1 "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + rpcv1beta1 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" + typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" + "github.com/opencloud-eu/opencloud/pkg/log" + revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" + "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" + "google.golang.org/grpc" +) + +const testEndpoint = "http://example.com/graph/v1beta1/me/drive/sharedWithMe" + +func getCookie(value string) *http.Cookie { + return &http.Cookie{Name: "__Host-oc_guest_session", HttpOnly: true, Secure: true, Value: value} +} + +func TestGuestLinkAuthenticator_Applicability(t *testing.T) { + logger := log.NewLogger() + + tests := []struct { + name string + enabled bool + cookieValue string + path string + expect AuthenticationState + }{ + {"disabled", false, "some-jwt", "/graph/users", AuthenticationNotApplicable}, + {"no cookie", true, "", "/graph/users", AuthenticationNotApplicable}, + {"empty cookie", true, "", "/graph/users", AuthenticationNotApplicable}, + {"unsupported path - archiver", true, "some-jwt", "/archiver", AuthenticationNotApplicable}, + {"unsupported path - root", true, "some-jwt", "/", AuthenticationNotApplicable}, + {"unsupported path - ocs", true, "some-jwt", "/ocs/v2.php/cloud/user", AuthenticationNotApplicable}, + {"supported path - graph", true, "some-jwt", "/graph/v1beta1/me/drive/sharedWithMe", AuthenticationFailed}, + {"supported path - dav", true, "some-jwt", "/dav/files/user", AuthenticationFailed}, + {"supported path - webdav", true, "some-jwt", "/webdav/files/user", AuthenticationFailed}, + {"supported path - remote.php", true, "some-jwt", "/remote.php/dav/files/user", AuthenticationFailed}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pool.RemoveSelector("GatewaySelector" + "eu.opencloud.api.gateway") + authenticator := &GuestLinkAuthenticator{ + Logger: logger, + Config: GuestLinkAuthConfig{ + CookieName: "__Host-oc_guest_session", + }, + RevaGatewaySelector: pool.GetSelector[gateway.GatewayAPIClient]( + "GatewaySelector", + "eu.opencloud.api.gateway", + func(cc grpc.ClientConnInterface) gateway.GatewayAPIClient { + return mockGatewayClient{ + AuthenticateFunc: func(authType, clientID, clientSecret string) (string, rpcv1beta1.Code) { + return "", rpcv1beta1.Code_CODE_UNAUTHENTICATED + }, + } + }, + ), + } + + req := httptest.NewRequest(http.MethodGet, "http://example.com"+tt.path, http.NoBody) + if tt.cookieValue != "" { + req.AddCookie(getCookie(tt.cookieValue)) + } + + result := authenticator.Authenticate(req) + + if result.State != tt.expect { + t.Errorf("expected state %v, got %v", tt.expect, result.State) + } + }) + } +} + +func TestGuestLinkAuthenticator_SupportsPathPrefixes(t *testing.T) { + supportedPrefixes := []string{"/graph/v1beta1/me/drive/sharedWithMe", "/dav/", "/webdav/", "/remote.php/dav/", "/remote.php/webdav/"} + for _, prefix := range supportedPrefixes { + t.Run("path prefix "+prefix, func(t *testing.T) { + if !isGuestLinkPath(prefix + "test") { + t.Errorf("expected %s to be a supported guest-link path", prefix) + } + }) + } + + unsupportedPaths := []string{"/archiver", "/", "/ocs/", "/konnect/", "/apps/"} + for _, path := range unsupportedPaths { + t.Run("unsupported "+path, func(t *testing.T) { + if isGuestLinkPath(path) { + t.Errorf("expected %s to not be a supported guest-link path", path) + } + }) + } +} + +func TestGuestLinkAuthenticator_Success(t *testing.T) { + pool.RemoveSelector("GatewaySelector" + "eu.opencloud.api.gateway") + authenticator := &GuestLinkAuthenticator{ + Logger: log.NewLogger(), + Config: GuestLinkAuthConfig{ + CookieName: "__Host-oc_guest_session", + }, + RevaGatewaySelector: pool.GetSelector[gateway.GatewayAPIClient]( + "GatewaySelector", + "eu.opencloud.api.gateway", + func(cc grpc.ClientConnInterface) gateway.GatewayAPIClient { + return mockGatewayClientWithUser{ + AuthenticateFunc: func(authType, clientID, clientSecret string) (string, *userpb.User, rpcv1beta1.Code) { + if authType != "guestlinks" { + return "", nil, rpcv1beta1.Code_CODE_NOT_FOUND + } + if clientSecret != "valid-jwt-token" { + return "", nil, rpcv1beta1.Code_CODE_UNAUTHENTICATED + } + guestUser := &userpb.User{ + Id: &userpb.UserId{OpaqueId: "guest-user-123", Type: userpb.UserType_USER_TYPE_GUEST}, + Username: "guest@example.com", + DisplayName: "Guest User", + } + return "reva-guest-token", guestUser, rpcv1beta1.Code_CODE_OK + }, + } + }, + ), + } + + req := httptest.NewRequest(http.MethodGet, testEndpoint, http.NoBody) + req.AddCookie(getCookie("valid-jwt-token")) + + result := authenticator.Authenticate(req) + + if result.State != AuthenticationSucceeded { + t.Errorf("expected AuthenticationSucceeded, got %v", result.State) + return + } + + ctx := result.Request.Context() + user, ok := revactx.ContextGetUser(ctx) + if !ok || user == nil { + t.Error("expected user in context") + } + if user != nil && user.GetId().GetOpaqueId() != "guest-user-123" { + t.Errorf("expected user ID 'guest-user-123', got '%s'", user.GetId().GetOpaqueId()) + } + + token := revactx.ContextMustGetToken(ctx) + if token != "reva-guest-token" { + t.Errorf("expected token 'reva-guest-token', got '%s'", token) + } + + if req.Header.Get(revactx.TokenHeader) != "reva-guest-token" { + t.Errorf("expected x-access-token header to be 'reva-guest-token'") + } +} + +func TestGuestLinkAuthenticator_MissingUserOnOK(t *testing.T) { + pool.RemoveSelector("GatewaySelector" + "eu.opencloud.api.gateway") + authenticator := &GuestLinkAuthenticator{ + Logger: log.NewLogger(), + Config: GuestLinkAuthConfig{ + CookieName: "__Host-oc_guest_session", + }, + RevaGatewaySelector: pool.GetSelector[gateway.GatewayAPIClient]( + "GatewaySelector", + "eu.opencloud.api.gateway", + func(cc grpc.ClientConnInterface) gateway.GatewayAPIClient { + return mockGatewayClient{ + AuthenticateFunc: func(authType, clientID, clientSecret string) (string, rpcv1beta1.Code) { + return "token-without-user", rpcv1beta1.Code_CODE_OK + }, + } + }, + ), + } + + req := httptest.NewRequest(http.MethodGet, testEndpoint, http.NoBody) + req.AddCookie(getCookie("jwt-token")) + + result := authenticator.Authenticate(req) + + if result.State != AuthenticationError { + t.Errorf("expected AuthenticationError, got %v", result.State) + } +} + +func TestGuestLinkAuthenticator_MissingTokenOnOK(t *testing.T) { + pool.RemoveSelector("GatewaySelector" + "eu.opencloud.api.gateway") + + authenticator := &GuestLinkAuthenticator{ + Logger: log.NewLogger(), + Config: GuestLinkAuthConfig{ + CookieName: "__Host-oc_guest_session", + }, + RevaGatewaySelector: pool.GetSelector[gateway.GatewayAPIClient]( + "GatewaySelector", + "eu.opencloud.api.gateway", + func(cc grpc.ClientConnInterface) gateway.GatewayAPIClient { + return mockGatewayClient{ + AuthenticateFunc: func(authType, clientID, clientSecret string) (string, rpcv1beta1.Code) { + return "", rpcv1beta1.Code_CODE_OK + }, + } + }, + ), + } + + req := httptest.NewRequest(http.MethodGet, testEndpoint, http.NoBody) + req.AddCookie(getCookie("jwt-token")) + + result := authenticator.Authenticate(req) + + if result.State != AuthenticationError { + t.Errorf("expected AuthenticationError, got %v", result.State) + } +} + +func TestGuestLinkAuthenticator_ExpiredSession(t *testing.T) { + pool.RemoveSelector("GatewaySelector" + "eu.opencloud.api.gateway") + + expiredInnerError := &typesv1beta1.OpaqueEntry{ + Decoder: "json", + Value: []byte(`{"type":"opencloud_guest_link_error","reason":"session_expired","share_id":"share-abc-123"}`), + } + + authenticator := &GuestLinkAuthenticator{ + Logger: log.NewLogger(), + Config: GuestLinkAuthConfig{ + CookieName: "__Host-oc_guest_session", + }, + RevaGatewaySelector: pool.GetSelector[gateway.GatewayAPIClient]( + "GatewaySelector", + "eu.opencloud.api.gateway", + func(cc grpc.ClientConnInterface) gateway.GatewayAPIClient { + return mockGatewayClientWithInnerError{ + innerError: expiredInnerError, + } + }, + ), + } + + req := httptest.NewRequest(http.MethodGet, testEndpoint, http.NoBody) + req.AddCookie(getCookie("expired-jwt")) + + result := authenticator.Authenticate(req) + + if result.State != AuthenticationFailed { + t.Errorf("expected AuthenticationFailed, got %v", result.State) + } + if !result.Terminal { + t.Error("expected Terminal to be true for expired session") + } + if result.ErrorDetails == nil { + t.Error("expected ErrorDetails for expired session") + return + } + details, ok := result.ErrorDetails.(GuestSessionExpiredDetails) + if !ok { + t.Errorf("expected GuestSessionExpiredDetails, got %T", result.ErrorDetails) + return + } + if details.PermissionID != "share-abc-123" { + t.Errorf("expected permissionId 'share-abc-123', got '%s'", details.PermissionID) + } + if len(result.CookiesToClear) != 1 || result.CookiesToClear[0] != "__Host-oc_guest_session" { + t.Errorf("expected cookie name '__Host-oc_guest_session' in CookiesToClear, got %v", result.CookiesToClear) + } +} + +func TestGuestLinkAuthenticator_GenericUnauthenticated(t *testing.T) { + pool.RemoveSelector("GatewaySelector" + "eu.opencloud.api.gateway") + + authenticator := &GuestLinkAuthenticator{ + Logger: log.NewLogger(), + Config: GuestLinkAuthConfig{ + CookieName: "__Host-oc_guest_session", + }, + RevaGatewaySelector: pool.GetSelector[gateway.GatewayAPIClient]( + "GatewaySelector", + "eu.opencloud.api.gateway", + func(cc grpc.ClientConnInterface) gateway.GatewayAPIClient { + return mockGatewayClient{ + AuthenticateFunc: func(authType, clientID, clientSecret string) (string, rpcv1beta1.Code) { + return "", rpcv1beta1.Code_CODE_UNAUTHENTICATED + }, + } + }, + ), + } + + req := httptest.NewRequest(http.MethodGet, testEndpoint, http.NoBody) + req.AddCookie(getCookie("invalid-jwt")) + + result := authenticator.Authenticate(req) + + if result.State != AuthenticationFailed { + t.Errorf("expected AuthenticationFailed, got %v", result.State) + } + if !result.Terminal { + t.Error("expected Terminal to be true for invalid credentials") + } + if result.ErrorDetails != nil { + t.Error("expected no ErrorDetails for generic unauthenticated") + } + if len(result.CookiesToClear) != 1 || result.CookiesToClear[0] != "__Host-oc_guest_session" { + t.Errorf("expected cookie name '__Host-oc_guest_session' in CookiesToClear, got %v", result.CookiesToClear) + } +} + +func TestGuestLinkAuthenticator_Unavailable(t *testing.T) { + pool.RemoveSelector("GatewaySelector" + "eu.opencloud.api.gateway") + + authenticator := &GuestLinkAuthenticator{ + Logger: log.NewLogger(), + Config: GuestLinkAuthConfig{ + CookieName: "__Host-oc_guest_session", + }, + RevaGatewaySelector: pool.GetSelector[gateway.GatewayAPIClient]( + "GatewaySelector", + "eu.opencloud.api.gateway", + func(cc grpc.ClientConnInterface) gateway.GatewayAPIClient { + return mockGatewayClient{ + AuthenticateFunc: func(authType, clientID, clientSecret string) (string, rpcv1beta1.Code) { + return "", rpcv1beta1.Code_CODE_UNAVAILABLE + }, + } + }, + ), + } + + req := httptest.NewRequest(http.MethodGet, testEndpoint, http.NoBody) + req.AddCookie(getCookie("jwt-token")) + + result := authenticator.Authenticate(req) + + if result.State != AuthenticationError { + t.Errorf("expected AuthenticationError, got %v", result.State) + } +} + +func TestGuestLinkAuthenticator_ChallengeSuppression(t *testing.T) { + authenticator := &GuestLinkAuthenticator{ + Config: GuestLinkAuthConfig{ + CookieName: "__Host-oc_guest_session", + }, + } + + reqNoCookie := httptest.NewRequest(http.MethodGet, testEndpoint, http.NoBody) + if authenticator.SuppressAuthenticationChallenge(reqNoCookie) { + t.Error("expected challenge not to be suppressed when no cookie present") + } + + reqWithCookie := httptest.NewRequest(http.MethodGet, testEndpoint, http.NoBody) + reqWithCookie.AddCookie(getCookie("some-jwt")) + if !authenticator.SuppressAuthenticationChallenge(reqWithCookie) { + t.Error("expected challenge to be suppressed when cookie present") + } +} + +func TestGuestLinkAuthenticator_JwtNotLogged(t *testing.T) { + // This test verifies that the raw JWT is not included in any logged fields. + // We can't easily capture log output, but we can verify the authenticator + // structure doesn't store the JWT. + authenticator := &GuestLinkAuthenticator{ + Config: GuestLinkAuthConfig{ + CookieName: "__Host-oc_guest_session", + }, + } + + _ = authenticator // Just verify it compiles without storing raw JWT +} + +func TestGuestLinkAuthenticator_GatewayCall(t *testing.T) { + pool.RemoveSelector("GatewaySelector" + "eu.opencloud.api.gateway") + + var lastAuthType, lastClientID, lastClientSecret string + authenticator := &GuestLinkAuthenticator{ + Logger: log.NewLogger(), + Config: GuestLinkAuthConfig{ + CookieName: "__Host-oc_guest_session", + }, + RevaGatewaySelector: pool.GetSelector[gateway.GatewayAPIClient]( + "GatewaySelector", + "eu.opencloud.api.gateway", + func(cc grpc.ClientConnInterface) gateway.GatewayAPIClient { + return mockGatewayClient{ + AuthenticateFunc: func(authType, clientID, clientSecret string) (string, rpcv1beta1.Code) { + lastAuthType = authType + lastClientID = clientID + lastClientSecret = clientSecret + return "", rpcv1beta1.Code_CODE_UNAUTHENTICATED + }, + } + }, + ), + } + + req := httptest.NewRequest(http.MethodGet, testEndpoint, http.NoBody) + req.AddCookie(getCookie("test-jwt-value")) + + authenticator.Authenticate(req) + + if lastAuthType != "guestlinks" { + t.Errorf("expected auth type 'guestlinks', got '%s'", lastAuthType) + } + if lastClientID != "" { + t.Errorf("expected empty client_id, got '%s'", lastClientID) + } + if lastClientSecret != "test-jwt-value" { + t.Errorf("expected client_secret 'test-jwt-value', got '%s'", lastClientSecret) + } +} + +// Helper test doubles. + +// mockGatewayClientWithUser is a test double that can return a user with the response. +type mockGatewayClientWithUser struct { + gatewayv1beta1.GatewayAPIClient + AuthenticateFunc func(authType, clientID, clientSecret string) (string, *userpb.User, rpcv1beta1.Code) +} + +func (c mockGatewayClientWithUser) Authenticate(ctx context.Context, in *gatewayv1beta1.AuthenticateRequest, opts ...grpc.CallOption) (*gatewayv1beta1.AuthenticateResponse, error) { + token, user, code := c.AuthenticateFunc(in.GetType(), in.GetClientId(), in.GetClientSecret()) + return &gatewayv1beta1.AuthenticateResponse{ + Status: &rpcv1beta1.Status{Code: code}, + Token: token, + User: user, + }, nil +} + +// mockGatewayClientWithInnerError is a test double that returns a status with InnerError. +type mockGatewayClientWithInnerError struct { + gatewayv1beta1.GatewayAPIClient + innerError *typesv1beta1.OpaqueEntry +} + +func (c mockGatewayClientWithInnerError) Authenticate(ctx context.Context, in *gatewayv1beta1.AuthenticateRequest, opts ...grpc.CallOption) (*gatewayv1beta1.AuthenticateResponse, error) { + return &gatewayv1beta1.AuthenticateResponse{ + Status: &rpcv1beta1.Status{ + Code: rpcv1beta1.Code_CODE_UNAUTHENTICATED, + InnerError: c.innerError, + }, + }, nil +} + +func TestParseInnerError(t *testing.T) { + tests := []struct { + name string + innerError *typesv1beta1.OpaqueEntry + wantExpired bool + wantShareID string + }{ + { + name: "nil inner error", + innerError: nil, + wantExpired: false, + }, + { + name: "empty value", + innerError: &typesv1beta1.OpaqueEntry{Decoder: "json", Value: []byte{}}, + wantExpired: false, + }, + { + name: "wrong decoder", + innerError: &typesv1beta1.OpaqueEntry{Decoder: "xml", Value: []byte(`{"type":"opencloud_guest_link_error"}`)}, + wantExpired: false, + }, + { + name: "malformed JSON", + innerError: &typesv1beta1.OpaqueEntry{Decoder: "json", Value: []byte(`not json`)}, + wantExpired: false, + }, + { + name: "wrong type", + innerError: &typesv1beta1.OpaqueEntry{Decoder: "json", Value: []byte(`{"type":"wrong_type","reason":"session_expired","share_id":"s1"}`)}, + wantExpired: false, + }, + { + name: "wrong reason", + innerError: &typesv1beta1.OpaqueEntry{Decoder: "json", Value: []byte(`{"type":"opencloud_guest_link_error","reason":"wrong_reason","share_id":"s1"}`)}, + wantExpired: false, + }, + { + name: "empty share ID", + innerError: &typesv1beta1.OpaqueEntry{Decoder: "json", Value: []byte(`{"type":"opencloud_guest_link_error","reason":"session_expired","share_id":""}`)}, + wantExpired: false, + }, + { + name: "valid expired session", + innerError: &typesv1beta1.OpaqueEntry{Decoder: "json", Value: []byte(`{"type":"opencloud_guest_link_error","reason":"session_expired","share_id":"share-abc-123"}`)}, + wantExpired: true, + wantShareID: "share-abc-123", + }, + { + name: "share ID too long", + innerError: &typesv1beta1.OpaqueEntry{Decoder: "json", Value: []byte(`{"type":"opencloud_guest_link_error","reason":"session_expired","share_id":"` + strings.Repeat("x", 513) + `"}`)}, + wantExpired: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + authenticator := &GuestLinkAuthenticator{} + expired, shareID := authenticator.parseInnerError(tt.innerError) + if expired != tt.wantExpired { + t.Errorf("want expired=%v, got=%v", tt.wantExpired, expired) + } + if shareID != tt.wantShareID { + t.Errorf("want shareID=%q, got=%q", tt.wantShareID, shareID) + } + }) + } +} + +func TestClearGuestCookie(t *testing.T) { + cookie := clearGuestCookie("test-cookie") + + if cookie.Name != "test-cookie" { + t.Errorf("expected name 'test-cookie', got '%s'", cookie.Name) + } + if cookie.Value != "" { + t.Errorf("expected empty value, got '%s'", cookie.Value) + } + if cookie.Path != "/" { + t.Errorf("expected path '/', got '%s'", cookie.Path) + } + if cookie.Domain != "" { + t.Errorf("expected empty domain, got '%s'", cookie.Domain) + } + if !cookie.Secure { + t.Error("expected Secure to be true") + } + if !cookie.HttpOnly { + t.Error("expected HttpOnly to be true") + } + if cookie.SameSite != http.SameSiteStrictMode { + t.Errorf("expected SameSite=Strict, got %v", cookie.SameSite) + } + if cookie.MaxAge != -1 { + t.Errorf("expected MaxAge=-1, got %d", cookie.MaxAge) + } +} diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index 03c663c193..17f5fc7871 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -204,17 +204,17 @@ func (m OIDCAuthenticator) shouldServe(req *http.Request) bool { } // Authenticate implements the authenticator interface to authenticate requests via oidc auth. -func (m *OIDCAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { +func (m *OIDCAuthenticator) Authenticate(r *http.Request) AuthenticationResult { // there is no bearer token on the request, if !m.shouldServe(r) { // The authentication of public path requests is handled by another authenticator. // Since we can't guarantee the order of execution of the authenticators, we better // implement an early return here for paths we can't authenticate in this authenticator. - return nil, false + return NotApplicable() } token := strings.TrimPrefix(r.Header.Get(_headerAuthorization), _bearerPrefix) if token == "" { - return nil, false + return NotApplicable() } claims, newSession, err := m.getClaims(token, r) @@ -239,7 +239,7 @@ func (m *OIDCAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) Str("network.peer.address", host). Str("network.peer.port", port). Msg("failed to authenticate the request") - return nil, false + return FailedWithErr(err) } m.Logger.Debug(). Str("authenticator", "oidc"). @@ -251,5 +251,5 @@ func (m *OIDCAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) ctx = oidc.NewContextSessionFlag(ctx, true) } - return r.WithContext(oidc.NewContext(ctx, claims)), true + return Succeeded(r.WithContext(oidc.NewContext(ctx, claims))) } diff --git a/services/proxy/pkg/middleware/oidc_auth_test.go b/services/proxy/pkg/middleware/oidc_auth_test.go index 2d77ab02cc..0e04d4d182 100644 --- a/services/proxy/pkg/middleware/oidc_auth_test.go +++ b/services/proxy/pkg/middleware/oidc_auth_test.go @@ -68,44 +68,44 @@ var _ = Describe("Authenticating requests", Label("OIDCAuthenticator"), func() { req := httptest.NewRequest(http.MethodGet, "http://example.com/example/path", http.NoBody) req.Header.Set(_headerAuthorization, "Bearer jwt.token.sig") - req2, valid := authenticator.Authenticate(req) + result := authenticator.Authenticate(req) - Expect(valid).To(Equal(true)) - Expect(req2).ToNot(BeNil()) + Expect(result.State).To(Equal(AuthenticationSucceeded)) + Expect(result.Request).ToNot(BeNil()) }) It("should successfully authenticate", func() { req := httptest.NewRequest(http.MethodGet, "http://example.com/dav/public-files", http.NoBody) req.Header.Set(_headerAuthorization, "Bearer jwt.token.sig") - req2, valid := authenticator.Authenticate(req) + result := authenticator.Authenticate(req) - Expect(valid).To(Equal(true)) - Expect(req2).ToNot(BeNil()) + Expect(result.State).To(Equal(AuthenticationSucceeded)) + Expect(result.Request).ToNot(BeNil()) }) It("should skip authenticate if the header ShareToken is set", func() { req := httptest.NewRequest(http.MethodGet, "http://example.com/dav/public-files/", http.NoBody) req.Header.Set(_headerAuthorization, "Bearer jwt.token.sig") req.Header.Set(headerShareToken, "sharetoken") - req2, valid := authenticator.Authenticate(req) + result := authenticator.Authenticate(req) // TODO Should the authentication of public path requests is handled by another authenticator? - //Expect(valid).To(Equal(false)) - //Expect(req2).To(BeNil()) - Expect(valid).To(Equal(true)) - Expect(req2).ToNot(BeNil()) + //Expect(result.State).To(Equal(AuthenticationNotApplicable)) + //Expect(result.Request).To(BeNil()) + Expect(result.State).To(Equal(AuthenticationSucceeded)) + Expect(result.Request).ToNot(BeNil()) }) It("should skip authenticate if the 'public-token' is set", func() { req := httptest.NewRequest(http.MethodGet, "http://example.com/dav/public-files/?public-token=sharetoken", http.NoBody) req.Header.Set(_headerAuthorization, "Bearer jwt.token.sig") - req2, valid := authenticator.Authenticate(req) + result := authenticator.Authenticate(req) // TODO Should the authentication of public path requests is handled by another authenticator? - //Expect(valid).To(Equal(false)) - //Expect(req2).To(BeNil()) - Expect(valid).To(Equal(true)) - Expect(req2).ToNot(BeNil()) + //Expect(result.State).To(Equal(AuthenticationNotApplicable)) + //Expect(result.Request).To(BeNil()) + Expect(result.State).To(Equal(AuthenticationSucceeded)) + Expect(result.Request).ToNot(BeNil()) }) }) @@ -114,7 +114,7 @@ var _ = Describe("Authenticating requests", Label("OIDCAuthenticator"), func() { expiredAuthenticator := &OIDCAuthenticator{ OIDCIss: "http://idp.example.com", Logger: log.Logger{zerolog.New(&buf).Level(zerolog.DebugLevel)}, - oidcClient: &oidcmocks.OIDCClient{}, + oidcClient: &oidcmocks.OIDCClient{}, userInfoCache: store.NewMemoryStore(), skipUserInfo: true, } @@ -129,10 +129,9 @@ var _ = Describe("Authenticating requests", Label("OIDCAuthenticator"), func() { req := httptest.NewRequest(http.MethodGet, "http://example.com/example/path", http.NoBody) req.Header.Set(_headerAuthorization, "Bearer jwt.token.sig") - req2, valid := expiredAuthenticator.Authenticate(req) + result := expiredAuthenticator.Authenticate(req) - Expect(valid).To(Equal(false)) - Expect(req2).To(BeNil()) + Expect(result.State).To(Equal(AuthenticationFailed)) }) It("should log the authentication failure at debug level", func() { @@ -152,7 +151,7 @@ var _ = Describe("Authenticating requests", Label("OIDCAuthenticator"), func() { failingAuthenticator := &OIDCAuthenticator{ OIDCIss: "http://idp.example.com", Logger: log.Logger{zerolog.New(&buf).Level(zerolog.DebugLevel)}, - oidcClient: &oidcmocks.OIDCClient{}, + oidcClient: &oidcmocks.OIDCClient{}, userInfoCache: store.NewMemoryStore(), skipUserInfo: true, } diff --git a/services/proxy/pkg/middleware/options.go b/services/proxy/pkg/middleware/options.go index 7e57d13ba2..0797cd7e42 100644 --- a/services/proxy/pkg/middleware/options.go +++ b/services/proxy/pkg/middleware/options.go @@ -80,8 +80,10 @@ type Options struct { // tenant ID in the OIDC claims via the gateway's TenantAPI before comparing it to the user's stored tenant ID. TenantIDMappingEnabled bool // ServiceAccount holds credentials used to authenticate internal service calls (e.g. TenantAPI lookups). - ServiceAccount config.ServiceAccount - EventsPublisher events.Publisher + ServiceAccount config.ServiceAccount + EventsPublisher events.Publisher + // GuestLinkAuth configures guest-link cookie authentication. + GuestLinkAuth config.GuestLinkAuth } // newOptions initializes the available default options. @@ -285,3 +287,10 @@ func EventsPublisher(ep events.Publisher) Option { o.EventsPublisher = ep } } + +// GuestLinkAuth sets the guest-link authentication configuration. +func GuestLinkAuth(cfg config.GuestLinkAuth) Option { + return func(o *Options) { + o.GuestLinkAuth = cfg + } +} diff --git a/services/proxy/pkg/middleware/public_share_auth.go b/services/proxy/pkg/middleware/public_share_auth.go index 047a332527..8cc87286e5 100644 --- a/services/proxy/pkg/middleware/public_share_auth.go +++ b/services/proxy/pkg/middleware/public_share_auth.go @@ -59,9 +59,9 @@ func isPublicWithShareToken(r *http.Request) bool { } // Authenticate implements the authenticator interface to authenticate requests via public share auth. -func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { +func (a PublicShareAuthenticator) Authenticate(r *http.Request) AuthenticationResult { if !isPublicPath(r.URL.Path) && !isPublicShareArchive(r) && !isPublicShareAppOpen(r) { - return nil, false + return NotApplicable() } query := r.URL.Query() @@ -73,7 +73,7 @@ func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, if shareToken == "" { // If the share token is not set then we don't need to inject the user to // the request context so we can just continue with the request. - return r, true + return Succeeded(r) } var sharePassword string @@ -81,7 +81,7 @@ func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, expiration := query.Get(_paramExpiration) if expiration == "" { a.Logger.Warn().Str("signature", signature).Msg("cannot do signature auth without the expiration") - return nil, false + return Failed() } sharePassword = strings.Join([]string{"signature", signature, expiration}, "|") } else { @@ -102,7 +102,7 @@ func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, Str("public_share_token", shareToken). Str("path", r.URL.Path). Msg("could not select next gateway client") - return nil, false + return AuthenticationErrorResult(err) } authResp, err := client.Authenticate(r.Context(), &gateway.AuthenticateRequest{ @@ -118,7 +118,7 @@ func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, Str("public_share_token", shareToken). Str("path", r.URL.Path). Msg("failed to authenticate request") - return nil, false + return AuthenticationErrorResult(err) } r.Header.Add(headerRevaAccessToken, authResp.Token) @@ -129,5 +129,5 @@ func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, Str("authenticator", "public_share"). Str("path", r.URL.Path). Msg("successfully authenticated request") - return r, true + return Succeeded(r) } diff --git a/services/proxy/pkg/middleware/public_share_auth_test.go b/services/proxy/pkg/middleware/public_share_auth_test.go index 442a4c2fcb..e81f4adf3e 100644 --- a/services/proxy/pkg/middleware/public_share_auth_test.go +++ b/services/proxy/pkg/middleware/public_share_auth_test.go @@ -52,12 +52,12 @@ var _ = Describe("Authenticating requests", Label("PublicShareAuthenticator"), f req := httptest.NewRequest(http.MethodGet, "http://example.com/dav/public-files/?public-token=sharetoken", http.NoBody) req.SetBasicAuth("public", "examples3cr3t") - req2, valid := authenticator.Authenticate(req) + result := authenticator.Authenticate(req) - Expect(valid).To(Equal(true)) - Expect(req2).ToNot(BeNil()) + Expect(result.State).To(Equal(AuthenticationSucceeded)) + Expect(result.Request).ToNot(BeNil()) - h := req2.Header + h := result.Request.Header Expect(h.Get(headerRevaAccessToken)).To(Equal("exampletoken")) }) }) @@ -65,12 +65,12 @@ var _ = Describe("Authenticating requests", Label("PublicShareAuthenticator"), f It("should successfully authenticate", func() { req := httptest.NewRequest(http.MethodGet, "http://example.com/dav/public-files/?public-token=sharetoken&signature=examplesignature&expiration=exampleexpiration", http.NoBody) - req2, valid := authenticator.Authenticate(req) + result := authenticator.Authenticate(req) - Expect(valid).To(Equal(true)) - Expect(req2).ToNot(BeNil()) + Expect(result.State).To(Equal(AuthenticationSucceeded)) + Expect(result.Request).ToNot(BeNil()) - h := req2.Header + h := result.Request.Header Expect(h.Get(headerRevaAccessToken)).To(Equal("exampletoken")) }) }) @@ -79,22 +79,21 @@ var _ = Describe("Authenticating requests", Label("PublicShareAuthenticator"), f Context("using a public-token", func() { It("should successfully authenticate", func() { req := httptest.NewRequest(http.MethodGet, "http://example.com/archiver?public-token=sharetoken", http.NoBody) - req2, valid := authenticator.Authenticate(req) + result := authenticator.Authenticate(req) - Expect(valid).To(Equal(true)) - Expect(req2).ToNot(BeNil()) + Expect(result.State).To(Equal(AuthenticationSucceeded)) + Expect(result.Request).ToNot(BeNil()) - h := req2.Header + h := result.Request.Header Expect(h.Get(headerRevaAccessToken)).To(Equal("otherexampletoken")) }) }) Context("not using a public-token", func() { - It("should fail to authenticate", func() { + It("should not be applicable", func() { req := httptest.NewRequest(http.MethodGet, "http://example.com/archiver", http.NoBody) - req2, valid := authenticator.Authenticate(req) + result := authenticator.Authenticate(req) - Expect(valid).To(Equal(false)) - Expect(req2).To(BeNil()) + Expect(result.State).To(Equal(AuthenticationNotApplicable)) }) }) }) diff --git a/services/proxy/pkg/middleware/signed_url_auth.go b/services/proxy/pkg/middleware/signed_url_auth.go index 1acf93d369..02e9f4b218 100644 --- a/services/proxy/pkg/middleware/signed_url_auth.go +++ b/services/proxy/pkg/middleware/signed_url_auth.go @@ -235,17 +235,17 @@ func (m SignedURLAuthenticator) createSignature(url string, signingKey []byte) s } // Authenticate implements the authenticator interface to authenticate requests via signed URL auth. -func (m SignedURLAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { +func (m SignedURLAuthenticator) Authenticate(r *http.Request) AuthenticationResult { switch { case m.shouldServeLegacy(r): return m.authenticateLegacy(r) case m.shouldServe(r): return m.authenticate(r) } - return nil, false + return NotApplicable() } -func (m SignedURLAuthenticator) authenticate(r *http.Request) (*http.Request, bool) { +func (m SignedURLAuthenticator) authenticate(r *http.Request) AuthenticationResult { if err := m.requestMethodIsAllowed(r.Method); err != nil { m.Logger.Error(). Err(err). @@ -253,7 +253,7 @@ func (m SignedURLAuthenticator) authenticate(r *http.Request) (*http.Request, bo Str("path", r.URL.Path). Str("method", r.Method). Msg("Request method not allowed for signed urls") - return nil, false + return Failed() } u := r.URL.String() @@ -269,7 +269,7 @@ func (m SignedURLAuthenticator) authenticate(r *http.Request) (*http.Request, bo Str("path", r.URL.Path). Str("url", u). Msg("Could not verify JWT signature") - return nil, false + return FailedWithErr(err) } user, _, err := m.UserProvider.GetUserByClaims(r.Context(), "userid", userid) if err != nil { @@ -278,7 +278,7 @@ func (m SignedURLAuthenticator) authenticate(r *http.Request) (*http.Request, bo Str("authenticator", "signed_url_jwt"). Str("path", r.URL.Path). Msg("Could not get user by claim") - return nil, false + return FailedWithErr(err) } user, err = m.UserRoleAssigner.ApplyUserRole(r.Context(), user) if err != nil { @@ -287,7 +287,7 @@ func (m SignedURLAuthenticator) authenticate(r *http.Request) (*http.Request, bo Str("authenticator", "signed_url"). Str("path", r.URL.Path). Msg("Could not get user by claim") - return nil, false + return FailedWithErr(err) } ctx := revactx.ContextSetUser(r.Context(), user) r = r.WithContext(ctx) @@ -295,12 +295,12 @@ func (m SignedURLAuthenticator) authenticate(r *http.Request) (*http.Request, bo Str("authenticator", "signed_url"). Str("path", r.URL.Path). Msg("successfully authenticated request") - return r, true + return Succeeded(r) } // authenticateLegacy is a helper function to authenticate requests that use the legacy // client side signed URLs -func (m SignedURLAuthenticator) authenticateLegacy(r *http.Request) (*http.Request, bool) { +func (m SignedURLAuthenticator) authenticateLegacy(r *http.Request) AuthenticationResult { user, _, err := m.UserProvider.GetUserByClaims(r.Context(), "username", r.URL.Query().Get(_paramOCCredential)) if err != nil { m.Logger.Error(). @@ -308,7 +308,7 @@ func (m SignedURLAuthenticator) authenticateLegacy(r *http.Request) (*http.Reque Str("authenticator", "signed_url"). Str("path", r.URL.Path). Msg("Could not get user by claim") - return nil, false + return FailedWithErr(err) } user, err = m.UserRoleAssigner.ApplyUserRole(r.Context(), user) @@ -318,7 +318,7 @@ func (m SignedURLAuthenticator) authenticateLegacy(r *http.Request) (*http.Reque Str("authenticator", "signed_url"). Str("path", r.URL.Path). Msg("Could not get user by claim") - return nil, false + return FailedWithErr(err) } ctx := revactx.ContextSetUser(r.Context(), user) @@ -332,12 +332,12 @@ func (m SignedURLAuthenticator) authenticateLegacy(r *http.Request) (*http.Reque Str("path", r.URL.Path). Str("url", r.URL.String()). Msg("Could not get user by claim") - return nil, false + return FailedWithErr(err) } m.Logger.Debug(). Str("authenticator", "signed_url"). Str("path", r.URL.Path). Msg("successfully authenticated request") - return r, true + return Succeeded(r) } diff --git a/services/proxy/pkg/middleware/signed_url_auth_test.go b/services/proxy/pkg/middleware/signed_url_auth_test.go index f16be89d5f..5f5279f8e8 100644 --- a/services/proxy/pkg/middleware/signed_url_auth_test.go +++ b/services/proxy/pkg/middleware/signed_url_auth_test.go @@ -92,7 +92,8 @@ func TestSignedURLAuth_authenticateRejectsDisallowedMethods(t *testing.T) { for _, method := range []string{http.MethodPost, http.MethodPut, http.MethodDelete, "PROPFIND", "MOVE"} { r := httptest.NewRequest(method, signed, nil) - if _, ok := pua.authenticate(r); ok { + result := pua.authenticate(r) + if result.State == AuthenticationSucceeded { t.Errorf("expected %s with a signed url to be rejected", method) } } diff --git a/services/proxy/pkg/middleware/terminal_failure_test.go b/services/proxy/pkg/middleware/terminal_failure_test.go new file mode 100644 index 0000000000..8039d0c119 --- /dev/null +++ b/services/proxy/pkg/middleware/terminal_failure_test.go @@ -0,0 +1,87 @@ +package middleware + +import ( + "encoding/json" + "encoding/xml" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// hostileShareID would break out of an XML/JSON document if it were not escaped. +const hostileShareID = `&'` + +func TestRenderTerminalFailure_Generic(t *testing.T) { + t.Run("non-dav request", func(t *testing.T) { + rr := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/graph/v1beta1/me/drive/sharedWithMe", nil) + + require.NoError(t, renderTerminalFailure(rr, req, TerminalFailed())) + + assert.Equal(t, http.StatusUnauthorized, rr.Code) + assert.Empty(t, rr.Body.String()) + }) + + t.Run("dav request", func(t *testing.T) { + rr := httptest.NewRecorder() + req := httptest.NewRequest("PROPFIND", "/dav/spaces/abc", nil) + + require.NoError(t, renderTerminalFailure(rr, req, TerminalFailed())) + + assert.Equal(t, http.StatusUnauthorized, rr.Code) + assert.Equal(t, "application/xml; charset=utf-8", rr.Header().Get("Content-Type")) + assert.True(t, strings.HasPrefix(rr.Body.String(), xml.Header)) + assert.Contains(t, rr.Body.String(), "Sabre\\DAV\\Exception\\NotAuthenticated") + }) +} + +func TestRenderTerminalFailure_GuestSessionExpiredJSON(t *testing.T) { + rr := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/graph/v1beta1/me/drive/sharedWithMe", nil) + + require.NoError(t, renderTerminalFailure(rr, req, AuthenticationResult{ + State: AuthenticationFailed, + Terminal: true, + ErrorDetails: GuestSessionExpiredDetails{PermissionID: hostileShareID}, + })) + + assert.Equal(t, http.StatusUnauthorized, rr.Code) + assert.Equal(t, "application/json", rr.Header().Get("Content-Type")) + + var body struct { + ErrorType string `json:"error_type"` + PermissionID string `json:"permissionId"` + } + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body)) + assert.Equal(t, "session_expired", body.ErrorType) + assert.Equal(t, hostileShareID, body.PermissionID) + // json.Encoder escapes <, > and & so the body is safe even if it were rendered as HTML + assert.NotContains(t, rr.Body.String(), "