Merge pull request #92 from owncloud/pre-signed-url-configuration

Pre signed url configuration

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

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.

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"`

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"},
+		},
 	}
 
 }

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
+	}
+}

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) {

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()
+	}
+}