From 1070c61c44bce3ce90dc8c7ba265d4e0505b30d5 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Thu, 20 Aug 2020 10:54:26 +0200 Subject: [PATCH] implement configuration options for the presigned url middleware Signed-off-by: David Christofas --- .../pre-signed-url-configuration.md | 7 + pkg/command/server.go | 2 + pkg/config/config.go | 6 + pkg/flagset/flagset.go | 6 + pkg/middleware/options.go | 9 ++ pkg/middleware/presigned_url.go | 75 ++++++++--- pkg/middleware/presigned_url_test.go | 121 ++++++++++++++++++ 7 files changed, 205 insertions(+), 21 deletions(-) create mode 100644 changelog/unreleased/pre-signed-url-configuration.md create mode 100644 pkg/middleware/presigned_url_test.go diff --git a/changelog/unreleased/pre-signed-url-configuration.md b/changelog/unreleased/pre-signed-url-configuration.md new file mode 100644 index 0000000000..c52fdb9756 --- /dev/null +++ b/changelog/unreleased/pre-signed-url-configuration.md @@ -0,0 +1,7 @@ +Enhancement: add configuration options for the pre-signed url middleware + +Added an option to define allowed http methods for pre-signed url requests. +This is useful since we only want clients to GET resources and don't upload anything with presigned requests. + +https://github.com/owncloud/ocis-proxy/issues/91 +https://github.com/owncloud/product/issues/150 diff --git a/pkg/command/server.go b/pkg/command/server.go index 3e948d8b68..a02019fa42 100644 --- a/pkg/command/server.go +++ b/pkg/command/server.go @@ -50,6 +50,7 @@ func Server(cfg *config.Config) *cli.Command { if cfg.HTTP.Root != "/" { cfg.HTTP.Root = strings.TrimSuffix(cfg.HTTP.Root, "/") } + cfg.PreSignedURL.AllowedHTTPMethods = ctx.StringSlice("presignedurl-allow-method") // When running on single binary mode the before hook from the root command won't get called. We manually // call this before hook from ocis command, so the configuration can be loaded. @@ -251,6 +252,7 @@ func loadMiddlewares(ctx context.Context, l log.Logger, cfg *config.Config) alic psMW := middleware.PresignedURL( middleware.Logger(l), middleware.Store(storepb.NewStoreService("com.owncloud.api.store", grpc.NewClient())), + middleware.PreSignedURLConfig(cfg.PreSignedURL), ) // TODO this won't work with a registry other than mdns. Look into Micro's client initialization. diff --git a/pkg/config/config.go b/pkg/config/config.go index 72423f7547..8b1273b9a9 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -90,6 +90,7 @@ type Config struct { TokenManager TokenManager PolicySelector *PolicySelector `mapstructure:"policy_selector"` Reva Reva + PreSignedURL PreSignedURL } // OIDC is the config for the OpenID-Connect middleware. If set the proxy will try to authenticate every request @@ -115,6 +116,11 @@ type TokenManager struct { JWTSecret string } +// PreSignedURL is the config for the presigned url middleware +type PreSignedURL struct { + AllowedHTTPMethods []string +} + // MigrationSelectorConf is the config for the migration-selector type MigrationSelectorConf struct { AccFoundPolicy string `mapstructure:"acc_found_policy"` diff --git a/pkg/flagset/flagset.go b/pkg/flagset/flagset.go index d7ad756d68..b4ba5f556c 100644 --- a/pkg/flagset/flagset.go +++ b/pkg/flagset/flagset.go @@ -195,6 +195,12 @@ func ServerWithConfig(cfg *config.Config) []cli.Flag { EnvVars: []string{"PROXY_OIDC_INSECURE"}, Destination: &cfg.OIDC.Insecure, }, + &cli.StringSliceFlag{ + Name: "presignedurl-allow-method", + Value: cli.NewStringSlice("GET"), + Usage: "--presignedurl-allow-method GET [--presignedurl-allow-method POST]", + EnvVars: []string{"PRESIGNEDURL_ALLOWED_METHODS"}, + }, } } diff --git a/pkg/middleware/options.go b/pkg/middleware/options.go index cf7f5d1178..21e7c8949d 100644 --- a/pkg/middleware/options.go +++ b/pkg/middleware/options.go @@ -31,6 +31,8 @@ type Options struct { RevaGatewayClient gateway.GatewayAPIClient // Store for persisting data Store storepb.StoreService + // PreSignedURLConfig to configure the middleware + PreSignedURLConfig config.PreSignedURL } // newOptions initializes the available default options. @@ -99,3 +101,10 @@ func Store(sc storepb.StoreService) Option { o.Store = sc } } + +// PreSignedURLConfig provides a function to set the PreSignedURL config +func PreSignedURLConfig(cfg config.PreSignedURL) Option { + return func(o *Options) { + o.PreSignedURLConfig = cfg + } +} diff --git a/pkg/middleware/presigned_url.go b/pkg/middleware/presigned_url.go index a6d328ec54..8993864e6f 100644 --- a/pkg/middleware/presigned_url.go +++ b/pkg/middleware/presigned_url.go @@ -10,19 +10,26 @@ import ( "github.com/owncloud/ocis-pkg/v2/log" ocisoidc "github.com/owncloud/ocis-pkg/v2/oidc" + "github.com/owncloud/ocis-proxy/pkg/config" storepb "github.com/owncloud/ocis-store/pkg/proto/v0" "golang.org/x/crypto/pbkdf2" ) +const ( + iterations = 10000 + keyLen = 32 +) + // PresignedURL provides a middleware to check access secured by a presigned URL. func PresignedURL(opts ...Option) func(next http.Handler) http.Handler { opt := newOptions(opts...) l := opt.Logger + cfg := opt.PreSignedURLConfig return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if isSignedRequest(r) { - if signedRequestIsValid(l, r, opt.Store) { + if signedRequestIsValid(l, r, opt.Store, cfg) { // use openid claims to let the account_uuid middleware do a lookup by username claims := ocisoidc.StandardClaims{ OcisID: r.URL.Query().Get("OC-Credential"), @@ -47,33 +54,56 @@ func isSignedRequest(r *http.Request) bool { return r.URL.Query().Get("OC-Signature") != "" } -func signedRequestIsValid(l log.Logger, r *http.Request, s storepb.StoreService) bool { - // cheap checks first +func signedRequestIsValid(l log.Logger, r *http.Request, s storepb.StoreService, cfg config.PreSignedURL) bool { // TODO OC-Algorithm - defined the used algo (e.g. sha256 or sha512 - we should agree on one default algo and make this parameter optional) + // TODO OC-Verb - defines for which http verb the request is valid - defaults to GET OPTIONAL + + return allRequiredParametersArePresent(r) && + requestMethodMatches(r) && + requestMethodIsAllowed(r.Method, cfg.AllowedHTTPMethods) && + !urlIsExpired(r, time.Now) && + signatureIsValid(l, r, s) +} + +func allRequiredParametersArePresent(r *http.Request) bool { // OC-Credential - defines the user scope (shall we use the owncloud user id here - this might leak internal data ....) REQUIRED // OC-Date - defined the date the url was signed (ISO 8601 UTC) REQUIRED // OC-Expires - defines the expiry interval in seconds (between 1 and 604800 = 7 days) REQUIRED - // TODO OC-Verb - defines for which http verb the request is valid - defaults to GET OPTIONAL // OC-Signature - the computed signature - server will verify the request upon this REQUIRED - if r.URL.Query().Get("OC-Signature") == "" || r.URL.Query().Get("OC-Credential") == "" || r.URL.Query().Get("OC-Date") == "" || r.URL.Query().Get("OC-Expires") == "" || r.URL.Query().Get("OC-Verb") == "" { - return false - } + return r.URL.Query().Get("OC-Signature") != "" && + r.URL.Query().Get("OC-Credential") != "" && + r.URL.Query().Get("OC-Date") != "" && + r.URL.Query().Get("OC-Expires") != "" && + r.URL.Query().Get("OC-Verb") != "" +} - if !strings.EqualFold(r.Method, r.URL.Query().Get("OC-Verb")) { - return false - } +func requestMethodMatches(r *http.Request) bool { + return strings.EqualFold(r.Method, r.URL.Query().Get("OC-Verb")) +} - if t, err := time.Parse(time.RFC3339, r.URL.Query().Get("OC-Date")); err != nil { - return false - } else if expires, err := time.ParseDuration(r.URL.Query().Get("OC-Expires") + "s"); err != nil { - return false - } else { - t.Add(expires) - if t.After(time.Now()) { - return false +func requestMethodIsAllowed(m string, allowedMethods []string) bool { + for _, allowed := range allowedMethods { + if strings.EqualFold(m, allowed) { + return true } } + return false +} +func urlIsExpired(r *http.Request, now func() time.Time) bool { + t, err := time.Parse(time.RFC3339, r.URL.Query().Get("OC-Date")) + if err != nil { + return true + } + expires, err := time.ParseDuration(r.URL.Query().Get("OC-Expires") + "s") + if err != nil { + return true + } + t.Add(expires) + return t.After(now()) +} + +func signatureIsValid(l log.Logger, r *http.Request, s storepb.StoreService) bool { signingKey, err := getSigningKey(r.Context(), s, r.URL.Query().Get("OC-Credential")) if err != nil { l.Error().Err(err).Msg("could not retrieve signing key") @@ -92,14 +122,17 @@ func signedRequestIsValid(l log.Logger, r *http.Request, s storepb.StoreService) if !r.URL.IsAbs() { url = "https://" + r.Host + url // TODO where do we get the scheme from } + return createSignature(url, signingKey) == signature +} + +func createSignature(url string, signingKey []byte) string { // the oc10 signature check: $hash = \hash_pbkdf2("sha512", $url, $signingKey, 10000, 64, false); // - sets the length of the output string to 64 // - sets raw output to false -> if raw_output is FALSE length corresponds to twice the byte-length of the derived key (as every byte of the key is returned as two hexits). // TODO change to length 128 in oc10? // fo golangs pbkdf2.Key we need to use 32 because it will be encoded into 64 hexits later - hash := pbkdf2.Key([]byte(url), signingKey, 10000, 32, sha512.New) - - return hex.EncodeToString(hash) == signature + hash := pbkdf2.Key([]byte(url), signingKey, iterations, keyLen, sha512.New) + return hex.EncodeToString(hash) } func getSigningKey(ctx context.Context, s storepb.StoreService, credential string) ([]byte, error) { diff --git a/pkg/middleware/presigned_url_test.go b/pkg/middleware/presigned_url_test.go new file mode 100644 index 0000000000..0e41d78ddb --- /dev/null +++ b/pkg/middleware/presigned_url_test.go @@ -0,0 +1,121 @@ +package middleware + +import ( + "net/http/httptest" + "testing" + "time" +) + +func TestIsSignedRequest(t *testing.T) { + tests := []struct { + url string + expected bool + }{ + {"https://example.com/example.jpg", false}, + {"https://example.com/example.jpg?OC-Signature=something", true}, + } + + for _, tt := range tests { + r := httptest.NewRequest("", tt.url, nil) + result := isSignedRequest(r) + if result != tt.expected { + t.Errorf("with %s expected %t got %t", tt.url, tt.expected, result) + } + } +} + +func TestAllRequiredParametersPresent(t *testing.T) { + baseURL := "https://example.com/example.jpg?" + tests := []struct { + params string + expected bool + }{ + {"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}, + } + for _, tt := range tests { + r := httptest.NewRequest("", baseURL+tt.params, nil) + result := allRequiredParametersArePresent(r) + if result != tt.expected { + t.Errorf("with %s expected %t got %t", tt.params, tt.expected, result) + } + } +} + +func TestRequestMethodMatches(t *testing.T) { + tests := []struct { + method string + url string + expected bool + }{ + {"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}, + } + + for _, tt := range tests { + r := httptest.NewRequest(tt.method, tt.url, nil) + result := requestMethodMatches(r) + if result != tt.expected { + t.Errorf("with method %s and url %s expected %t got %t", tt.method, tt.url, tt.expected, result) + } + } +} + +func TestRequestMethodIsAllowed(t *testing.T) { + tests := []struct { + method string + allowed []string + expected bool + }{ + {"GET", []string{}, false}, + {"GET", []string{"POST"}, false}, + {"GET", []string{"GET"}, true}, + {"GET", []string{"get"}, true}, + {"GET", []string{"POST", "GET"}, true}, + } + + for _, tt := range tests { + result := requestMethodIsAllowed(tt.method, tt.allowed) + if result != tt.expected { + t.Errorf("with method %s and allowed methods %s expected %t got %t", tt.method, tt.allowed, tt.expected, result) + } + } +} + +func TestUrlIsExpired(t *testing.T) { + nowFunc := func() time.Time { + t, _ := time.Parse(time.RFC3339, "2020-08-19T15:12:43.478Z") + return t + } + + tests := []struct { + url string + expected bool + }{ + {"http://example.com/example.jpg?OC-Date=2020-08-19T15:02:43.478Z&OC-Expires=1200", false}, + {"http://example.com/example.jpg?OC-Date=invalid&OC-Expires=1200", true}, + {"http://example.com/example.jpg?OC-Date=2020-08-19T15:02:43.478Z&OC-Expires=invalid", true}, + } + + for _, tt := range tests { + r := httptest.NewRequest("", tt.url, nil) + result := urlIsExpired(r, nowFunc) + if result != tt.expected { + t.Errorf("with %s expected %t got %t", tt.url, tt.expected, result) + } + } +} + +func TestCreateSignature(t *testing.T) { + expected := "27d2ebea381384af3179235114801dcd00f91e46f99fca72575301cf3948101d" + s := createSignature("something", []byte("somerandomkey")) + + if s != expected { + t.Fail() + } +}