From 24545be7ade75e33bfea6f5031d53eb23760931c Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Tue, 8 Sep 2026 10:39:39 +0200 Subject: [PATCH] fix(collaboration): harden wopi token handling --- .../pkg/connector/fileconnector_test.go | 17 ++- .../pkg/middleware/wopicontext.go | 38 +++++- .../pkg/middleware/wopicontext_test.go | 117 +++++++++++++++++- .../pkg/service/grpc/v0/service_test.go | 14 ++- 4 files changed, 169 insertions(+), 17 deletions(-) diff --git a/services/collaboration/pkg/connector/fileconnector_test.go b/services/collaboration/pkg/connector/fileconnector_test.go index 48c368133a..cd01123a29 100644 --- a/services/collaboration/pkg/connector/fileconnector_test.go +++ b/services/collaboration/pkg/connector/fileconnector_test.go @@ -8,6 +8,7 @@ import ( "path" "regexp" "strings" + "time" appproviderv1beta1 "github.com/cs3org/go-cs3apis/cs3/app/provider/v1beta1" auth "github.com/cs3org/go-cs3apis/cs3/auth/provider/v1beta1" @@ -17,6 +18,7 @@ import ( link "github.com/cs3org/go-cs3apis/cs3/sharing/link/v1beta1" providerv1beta1 "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" + "github.com/golang-jwt/jwt/v5" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/opencloud-eu/opencloud/pkg/shared" @@ -67,10 +69,19 @@ var _ = Describe("FileConnector", func() { gatewaySelector.On("Next").Return(gatewayClient, nil) fc = connector.NewFileConnector(gatewaySelector, cfg, nil, nil) + // PutRelativeFile mints a wopi token from this one and verifies it + now := time.Now() + accessToken, err := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{ + "aud": "reva", + "iss": "https://cloud.opencloud.test", + "iat": now.Unix(), + "exp": now.Add(5 * time.Hour).Unix(), + "sub": "alice", + }).SignedString([]byte(cfg.TokenManager.JWTSecret)) + Expect(err).ToNot(HaveOccurred()) + wopiCtx = middleware.WopiContext{ - // a real token is needed for the PutRelativeFileSuggested tests - // although we aren't checking anything inside the token - AccessToken: "eyJhbGciOiJQUzI1NiIsImtpZCI6InByaXZhdGUta2V5IiwidHlwIjoiSldUIn0.eyJhdWQiOiJ3ZWIiLCJleHAiOjE3MjAwOTIyODAsImlhdCI6MTcyMDA5MTk4MCwiaXNzIjoiaHR0cHM6Ly9vY2lzLmpwLnNvbGlkZ2Vhci5wcnYiLCJqdGkiOiJmQldpN0FYaFFQdUhhaDJDV0VQVFFLcENmZ3BGbEFpTCIsImxnLmkiOnsiZG4iOiJicm8iLCJpZCI6Im93bkNsb3VkVVVJRD1mYWYxMTY0Ny03NDUxLTRiOWEtYmZmZS0zYjVkZGNjNTk3MmIiLCJ1biI6ImJyb3RhdG8ifSwibGcucCI6ImlkZW50aWZpZXItbGRhcCIsImxnLnQiOiIxIiwic2NwIjoib3BlbmlkIHByb2ZpbGUgZW1haWwiLCJzdWIiOiJjQXZ1elg4Z1hMWmRpWHgtQDFOV1RKdENQRHFVSjQ0bnQ0NkZ0RDlwNUw3dGplUEZkWk1WSjlFMzBOeDItZHVpN0hLQ0x4QWlXYUNUdGJYNTExSmNkSHcifQ.StpQpE4ipxk8Nhk6xgob1Tovbk6bcUVs5-fkej2hIoKoJKfR2OY-CiFQ3wwgEcFro8notxeVfOmxs36z_ezFeJBZRbxpSggcr77LFtQwlsWvD5AuAgLZN1otdvULehunXE_DtxRJZ1rqnsOBT03zKOZLx8Q7QTy6DeRuf1KQtCIowa9D4ymPM4TTmtQdiW2XjByO3OCLFEMVBfDFGPibR6gMnftGQ5kfiZGDTUVCauEXwE-msZVZ42QY-wFRppX_RIL1Z0p6T4dr_6_y-VM1lNYJ5-dB5c5rg_c03Xu1y_TIxs31-8--dtUyZmBVOZFk8bB9msNk-iaOEjzKeUZLymo_-2qVYvXxzNrkq1QA8luaLR6jec_CRT2P8wsB2nyebFU6_myKe34m6f8uqGhOzcOwPB4TpoxPx4ucQgo1CQJwQZHZsZ7Q6TVYZUXJdWwzzMuvJXmnn36iybw0Ub6On4sGKj3gHetjoJg8VnL-TQkBvf1iHX2ktRG3Nq2rnPrB2OTpi2rLpleWg_s8Y8FXxIgYqM0JG8kO1n5RPGMeYQG7qd6f9wdcaPIvgxCa_HsZtMr7eGcDzZtxp-NivgJOS6ode0ZAJ3wGU-AVhmyshpds3DFECcvkBcP_4dD52AXiAq9X3UVkVdNsxs_yB9P7zBcdsKsD6QDJv5gf-6DEu34", + AccessToken: accessToken, FileReference: &providerv1beta1.Reference{ ResourceId: &providerv1beta1.ResourceId{ StorageId: "abc", diff --git a/services/collaboration/pkg/middleware/wopicontext.go b/services/collaboration/pkg/middleware/wopicontext.go index 389ebd7704..44153f5fca 100644 --- a/services/collaboration/pkg/middleware/wopicontext.go +++ b/services/collaboration/pkg/middleware/wopicontext.go @@ -91,8 +91,7 @@ func WopiContextAuthMiddleware(cfg *config.Config, st microstore.Store, next htt claims := &Claims{} _, err := jwt.ParseWithClaims(accessToken, claims, func(token *jwt.Token) (any, error) { - - if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok { + if token.Method != jwt.SigningMethodHS256 { return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"]) } @@ -129,6 +128,17 @@ func WopiContextAuthMiddleware(cfg *config.Config, st microstore.Store, next htt claims.WopiContext.AccessToken = wopiContextAccessToken + // encrypted in GenerateWopiToken, only set for view only shares + if claims.WopiContext.ViewOnlyToken != "" { + wopiContextViewOnlyToken, err := DecryptAES([]byte(cfg.Wopi.Secret), claims.WopiContext.ViewOnlyToken) + if err != nil { + wopiLogger.Error().Err(err).Msg("failed to decrypt view only token") + http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) + return + } + claims.WopiContext.ViewOnlyToken = wopiContextViewOnlyToken + } + ctx = context.WithValue(ctx, wopiContextKey, claims.WopiContext) // authentication for the CS3 api ctx = metadata.AppendToOutgoingContext(ctx, ctxpkg.TokenHeader, claims.WopiContext.AccessToken) @@ -203,9 +213,24 @@ func GenerateWopiToken(wopiContext WopiContext, cfg *config.Config, st microstor return "", 0, err } + // impersonates the owner, and the wopi token payload is readable + if wopiContext.ViewOnlyToken != "" { + cryptedViewOnlyToken, err := EncryptAES([]byte(cfg.Wopi.Secret), wopiContext.ViewOnlyToken) + if err != nil { + return "", 0, err + } + wopiContext.ViewOnlyToken = cryptedViewOnlyToken + } + + // the wopi token inherits this expiry, so verify instead of just decoding cs3Claims := &jwt.RegisteredClaims{} - cs3JWTparser := jwt.Parser{} - _, _, err = cs3JWTparser.ParseUnverified(wopiContext.AccessToken, cs3Claims) + _, err = jwt.ParseWithClaims(wopiContext.AccessToken, cs3Claims, func(token *jwt.Token) (any, error) { + if token.Method != jwt.SigningMethodHS256 { + return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"]) + } + + return []byte(cfg.TokenManager.JWTSecret), nil + }) if err != nil { return "", 0, err } @@ -249,7 +274,10 @@ func parseWopiFileID(cfg *config.Config, path string) string { } // check if the fileid is a jwt if strings.Contains(s[3], ".") { - token, err := jwt.Parse(s[3], func(_ *jwt.Token) (any, error) { + token, err := jwt.Parse(s[3], func(token *jwt.Token) (any, error) { + if token.Method != jwt.SigningMethodHS256 { + return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"]) + } return []byte(cfg.Wopi.ProxySecret), nil }) if err != nil { diff --git a/services/collaboration/pkg/middleware/wopicontext_test.go b/services/collaboration/pkg/middleware/wopicontext_test.go index 82eaafbfee..b50ae9b2ef 100644 --- a/services/collaboration/pkg/middleware/wopicontext_test.go +++ b/services/collaboration/pkg/middleware/wopicontext_test.go @@ -2,15 +2,19 @@ package middleware_test import ( "context" + "encoding/base64" "net/http" "net/http/httptest" "net/url" "path" "strconv" + "strings" + "time" appprovider "github.com/cs3org/go-cs3apis/cs3/app/provider/v1beta1" userv1beta1 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" providerv1beta1 "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/golang-jwt/jwt/v5" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/opencloud-eu/opencloud/services/collaboration/pkg/config" @@ -29,6 +33,7 @@ var _ = Describe("Wopi Context Middleware", func() { rid *providerv1beta1.ResourceId tknMngr token.Manager user *userv1beta1.User + owner *userv1beta1.User src *url.URL ) @@ -65,6 +70,16 @@ var _ = Describe("Wopi Context Middleware", func() { Mail: "admin@example.com", } + owner = &userv1beta1.User{ + Id: &userv1beta1.UserId{ + Idp: "example.com", + OpaqueId: "67890", + Type: userv1beta1.UserType_USER_TYPE_PRIMARY, + }, + Username: "alice", + Mail: "alice@example.com", + } + rid = &providerv1beta1.ResourceId{ StorageId: "storageID", OpaqueId: "opaqueID", @@ -130,9 +145,12 @@ var _ = Describe("Wopi Context Middleware", func() { AccessToken: token, } // use wrong wopi secret when generating the wopi token - wopiToken, ttl, err := middleware.GenerateWopiToken(wopiContext, &config.Config{Wopi: config.Wopi{ - Secret: "wrongSecret", - }}, nil) + wopiToken, ttl, err := middleware.GenerateWopiToken(wopiContext, &config.Config{ + TokenManager: &config.TokenManager{JWTSecret: cfg.TokenManager.JWTSecret}, + Wopi: config.Wopi{ + Secret: "wrongSecret", + }, + }, nil) q := req.URL.Query() q.Add("access_token", wopiToken) q.Add("access_token_ttl", strconv.FormatInt(ttl, 10)) @@ -287,4 +305,97 @@ var _ = Describe("Wopi Context Middleware", func() { mw.ServeHTTP(resp, req) Expect(resp.Code).To(Equal(http.StatusOK)) }) + It("Should not carry the view only token in plaintext", func() { + accessToken, err := tknMngr.MintToken(ctx, user, nil) + Expect(err).ToNot(HaveOccurred()) + viewOnlyToken, err := tknMngr.MintToken(ctx, owner, nil) + Expect(err).ToNot(HaveOccurred()) + + wopiContext := middleware.WopiContext{ + AccessToken: accessToken, + ViewOnlyToken: viewOnlyToken, + ViewMode: appprovider.ViewMode_VIEW_MODE_VIEW_ONLY, + FileReference: &providerv1beta1.Reference{ + ResourceId: rid, + Path: ".", + }, + } + wopiToken, _, err := middleware.GenerateWopiToken(wopiContext, cfg, nil) + Expect(err).ToNot(HaveOccurred()) + + parts := strings.Split(wopiToken, ".") + Expect(parts).To(HaveLen(3)) + payload, err := base64.RawURLEncoding.DecodeString(parts[1]) + Expect(err).ToNot(HaveOccurred()) + Expect(string(payload)).ToNot(ContainSubstring(viewOnlyToken)) + }) + It("Should hand the decrypted view only token to the next handler", func() { + accessToken, err := tknMngr.MintToken(ctx, user, nil) + Expect(err).ToNot(HaveOccurred()) + viewOnlyToken, err := tknMngr.MintToken(ctx, owner, nil) + Expect(err).ToNot(HaveOccurred()) + + var seen middleware.WopiContext + var seenErr error + capturing := middleware.WopiContextAuthMiddleware(cfg, nil, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + seen, seenErr = middleware.WopiContextFromCtx(r.Context()) + w.WriteHeader(http.StatusOK) + })) + + wopiContext := middleware.WopiContext{ + AccessToken: accessToken, + ViewOnlyToken: viewOnlyToken, + ViewMode: appprovider.ViewMode_VIEW_MODE_VIEW_ONLY, + FileReference: &providerv1beta1.Reference{ + ResourceId: rid, + Path: ".", + }, + } + wopiToken, ttl, err := middleware.GenerateWopiToken(wopiContext, cfg, nil) + Expect(err).ToNot(HaveOccurred()) + + req := httptest.NewRequest("GET", src.String(), nil).WithContext(ctx) + q := req.URL.Query() + q.Add("access_token", wopiToken) + q.Add("access_token_ttl", strconv.FormatInt(ttl, 10)) + req.URL.RawQuery = q.Encode() + resp := httptest.NewRecorder() + + capturing.ServeHTTP(resp, req) + Expect(resp.Code).To(Equal(http.StatusOK)) + Expect(seenErr).ToNot(HaveOccurred()) + Expect(seen.ViewOnlyToken).To(Equal(viewOnlyToken)) + }) + It("Should not authorize a token signed with another hmac variant", func() { + accessToken, err := tknMngr.MintToken(ctx, user, nil) + Expect(err).ToNot(HaveOccurred()) + cryptedAccessToken, err := middleware.EncryptAES([]byte(cfg.Wopi.Secret), accessToken) + Expect(err).ToNot(HaveOccurred()) + + // valid apart from the algorithm + claims := &middleware.Claims{ + WopiContext: middleware.WopiContext{ + AccessToken: cryptedAccessToken, + ViewMode: appprovider.ViewMode_VIEW_MODE_READ_WRITE, + FileReference: &providerv1beta1.Reference{ + ResourceId: rid, + Path: ".", + }, + }, + RegisteredClaims: jwt.RegisteredClaims{ + ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour)), + }, + } + forged, err := jwt.NewWithClaims(jwt.SigningMethodHS512, claims).SignedString([]byte(cfg.Wopi.Secret)) + Expect(err).ToNot(HaveOccurred()) + + req := httptest.NewRequest("GET", src.String(), nil).WithContext(ctx) + q := req.URL.Query() + q.Add("access_token", forged) + req.URL.RawQuery = q.Encode() + resp := httptest.NewRecorder() + + mw.ServeHTTP(resp, req) + Expect(resp.Code).To(Equal(http.StatusUnauthorized)) + }) }) diff --git a/services/collaboration/pkg/service/grpc/v0/service_test.go b/services/collaboration/pkg/service/grpc/v0/service_test.go index 22707752e5..7a078fb435 100644 --- a/services/collaboration/pkg/service/grpc/v0/service_test.go +++ b/services/collaboration/pkg/service/grpc/v0/service_test.go @@ -75,7 +75,9 @@ var _ = Describe("Discovery", func() { ) BeforeEach(func() { - cfg = &config.Config{} + cfg = &config.Config{ + TokenManager: &config.TokenManager{JWTSecret: "jwtSecret"}, + } gatewayClient = &cs3mocks.GatewayAPIClient{} gatewaySelector := mocks.NewSelectable[gatewayv1beta1.GatewayAPIClient](GinkgoT()) @@ -175,7 +177,7 @@ var _ = Describe("Discovery", func() { Path: "/path/to/file.docx", }, ViewMode: appproviderv1beta1.ViewMode_VIEW_MODE_READ_WRITE, - AccessToken: MintToken(myself, cfg.Wopi.Secret, nowTime), + AccessToken: MintToken(myself, cfg.TokenManager.JWTSecret, nowTime), } if lang != "" { req.Opaque = utils.AppendPlainToOpaque(req.Opaque, "lang", lang) @@ -235,7 +237,7 @@ var _ = Describe("Discovery", func() { Path: "/path/to/file.docx", }, ViewMode: appproviderv1beta1.ViewMode_VIEW_MODE_READ_WRITE, - AccessToken: MintToken(myself, cfg.Wopi.Secret, nowTime), + AccessToken: MintToken(myself, cfg.TokenManager.JWTSecret, nowTime), } req.Opaque = utils.AppendPlainToOpaque(req.Opaque, "lang", "en") @@ -278,7 +280,7 @@ var _ = Describe("Discovery", func() { Path: "/path/to/file.invalid", }, ViewMode: appproviderv1beta1.ViewMode_VIEW_MODE_READ_WRITE, - AccessToken: MintToken(myself, cfg.Wopi.Secret, nowTime), + AccessToken: MintToken(myself, cfg.TokenManager.JWTSecret, nowTime), } req.Opaque = utils.AppendPlainToOpaque(req.Opaque, "lang", "en") @@ -319,7 +321,7 @@ var _ = Describe("Discovery", func() { Path: "/path/to/file.docx", }, ViewMode: appproviderv1beta1.ViewMode_VIEW_MODE_READ_WRITE, - AccessToken: MintToken(myself, cfg.Wopi.Secret, nowTime), + AccessToken: MintToken(myself, cfg.TokenManager.JWTSecret, nowTime), } req.Opaque = utils.AppendPlainToOpaque(req.Opaque, "lang", "en") req.Opaque = utils.AppendPlainToOpaque(req.Opaque, "template", "&file_id") @@ -362,7 +364,7 @@ var _ = Describe("Discovery", func() { Path: "/path/to/file.docx", }, ViewMode: appproviderv1beta1.ViewMode_VIEW_MODE_READ_WRITE, - AccessToken: MintToken(myself, cfg.Wopi.Secret, nowTime), + AccessToken: MintToken(myself, cfg.TokenManager.JWTSecret, nowTime), } req.Opaque = utils.AppendPlainToOpaque(req.Opaque, "lang", "en") req.Opaque = utils.AppendPlainToOpaque(req.Opaque, "template", "prodiderID$spaceID!opaqueID")