From 2b667003b5509de2de84d351251e5061d182bb35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Wed, 7 Feb 2024 17:13:28 +0100 Subject: [PATCH] fix: properly check expiry and verify signature of signed urls (#8383) fix: signed url expiry validation only checks for expiry and not for used before --- changelog/unreleased/fix-signed-url-expiry.md | 5 + services/proxy/pkg/command/server.go | 1 + .../proxy/pkg/middleware/signed_url_auth.go | 97 ++++++--- .../pkg/middleware/signed_url_auth_test.go | 192 ++++++++++++++---- 4 files changed, 221 insertions(+), 74 deletions(-) create mode 100644 changelog/unreleased/fix-signed-url-expiry.md diff --git a/changelog/unreleased/fix-signed-url-expiry.md b/changelog/unreleased/fix-signed-url-expiry.md new file mode 100644 index 000000000..9b0cdf3da --- /dev/null +++ b/changelog/unreleased/fix-signed-url-expiry.md @@ -0,0 +1,5 @@ +Bugfix: signed url verification + +Signed urls now expire properly + +https://github.com/owncloud/ocis/pull/8385 diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index e917f0ef5..3363f048c 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -374,6 +374,7 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config, UserProvider: userProvider, UserRoleAssigner: roleAssigner, Store: storeClient, + Now: time.Now, }) return alice.New( diff --git a/services/proxy/pkg/middleware/signed_url_auth.go b/services/proxy/pkg/middleware/signed_url_auth.go index a6bc3a703..b3c842a26 100644 --- a/services/proxy/pkg/middleware/signed_url_auth.go +++ b/services/proxy/pkg/middleware/signed_url_auth.go @@ -27,6 +27,7 @@ const ( _paramOCDate = "OC-Date" _paramOCExpires = "OC-Expires" _paramOCVerb = "OC-Verb" + _paramOCAlgo = "OC-Algo" ) var ( @@ -46,6 +47,7 @@ type SignedURLAuthenticator struct { UserProvider backend.UserBackend UserRoleAssigner userroles.UserRoleAssigner Store storesvc.StoreService + Now func() time.Time } func (m SignedURLAuthenticator) shouldServe(req *http.Request) bool { @@ -58,30 +60,30 @@ func (m SignedURLAuthenticator) shouldServe(req *http.Request) bool { func (m SignedURLAuthenticator) validate(req *http.Request) (err error) { query := req.URL.Query() - if ok, err := m.allRequiredParametersArePresent(query); !ok { + if err := m.allRequiredParametersArePresent(query); err != nil { return err } - if ok, err := m.requestMethodMatches(req.Method, query); !ok { + if err := m.requestMethodMatches(req.Method, query); err != nil { return err } - if ok, err := m.requestMethodIsAllowed(req.Method); !ok { + if err := m.requestMethodIsAllowed(req.Method); err != nil { return err } - if expired, err := m.urlIsExpired(query, time.Now); expired { + if err = m.urlIsExpired(query); err != nil { return err } - if ok, err := m.signatureIsValid(req); !ok { + if err := m.signatureIsValid(req); err != nil { return err } return nil } -func (m SignedURLAuthenticator) allRequiredParametersArePresent(query url.Values) (ok bool, err error) { +func (m SignedURLAuthenticator) allRequiredParametersArePresent(query url.Values) (err error) { // check if required query parameters exist in given request query parameters // OC-Signature - the computed signature - server will verify the request upon this REQUIRED // OC-Credential - defines the user scope (shall we use the owncloud user id here - this might leak internal data ....) REQUIRED @@ -90,23 +92,23 @@ func (m SignedURLAuthenticator) allRequiredParametersArePresent(query url.Values // TODO OC-Verb - defines for which http verb the request is valid - defaults to GET OPTIONAL for _, p := range _requiredParams { if query.Get(p) == "" { - return false, fmt.Errorf("required %s parameter not found", p) + return fmt.Errorf("required %s parameter not found", p) } } - return true, nil + return nil } -func (m SignedURLAuthenticator) requestMethodMatches(meth string, query url.Values) (ok bool, err error) { +func (m SignedURLAuthenticator) requestMethodMatches(meth string, query url.Values) (err error) { // check if given url query parameter OC-Verb matches given request method if !strings.EqualFold(meth, query.Get(_paramOCVerb)) { - return false, errors.New("required OC-Verb parameter did not match request method") + return errors.New("required OC-Verb parameter did not match request method") } - return true, nil + return nil } -func (m SignedURLAuthenticator) requestMethodIsAllowed(meth string) (ok bool, err error) { +func (m SignedURLAuthenticator) requestMethodIsAllowed(meth string) (err error) { // check if given request method is allowed methodIsAllowed := false for _, am := range m.PreSignedURLConfig.AllowedHTTPMethods { @@ -117,50 +119,81 @@ func (m SignedURLAuthenticator) requestMethodIsAllowed(meth string) (ok bool, er } if !methodIsAllowed { - return false, errors.New("request method is not listed in PreSignedURLConfig AllowedHTTPMethods") + return errors.New("request method is not listed in PreSignedURLConfig AllowedHTTPMethods") } - return true, nil + return nil } -func (m SignedURLAuthenticator) urlIsExpired(query url.Values, now func() time.Time) (expired bool, err error) { +func (m SignedURLAuthenticator) urlIsExpired(query url.Values) (err error) { // check if url is expired by checking if given date (OC-Date) + expires in seconds (OC-Expires) is after now validFrom, err := time.Parse(time.RFC3339, query.Get(_paramOCDate)) if err != nil { - return true, err + return err } requestExpiry, err := time.ParseDuration(query.Get(_paramOCExpires) + "s") if err != nil { - return true, err + return err } validTo := validFrom.Add(requestExpiry) + if !(m.Now().Before(validTo)) { + return errors.New("URL is expired") + } - return !(now().After(validFrom) && now().Before(validTo)), nil + return nil } -func (m SignedURLAuthenticator) signatureIsValid(req *http.Request) (ok bool, err error) { - u := revactx.ContextMustGetUser(req.Context()) - signingKey, err := m.getSigningKey(req.Context(), u.Id.OpaqueId) +func (m SignedURLAuthenticator) signatureIsValid(req *http.Request) (err error) { + c := revactx.ContextMustGetUser(req.Context()) + signingKey, err := m.getSigningKey(req.Context(), c.Id.OpaqueId) if err != nil { m.Logger.Error().Err(err).Msg("could not retrieve signing key") - return false, err + return err } if len(signingKey) == 0 { m.Logger.Error().Err(err).Msg("signing key empty") - return false, err + return err } - q := req.URL.Query() - signature := q.Get(_paramOCSignature) - q.Del(_paramOCSignature) - req.URL.RawQuery = q.Encode() - url := req.URL.String() - if !req.URL.IsAbs() { - url = "https://" + req.Host + url // TODO where do we get the scheme from + u := m.buildUrlToSign(req) + computedSignature := m.createSignature(u, signingKey) + signatureInURL := req.URL.Query().Get(_paramOCSignature) + if computedSignature == signatureInURL { + return nil } + return fmt.Errorf("signature mismatch: expected %s != actual %s", computedSignature, signatureInURL) +} - return m.createSignature(url, signingKey) == signature, nil +func (m SignedURLAuthenticator) buildUrlToSign(req *http.Request) string { + q := req.URL.Query() + + // only params required for signing + signParameters := make(url.Values) + signParameters.Add(_paramOCCredential, q.Get(_paramOCCredential)) + signParameters.Add(_paramOCDate, q.Get(_paramOCDate)) + signParameters.Add(_paramOCExpires, q.Get(_paramOCExpires)) + signParameters.Add(_paramOCVerb, q.Get(_paramOCVerb)) + + // remaining query params + q.Del(_paramOCAlgo) + q.Del(_paramOCCredential) + q.Del(_paramOCDate) + q.Del(_paramOCExpires) + q.Del(_paramOCSignature) + q.Del(_paramOCVerb) + + url := *req.URL + if len(q) == 0 { + url.RawQuery = signParameters.Encode() + } else { + url.RawQuery = strings.Join([]string{q.Encode(), signParameters.Encode()}, "&") + } + u := url.String() + if !url.IsAbs() { + u = "https://" + req.Host + u // TODO where do we get the scheme + } + return u } func (m SignedURLAuthenticator) createSignature(url string, signingKey []byte) string { @@ -223,10 +256,12 @@ func (m SignedURLAuthenticator) Authenticate(r *http.Request) (*http.Request, bo Err(err). Str("authenticator", "signed_url"). Str("path", r.URL.Path). + Str("url", r.URL.String()). Msg("Could not get user by claim") return nil, false } + // TODO: set user in context m.Logger.Debug(). Str("authenticator", "signed_url"). Str("path", r.URL.Path). diff --git a/services/proxy/pkg/middleware/signed_url_auth_test.go b/services/proxy/pkg/middleware/signed_url_auth_test.go index 35f84e665..a14edc01f 100644 --- a/services/proxy/pkg/middleware/signed_url_auth_test.go +++ b/services/proxy/pkg/middleware/signed_url_auth_test.go @@ -1,6 +1,14 @@ package middleware import ( + "context" + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + revactx "github.com/cs3org/reva/v2/pkg/ctx" + storemsg "github.com/owncloud/ocis/v2/protogen/gen/ocis/messages/store/v0" + v0 "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/store/v0" + "github.com/owncloud/ocis/v2/services/proxy/pkg/config" + "github.com/stretchr/testify/assert" + "go-micro.dev/v4/client" "net/http/httptest" "testing" "time" @@ -34,21 +42,23 @@ func TestSignedURLAuth_allRequiredParametersPresent(t *testing.T) { pua := SignedURLAuthenticator{} baseURL := "https://example.com/example.jpg?" tests := []struct { - params string - expected bool + params string + errorMessage string }{ - {"OC-Signature=something&OC-Credential=something&OC-Date=something&OC-Expires=something&OC-Verb=something", true}, - {"OC-Credential=something&OC-Date=something&OC-Expires=something&OC-Verb=something", false}, - {"OC-Signature=something&OC-Date=something&OC-Expires=something&OC-Verb=something", false}, - {"OC-Signature=something&OC-Credential=something&OC-Expires=something&OC-Verb=something", false}, - {"OC-Signature=something&OC-Credential=something&OC-Date=something&OC-Verb=something", false}, - {"OC-Signature=something&OC-Credential=something&OC-Date=something&OC-Expires=something", false}, + {"OC-Signature=something&OC-Credential=something&OC-Date=something&OC-Expires=something&OC-Verb=something", ""}, + {"OC-Credential=something&OC-Date=something&OC-Expires=something&OC-Verb=something", "required OC-Signature parameter not found"}, + {"OC-Signature=something&OC-Date=something&OC-Expires=something&OC-Verb=something", "required OC-Credential parameter not found"}, + {"OC-Signature=something&OC-Credential=something&OC-Expires=something&OC-Verb=something", "required OC-Date parameter not found"}, + {"OC-Signature=something&OC-Credential=something&OC-Date=something&OC-Verb=something", "required OC-Expires parameter not found"}, + {"OC-Signature=something&OC-Credential=something&OC-Date=something&OC-Expires=something", "required OC-Verb parameter not found"}, } for _, tt := range tests { r := httptest.NewRequest("", baseURL+tt.params, nil) - ok, _ := pua.allRequiredParametersArePresent(r.URL.Query()) - if ok != tt.expected { - t.Errorf("with %s expected %t got %t", tt.params, tt.expected, ok) + err := pua.allRequiredParametersArePresent(r.URL.Query()) + if tt.errorMessage != "" { + assert.EqualError(t, err, tt.errorMessage, tt.params) + } else { + assert.Nil(t, err) } } } @@ -56,20 +66,22 @@ func TestSignedURLAuth_allRequiredParametersPresent(t *testing.T) { func TestSignedURLAuth_requestMethodMatches(t *testing.T) { pua := SignedURLAuthenticator{} tests := []struct { - method string - url string - expected bool + method string + url string + errorMessage string }{ - {"GET", "https://example.com/example.jpg?OC-Verb=GET", true}, - {"GET", "https://example.com/example.jpg?OC-Verb=get", true}, - {"POST", "https://example.com/example.jpg?OC-Verb=GET", false}, + {"GET", "https://example.com/example.jpg?OC-Verb=GET", ""}, + {"GET", "https://example.com/example.jpg?OC-Verb=get", ""}, + {"POST", "https://example.com/example.jpg?OC-Verb=GET", "required OC-Verb parameter did not match request method"}, } for _, tt := range tests { r := httptest.NewRequest(tt.method, tt.url, nil) - ok, _ := pua.requestMethodMatches(r.Method, r.URL.Query()) - if ok != tt.expected { - t.Errorf("with method %s and url %s expected %t got %t", tt.method, tt.url, tt.expected, ok) + err := pua.requestMethodMatches(r.Method, r.URL.Query()) + if tt.errorMessage != "" { + assert.EqualError(t, err, tt.errorMessage, tt.url) + } else { + assert.Nil(t, err) } } } @@ -77,50 +89,62 @@ func TestSignedURLAuth_requestMethodMatches(t *testing.T) { func TestSignedURLAuth_requestMethodIsAllowed(t *testing.T) { pua := SignedURLAuthenticator{} tests := []struct { - method string - allowed []string - expected bool + method string + allowed []string + errorMessage string }{ - {"GET", []string{}, false}, - {"GET", []string{"POST"}, false}, - {"GET", []string{"GET"}, true}, - {"GET", []string{"get"}, true}, - {"GET", []string{"POST", "GET"}, true}, + {"GET", []string{}, "request method is not listed in PreSignedURLConfig AllowedHTTPMethods"}, + {"GET", []string{"POST"}, "request method is not listed in PreSignedURLConfig AllowedHTTPMethods"}, + {"GET", []string{"GET"}, ""}, + {"GET", []string{"get"}, ""}, + {"GET", []string{"POST", "GET"}, ""}, } for _, tt := range tests { pua.PreSignedURLConfig.AllowedHTTPMethods = tt.allowed - ok, _ := pua.requestMethodIsAllowed(tt.method) - - if ok != tt.expected { - t.Errorf("with method %s and allowed methods %s expected %t got %t", tt.method, tt.allowed, tt.expected, ok) + err := pua.requestMethodIsAllowed(tt.method) + if tt.errorMessage != "" { + assert.EqualError(t, err, tt.errorMessage) + } else { + assert.Nil(t, err) } } } func TestSignedURLAuth_urlIsExpired(t *testing.T) { - pua := SignedURLAuthenticator{} nowFunc := func() time.Time { t, _ := time.Parse(time.RFC3339, "2020-02-02T12:30:00.000Z") return t } + pua := SignedURLAuthenticator{ + Now: nowFunc, + } tests := []struct { - url string - isExpired bool + url string + errorMessage string }{ - {"http://example.com/example.jpg?OC-Date=2020-02-02T12:29:00.000Z&OC-Expires=61", false}, - {"http://example.com/example.jpg?OC-Date=2020-02-02T12:29:00.000Z&OC-Expires=invalid", true}, - {"http://example.com/example.jpg?OC-Date=2020-02-02T12:29:00.000Z&OC-Expires=59", true}, - {"http://example.com/example.jpg?OC-Date=2020-02-03T12:29:00.000Z&OC-Expires=59", true}, - {"http://example.com/example.jpg?OC-Date=2020-02-01T12:29:00.000Z&OC-Expires=59", true}, + // a valid signed url + {"http://example.com/example.jpg?OC-Date=2020-02-02T12:29:00.000Z&OC-Expires=61", ""}, + // invalid expiry + {"http://example.com/example.jpg?OC-Date=2020-02-02T12:29:00.000Z&OC-Expires=invalid", "time: invalid duration \"invalids\""}, + // wrong date format on OC-Date + {"http://example.com/example.jpg?OC-Date=2020-02-02TTT12:29:00.000Z&OC-Expires=5", "parsing time \"2020-02-02TTT12:29:00.000Z\" as \"2006-01-02T15:04:05Z07:00\": cannot parse \"TT12:29:00.000Z\" as \"15\""}, + // expired - 12:29:00 + 59s < 12:30 + {"http://example.com/example.jpg?OC-Date=2020-02-02T12:29:00.000Z&OC-Expires=59", "URL is expired"}, + // expired - basically url was created yesterday + {"http://example.com/example.jpg?OC-Date=2020-02-01T12:29:00.000Z&OC-Expires=59", "URL is expired"}, + // future OC-Date - also valid now + {"http://example.com/example.jpg?OC-Date=2020-02-03T12:29:00.000Z&OC-Expires=59", ""}, } for _, tt := range tests { r := httptest.NewRequest("", tt.url, nil) - expired, _ := pua.urlIsExpired(r.URL.Query(), nowFunc) - if expired != tt.isExpired { - t.Errorf("with %s expected %t got %t", tt.url, tt.isExpired, expired) + err := pua.urlIsExpired(r.URL.Query()) + if tt.errorMessage != "" { + assert.EqualError(t, err, tt.errorMessage, tt.url) + } else { + assert.Nil(t, err) } } } @@ -134,3 +158,85 @@ func TestSignedURLAuth_createSignature(t *testing.T) { t.Fail() } } + +type MyStoreService struct { +} + +func (s MyStoreService) Read(_ context.Context, _ *v0.ReadRequest, _ ...client.CallOption) (*v0.ReadResponse, error) { + r := v0.ReadResponse{ + Records: []*storemsg.Record{{ + Key: "foo", + Value: []byte("1234567890"), + Expiry: 0, + Metadata: nil, + }}, + } + return &r, nil +} +func (s MyStoreService) Write(_ context.Context, _ *v0.WriteRequest, _ ...client.CallOption) (*v0.WriteResponse, error) { + return nil, nil +} +func (s MyStoreService) Delete(_ context.Context, _ *v0.DeleteRequest, _ ...client.CallOption) (*v0.DeleteResponse, error) { + return nil, nil +} +func (s MyStoreService) List(_ context.Context, _ *v0.ListRequest, _ ...client.CallOption) (v0.Store_ListService, error) { + return nil, nil +} +func (s MyStoreService) Databases(_ context.Context, _ *v0.DatabasesRequest, _ ...client.CallOption) (*v0.DatabasesResponse, error) { + return nil, nil +} +func (s MyStoreService) Tables(_ context.Context, _ *v0.TablesRequest, _ ...client.CallOption) (*v0.TablesResponse, error) { + return nil, nil +} + +func TestSignedURLAuth_validate(t *testing.T) { + nowFunc := func() time.Time { + t, _ := time.Parse(time.RFC3339, "2020-02-02T12:30:00.000Z") + return t + } + cfg := config.PreSignedURL{ + AllowedHTTPMethods: []string{"get"}, + Enabled: true, + } + store := MyStoreService{} + pua := SignedURLAuthenticator{ + PreSignedURLConfig: cfg, + Store: store, + Now: nowFunc, + } + + tests := []struct { + now string + url string + errorMessage string + }{ + {"2020-02-02T12:30:00.000Z", "http://example.com/example.jpg?OC-Date=2020-02-02T12:29:00.000Z&OC-Expires=invalid", "required OC-Signature parameter not found"}, + {"2020-02-02T12:30:00.000Z", "http://cloud.example.net/?OC-Credential=alice&OC-Date=2019-05-14T11%3A01%3A58.135Z&OC-Expires=1200&OC-Verb=GET&OC-Signature=f9e53a1ee23caef10f72ec392c1b537317491b687bfdd224c782be197d9ca2b6", "URL is expired"}, + {"2019-05-14T11:02:00.000Z", "http://cloud.example.net/?OC-Credential=alice&OC-Date=2019-05-14T11%3A01%3A58.135Z&OC-Expires=1200&OC-Verb=GET&OC-Signature=f9e53a1ee23caef10f72ec392c1b537317491b687bfdd224c782be197d9ca2b", "signature mismatch: expected f9e53a1ee23caef10f72ec392c1b537317491b687bfdd224c782be197d9ca2b6 != actual f9e53a1ee23caef10f72ec392c1b537317491b687bfdd224c782be197d9ca2b"}, + {"2019-05-14T11:02:00.000Z", "http://cloud.example.net/?OC-Credential=alice&OC-Date=2019-05-14T11%3A01%3A58.135Z&OC-Expires=1200&OC-Verb=GET&OC-Signature=f9e53a1ee23caef10f72ec392c1b537317491b687bfdd224c782be197d9ca2b6", ""}, + {"2019-05-14T11:02:00.000Z", "http://cloud.example.net/?OC-Date=2019-05-14T11%3A01%3A58.135Z&OC-Expires=1200&OC-Verb=GET&OC-Credential=alice&OC-Signature=f9e53a1ee23caef10f72ec392c1b537317491b687bfdd224c782be197d9ca2b6", ""}, + {"2019-05-14T11:02:00.000Z", "http://cloud.example.net/?OC-Algo=PBKDF2%2F10000-SHA512&OC-Date=2019-05-14T11%3A01%3A58.135Z&OC-Expires=1200&OC-Verb=GET&OC-Credential=alice&OC-Signature=f9e53a1ee23caef10f72ec392c1b537317491b687bfdd224c782be197d9ca2b6", ""}, + {"2024-02-07T12:03:11.966Z", "http://localhost:33001/try?id=1&id=2&OC-Credential=user&OC-Date=2024-02-07T12%3A03%3A11.966Z&OC-Expires=2&OC-Verb=GET&OC-Algo=PBKDF2%2F10000-SHA512&OC-Signature=86e21a1efbf0be989a206109cfedf70a22f338dc8995e849ce002032bc6741c5", ""}, + } + + for _, tt := range tests { + u := userpb.User{ + Id: &userpb.UserId{OpaqueId: "useri"}, + DisplayName: "Test User", + } + ctx := revactx.ContextSetUser(context.Background(), &u) + + pua.Now = func() time.Time { + t, _ := time.Parse(time.RFC3339, tt.now) + return t + } + + r := httptest.NewRequest("", tt.url, nil).WithContext(ctx) + err := pua.validate(r) + if tt.errorMessage == "" { + assert.Nil(t, err) + } else { + assert.EqualError(t, err, tt.errorMessage) + } + } +}