From d5bb95a35a5746344294ae828bff02e6fc55cc57 Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Wed, 8 Jan 2020 20:37:19 +0100 Subject: [PATCH 01/15] rule: update github.com/ory/ladon to v.1.1.0 This update allows to use negative lookahead in regexp. Signed-off-by: Aynur Zulkarnaev --- go.mod | 8 ++ go.sum | 6 ++ pipeline/authz/keto_engine_acp_ory.go | 14 ++- pipeline/rule.go | 6 +- rule/repository_memory.go | 5 +- rule/rule.go | 21 ++--- rule/rule_test.go | 119 +++++++++++++++++++++++++- 7 files changed, 158 insertions(+), 21 deletions(-) diff --git a/go.mod b/go.mod index 3d0f178126..9bfdb715ab 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/blang/semver v3.5.1+incompatible github.com/bxcodec/faker v2.0.1+incompatible github.com/dgrijalva/jwt-go v3.2.0+incompatible + github.com/dlclark/regexp2 v1.2.0 github.com/fsnotify/fsnotify v1.4.7 github.com/ghodss/yaml v1.0.0 github.com/go-openapi/errors v0.19.2 @@ -23,6 +24,7 @@ require ( github.com/golang/mock v1.3.1 github.com/google/uuid v1.1.1 github.com/gorilla/mux v1.7.1 // indirect + github.com/hashicorp/golang-lru v0.5.3 // indirect github.com/huandu/xstrings v1.2.0 // indirect github.com/imdario/mergo v0.3.7 github.com/julienschmidt/httprouter v1.2.0 @@ -38,6 +40,9 @@ require ( github.com/ory/sdk/swagutil v0.0.0-20200202121523-307941feee4b github.com/ory/viper v1.5.7 github.com/ory/x v0.0.93 + github.com/ory/ladon v1.1.0 + github.com/ory/viper v1.5.6 + github.com/ory/x v0.0.87 github.com/pborman/uuid v1.2.0 github.com/pelletier/go-toml v1.6.0 // indirect github.com/phayes/freeport v0.0.0-20180830031419-95f893ade6f2 @@ -58,6 +63,9 @@ require ( golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45 golang.org/x/tools v0.0.0-20191224055732-dd894d0a8a40 gopkg.in/square/go-jose.v2 v2.3.1 + golang.org/x/tools v0.0.0-20191026034945-b2104f82a97d + gopkg.in/square/go-jose.v2 v2.3.0 + gopkg.in/yaml.v2 v2.2.7 // indirect ) // Fix for https://github.com/golang/lint/issues/436 diff --git a/go.sum b/go.sum index ce7633dd33..6869e36cea 100644 --- a/go.sum +++ b/go.sum @@ -84,6 +84,8 @@ github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs github.com/dgrijalva/jwt-go v3.2.0+incompatible h1:7qlOGliEKZXTDg6OTjfoBKDXWrumCAMpl/TFQ4/5kLM= github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ= github.com/dgryski/go-sip13 v0.0.0-20181026042036-e10d5fee7954/go.mod h1:vAd38F8PWV+bWy6jNmig1y/TA+kYO4g3RSRF0IAv0no= +github.com/dlclark/regexp2 v1.2.0 h1:8sAhBGEM0dRWogWqWyQeIJnxjWO6oIjl8FKqREDsGfk= +github.com/dlclark/regexp2 v1.2.0/go.mod h1:2pZnwuY/m+8K6iRw6wQdMtk+rH5tNGR1i55kozfMjCc= github.com/docker/go-connections v0.4.0 h1:El9xVISelRB7BuFusrZozjnkIM5YnzCViNKohAFqRJQ= github.com/docker/go-connections v0.4.0/go.mod h1:Gbd7IOopHjR8Iph03tsViu4nIes5XhDvyHbTtUxmeec= github.com/docker/go-units v0.3.3/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk= @@ -409,6 +411,8 @@ github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgf github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY= github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= +github.com/hashicorp/golang-lru v0.5.3 h1:YPkqC67at8FYaadspW/6uE0COsBxS2656RLEr8Bppgk= +github.com/hashicorp/golang-lru v0.5.3/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4= github.com/hashicorp/hcl v1.0.0 h1:0Anlzjpi4vEasTeNFn2mLJgTSwt0+6sfsiTG8qcWGx4= github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ= github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU= @@ -554,6 +558,8 @@ github.com/ory/herodot v0.6.2 h1:zOb5MsuMn7AH9/Ewc/EK83yqcNViK1m1l3C2UuP3RcA= github.com/ory/herodot v0.6.2/go.mod h1:3BOneqcyBsVybCPAJoi92KN2BpJHcmDqAMcAAaJiJow= github.com/ory/ladon v1.0.1 h1:zCEfqnv8Ro62id0Z9cVPRPv4ejTu6HHtuPbXmTWBxks= github.com/ory/ladon v1.0.1/go.mod h1:1VhCA2mBtaMhRUS6VS0d9qrNVDQnFXqSRb5D0NvQUPY= +github.com/ory/ladon v1.1.0 h1:6tgazU2J3Z3odPs1f0qn729kRXCAtlJROliuWUHedV0= +github.com/ory/ladon v1.1.0/go.mod h1:25bNc/Glx/8xCH7MbItDxjvviAmFQ+aYxb1V1SE5wlg= github.com/ory/pagination v0.0.1/go.mod h1:d1ToRROAUleriPhmb2dYbhANhhLwZ8s395m2yJCDFh8= github.com/ory/sdk/swagutil v0.0.0-20200123152503-0d50960e70bd h1:QrEYSnaOX6kpyBcQGUlExcI4RwCq2S/Wta/zbgT74Kk= github.com/ory/sdk/swagutil v0.0.0-20200123152503-0d50960e70bd/go.mod h1:Ufg1eAyz+Zt3+oweSZVThG13ewewWCKwBmoNmK8Z0co= diff --git a/pipeline/authz/keto_engine_acp_ory.go b/pipeline/authz/keto_engine_acp_ory.go index 26ef6608ec..65d2fbe35a 100644 --- a/pipeline/authz/keto_engine_acp_ory.go +++ b/pipeline/authz/keto_engine_acp_ory.go @@ -115,9 +115,19 @@ func (a *AuthorizerKetoEngineACPORY) Authorize(r *http.Request, session *authn.A var b bytes.Buffer u := fmt.Sprintf("%s://%s%s", r.URL.Scheme, r.URL.Host, r.URL.Path) + + action, err := compiled.Replace(u, cf.RequiredAction, -1, -1) + if err != nil { + return errors.WithStack(err) + } + resource, err := compiled.Replace(u, cf.RequiredResource, -1, -1) + if err != nil { + return errors.WithStack(err) + } + if err := json.NewEncoder(&b).Encode(&AuthorizerKetoEngineACPORYRequestBody{ - Action: compiled.ReplaceAllString(u, cf.RequiredAction), - Resource: compiled.ReplaceAllString(u, cf.RequiredResource), + Action: action, + Resource: resource, Context: a.contextCreator(r), Subject: subject, }); err != nil { diff --git a/pipeline/rule.go b/pipeline/rule.go index 469282a0b4..21acba499a 100644 --- a/pipeline/rule.go +++ b/pipeline/rule.go @@ -1,8 +1,10 @@ package pipeline -import "regexp" +import ( + "github.com/dlclark/regexp2" +) type Rule interface { GetID() string - CompileURL() (*regexp.Regexp, error) + CompileURL() (*regexp2.Regexp, error) } diff --git a/rule/repository_memory.go b/rule/repository_memory.go index da4ac1bac0..213052796d 100644 --- a/rule/repository_memory.go +++ b/rule/repository_memory.go @@ -111,10 +111,11 @@ func (m *RepositoryMemory) Match(ctx context.Context, method string, u *url.URL) var rules []Rule for k := range m.rules { r := &m.rules[k] - if err := r.IsMatching(method, u); err == nil { + if matched, err := r.IsMatching(method, u); err != nil { + return nil, errors.WithStack(err) + } else if matched { rules = append(rules, *r) } - m.rules[k] = *r } if len(rules) == 0 { diff --git a/rule/rule.go b/rule/rule.go index 80218c8c0f..b46726540a 100644 --- a/rule/rule.go +++ b/rule/rule.go @@ -25,9 +25,9 @@ import ( "fmt" "hash/crc32" "net/url" - "regexp" "strings" + "github.com/dlclark/regexp2" "github.com/pkg/errors" "github.com/ory/ladon/compiler" @@ -50,7 +50,7 @@ type Match struct { // brackets < and >. The following example matches all paths of the domain `mydomain.com`: `https://mydomain.com/<.*>`. URL string `json:"url"` - compiledURL *regexp.Regexp + compiledURL *regexp2.Regexp compiledURLChecksum uint32 } @@ -177,25 +177,22 @@ func (r *Rule) GetID() string { return r.ID } -// IsMatching returns an error if the provided method and URL do not match the rule. -func (r *Rule) IsMatching(method string, u *url.URL) error { +// IsMatching checks whether the provided url and method match the rule. +// An error will be returned if a regexp timeout occurs. +func (r *Rule) IsMatching(method string, u *url.URL) (bool, error) { if !stringInSlice(method, r.Match.Methods) { - return errors.Errorf("rule %s does not match URL %s", r.ID, u) + return false, nil } c, err := r.CompileURL() if err != nil { - return errors.WithStack(err) + return false, errors.WithStack(err) } - if !c.MatchString(fmt.Sprintf("%s://%s%s", u.Scheme, u.Host, u.Path)) { - return errors.Errorf("rule %s does not match URL %s", r.ID, u) - } - - return nil + return c.MatchString(fmt.Sprintf("%s://%s%s", u.Scheme, u.Host, u.Path)) } -func (r *Rule) CompileURL() (*regexp.Regexp, error) { +func (r *Rule) CompileURL() (*regexp2.Regexp, error) { m := r.Match c := crc32.ChecksumIEEE([]byte(m.URL)) if m.compiledURL == nil || c != m.compiledURLChecksum { diff --git a/rule/rule_test.go b/rule/rule_test.go index e5144e3d2d..4e11eba803 100644 --- a/rule/rule_test.go +++ b/rule/rule_test.go @@ -42,7 +42,120 @@ func TestRule(t *testing.T) { }, } - assert.NoError(t, r.IsMatching("DELETE", mustParse(t, "https://localhost/users/1234"))) - assert.NoError(t, r.IsMatching("DELETE", mustParse(t, "https://localhost/users/1234?key=value&key1=value1"))) - assert.Error(t, r.IsMatching("DELETE", mustParse(t, "https://localhost/users/abcd"))) + var tests = []struct { + method string + url string + expectedMatch bool + expectedErr error + }{ + { + method: "DELETE", + url: "https://localhost/users/1234", + expectedMatch: true, + expectedErr: nil, + }, + { + method: "DELETE", + url: "https://localhost/users/1234?key=value&key1=value1", + expectedMatch: true, + expectedErr: nil, + }, + { + method: "DELETE", + url: "https://localhost/users/abcd", + expectedMatch: false, + expectedErr: nil, + }, + } + for ind, tcase := range tests { + t.Run(string(ind), func(t *testing.T) { + matched, err := r.IsMatching(tcase.method, mustParse(t, tcase.url)) + assert.Equal(t, tcase.expectedMatch, matched) + assert.Equal(t, tcase.expectedErr, err) + }) + } +} + +func TestRule1(t *testing.T) { + r := &Rule{ + Match: &Match{ + Methods: []string{"DELETE"}, + URL: "https://localhost/users/<[[:digit:]]*>", + }, + } + + var tests = []struct { + method string + url string + expectedMatch bool + expectedErr error + }{ + { + method: "DELETE", + url: "https://localhost/users/1234", + expectedMatch: true, + expectedErr: nil, + }, + { + method: "DELETE", + url: "https://localhost/users/1234?key=value&key1=value1", + expectedMatch: true, + expectedErr: nil, + }, + { + method: "DELETE", + url: "https://localhost/users/abcd", + expectedMatch: false, + expectedErr: nil, + }, + } + for ind, tcase := range tests { + t.Run(string(ind), func(t *testing.T) { + matched, err := r.IsMatching(tcase.method, mustParse(t, tcase.url)) + assert.Equal(t, tcase.expectedMatch, matched) + assert.Equal(t, tcase.expectedErr, err) + }) + } +} + +func TestRule2(t *testing.T) { + r := &Rule{ + Match: &Match{ + Methods: []string{"DELETE"}, + URL: "https://localhost/users/<(?!admin).*>", + }, + } + + var tests = []struct { + method string + url string + expectedMatch bool + expectedErr error + }{ + { + method: "DELETE", + url: "https://localhost/users/manager", + expectedMatch: true, + expectedErr: nil, + }, + { + method: "DELETE", + url: "https://localhost/users/1234?key=value&key1=value1", + expectedMatch: true, + expectedErr: nil, + }, + { + method: "DELETE", + url: "https://localhost/users/admin", + expectedMatch: false, + expectedErr: nil, + }, + } + for ind, tcase := range tests { + t.Run(string(ind), func(t *testing.T) { + matched, err := r.IsMatching(tcase.method, mustParse(t, tcase.url)) + assert.Equal(t, tcase.expectedMatch, matched) + assert.Equal(t, tcase.expectedErr, err) + }) + } } From 643eecfa5b235be9bbbd56c0b610075215077a0e Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Wed, 8 Jan 2020 20:51:42 +0100 Subject: [PATCH 02/15] rule: add MatchingStrategy parameter to configuration Signed-off-by: Aynur Zulkarnaev --- .schemas/config.schema.json | 10 +++++++ driver/configuration/provider.go | 10 +++++++ driver/configuration/provider_viper.go | 22 +++++++++------ rule/fetcher_default.go | 37 ++++++++++++++++++++++---- rule/repository.go | 4 +++ rule/repository_memory.go | 21 +++++++++++++-- 6 files changed, 89 insertions(+), 15 deletions(-) diff --git a/.schemas/config.schema.json b/.schemas/config.schema.json index da022c0061..3e16905e4b 100644 --- a/.schemas/config.schema.json +++ b/.schemas/config.schema.json @@ -982,6 +982,16 @@ "examples": [ "[\"file://path/to/rules.json\",\"inline://W3siaWQiOiJmb28tcnVsZSIsImF1dGhlbnRpY2F0b3JzIjpbXX1d\",\"https://path-to-my-rules/rules.json\"]" ] + }, + "matching_strategy": { + "title": "Matching strategy", + "description": "This an optional field describing matching strategy. Currently supported values are 'glob' and 'regexp'.", + "type": "string", + "default": "regexp", + "enum": ["glob", "regexp"], + "examples": [ + "glob", "regexp" + ] } } }, diff --git a/driver/configuration/provider.go b/driver/configuration/provider.go index 4bf1870517..36a46aab3b 100644 --- a/driver/configuration/provider.go +++ b/driver/configuration/provider.go @@ -19,6 +19,15 @@ const ( ForbiddenStrategyErrorType = "forbidden" ) +// MatchingStrategy defines matching strategy such as Regexp or Glob. +type MatchingStrategy int + +// Possible values. +const ( + Regexp MatchingStrategy = iota + Glob +) + type Provider interface { CORSEnabled(iface string) bool CORSOptions(iface string) cors.Options @@ -33,6 +42,7 @@ type Provider interface { ProxyIdleTimeout() time.Duration AccessRuleRepositories() []url.URL + AccessRuleMatchingStrategy() MatchingStrategy ProxyServeAddress() string APIServeAddress() string diff --git a/driver/configuration/provider_viper.go b/driver/configuration/provider_viper.go index c6dd0bad81..2b8f48c427 100644 --- a/driver/configuration/provider_viper.go +++ b/driver/configuration/provider_viper.go @@ -37,14 +37,15 @@ func init() { } const ( - ViperKeyProxyReadTimeout = "serve.proxy.timeout.read" - ViperKeyProxyWriteTimeout = "serve.proxy.timeout.write" - ViperKeyProxyIdleTimeout = "serve.proxy.timeout.idle" - ViperKeyProxyServeAddressHost = "serve.proxy.host" - ViperKeyProxyServeAddressPort = "serve.proxy.port" - ViperKeyAPIServeAddressHost = "serve.api.host" - ViperKeyAPIServeAddressPort = "serve.api.port" - ViperKeyAccessRuleRepositories = "access_rules.repositories" + ViperKeyProxyReadTimeout = "serve.proxy.timeout.read" + ViperKeyProxyWriteTimeout = "serve.proxy.timeout.write" + ViperKeyProxyIdleTimeout = "serve.proxy.timeout.idle" + ViperKeyProxyServeAddressHost = "serve.proxy.host" + ViperKeyProxyServeAddressPort = "serve.proxy.port" + ViperKeyAPIServeAddressHost = "serve.api.host" + ViperKeyAPIServeAddressPort = "serve.api.port" + ViperKeyAccessRuleRepositories = "access_rules.repositories" + ViperKeyAccessRuleMatchingStrategy = "access_rules.matching_strategy" ) // Authorizers @@ -131,6 +132,11 @@ func (v *ViperProvider) AccessRuleRepositories() []url.URL { return repositories } +// AccessRuleMatchingStrategy returns current MatchingStrategy. +func (v *ViperProvider) AccessRuleMatchingStrategy() MatchingStrategy { + return MatchingStrategy(viperx.GetInt(v.l, ViperKeyAccessRuleMatchingStrategy, int(Regexp))) +} + func (v *ViperProvider) CORSEnabled(iface string) bool { return corsx.IsEnabled(v.l, "serve."+iface) } diff --git a/rule/fetcher_default.go b/rule/fetcher_default.go index 25da8ec20f..611c3cdfed 100644 --- a/rule/fetcher_default.go +++ b/rule/fetcher_default.go @@ -40,8 +40,9 @@ type event struct { type eventType int const ( - eventRepositoryConfigChange eventType = iota + eventRepositoryConfigChanged eventType = iota eventFileChanged + eventMatchingStrategyChanged ) var _ Fetcher = new(FetcherDefault) @@ -197,12 +198,30 @@ func (f *FetcherDefault) watch(ctx context.Context, watcher *fsnotify.Watcher, e return nil } - f.enqueueEvent(events, event{et: eventRepositoryConfigChange, source: "viper_watcher"}) + f.enqueueEvent(events, event{et: eventRepositoryConfigChanged, source: "viper_watcher"}) return nil }) - f.enqueueEvent(events, event{et: eventRepositoryConfigChange, source: "entrypoint"}) + var initialConfig map[string]interface{} + + f.enqueueEvent(events, event{et: eventRepositoryConfigChanged, source: "entrypoint"}) + + viperx.AddWatcher(func(e fsnotify.Event) error { + // TODO: clarify why we don't save initial state as a string and then compare it with the current state + // by invoking f.c.AccessRuleMatchingStrategy method? + if reflect.DeepEqual(initialConfig, viper.Get(configuration.ViperKeyAccessRuleMatchingStrategy)) { + f.r.Logger(). + Debug("Not reloading access rule matching strategy because configuration value has not changed.") + return nil + } + + f.enqueueEvent(events, event{et: eventMatchingStrategyChanged, source: "viper_watcher"}) + + return nil + }) + + f.enqueueEvent(events, event{et: eventMatchingStrategyChanged, source: "entrypoint"}) for { select { @@ -238,7 +257,7 @@ func (f *FetcherDefault) watch(ctx context.Context, watcher *fsnotify.Watcher, e WithField("op", e.Op.String()). Debugf("Detected file change in directory containing access rules. Triggering a reload.") - f.enqueueEvent(events, event{et: eventRepositoryConfigChange, source: "fsnotify"}) + f.enqueueEvent(events, event{et: eventRepositoryConfigChanged, source: "fsnotify"}) case e, ok := <-events: if !ok { // channel was closed @@ -247,7 +266,7 @@ func (f *FetcherDefault) watch(ctx context.Context, watcher *fsnotify.Watcher, e } switch e.et { - case eventRepositoryConfigChange: + case eventRepositoryConfigChanged: f.r.Logger(). WithField("event", "config_change"). WithField("source", e.source). @@ -255,6 +274,14 @@ func (f *FetcherDefault) watch(ctx context.Context, watcher *fsnotify.Watcher, e if err := f.configUpdate(ctx, watcher, f.c.AccessRuleRepositories(), events); err != nil { return err } + case eventMatchingStrategyChanged: + f.r.Logger(). + WithField("event", "config_change"). + WithField("source", e.source). + Debugf("Viper detected a configuration change, reloading config.") + if err := f.r.RuleRepository().SetMatchingStrategy(ctx, f.c.AccessRuleMatchingStrategy()); err != nil { + return errors.Wrapf(err, "unable to update matching strategy") + } case eventFileChanged: f.r.Logger(). WithField("event", "repository_change"). diff --git a/rule/repository.go b/rule/repository.go index 9a63761acb..520b0b63b4 100644 --- a/rule/repository.go +++ b/rule/repository.go @@ -22,6 +22,8 @@ package rule import ( "context" + + "github.com/ory/oathkeeper/driver/configuration" ) type Repository interface { @@ -29,4 +31,6 @@ type Repository interface { Set(context.Context, []Rule) error Get(context.Context, string) (*Rule, error) Count(context.Context) (int, error) + MatchingStrategy(context.Context) (configuration.MatchingStrategy, error) + SetMatchingStrategy(context.Context, configuration.MatchingStrategy) error } diff --git a/rule/repository_memory.go b/rule/repository_memory.go index 213052796d..8332a9695d 100644 --- a/rule/repository_memory.go +++ b/rule/repository_memory.go @@ -29,6 +29,7 @@ import ( "github.com/ory/x/viperx" + "github.com/ory/oathkeeper/driver/configuration" "github.com/ory/oathkeeper/helper" "github.com/ory/oathkeeper/x" @@ -44,8 +45,24 @@ type repositoryMemoryRegistry interface { type RepositoryMemory struct { sync.RWMutex - rules []Rule - r repositoryMemoryRegistry + rules []Rule + matchingStrategy configuration.MatchingStrategy + r repositoryMemoryRegistry +} + +// MatchingStrategy returns current MatchingStrategy. +func (m *RepositoryMemory) MatchingStrategy(_ context.Context) (configuration.MatchingStrategy, error) { + m.RLock() + defer m.RUnlock() + return m.matchingStrategy, nil +} + +// SetMatchingStrategy updates MatchingStrategy. +func (m *RepositoryMemory) SetMatchingStrategy(_ context.Context, ms configuration.MatchingStrategy) error { + m.Lock() + defer m.Unlock() + m.matchingStrategy = ms + return nil } func NewRepositoryMemory(r repositoryMemoryRegistry) *RepositoryMemory { From 71594cd21dbac0d9db63ece57ff610114ea08eea Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Wed, 8 Jan 2020 21:20:07 +0100 Subject: [PATCH 03/15] rule: change pipeline.Rule interface so that it does not return regexp object This change can help to unify regexp and glob matching strategies. Signed-off-by: Aynur Zulkarnaev --- pipeline/authz/keto_engine_acp_ory.go | 11 +++-------- pipeline/rule.go | 8 +++----- rule/rule.go | 23 +++++++++++++---------- 3 files changed, 19 insertions(+), 23 deletions(-) diff --git a/pipeline/authz/keto_engine_acp_ory.go b/pipeline/authz/keto_engine_acp_ory.go index 65d2fbe35a..5b69930788 100644 --- a/pipeline/authz/keto_engine_acp_ory.go +++ b/pipeline/authz/keto_engine_acp_ory.go @@ -94,11 +94,6 @@ func (a *AuthorizerKetoEngineACPORY) Authorize(r *http.Request, session *authn.A return err } - compiled, err := rule.CompileURL() - if err != nil { - return errors.WithStack(err) - } - subject := session.Subject if cf.Subject != "" { templateId := fmt.Sprintf("%s:%s", rule.GetID(), "subject") @@ -116,11 +111,11 @@ func (a *AuthorizerKetoEngineACPORY) Authorize(r *http.Request, session *authn.A var b bytes.Buffer u := fmt.Sprintf("%s://%s%s", r.URL.Scheme, r.URL.Host, r.URL.Path) - action, err := compiled.Replace(u, cf.RequiredAction, -1, -1) + action, err := rule.Replace(u, cf.RequiredAction) if err != nil { return errors.WithStack(err) } - resource, err := compiled.Replace(u, cf.RequiredResource, -1, -1) + resource, err := rule.Replace(u, cf.RequiredResource) if err != nil { return errors.WithStack(err) } @@ -140,10 +135,10 @@ func (a *AuthorizerKetoEngineACPORY) Authorize(r *http.Request, session *authn.A } req, err := http.NewRequest("POST", urlx.AppendPaths(baseURL, "/engines/acp/ory", flavor, "/allowed").String(), &b) - req.Header.Add("Content-Type", "application/json") if err != nil { return errors.WithStack(err) } + req.Header.Add("Content-Type", "application/json") res, err := a.client.Do(req) if err != nil { diff --git a/pipeline/rule.go b/pipeline/rule.go index 21acba499a..33a7ae002a 100644 --- a/pipeline/rule.go +++ b/pipeline/rule.go @@ -1,10 +1,8 @@ package pipeline -import ( - "github.com/dlclark/regexp2" -) - type Rule interface { GetID() string - CompileURL() (*regexp2.Regexp, error) + // Replace searches the input string and replaces each match (with the rule's pattern) + // found with the replacement text. + Replace(input, replacement string) (string, error) } diff --git a/rule/rule.go b/rule/rule.go index b46726540a..5c23eb208e 100644 --- a/rule/rule.go +++ b/rule/rule.go @@ -138,14 +138,6 @@ type Upstream struct { var _ json.Unmarshaler = new(Rule) -func NewRule() *Rule { - return &Rule{ - Match: &Match{}, - Authenticators: []Handler{}, - Mutators: []Handler{}, - } -} - func (r *Rule) UnmarshalJSON(raw []byte) error { var rr struct { ID string `json:"id"` @@ -184,7 +176,7 @@ func (r *Rule) IsMatching(method string, u *url.URL) (bool, error) { return false, nil } - c, err := r.CompileURL() + c, err := r.compileRegexp() if err != nil { return false, errors.WithStack(err) } @@ -192,7 +184,7 @@ func (r *Rule) IsMatching(method string, u *url.URL) (bool, error) { return c.MatchString(fmt.Sprintf("%s://%s%s", u.Scheme, u.Host, u.Path)) } -func (r *Rule) CompileURL() (*regexp2.Regexp, error) { +func (r *Rule) compileRegexp() (*regexp2.Regexp, error) { m := r.Match c := crc32.ChecksumIEEE([]byte(m.URL)) if m.compiledURL == nil || c != m.compiledURLChecksum { @@ -207,6 +199,17 @@ func (r *Rule) CompileURL() (*regexp2.Regexp, error) { return m.compiledURL, nil } +// Replace searches the input string and replaces each match (with the rule's pattern) +// found with the replacement text. +func (r *Rule) Replace(input, replacement string) (string, error) { + re, err := r.compileRegexp() + if err != nil { + return "", errors.WithStack(err) + } + + return re.Replace(input, replacement, -1, -1) +} + func stringInSlice(a string, list []string) bool { for _, b := range list { if strings.EqualFold(a, b) { From 93772fff27482456f650f10a69c6f2b1016c7b9b Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Wed, 8 Jan 2020 21:33:07 +0100 Subject: [PATCH 04/15] rule: added MatchingStrategy stub switch in rule.IsMatching Signed-off-by: Aynur Zulkarnaev --- rule/repository_memory.go | 4 ++-- rule/rule.go | 20 +++++++++++++++----- rule/rule_test.go | 8 +++++--- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/rule/repository_memory.go b/rule/repository_memory.go index 8332a9695d..a8bbcb56c0 100644 --- a/rule/repository_memory.go +++ b/rule/repository_memory.go @@ -121,14 +121,14 @@ func (m *RepositoryMemory) Set(ctx context.Context, rules []Rule) error { return nil } -func (m *RepositoryMemory) Match(ctx context.Context, method string, u *url.URL) (*Rule, error) { +func (m *RepositoryMemory) Match(_ context.Context, method string, u *url.URL) (*Rule, error) { m.Lock() defer m.Unlock() var rules []Rule for k := range m.rules { r := &m.rules[k] - if matched, err := r.IsMatching(method, u); err != nil { + if matched, err := r.IsMatching(m.matchingStrategy, method, u); err != nil { return nil, errors.WithStack(err) } else if matched { rules = append(rules, *r) diff --git a/rule/rule.go b/rule/rule.go index 5c23eb208e..b031ed32ca 100644 --- a/rule/rule.go +++ b/rule/rule.go @@ -31,6 +31,8 @@ import ( "github.com/pkg/errors" "github.com/ory/ladon/compiler" + + "github.com/ory/oathkeeper/driver/configuration" ) type Match struct { @@ -171,17 +173,25 @@ func (r *Rule) GetID() string { // IsMatching checks whether the provided url and method match the rule. // An error will be returned if a regexp timeout occurs. -func (r *Rule) IsMatching(method string, u *url.URL) (bool, error) { +func (r *Rule) IsMatching(strategy configuration.MatchingStrategy, method string, u *url.URL) (bool, error) { if !stringInSlice(method, r.Match.Methods) { return false, nil } - c, err := r.compileRegexp() - if err != nil { - return false, errors.WithStack(err) + switch strategy { + case configuration.Regexp: + c, err := r.compileRegexp() + if err != nil { + return false, errors.WithStack(err) + } + + return c.MatchString(fmt.Sprintf("%s://%s%s", u.Scheme, u.Host, u.Path)) + case configuration.Glob: + // TODO: implement } - return c.MatchString(fmt.Sprintf("%s://%s%s", u.Scheme, u.Host, u.Path)) + return false, errors.Errorf("unknown matching strategy: %v", strategy) + } func (r *Rule) compileRegexp() (*regexp2.Regexp, error) { diff --git a/rule/rule_test.go b/rule/rule_test.go index 4e11eba803..767ca3d285 100644 --- a/rule/rule_test.go +++ b/rule/rule_test.go @@ -26,6 +26,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/ory/oathkeeper/driver/configuration" ) func mustParse(t *testing.T, u string) *url.URL { @@ -69,7 +71,7 @@ func TestRule(t *testing.T) { } for ind, tcase := range tests { t.Run(string(ind), func(t *testing.T) { - matched, err := r.IsMatching(tcase.method, mustParse(t, tcase.url)) + matched, err := r.IsMatching(configuration.Regexp, tcase.method, mustParse(t, tcase.url)) assert.Equal(t, tcase.expectedMatch, matched) assert.Equal(t, tcase.expectedErr, err) }) @@ -111,7 +113,7 @@ func TestRule1(t *testing.T) { } for ind, tcase := range tests { t.Run(string(ind), func(t *testing.T) { - matched, err := r.IsMatching(tcase.method, mustParse(t, tcase.url)) + matched, err := r.IsMatching(configuration.Regexp, tcase.method, mustParse(t, tcase.url)) assert.Equal(t, tcase.expectedMatch, matched) assert.Equal(t, tcase.expectedErr, err) }) @@ -153,7 +155,7 @@ func TestRule2(t *testing.T) { } for ind, tcase := range tests { t.Run(string(ind), func(t *testing.T) { - matched, err := r.IsMatching(tcase.method, mustParse(t, tcase.url)) + matched, err := r.IsMatching(configuration.Regexp, tcase.method, mustParse(t, tcase.url)) assert.Equal(t, tcase.expectedMatch, matched) assert.Equal(t, tcase.expectedErr, err) }) From ce6da7254576df10d8d22df7eb0a06d60cbda96c Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Fri, 10 Jan 2020 20:27:02 +0100 Subject: [PATCH 05/15] docs: update API docs link Signed-off-by: Aynur Zulkarnaev --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 6fa6932ba1..ff7116661d 100644 --- a/README.md +++ b/README.md @@ -4,6 +4,8 @@ Chat | Forums | Newsletter

+ Guide | + API Docs | Guide | API Docs | Code Docs

From e906e2701406b20bdfeff0d066c85819f6cab0ea Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Sun, 12 Jan 2020 21:40:07 +0100 Subject: [PATCH 06/15] rule: added MatchingEngine interface and it's implementation based on glob. Signed-off-by: Aynur Zulkarnaev --- go.mod | 2 +- go.sum | 6 +- pipeline/authz/keto_engine_acp_ory.go | 4 +- pipeline/rule.go | 8 +- rule/engine_glob.py.go | 95 ++++++++++ rule/engine_glob_test.go | 259 ++++++++++++++++++++++++++ rule/engine_regexp.go | 46 +++++ rule/matcher_test.go | 3 +- rule/matching_engine.go | 19 ++ rule/repository_memory.go | 1 + rule/rule.go | 70 +++---- 11 files changed, 460 insertions(+), 53 deletions(-) create mode 100644 rule/engine_glob.py.go create mode 100644 rule/engine_glob_test.go create mode 100644 rule/engine_regexp.go create mode 100644 rule/matching_engine.go diff --git a/go.mod b/go.mod index 9bfdb715ab..fa843bc36d 100644 --- a/go.mod +++ b/go.mod @@ -20,11 +20,11 @@ require ( github.com/go-swagger/go-swagger v0.21.1-0.20200107003254-1c98855b472d github.com/gobuffalo/httptest v1.0.2 github.com/gobuffalo/packr/v2 v2.0.0-rc.15 + github.com/gobwas/glob v0.2.3 github.com/golang/gddo v0.0.0-20190904175337-72a348e765d2 github.com/golang/mock v1.3.1 github.com/google/uuid v1.1.1 github.com/gorilla/mux v1.7.1 // indirect - github.com/hashicorp/golang-lru v0.5.3 // indirect github.com/huandu/xstrings v1.2.0 // indirect github.com/imdario/mergo v0.3.7 github.com/julienschmidt/httprouter v1.2.0 diff --git a/go.sum b/go.sum index 6869e36cea..a29707b5a7 100644 --- a/go.sum +++ b/go.sum @@ -351,6 +351,8 @@ github.com/gobuffalo/uuid v2.0.5+incompatible/go.mod h1:ErhIzkRhm0FtRuiE/PeORqcw github.com/gobuffalo/validate v2.0.3+incompatible/go.mod h1:N+EtDe0J8252BgfzQUChBgfd6L93m9weay53EWFVsMM= github.com/gobuffalo/x v0.0.0-20181003152136-452098b06085/go.mod h1:WevpGD+5YOreDJznWevcn8NTmQEW5STSBgIkpkjzqXc= github.com/gobuffalo/x v0.0.0-20181007152206-913e47c59ca7/go.mod h1:9rDPXaB3kXdKWzMc4odGQQdG2e2DIEmANy5aSJ9yesY= +github.com/gobwas/glob v0.2.3 h1:A4xDbljILXROh+kObIiy5kIaPYD8e96x1tgBhUI5J+Y= +github.com/gobwas/glob v0.2.3/go.mod h1:d3Ez4x06l9bZtSvzIay5+Yzi0fmZzPgnTbPcKjJAkT8= github.com/gofrs/uuid v3.1.0+incompatible/go.mod h1:b2aQJv3Z4Fp6yNu3cdSllBxTCLRxnplIgP/c0N/04lM= github.com/gofrs/uuid v3.2.0+incompatible/go.mod h1:b2aQJv3Z4Fp6yNu3cdSllBxTCLRxnplIgP/c0N/04lM= github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ= @@ -411,8 +413,6 @@ github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgf github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY= github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= -github.com/hashicorp/golang-lru v0.5.3 h1:YPkqC67at8FYaadspW/6uE0COsBxS2656RLEr8Bppgk= -github.com/hashicorp/golang-lru v0.5.3/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4= github.com/hashicorp/hcl v1.0.0 h1:0Anlzjpi4vEasTeNFn2mLJgTSwt0+6sfsiTG8qcWGx4= github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ= github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU= @@ -556,8 +556,6 @@ github.com/ory/graceful v0.1.1 h1:zx+8tDObLPrG+7Tc8jKYlXsqWnLtOQA1IZ/FAAKHMXU= github.com/ory/graceful v0.1.1/go.mod h1:zqu70l95WrKHF4AZ6tXHvAqAvpY6M7g6ttaAVcMm7KU= github.com/ory/herodot v0.6.2 h1:zOb5MsuMn7AH9/Ewc/EK83yqcNViK1m1l3C2UuP3RcA= github.com/ory/herodot v0.6.2/go.mod h1:3BOneqcyBsVybCPAJoi92KN2BpJHcmDqAMcAAaJiJow= -github.com/ory/ladon v1.0.1 h1:zCEfqnv8Ro62id0Z9cVPRPv4ejTu6HHtuPbXmTWBxks= -github.com/ory/ladon v1.0.1/go.mod h1:1VhCA2mBtaMhRUS6VS0d9qrNVDQnFXqSRb5D0NvQUPY= github.com/ory/ladon v1.1.0 h1:6tgazU2J3Z3odPs1f0qn729kRXCAtlJROliuWUHedV0= github.com/ory/ladon v1.1.0/go.mod h1:25bNc/Glx/8xCH7MbItDxjvviAmFQ+aYxb1V1SE5wlg= github.com/ory/pagination v0.0.1/go.mod h1:d1ToRROAUleriPhmb2dYbhANhhLwZ8s395m2yJCDFh8= diff --git a/pipeline/authz/keto_engine_acp_ory.go b/pipeline/authz/keto_engine_acp_ory.go index 5b69930788..c57e667cd5 100644 --- a/pipeline/authz/keto_engine_acp_ory.go +++ b/pipeline/authz/keto_engine_acp_ory.go @@ -111,11 +111,11 @@ func (a *AuthorizerKetoEngineACPORY) Authorize(r *http.Request, session *authn.A var b bytes.Buffer u := fmt.Sprintf("%s://%s%s", r.URL.Scheme, r.URL.Host, r.URL.Path) - action, err := rule.Replace(u, cf.RequiredAction) + action, err := rule.ReplaceAllString(a.c.AccessRuleMatchingStrategy(), u, cf.RequiredAction) if err != nil { return errors.WithStack(err) } - resource, err := rule.Replace(u, cf.RequiredResource) + resource, err := rule.ReplaceAllString(a.c.AccessRuleMatchingStrategy(), u, cf.RequiredResource) if err != nil { return errors.WithStack(err) } diff --git a/pipeline/rule.go b/pipeline/rule.go index 33a7ae002a..56360d0fe1 100644 --- a/pipeline/rule.go +++ b/pipeline/rule.go @@ -1,8 +1,12 @@ package pipeline +import ( + "github.com/ory/oathkeeper/driver/configuration" +) + type Rule interface { GetID() string - // Replace searches the input string and replaces each match (with the rule's pattern) + // ReplaceAllString searches the input string and replaces each match (with the rule's pattern) // found with the replacement text. - Replace(input, replacement string) (string, error) + ReplaceAllString(strategy configuration.MatchingStrategy, input, replacement string) (string, error) } diff --git a/rule/engine_glob.py.go b/rule/engine_glob.py.go new file mode 100644 index 0000000000..0d249d76f1 --- /dev/null +++ b/rule/engine_glob.py.go @@ -0,0 +1,95 @@ +package rule + +import ( + "bytes" + "hash/crc32" + + "github.com/gobwas/glob" +) + +type globMatchingEngine struct { + compiled glob.Glob + checksum uint32 +} + +// Checksum of a saved pattern. +func (ge *globMatchingEngine) Checksum() uint32 { + return ge.checksum +} + +// IsMatching determines whether the input matches the pattern. +func (ge *globMatchingEngine) IsMatching(pattern, matchAgainst string) (bool, error) { + if err := ge.compile(pattern); err != nil { + return false, err + } + return ge.compiled.Match(matchAgainst), nil +} + +// ReplaceAllString is noop for now and always returns an error. +func (ge *globMatchingEngine) ReplaceAllString(_, _, _ string) (string, error) { + return "", ErrMethodNotImplemented +} + +func (ge *globMatchingEngine) compile(pattern string) error { + if checksum := crc32.ChecksumIEEE([]byte(pattern)); checksum != ge.checksum { + compiled, err := compileGlob(pattern, '<', '>') + if err != nil { + return err + } + ge.checksum = checksum + ge.compiled = compiled + } + return nil +} + +// delimiterIndices returns the first level delimiter indices from a string. +// It returns an error in case of unbalanced delimiters. +func delimiterIndices(s string, delimiterStart, delimiterEnd rune) ([]int, error) { + var level, idx int + idxs := make([]int, 0) + for ind := 0; ind < len(s); ind++ { + switch s[ind] { + case byte(delimiterStart): + if level++; level == 1 { + idx = ind + } + case byte(delimiterEnd): + if level--; level == 0 { + idxs = append(idxs, idx, ind+1) + } else if level < 0 { + return nil, ErrUnbalancedPattern + } + } + } + + if level != 0 { + return nil, ErrUnbalancedPattern + } + return idxs, nil +} + +func compileGlob(pattern string, delimiterStart, delimiterEnd rune) (glob.Glob, error) { + // Check if it is well-formed. + idxs, errBraces := delimiterIndices(pattern, delimiterStart, delimiterEnd) + if errBraces != nil { + return nil, errBraces + } + buffer := bytes.NewBufferString("") + + var end int + for ind := 0; ind < len(idxs); ind += 2 { + // Set all values we are interested in. + raw := pattern[end:idxs[ind]] + end = idxs[ind+1] + patt := pattern[idxs[ind]+1 : end-1] + buffer.WriteString(glob.QuoteMeta(raw)) + buffer.WriteString(patt) + } + + // Add the remaining. + raw := pattern[end:] + buffer.WriteString(glob.QuoteMeta(raw)) + + // Compile full regexp. + return glob.Compile(buffer.String()) +} diff --git a/rule/engine_glob_test.go b/rule/engine_glob_test.go new file mode 100644 index 0000000000..da08639c5c --- /dev/null +++ b/rule/engine_glob_test.go @@ -0,0 +1,259 @@ +package rule + +import ( + "strconv" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestDelimiters(t *testing.T) { + var tests = []struct { + input string + out []int + err error + }{ + { + input: "<", + err: ErrUnbalancedPattern, + }, + { + input: ">", + err: ErrUnbalancedPattern, + }, + { + input: ">>", + err: ErrUnbalancedPattern, + }, + { + input: "><>", + err: ErrUnbalancedPattern, + }, + { + input: "foo.barvar", + err: ErrUnbalancedPattern, + }, + { + input: "foo.bar>var", + err: ErrUnbalancedPattern, + }, + { + input: "foo.bar<<>>", + out: []int{ + 7, 11, + }, + }, + { + input: "foo.bar<<>><>", + out: []int{ + 7, 11, + 11, 13, + }, + }, + { + input: "foo.bar<<>><>tt<>", + out: []int{ + 7, 11, + 11, 13, + 15, 17, + }, + }, + } + + for tn, tc := range tests { + t.Run(strconv.Itoa(tn), func(t *testing.T) { + out, err := delimiterIndices(tc.input, '<', '>') + assert.Equal(t, tc.out, out) + assert.Equal(t, tc.err, err) + + }) + } +} + +func TestIsMatch(t *testing.T) { + type args struct { + pattern string + matchAgainst string + } + tests := []struct { + name string + args args + want bool + wantErr bool + }{ + { + name: "question mark1", + args: args{ + pattern: `urn:foo:`, + matchAgainst: "urn:foo:user", + }, + want: false, + wantErr: false, + }, + { + name: "question mark2", + args: args{ + pattern: `urn:foo:`, + matchAgainst: "urn:foo:u", + }, + want: true, + wantErr: false, + }, + { + name: "question mark3", + args: args{ + pattern: `urn:foo:`, + matchAgainst: "urn:foo:", + }, + want: false, + wantErr: false, + }, + { + name: "question mark4", + args: args{ + pattern: `urn:foo:&&`, + matchAgainst: "urn:foo:w&&r", + }, + want: true, + wantErr: false, + }, + { + name: "question mark5 - both as a special char and a literal", + args: args{ + pattern: `urn:foo:?`, + matchAgainst: "urn:foo:w&r", + }, + want: false, + wantErr: false, + }, + { + name: "question mark5 - both as a special char and a literal1", + args: args{ + pattern: `urn:foo:?`, + matchAgainst: "urn:foo:w?r", + }, + want: true, + wantErr: false, + }, + { + name: "asterisk", + args: args{ + pattern: `urn:foo:<*>`, + matchAgainst: "urn:foo:user", + }, + want: true, + wantErr: false, + }, + { + name: "asterisk1", + args: args{ + pattern: `urn:foo:<*>`, + matchAgainst: "urn:foo:", + }, + want: true, + wantErr: false, + }, + { + name: "asterisk2", + args: args{ + pattern: `urn:foo:<*>:<*>`, + matchAgainst: "urn:foo:usr:swen", + }, + want: true, + wantErr: false, + }, + { + name: "asterisk: both as a special char and a literal", + args: args{ + pattern: `*:foo:<*>:<*>`, + matchAgainst: "urn:foo:usr:swen", + }, + want: false, + wantErr: false, + }, + { + name: "asterisk: both as a special char and a literal1", + args: args{ + pattern: `*:foo:<*>:<*>`, + matchAgainst: "*:foo:usr:swen", + }, + want: true, + wantErr: false, + }, + { + name: "asterisk + question mark", + args: args{ + pattern: `urn:foo:<*>:role:`, + matchAgainst: "urn:foo:usr:role:a", + }, + want: true, + wantErr: false, + }, + { + name: "asterisk + question mark1", + args: args{ + pattern: `urn:foo:<*>:role:`, + matchAgainst: "urn:foo:usr:role:admin", + }, + want: false, + wantErr: false, + }, + { + name: "square brackets", + args: args{ + pattern: `urn:foo:`, + matchAgainst: "urn:foo:moon", + }, + want: false, + wantErr: false, + }, + { + name: "square brackets1", + args: args{ + pattern: `urn:foo:`, + matchAgainst: "urn:foo:man", + }, + want: true, + wantErr: false, + }, + { + name: "square brackets2", + args: args{ + pattern: `urn:foo:`, + matchAgainst: "urn:foo:man", + }, + want: false, + wantErr: false, + }, + { + name: "square brackets3", + args: args{ + pattern: `urn:foo:`, + matchAgainst: "urn:foo:min", + }, + want: true, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + globEngine := new(globMatchingEngine) + got, err := globEngine.IsMatching(tt.args.pattern, tt.args.matchAgainst) + if (err != nil) != tt.wantErr { + t.Errorf("IsMatching() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("IsMatching() got = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/rule/engine_regexp.go b/rule/engine_regexp.go new file mode 100644 index 0000000000..833783b777 --- /dev/null +++ b/rule/engine_regexp.go @@ -0,0 +1,46 @@ +package rule + +import ( + "hash/crc32" + + "github.com/dlclark/regexp2" + "github.com/ory/ladon/compiler" +) + +type regexpMatchingEngine struct { + compiled *regexp2.Regexp + checksum uint32 +} + +func (re *regexpMatchingEngine) compile(pattern string) error { + if checksum := crc32.ChecksumIEEE([]byte(pattern)); checksum != re.checksum { + compiled, err := compiler.CompileRegex(pattern, '<', '>') + if err != nil { + return err + } + re.compiled = compiled + re.checksum = checksum + } + return nil +} + +// Checksum of a saved pattern. +func (re *regexpMatchingEngine) Checksum() uint32 { + return re.checksum +} + +// IsMatching determines whether the input matches the pattern. +func (re *regexpMatchingEngine) IsMatching(pattern, matchAgainst string) (bool, error) { + if err := re.compile(pattern); err != nil { + return false, err + } + return re.compiled.MatchString(matchAgainst) +} + +// ReplaceAllString replaces all matches in `input` with `replacement`. +func (re *regexpMatchingEngine) ReplaceAllString(pattern, input, replacement string) (string, error) { + if err := re.compile(pattern); err != nil { + return "", err + } + return re.compiled.Replace(input, replacement, -1, -1) +} diff --git a/rule/matcher_test.go b/rule/matcher_test.go index f82ee4a813..6856c9b9ca 100644 --- a/rule/matcher_test.go +++ b/rule/matcher_test.go @@ -105,8 +105,7 @@ func TestMatcher(t *testing.T) { require.NoError(t, err) got, err := matcher.Get(context.Background(), r.ID) require.NoError(t, err) - assert.NotEmpty(t, got.Match.compiledURL) - assert.NotEmpty(t, got.Match.compiledURLChecksum) + assert.NotEmpty(t, got.matchingEngine.Checksum()) }) require.NoError(t, matcher.Set(context.Background(), testRules[1:])) diff --git a/rule/matching_engine.go b/rule/matching_engine.go new file mode 100644 index 0000000000..28d1de6bdf --- /dev/null +++ b/rule/matching_engine.go @@ -0,0 +1,19 @@ +package rule + +import ( + "github.com/pkg/errors" +) + +// common errors for MatchingEngine. +var ( + ErrUnbalancedPattern = errors.New("unbalanced pattern") + ErrMethodNotImplemented = errors.New("the method is not implemented") + ErrUnknownMatchingStrategy = errors.New("unknown matching strategy") +) + +// MatchingEngine describes an interface of matching engine such as regexp or glob. +type MatchingEngine interface { + IsMatching(pattern, matchAgainst string) (bool, error) + ReplaceAllString(pattern, input, replacement string) (string, error) + Checksum() uint32 +} diff --git a/rule/repository_memory.go b/rule/repository_memory.go index a8bbcb56c0..9ee4b7d46f 100644 --- a/rule/repository_memory.go +++ b/rule/repository_memory.go @@ -133,6 +133,7 @@ func (m *RepositoryMemory) Match(_ context.Context, method string, u *url.URL) ( } else if matched { rules = append(rules, *r) } + m.rules[k] = *r } if len(rules) == 0 { diff --git a/rule/rule.go b/rule/rule.go index b031ed32ca..ade0c3dc2b 100644 --- a/rule/rule.go +++ b/rule/rule.go @@ -23,15 +23,11 @@ package rule import ( "encoding/json" "fmt" - "hash/crc32" "net/url" "strings" - "github.com/dlclark/regexp2" "github.com/pkg/errors" - "github.com/ory/ladon/compiler" - "github.com/ory/oathkeeper/driver/configuration" ) @@ -51,9 +47,6 @@ type Match struct { // You can use regular expressions in this field to match more than one url. Regular expressions are encapsulated in // brackets < and >. The following example matches all paths of the domain `mydomain.com`: `https://mydomain.com/<.*>`. URL string `json:"url"` - - compiledURL *regexp2.Regexp - compiledURLChecksum uint32 } type Handler struct { @@ -124,6 +117,8 @@ type Rule struct { // Upstream is the location of the server where requests matching this rule should be forwarded to. Upstream Upstream `json:"upstream"` + + matchingEngine MatchingEngine } type Upstream struct { @@ -151,6 +146,7 @@ func (r *Rule) UnmarshalJSON(raw []byte) error { Mutators []Handler `json:"mutators"` Errors []ErrorHandler `json:"errors"` Upstream Upstream `json:"upstream"` + matchingEngine MatchingEngine } transformed, err := migrateRuleJSON(raw) @@ -177,47 +173,21 @@ func (r *Rule) IsMatching(strategy configuration.MatchingStrategy, method string if !stringInSlice(method, r.Match.Methods) { return false, nil } - - switch strategy { - case configuration.Regexp: - c, err := r.compileRegexp() - if err != nil { - return false, errors.WithStack(err) - } - - return c.MatchString(fmt.Sprintf("%s://%s%s", u.Scheme, u.Host, u.Path)) - case configuration.Glob: - // TODO: implement + if err := ensureMatchingEngine(r, strategy); err != nil { + return false, err } - - return false, errors.Errorf("unknown matching strategy: %v", strategy) - + matchAgainst := fmt.Sprintf("%s://%s%s", u.Scheme, u.Host, u.Path) + return r.matchingEngine.IsMatching(r.Match.URL, matchAgainst) } -func (r *Rule) compileRegexp() (*regexp2.Regexp, error) { - m := r.Match - c := crc32.ChecksumIEEE([]byte(m.URL)) - if m.compiledURL == nil || c != m.compiledURLChecksum { - regex, err := compiler.CompileRegex(m.URL, '<', '>') - if err != nil { - return nil, errors.Wrap(err, "Unable to compile URL matcher") - } - m.compiledURL = regex - m.compiledURLChecksum = c - } - - return m.compiledURL, nil -} - -// Replace searches the input string and replaces each match (with the rule's pattern) +// ReplaceAllString searches the input string and replaces each match (with the rule's pattern) // found with the replacement text. -func (r *Rule) Replace(input, replacement string) (string, error) { - re, err := r.compileRegexp() - if err != nil { - return "", errors.WithStack(err) +func (r *Rule) ReplaceAllString(strategy configuration.MatchingStrategy, input, replacement string) (string, error) { + if err := ensureMatchingEngine(r, strategy); err != nil { + return "", err } - return re.Replace(input, replacement, -1, -1) + return r.matchingEngine.ReplaceAllString(r.Match.URL, input, replacement) } func stringInSlice(a string, list []string) bool { @@ -228,3 +198,19 @@ func stringInSlice(a string, list []string) bool { } return false } + +func ensureMatchingEngine(rule *Rule, strategy configuration.MatchingStrategy) error { + if rule.matchingEngine != nil { + return nil + } + switch strategy { + case configuration.Glob: + rule.matchingEngine = new(globMatchingEngine) + return nil + case "", configuration.Regexp: + rule.matchingEngine = new(regexpMatchingEngine) + return nil + } + + return errors.Wrap(ErrUnknownMatchingStrategy, string(strategy)) +} From 320981fd89105678a520cd17ac8d2058120ad72e Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Sun, 12 Jan 2020 23:04:36 +0100 Subject: [PATCH 07/15] rule: added tests for glob MatchingEngine Signed-off-by: Aynur Zulkarnaev --- proxy/proxy_test.go | 231 +++++++++++++++++++++++++++++++++++ rule/fetcher_default_test.go | 19 ++- rule/matcher_test.go | 98 ++++++++++++++- rule/repository_test.go | 14 +++ rule/rule_test.go | 42 +++++++ 5 files changed, 393 insertions(+), 11 deletions(-) diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index 3f467fcbc1..c12ade2594 100644 --- a/proxy/proxy_test.go +++ b/proxy/proxy_test.go @@ -21,6 +21,7 @@ package proxy_test import ( + "context" "fmt" "io/ioutil" "net/http" @@ -276,6 +277,236 @@ backend_url=%s } } +func TestProxy_GlobMatchingEngine(t *testing.T) { + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // assert.NotEmpty(t, helper.BearerTokenFromRequest(r)) + fmt.Fprint(w, "authorization="+r.Header.Get("Authorization")+"\n") + fmt.Fprint(w, "host="+r.Host+"\n") + fmt.Fprint(w, "url="+r.URL.String()) + })) + defer backend.Close() + + viper.Set(configuration.ViperKeyAccessRuleMatchingStrategy, configuration.Glob) + + conf := internal.NewConfigurationWithDefaults() + reg := internal.NewRegistry(conf).WithBrokenPipelineMutator() + + d := reg.Proxy() + ts := httptest.NewServer(&httputil.ReverseProxy{Director: d.Director, Transport: d}) + defer ts.Close() + + viper.Set(configuration.ViperKeyAuthenticatorNoopIsEnabled, true) + viper.Set(configuration.ViperKeyAuthenticatorUnauthorizedIsEnabled, true) + viper.Set(configuration.ViperKeyAuthenticatorAnonymousIsEnabled, true) + viper.Set(configuration.ViperKeyAuthorizerAllowIsEnabled, true) + viper.Set(configuration.ViperKeyAuthorizerDenyIsEnabled, true) + viper.Set(configuration.ViperKeyMutatorNoopIsEnabled, true) + viper.Set(configuration.ViperKeyErrorsWWWAuthenticateIsEnabled, true) + + ruleNoOpAuthenticator := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "noop"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: backend.URL}, + } + ruleNoOpAuthenticatorModifyUpstream := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "noop"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: backend.URL, StripPath: "/strip-path/", PreserveHost: true}, + } + + for k, tc := range []struct { + url string + code int + messages []string + rules []rule.Rule + transform func(r *http.Request) + d string + }{ + { + d: "should fail because url does not exist in rule set", + url: ts.URL + "/invalid", + rules: []rule.Rule{}, + code: http.StatusNotFound, + }, + { + d: "should fail because url does exist but is matched by two rules", + url: ts.URL + "/authn-noop/1234", + rules: []rule.Rule{ruleNoOpAuthenticator, ruleNoOpAuthenticator}, + code: http.StatusInternalServerError, + }, + { + d: "should pass", + url: ts.URL + "/authn-noop/1234", + rules: []rule.Rule{ruleNoOpAuthenticator}, + code: http.StatusOK, + transform: func(r *http.Request) { + r.Header.Add("Authorization", "bearer token") + }, + messages: []string{ + "authorization=bearer token", + "url=/authn-noop/1234", + "host=" + urlx.ParseOrPanic(backend.URL).Host, + }, + }, + { + d: "should pass", + url: ts.URL + "/strip-path/authn-noop/1234", + rules: []rule.Rule{ruleNoOpAuthenticatorModifyUpstream}, + code: http.StatusOK, + transform: func(r *http.Request) { + r.Header.Add("Authorization", "bearer token") + }, + messages: []string{ + "authorization=bearer token", + "url=/authn-noop/1234", + "host=" + urlx.ParseOrPanic(ts.URL).Host, + }, + }, + { + d: "should fail because no authorizer was configured", + url: ts.URL + "/authn-anon/authz-none/cred-none/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-none/cred-none/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, + transform: func(r *http.Request) { + r.Header.Add("Authorization", "bearer token") + }, + code: http.StatusUnauthorized, + }, + { + d: "should fail because no credentials issuer was configured", + url: ts.URL + "/authn-anon/authz-allow/cred-none/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-none/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, + code: http.StatusInternalServerError, + }, + { + d: "should pass with anonymous and everything else set to noop", + url: ts.URL + "/authn-anon/authz-allow/cred-noop/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, + code: http.StatusOK, + messages: []string{ + "authorization=", + "url=/authn-anon/authz-allow/cred-noop/1234", + "host=" + urlx.ParseOrPanic(backend.URL).Host, + }, + }, + { + d: "should fail when authorizer fails", + url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "deny"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, + code: http.StatusForbidden, + }, + { + d: "should fail when authorizer fails and send www_authenticate as defined in the rule", + url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "deny"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: backend.URL}, + Errors: []rule.ErrorHandler{{Handler: "www_authenticate"}}, + }}, + code: http.StatusUnauthorized, + }, + { + d: "should fail when authenticator fails", + url: ts.URL + "/authn-broken/authz-none/cred-none/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-broken/authz-none/cred-none/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "unauthorized"}}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, + code: http.StatusUnauthorized, + }, + { + d: "should fail because no mutator was configured", + url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, + code: http.StatusInternalServerError, + }, + { + d: "should fail when one of the mutators fails", + url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}, {Handler: "broken"}}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, + code: http.StatusInternalServerError, + }, + { + d: "should fail when credentials issuer fails", + url: ts.URL + "/authn-anonymous/authz-allow/cred-broken/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "broken"}}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, + code: http.StatusInternalServerError, + }, + } { + t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { + reg.RuleRepository().(*rule.RepositoryMemory).WithRules(tc.rules) + reg.RuleRepository().SetMatchingStrategy(context.Background(), configuration.Glob) + + req, err := http.NewRequest("GET", tc.url, nil) + require.NoError(t, err) + if tc.transform != nil { + tc.transform(req) + } + + res, err := http.DefaultClient.Do(req) + require.NoError(t, err) + + greeting, err := ioutil.ReadAll(res.Body) + require.NoError(t, res.Body.Close()) + require.NoError(t, err) + + assert.Equal(t, tc.code, res.StatusCode, "%s", res.Body) + for _, m := range tc.messages { + assert.True(t, strings.Contains(string(greeting), m), `Value "%s" not found in message: +%s +proxy_url=%s +backend_url=%s +`, m, greeting, ts.URL, backend.URL) + } + }) + } +} + func TestConfigureBackendURL(t *testing.T) { for k, tc := range []struct { r *http.Request diff --git a/rule/fetcher_default_test.go b/rule/fetcher_default_test.go index 3fc37bea8d..eed58df9b4 100644 --- a/rule/fetcher_default_test.go +++ b/rule/fetcher_default_test.go @@ -54,10 +54,11 @@ func TestFetcherWatchConfig(t *testing.T) { }() for k, tc := range []struct { - config string - tmpContent string - expectIDs []string - expectNone bool + config string + tmpContent string + expectIDs []string + expectNone bool + expectedStrategy configuration.MatchingStrategy }{ {config: ""}, { @@ -86,14 +87,18 @@ access_rules: access_rules: repositories: - file://../test/stub/rules.yaml + matching_strategy: glob `, - expectIDs: []string{"test-rule-1-yaml"}, + expectIDs: []string{"test-rule-1-yaml"}, + expectedStrategy: configuration.Glob, }, { config: ` access_rules: repositories: + matching_strategy: regexp `, + expectedStrategy: configuration.Regexp, }, } { t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { @@ -104,6 +109,10 @@ access_rules: require.NoError(t, err) require.Len(t, rules, len(tc.expectIDs)) + strategy, err := r.RuleRepository().MatchingStrategy(context.Background()) + require.NoError(t, err) + require.Equal(t, tc.expectedStrategy, strategy) + ids := make([]string, len(rules)) for k, r := range rules { ids[k] = r.ID diff --git a/rule/matcher_test.go b/rule/matcher_test.go index 6856c9b9ca..cf44829b62 100644 --- a/rule/matcher_test.go +++ b/rule/matcher_test.go @@ -28,8 +28,16 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/ory/oathkeeper/driver/configuration" ) +func mustParseURL(t *testing.T, u string) *url.URL { + p, err := url.Parse(u) + require.NoError(t, err) + return p +} + var testRules = []Rule{ { ID: "foo1", @@ -60,12 +68,6 @@ var testRules = []Rule{ }, } -func mustParseURL(t *testing.T, u string) *url.URL { - p, err := url.Parse(u) - require.NoError(t, err) - return p -} - func TestMatcher(t *testing.T) { type m interface { Matcher @@ -118,3 +120,87 @@ func TestMatcher(t *testing.T) { }) } } + +var testRulesGlob = []Rule{ + { + ID: "foo1", + Match: &Match{URL: "https://localhost:1234/<{foo*,bar*}>", Methods: []string{"POST"}}, + Description: "Create users rule", + Authorizer: Handler{Handler: "allow", Config: []byte(`{"type":"any"}`)}, + Authenticators: []Handler{{Handler: "anonymous", Config: []byte(`{"name":"anonymous1"}`)}}, + Mutators: []Handler{{Handler: "id_token", Config: []byte(`{"issuer":"anything"}`)}}, + Upstream: Upstream{URL: "http://localhost:1235/", StripPath: "/bar", PreserveHost: true}, + }, + { + ID: "foo2", + Match: &Match{URL: "https://localhost:34/<{baz*,bar*}>", Methods: []string{"GET"}}, + Description: "Get users rule", + Authorizer: Handler{Handler: "deny", Config: []byte(`{"type":"any"}`)}, + Authenticators: []Handler{{Handler: "oauth2_introspection", Config: []byte(`{"name":"anonymous1"}`)}}, + Mutators: []Handler{{Handler: "id_token", Config: []byte(`{"issuer":"anything"}`)}}, + Upstream: Upstream{URL: "http://localhost:333/", StripPath: "/foo", PreserveHost: false}, + }, + { + ID: "foo3", + Match: &Match{URL: "https://localhost:343/<{baz*,bar*}>", Methods: []string{"GET"}}, + Description: "Get users rule", + Authorizer: Handler{Handler: "deny"}, + Authenticators: []Handler{{Handler: "oauth2_introspection"}}, + Mutators: []Handler{{Handler: "id_token"}}, + Upstream: Upstream{URL: "http://localhost:3333/", StripPath: "/foo", PreserveHost: false}, + }, +} + +func TestMatcherGlob(t *testing.T) { + type m interface { + Matcher + Repository + } + + var testMatcher = func(t *testing.T, matcher Matcher, method string, url string, expectErr bool, expect *Rule) { + r, err := matcher.Match(context.Background(), method, mustParseURL(t, url)) + if expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + assert.EqualValues(t, *expect, *r) + } + } + + for name, matcher := range map[string]m{ + "memory": NewRepositoryMemory(new(mockRepositoryRegistry)), + } { + t.Run(fmt.Sprintf("matcher=%s", name), func(t *testing.T) { + require.NoError(t, matcher.SetMatchingStrategy(context.Background(), configuration.Glob)) + t.Run("case=empty", func(t *testing.T) { + testMatcher(t, matcher, "GET", "https://localhost:34/baz", true, nil) + testMatcher(t, matcher, "POST", "https://localhost:1234/foo", true, nil) + testMatcher(t, matcher, "DELETE", "https://localhost:1234/foo", true, nil) + }) + + require.NoError(t, matcher.Set(context.Background(), testRulesGlob)) + + t.Run("case=created", func(t *testing.T) { + testMatcher(t, matcher, "GET", "https://localhost:34/baz", false, &testRulesGlob[1]) + testMatcher(t, matcher, "POST", "https://localhost:1234/foo", false, &testRulesGlob[0]) + testMatcher(t, matcher, "DELETE", "https://localhost:1234/foo", true, nil) + }) + + t.Run("case=cache", func(t *testing.T) { + r, err := matcher.Match(context.Background(), "GET", mustParseURL(t, "https://localhost:34/baz")) + require.NoError(t, err) + got, err := matcher.Get(context.Background(), r.ID) + require.NoError(t, err) + assert.NotEmpty(t, got.matchingEngine.Checksum()) + }) + + require.NoError(t, matcher.Set(context.Background(), testRulesGlob[1:])) + + t.Run("case=updated", func(t *testing.T) { + testMatcher(t, matcher, "GET", "https://localhost:34/baz", false, &testRulesGlob[1]) + testMatcher(t, matcher, "POST", "https://localhost:1234/foo", true, nil) + testMatcher(t, matcher, "DELETE", "https://localhost:1234/foo", true, nil) + }) + }) + } +} diff --git a/rule/repository_test.go b/rule/repository_test.go index 6ba7f5b1a9..103485c966 100644 --- a/rule/repository_test.go +++ b/rule/repository_test.go @@ -35,6 +35,8 @@ import ( "github.com/stretchr/testify/require" "github.com/ory/x/sqlcon/dockertest" + + "github.com/ory/oathkeeper/driver/configuration" ) func TestMain(m *testing.M) { @@ -118,6 +120,18 @@ func TestRepository(t *testing.T) { count, err = repo.Count(context.Background()) require.NoError(t, err) assert.Equal(t, len(rules)-1, count) + + strategy, err := repo.MatchingStrategy(context.Background()) + require.NoError(t, err) + require.Equal(t, configuration.MatchingStrategy(""), strategy) + + err = repo.SetMatchingStrategy(context.Background(), configuration.Glob) + require.NoError(t, err) + + strategy, err = repo.MatchingStrategy(context.Background()) + require.NoError(t, err) + require.Equal(t, configuration.Glob, strategy) + }) } diff --git a/rule/rule_test.go b/rule/rule_test.go index 767ca3d285..27ed34c83f 100644 --- a/rule/rule_test.go +++ b/rule/rule_test.go @@ -161,3 +161,45 @@ func TestRule2(t *testing.T) { }) } } + +func TestRuleGlob(t *testing.T) { + r := &Rule{ + Match: &Match{ + Methods: []string{"DELETE"}, + URL: "https://localhost/users/<[0-9]*>", + }, + } + + var tests = []struct { + method string + url string + expectedMatch bool + expectedErr error + }{ + { + method: "DELETE", + url: "https://localhost/users/1234", + expectedMatch: true, + expectedErr: nil, + }, + { + method: "DELETE", + url: "https://localhost/users/1234?key=value&key1=value1", + expectedMatch: true, + expectedErr: nil, + }, + { + method: "DELETE", + url: "https://localhost/users/abcd", + expectedMatch: false, + expectedErr: nil, + }, + } + for ind, tcase := range tests { + t.Run(string(ind), func(t *testing.T) { + matched, err := r.IsMatching(configuration.Glob, tcase.method, mustParse(t, tcase.url)) + assert.Equal(t, tcase.expectedMatch, matched) + assert.Equal(t, tcase.expectedErr, err) + }) + } +} From 7288a4b1fb1daf008f77d0157720f5d215050625 Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Sun, 12 Jan 2020 23:14:21 +0100 Subject: [PATCH 08/15] rule: changed MatchingStrategy type from int to string Signed-off-by: Aynur Zulkarnaev --- driver/configuration/provider.go | 9 +++++---- driver/configuration/provider_viper.go | 2 +- helper/errors.go | 6 ++++++ pipeline/authz/keto_engine_acp_ory.go | 5 +++++ 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/driver/configuration/provider.go b/driver/configuration/provider.go index 36a46aab3b..4c28e9839c 100644 --- a/driver/configuration/provider.go +++ b/driver/configuration/provider.go @@ -20,12 +20,13 @@ const ( ) // MatchingStrategy defines matching strategy such as Regexp or Glob. -type MatchingStrategy int +// Empty string defaults to "regexp" +type MatchingStrategy string -// Possible values. +// Possible matching strategies. const ( - Regexp MatchingStrategy = iota - Glob + Regexp MatchingStrategy = "regexp" + Glob MatchingStrategy = "glob" ) type Provider interface { diff --git a/driver/configuration/provider_viper.go b/driver/configuration/provider_viper.go index 2b8f48c427..42b62d063f 100644 --- a/driver/configuration/provider_viper.go +++ b/driver/configuration/provider_viper.go @@ -134,7 +134,7 @@ func (v *ViperProvider) AccessRuleRepositories() []url.URL { // AccessRuleMatchingStrategy returns current MatchingStrategy. func (v *ViperProvider) AccessRuleMatchingStrategy() MatchingStrategy { - return MatchingStrategy(viperx.GetInt(v.l, ViperKeyAccessRuleMatchingStrategy, int(Regexp))) + return MatchingStrategy(viperx.GetString(v.l, ViperKeyAccessRuleMatchingStrategy, "")) } func (v *ViperProvider) CORSEnabled(iface string) bool { diff --git a/helper/errors.go b/helper/errors.go index 3ba6b42d58..34bffbf820 100644 --- a/helper/errors.go +++ b/helper/errors.go @@ -47,6 +47,12 @@ var ( CodeField: http.StatusInternalServerError, StatusField: http.StatusText(http.StatusInternalServerError), } + // TODO: discuss the text and status code + ErrNonRegexpMatchingStrategy = &herodot.DefaultError{ + ErrorField: "The matched handler uses Regexp MatchingStrategy which is not selected in the configuration", + CodeField: http.StatusInternalServerError, + StatusField: http.StatusText(http.StatusInternalServerError), + } ErrMatchesNoRule = &herodot.DefaultError{ ErrorField: "Requested url does not match any rules", CodeField: http.StatusNotFound, diff --git a/pipeline/authz/keto_engine_acp_ory.go b/pipeline/authz/keto_engine_acp_ory.go index c57e667cd5..b81945511a 100644 --- a/pipeline/authz/keto_engine_acp_ory.go +++ b/pipeline/authz/keto_engine_acp_ory.go @@ -94,6 +94,11 @@ func (a *AuthorizerKetoEngineACPORY) Authorize(r *http.Request, session *authn.A return err } + // only Regexp matching strategy is supported for now. + if !(a.c.AccessRuleMatchingStrategy() == "" || a.c.AccessRuleMatchingStrategy() == configuration.Regexp) { + return helper.ErrNonRegexpMatchingStrategy + } + subject := session.Subject if cf.Subject != "" { templateId := fmt.Sprintf("%s:%s", rule.GetID(), "subject") From 1c93b5d90cdb05f680e05cfe36904c3ede52597e Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Mon, 13 Jan 2020 11:41:50 +0100 Subject: [PATCH 09/15] rule: add tests for glob matching strategy Signed-off-by: Aynur Zulkarnaev --- api/decision_test.go | 193 +++++++++++++++++++++++++++++++ driver/configuration/provider.go | 2 +- proxy/proxy_test.go | 3 +- rule/rule.go | 9 +- 4 files changed, 201 insertions(+), 6 deletions(-) diff --git a/api/decision_test.go b/api/decision_test.go index 334909491e..0fe68a3534 100644 --- a/api/decision_test.go +++ b/api/decision_test.go @@ -21,6 +21,7 @@ package api_test import ( + "context" "fmt" "net/http" "net/http/httptest" @@ -230,3 +231,195 @@ func TestDecisionAPI(t *testing.T) { }) } } + +func TestDecisionAPIGlob(t *testing.T) { + conf := internal.NewConfigurationWithDefaults() + viper.Set(configuration.ViperKeyAuthenticatorNoopIsEnabled, true) + viper.Set(configuration.ViperKeyAuthenticatorUnauthorizedIsEnabled, true) + viper.Set(configuration.ViperKeyAuthenticatorAnonymousIsEnabled, true) + viper.Set(configuration.ViperKeyAuthorizerAllowIsEnabled, true) + viper.Set(configuration.ViperKeyAuthorizerDenyIsEnabled, true) + viper.Set(configuration.ViperKeyMutatorNoopIsEnabled, true) + viper.Set(configuration.ViperKeyErrorsWWWAuthenticateIsEnabled, true) + reg := internal.NewRegistry(conf).WithBrokenPipelineMutator() + + d := reg.DecisionHandler() + + n := negroni.New(d) + n.UseHandler(httprouter.New()) + + ts := httptest.NewServer(n) + defer ts.Close() + + ruleNoOpAuthenticator := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "noop"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: ""}, + } + ruleNoOpAuthenticatorModifyUpstream := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "noop"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: "", StripPath: "/strip-path/", PreserveHost: true}, + } + + for k, tc := range []struct { + url string + code int + messages []string + rules []rule.Rule + transform func(r *http.Request) + authz string + d string + }{ + { + d: "should fail because url does not exist in rule set", + url: ts.URL + "/decisions" + "/invalid", + rules: []rule.Rule{}, + code: http.StatusNotFound, + }, + { + d: "should fail because url does exist but is matched by two rules", + url: ts.URL + "/decisions" + "/authn-noop/1234", + rules: []rule.Rule{ruleNoOpAuthenticator, ruleNoOpAuthenticator}, + code: http.StatusInternalServerError, + }, + { + d: "should pass", + url: ts.URL + "/decisions" + "/authn-noop/1234", + rules: []rule.Rule{ruleNoOpAuthenticator}, + code: http.StatusOK, + transform: func(r *http.Request) { + r.Header.Add("Authorization", "bearer token") + }, + authz: "bearer token", + }, + { + d: "should pass", + url: ts.URL + "/decisions" + "/strip-path/authn-noop/1234", + rules: []rule.Rule{ruleNoOpAuthenticatorModifyUpstream}, + code: http.StatusOK, + transform: func(r *http.Request) { + r.Header.Add("Authorization", "bearer token") + }, + authz: "bearer token", + }, + { + d: "should fail because no authorizer was configured", + url: ts.URL + "/decisions" + "/authn-anon/authz-none/cred-none/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-none/cred-none/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + transform: func(r *http.Request) { + r.Header.Add("Authorization", "bearer token") + }, + code: http.StatusUnauthorized, + }, + { + d: "should fail because no mutator was configured", + url: ts.URL + "/decisions" + "/authn-anon/authz-allow/cred-none/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-none/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Upstream: rule.Upstream{URL: ""}, + }}, + code: http.StatusInternalServerError, + }, + { + d: "should pass with anonymous and everything else set to noop", + url: ts.URL + "/decisions" + "/authn-anon/authz-allow/cred-noop/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + code: http.StatusOK, + authz: "", + }, + { + d: "should fail when authorizer fails", + url: ts.URL + "/decisions" + "/authn-anon/authz-deny/cred-noop/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "deny"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + code: http.StatusForbidden, + }, + { + d: "should fail when authenticator fails", + url: ts.URL + "/decisions" + "/authn-broken/authz-none/cred-none/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-broken/authz-none/cred-none/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "unauthorized"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + code: http.StatusUnauthorized, + }, + { + d: "should fail when mutator fails", + url: ts.URL + "/decisions" + "/authn-anonymous/authz-allow/cred-broken/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "broken"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + code: http.StatusInternalServerError, + }, + { + d: "should fail when one of the mutators fails", + url: ts.URL + "/decisions" + "/authn-anonymous/authz-allow/cred-broken/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}, {Handler: "broken"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + code: http.StatusInternalServerError, + }, + { + d: "should fail when authorizer fails and send www_authenticate as defined in the rule", + url: ts.URL + "/decisions" + "/authn-anon/authz-deny/cred-noop/1234", + rules: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "deny"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: ""}, + Errors: []rule.ErrorHandler{{Handler: "www_authenticate"}}, + }}, + code: http.StatusUnauthorized, + }, + } { + t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { + reg.RuleRepository().(*rule.RepositoryMemory).WithRules(tc.rules) + require.NoError(t, reg.RuleRepository().SetMatchingStrategy(context.Background(), configuration.Glob)) + + req, err := http.NewRequest("GET", tc.url, nil) + require.NoError(t, err) + if tc.transform != nil { + tc.transform(req) + } + + res, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer res.Body.Close() + + assert.Equal(t, tc.authz, res.Header.Get("Authorization")) + assert.Equal(t, tc.code, res.StatusCode) + }) + } +} diff --git a/driver/configuration/provider.go b/driver/configuration/provider.go index 4c28e9839c..9400bc6411 100644 --- a/driver/configuration/provider.go +++ b/driver/configuration/provider.go @@ -20,7 +20,7 @@ const ( ) // MatchingStrategy defines matching strategy such as Regexp or Glob. -// Empty string defaults to "regexp" +// Empty string defaults to "regexp". type MatchingStrategy string // Possible matching strategies. diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index c12ade2594..d8202ec3b8 100644 --- a/proxy/proxy_test.go +++ b/proxy/proxy_test.go @@ -279,7 +279,6 @@ backend_url=%s func TestProxy_GlobMatchingEngine(t *testing.T) { backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // assert.NotEmpty(t, helper.BearerTokenFromRequest(r)) fmt.Fprint(w, "authorization="+r.Header.Get("Authorization")+"\n") fmt.Fprint(w, "host="+r.Host+"\n") fmt.Fprint(w, "url="+r.URL.String()) @@ -480,7 +479,7 @@ func TestProxy_GlobMatchingEngine(t *testing.T) { } { t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { reg.RuleRepository().(*rule.RepositoryMemory).WithRules(tc.rules) - reg.RuleRepository().SetMatchingStrategy(context.Background(), configuration.Glob) + require.NoError(t, reg.RuleRepository().SetMatchingStrategy(context.Background(), configuration.Glob)) req, err := http.NewRequest("GET", tc.url, nil) require.NoError(t, err) diff --git a/rule/rule.go b/rule/rule.go index ade0c3dc2b..5398ea475d 100644 --- a/rule/rule.go +++ b/rule/rule.go @@ -44,8 +44,11 @@ type Match struct { // request with this field. If a match is found, the rule is considered a partial match. // If the matchesMethods field is satisfied as well, the rule is considered a full match. // - // You can use regular expressions in this field to match more than one url. Regular expressions are encapsulated in - // brackets < and >. The following example matches all paths of the domain `mydomain.com`: `https://mydomain.com/<.*>`. + // You can use regular expressions or glob patterns in this field to match more than one url. + // The matching strategy is determined by configuration parameter MatchingStrategy. + // Regular expressions and glob patterns are encapsulated in brackets < and >. + // The following regexp example matches all paths of the domain `mydomain.com`: `https://mydomain.com/<.*>`. + // The glob equivalent of the above regexp example is `https://mydomain.com/<*>`. URL string `json:"url"` } @@ -168,7 +171,7 @@ func (r *Rule) GetID() string { } // IsMatching checks whether the provided url and method match the rule. -// An error will be returned if a regexp timeout occurs. +// An error will be returned if a regexp matching strategy is selected and regexp timeout occurs. func (r *Rule) IsMatching(strategy configuration.MatchingStrategy, method string, u *url.URL) (bool, error) { if !stringInSlice(method, r.Match.Methods) { return false, nil From 1202198e6575fa9784a14426232b7bc18fb87d5d Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Mon, 13 Jan 2020 17:13:39 +0100 Subject: [PATCH 10/15] rule: add a test for glob matching strategy Signed-off-by: Aynur Zulkarnaev --- .gitignore | 3 +- README.md | 1 - api/rule_test.go | 81 +++++++++++++++++++++++++++++++++++++++++ docs/.oathkeeper.yaml | 2 + rule/fetcher_default.go | 4 +- 5 files changed, 87 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index 73b753d1fc..b281e6b866 100644 --- a/.gitignore +++ b/.gitignore @@ -3,4 +3,5 @@ vendor _book node_modules/ LICENSE.txt -*-packr.go \ No newline at end of file +*-packr.go +dev \ No newline at end of file diff --git a/README.md b/README.md index ff7116661d..4772fd7ea6 100644 --- a/README.md +++ b/README.md @@ -257,7 +257,6 @@ The Guide is available [here](https://www.ory.sh/docs/oathkeeper/). ### HTTP API documentation - The HTTP API is documented [here](https://www.ory.sh/docs/oathkeeper/sdk/api). diff --git a/api/rule_test.go b/api/rule_test.go index abb4ca0571..c7ff41d782 100644 --- a/api/rule_test.go +++ b/api/rule_test.go @@ -22,11 +22,14 @@ package api_test import ( "bytes" + "context" "encoding/json" + "net/http" "net/http/httptest" "net/url" "testing" + "github.com/ory/oathkeeper/driver/configuration" "github.com/ory/oathkeeper/x" "github.com/ory/oathkeeper/internal" @@ -110,3 +113,81 @@ func TestHandler(t *testing.T) { assert.EqualValues(t, rules[1], ruleResult) }) } + +func TestHandlerGlob(t *testing.T) { + conf := internal.NewConfigurationWithDefaults() + reg := internal.NewRegistry(conf).WithBrokenPipelineMutator() + + router := x.NewAPIRouter() + reg.RuleHandler().SetRoutes(router) + server := httptest.NewServer(router) + + u, err := url.ParseRequestURI(server.URL) + require.NoError(t, err) + + cl := client.NewHTTPClientWithConfig(nil, &client.TransportConfig{ + Host: u.Host, + BasePath: u.Path, + Schemes: []string{u.Scheme}, + }) + + rules := []rule.Rule{ + { + ID: "foo1", + Match: &rule.Match{ + URL: "https://localhost:1234/<{foo*,bar*}>>", + Methods: []string{"POST"}, + }, + Description: "Create users rule", + Authorizer: rule.Handler{Handler: "allow", Config: json.RawMessage(`{"type":"any"}`)}, + Authenticators: []rule.Handler{{Handler: "anonymous", Config: json.RawMessage(`{"name":"anonymous1"}`)}}, + Mutators: []rule.Handler{{Handler: "id_token", Config: json.RawMessage(`{"issuer":"anything"}`)}}, + Upstream: rule.Upstream{ + URL: "http://localhost:1235/", + StripPath: "/bar", + PreserveHost: true, + }, + }, + { + ID: "foo2", + Match: &rule.Match{ + URL: "https://localhost:34/<{baz*,bar*}>", + Methods: []string{"GET"}, + }, + Description: "Get users rule", + Authorizer: rule.Handler{Handler: "deny", Config: json.RawMessage(`{"type":"any"}`)}, + Authenticators: []rule.Handler{{Handler: "oauth2_introspection", Config: json.RawMessage(`{"name":"anonymous1"}`)}}, + Mutators: []rule.Handler{{Handler: "id_token", Config: json.RawMessage(`{"issuer":"anything"}`)}, {Handler: "headers", Config: json.RawMessage(`{"headers":{"X-User":"user"}}`)}}, + Upstream: rule.Upstream{ + URL: "http://localhost:333/", + StripPath: "/foo", + PreserveHost: false, + }, + }, + } + + reg.RuleRepository().(*rule.RepositoryMemory).WithRules(rules) + require.NoError(t, reg.RuleRepository().SetMatchingStrategy(context.Background(), configuration.Glob)) + + t.Run("case=create a new rule", func(t *testing.T) { + results, err := cl.API.ListRules(sdkrule.NewListRulesParams().WithLimit(pointerx.Int64(10))) + require.NoError(t, err) + require.Len(t, results.Payload, 2) + assert.True(t, results.Payload[0].ID != results.Payload[1].ID) + + result, err := cl.API.GetRule(sdkrule.NewGetRuleParams().WithID(rules[1].ID)) + require.NoError(t, err) + + var b bytes.Buffer + var ruleResult rule.Rule + require.NoError(t, json.NewEncoder(&b).Encode(result.Payload)) + require.NoError(t, json.NewDecoder(&b).Decode(&ruleResult)) + assert.EqualValues(t, rules[1], ruleResult) + + u, err := url.Parse("https://localhost:34/baz") + require.NoError(t, err) + matchedRule, err := reg.RuleRepository().(*rule.RepositoryMemory).Match(context.Background(), http.MethodGet, u) + require.NoError(t, err) + assert.Equal(t, ruleResult.ID, matchedRule.ID) + }) +} diff --git a/docs/.oathkeeper.yaml b/docs/.oathkeeper.yaml index f6518a8e1c..9246af46ff 100644 --- a/docs/.oathkeeper.yaml +++ b/docs/.oathkeeper.yaml @@ -87,6 +87,8 @@ access_rules: # If the URL Scheme is `http://` or `https://`, the access rules (an array of access rules is expected) will be # fetched from the provided HTTP(s) location. - https://path-to-my-rules/rules.json + # Optional fields describing matching strategy, defaults to "regexp". + matching_strategy: glob errors: fallback: diff --git a/rule/fetcher_default.go b/rule/fetcher_default.go index 611c3cdfed..7dd3d44beb 100644 --- a/rule/fetcher_default.go +++ b/rule/fetcher_default.go @@ -276,9 +276,9 @@ func (f *FetcherDefault) watch(ctx context.Context, watcher *fsnotify.Watcher, e } case eventMatchingStrategyChanged: f.r.Logger(). - WithField("event", "config_change"). + WithField("event", "matching_strategy_config_change"). WithField("source", e.source). - Debugf("Viper detected a configuration change, reloading config.") + Debugf("Viper detected a configuration change, updating matching strategy") if err := f.r.RuleRepository().SetMatchingStrategy(ctx, f.c.AccessRuleMatchingStrategy()); err != nil { return errors.Wrapf(err, "unable to update matching strategy") } From e78681fdfacff69beea97a33ed00b4301fdc22f6 Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Mon, 13 Jan 2020 18:47:34 +0100 Subject: [PATCH 11/15] rule: add a test for reloading MatchingStrategy config parameter Signed-off-by: Aynur Zulkarnaev --- rule/fetcher_default.go | 11 +--- rule/fetcher_default_test.go | 101 ++++++++++++++++++++++++++++++++ test/update/config_default.yaml | 2 + test/update/config_error.yaml | 3 + test/update/config_glob.yaml | 3 + test/update/config_no_repo.yaml | 2 + test/update/config_regexp.yaml | 3 + test/update/rules_glob.yaml | 6 ++ 8 files changed, 123 insertions(+), 8 deletions(-) create mode 100644 test/update/config_default.yaml create mode 100644 test/update/config_error.yaml create mode 100644 test/update/config_glob.yaml create mode 100644 test/update/config_no_repo.yaml create mode 100644 test/update/config_regexp.yaml create mode 100644 test/update/rules_glob.yaml diff --git a/rule/fetcher_default.go b/rule/fetcher_default.go index 7dd3d44beb..7a6f227c51 100644 --- a/rule/fetcher_default.go +++ b/rule/fetcher_default.go @@ -202,25 +202,20 @@ func (f *FetcherDefault) watch(ctx context.Context, watcher *fsnotify.Watcher, e return nil }) - - var initialConfig map[string]interface{} - f.enqueueEvent(events, event{et: eventRepositoryConfigChanged, source: "entrypoint"}) + var strategy configuration.MatchingStrategy viperx.AddWatcher(func(e fsnotify.Event) error { - // TODO: clarify why we don't save initial state as a string and then compare it with the current state - // by invoking f.c.AccessRuleMatchingStrategy method? - if reflect.DeepEqual(initialConfig, viper.Get(configuration.ViperKeyAccessRuleMatchingStrategy)) { + if strategy == f.c.AccessRuleMatchingStrategy() { f.r.Logger(). Debug("Not reloading access rule matching strategy because configuration value has not changed.") return nil } f.enqueueEvent(events, event{et: eventMatchingStrategyChanged, source: "viper_watcher"}) - + strategy = f.c.AccessRuleMatchingStrategy() return nil }) - f.enqueueEvent(events, event{et: eventMatchingStrategyChanged, source: "entrypoint"}) for { diff --git a/rule/fetcher_default_test.go b/rule/fetcher_default_test.go index eed58df9b4..f606a9ea3b 100644 --- a/rule/fetcher_default_test.go +++ b/rule/fetcher_default_test.go @@ -28,6 +28,107 @@ import ( const testRule = `[{"id":"test-rule-5","upstream":{"preserve_host":true,"strip_path":"/api","url":"mybackend.com/api"},"match":{"url":"myproxy.com/api","methods":["GET","POST"]},"authenticators":[{"handler":"noop"},{"handler":"anonymous"}],"authorizer":{"handler":"allow"},"mutators":[{"handler":"noop"}]}]` +func TestFetcherReload(t *testing.T) { + viper.Reset() + conf := internal.NewConfigurationWithDefaults() // this resets viper and must be at the top + r := internal.NewRegistry(conf) + testConfigPath := "../test/update" + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(testRule)) + })) + defer ts.Close() + + tempdir := os.TempDir() + + id := uuid.New().String() + configFile := filepath.Join(tempdir, ".oathkeeper-"+id+".yml") + require.NoError(t, ioutil.WriteFile(configFile, []byte(""), 0666)) + + l := logrus.New() + l.Level = logrus.TraceLevel + viperx.InitializeConfig("oathkeeper-"+id, tempdir, nil) + viperx.WatchConfig(l, nil) + + go func() { + require.NoError(t, r.RuleFetcher().Watch(context.TODO())) + }() + + // initial config without a repo and without a matching strategy + config, err := ioutil.ReadFile(path.Join(testConfigPath, "config_no_repo.yaml")) + require.NoError(t, err) + require.NoError(t, ioutil.WriteFile(configFile, config, 0666)) + time.Sleep(time.Millisecond * 500) + + rules, err := r.RuleRepository().List(context.Background(), 500, 0) + require.NoError(t, err) + require.Empty(t, rules) + + strategy, err := r.RuleRepository().MatchingStrategy(context.Background()) + require.NoError(t, err) + require.Equal(t, configuration.MatchingStrategy(""), strategy) + + // config with a repo and without a matching strategy + config, err = ioutil.ReadFile(path.Join(testConfigPath, "config_default.yaml")) + require.NoError(t, err) + require.NoError(t, ioutil.WriteFile(configFile, config, 0666)) + time.Sleep(time.Millisecond * 500) + + rules, err = r.RuleRepository().List(context.Background(), 500, 0) + require.NoError(t, err) + require.Equal(t, 1, len(rules)) + require.Equal(t, "test-rule-1-glob", rules[0].ID) + + strategy, err = r.RuleRepository().MatchingStrategy(context.Background()) + require.NoError(t, err) + require.Equal(t, configuration.MatchingStrategy(""), strategy) + + // config with a glob matching strategy + config, err = ioutil.ReadFile(path.Join(testConfigPath, "config_glob.yaml")) + require.NoError(t, err) + require.NoError(t, ioutil.WriteFile(configFile, config, 0666)) + time.Sleep(time.Millisecond * 500) + + rules, err = r.RuleRepository().List(context.Background(), 500, 0) + require.NoError(t, err) + require.Equal(t, 1, len(rules)) + require.Equal(t, "test-rule-1-glob", rules[0].ID) + + strategy, err = r.RuleRepository().MatchingStrategy(context.Background()) + require.NoError(t, err) + require.Equal(t, configuration.Glob, strategy) + + // config with unknown matching strategy + config, err = ioutil.ReadFile(path.Join(testConfigPath, "config_error.yaml")) + require.NoError(t, err) + require.NoError(t, ioutil.WriteFile(configFile, config, 0666)) + time.Sleep(time.Millisecond * 500) + + rules, err = r.RuleRepository().List(context.Background(), 500, 0) + require.NoError(t, err) + require.Equal(t, 1, len(rules)) + require.Equal(t, "test-rule-1-glob", rules[0].ID) + + strategy, err = r.RuleRepository().MatchingStrategy(context.Background()) + require.NoError(t, err) + require.Equal(t, configuration.MatchingStrategy("UNKNOWN"), strategy) + + // config with regexp matching strategy + config, err = ioutil.ReadFile(path.Join(testConfigPath, "config_regexp.yaml")) + require.NoError(t, err) + require.NoError(t, ioutil.WriteFile(configFile, config, 0666)) + time.Sleep(time.Millisecond * 500) + + rules, err = r.RuleRepository().List(context.Background(), 500, 0) + require.NoError(t, err) + require.Equal(t, 1, len(rules)) + require.Equal(t, "test-rule-1-glob", rules[0].ID) + + strategy, err = r.RuleRepository().MatchingStrategy(context.Background()) + require.NoError(t, err) + require.Equal(t, configuration.Regexp, strategy) +} + func TestFetcherWatchConfig(t *testing.T) { viper.Reset() conf := internal.NewConfigurationWithDefaults() // this resets viper and must be at the top diff --git a/test/update/config_default.yaml b/test/update/config_default.yaml new file mode 100644 index 0000000000..a11b1ef842 --- /dev/null +++ b/test/update/config_default.yaml @@ -0,0 +1,2 @@ +access_rules: + repositories: file://../test/update/rules_glob.yaml \ No newline at end of file diff --git a/test/update/config_error.yaml b/test/update/config_error.yaml new file mode 100644 index 0000000000..a9c0376ca6 --- /dev/null +++ b/test/update/config_error.yaml @@ -0,0 +1,3 @@ +access_rules: + repositories: file://../test/update/rules_glob.yaml + matching_strategy: UNKNOWN \ No newline at end of file diff --git a/test/update/config_glob.yaml b/test/update/config_glob.yaml new file mode 100644 index 0000000000..ec4aba7b3d --- /dev/null +++ b/test/update/config_glob.yaml @@ -0,0 +1,3 @@ +access_rules: + repositories: file://../test/update/rules_glob.yaml + matching_strategy: glob \ No newline at end of file diff --git a/test/update/config_no_repo.yaml b/test/update/config_no_repo.yaml new file mode 100644 index 0000000000..304048484a --- /dev/null +++ b/test/update/config_no_repo.yaml @@ -0,0 +1,2 @@ +access_rules: + repositories: \ No newline at end of file diff --git a/test/update/config_regexp.yaml b/test/update/config_regexp.yaml new file mode 100644 index 0000000000..369ac5c545 --- /dev/null +++ b/test/update/config_regexp.yaml @@ -0,0 +1,3 @@ +access_rules: + repositories: file://../test/update/rules_glob.yaml + matching_strategy: regexp \ No newline at end of file diff --git a/test/update/rules_glob.yaml b/test/update/rules_glob.yaml new file mode 100644 index 0000000000..e898ab305a --- /dev/null +++ b/test/update/rules_glob.yaml @@ -0,0 +1,6 @@ +- id: test-rule-1-glob + match: + url: myproxy.com/ + methods: + - GET + - POST \ No newline at end of file From c65dce5ee8e1cdfda0907198ca9678674fee93c8 Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Wed, 15 Jan 2020 15:08:02 +0100 Subject: [PATCH 12/15] rule: configuration update for MatchingStrategy Signed-off-by: Aynur Zulkarnaev --- rule/fetcher_default.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/rule/fetcher_default.go b/rule/fetcher_default.go index 7a6f227c51..0de8565260 100644 --- a/rule/fetcher_default.go +++ b/rule/fetcher_default.go @@ -204,16 +204,15 @@ func (f *FetcherDefault) watch(ctx context.Context, watcher *fsnotify.Watcher, e }) f.enqueueEvent(events, event{et: eventRepositoryConfigChanged, source: "entrypoint"}) - var strategy configuration.MatchingStrategy + var strategy map[string]interface{} viperx.AddWatcher(func(e fsnotify.Event) error { - if strategy == f.c.AccessRuleMatchingStrategy() { + if reflect.DeepEqual(strategy, viper.Get(configuration.ViperKeyAccessRuleMatchingStrategy)) { f.r.Logger(). Debug("Not reloading access rule matching strategy because configuration value has not changed.") return nil } f.enqueueEvent(events, event{et: eventMatchingStrategyChanged, source: "viper_watcher"}) - strategy = f.c.AccessRuleMatchingStrategy() return nil }) f.enqueueEvent(events, event{et: eventMatchingStrategyChanged, source: "entrypoint"}) From 415450352c2b0c9710f968148e80f7eff238f3fa Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Sun, 2 Feb 2020 19:01:36 +0100 Subject: [PATCH 13/15] refactor: merge glob and regexp tests Signed-off-by: Aynur Zulkarnaev --- api/decision_test.go | 333 ++++++++++++------------------------ api/rule_test.go | 93 ++++------- proxy/proxy_test.go | 390 ++++++++++++++----------------------------- rule/rule_test.go | 115 ++++--------- 4 files changed, 293 insertions(+), 638 deletions(-) diff --git a/api/decision_test.go b/api/decision_test.go index 0fe68a3534..6078c6f71e 100644 --- a/api/decision_test.go +++ b/api/decision_test.go @@ -75,190 +75,14 @@ func TestDecisionAPI(t *testing.T) { Upstream: rule.Upstream{URL: "", StripPath: "/strip-path/", PreserveHost: true}, } - for k, tc := range []struct { - url string - code int - messages []string - rules []rule.Rule - transform func(r *http.Request) - authz string - d string - }{ - { - d: "should fail because url does not exist in rule set", - url: ts.URL + "/decisions" + "/invalid", - rules: []rule.Rule{}, - code: http.StatusNotFound, - }, - { - d: "should fail because url does exist but is matched by two rules", - url: ts.URL + "/decisions" + "/authn-noop/1234", - rules: []rule.Rule{ruleNoOpAuthenticator, ruleNoOpAuthenticator}, - code: http.StatusInternalServerError, - }, - { - d: "should pass", - url: ts.URL + "/decisions" + "/authn-noop/1234", - rules: []rule.Rule{ruleNoOpAuthenticator}, - code: http.StatusOK, - transform: func(r *http.Request) { - r.Header.Add("Authorization", "bearer token") - }, - authz: "bearer token", - }, - { - d: "should pass", - url: ts.URL + "/decisions" + "/strip-path/authn-noop/1234", - rules: []rule.Rule{ruleNoOpAuthenticatorModifyUpstream}, - code: http.StatusOK, - transform: func(r *http.Request) { - r.Header.Add("Authorization", "bearer token") - }, - authz: "bearer token", - }, - { - d: "should fail because no authorizer was configured", - url: ts.URL + "/decisions" + "/authn-anon/authz-none/cred-none/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-none/cred-none/<[0-9]+>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Upstream: rule.Upstream{URL: ""}, - }}, - transform: func(r *http.Request) { - r.Header.Add("Authorization", "bearer token") - }, - code: http.StatusUnauthorized, - }, - { - d: "should fail because no mutator was configured", - url: ts.URL + "/decisions" + "/authn-anon/authz-allow/cred-none/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-none/<[0-9]+>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Authorizer: rule.Handler{Handler: "allow"}, - Upstream: rule.Upstream{URL: ""}, - }}, - code: http.StatusInternalServerError, - }, - { - d: "should pass with anonymous and everything else set to noop", - url: ts.URL + "/decisions" + "/authn-anon/authz-allow/cred-noop/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]+>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Authorizer: rule.Handler{Handler: "allow"}, - Mutators: []rule.Handler{{Handler: "noop"}}, - Upstream: rule.Upstream{URL: ""}, - }}, - code: http.StatusOK, - authz: "", - }, - { - d: "should fail when authorizer fails", - url: ts.URL + "/decisions" + "/authn-anon/authz-deny/cred-noop/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Authorizer: rule.Handler{Handler: "deny"}, - Mutators: []rule.Handler{{Handler: "noop"}}, - Upstream: rule.Upstream{URL: ""}, - }}, - code: http.StatusForbidden, - }, - { - d: "should fail when authenticator fails", - url: ts.URL + "/decisions" + "/authn-broken/authz-none/cred-none/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-broken/authz-none/cred-none/<[0-9]+>"}, - Authenticators: []rule.Handler{{Handler: "unauthorized"}}, - Upstream: rule.Upstream{URL: ""}, - }}, - code: http.StatusUnauthorized, - }, - { - d: "should fail when mutator fails", - url: ts.URL + "/decisions" + "/authn-anonymous/authz-allow/cred-broken/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]+>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Authorizer: rule.Handler{Handler: "allow"}, - Mutators: []rule.Handler{{Handler: "broken"}}, - Upstream: rule.Upstream{URL: ""}, - }}, - code: http.StatusInternalServerError, - }, - { - d: "should fail when one of the mutators fails", - url: ts.URL + "/decisions" + "/authn-anonymous/authz-allow/cred-broken/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]+>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Authorizer: rule.Handler{Handler: "allow"}, - Mutators: []rule.Handler{{Handler: "noop"}, {Handler: "broken"}}, - Upstream: rule.Upstream{URL: ""}, - }}, - code: http.StatusInternalServerError, - }, - { - d: "should fail when authorizer fails and send www_authenticate as defined in the rule", - url: ts.URL + "/decisions" + "/authn-anon/authz-deny/cred-noop/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Authorizer: rule.Handler{Handler: "deny"}, - Mutators: []rule.Handler{{Handler: "noop"}}, - Upstream: rule.Upstream{URL: ""}, - Errors: []rule.ErrorHandler{{Handler: "www_authenticate"}}, - }}, - code: http.StatusUnauthorized, - }, - } { - t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { - reg.RuleRepository().(*rule.RepositoryMemory).WithRules(tc.rules) - - req, err := http.NewRequest("GET", tc.url, nil) - require.NoError(t, err) - if tc.transform != nil { - tc.transform(req) - } - - res, err := http.DefaultClient.Do(req) - require.NoError(t, err) - defer res.Body.Close() - - assert.Equal(t, tc.authz, res.Header.Get("Authorization")) - assert.Equal(t, tc.code, res.StatusCode) - }) - } -} - -func TestDecisionAPIGlob(t *testing.T) { - conf := internal.NewConfigurationWithDefaults() - viper.Set(configuration.ViperKeyAuthenticatorNoopIsEnabled, true) - viper.Set(configuration.ViperKeyAuthenticatorUnauthorizedIsEnabled, true) - viper.Set(configuration.ViperKeyAuthenticatorAnonymousIsEnabled, true) - viper.Set(configuration.ViperKeyAuthorizerAllowIsEnabled, true) - viper.Set(configuration.ViperKeyAuthorizerDenyIsEnabled, true) - viper.Set(configuration.ViperKeyMutatorNoopIsEnabled, true) - viper.Set(configuration.ViperKeyErrorsWWWAuthenticateIsEnabled, true) - reg := internal.NewRegistry(conf).WithBrokenPipelineMutator() - - d := reg.DecisionHandler() - - n := negroni.New(d) - n.UseHandler(httprouter.New()) - - ts := httptest.NewServer(n) - defer ts.Close() - - ruleNoOpAuthenticator := rule.Rule{ + ruleNoOpAuthenticatorGLOB := rule.Rule{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-noop/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "noop"}}, Authorizer: rule.Handler{Handler: "allow"}, Mutators: []rule.Handler{{Handler: "noop"}}, Upstream: rule.Upstream{URL: ""}, } - ruleNoOpAuthenticatorModifyUpstream := rule.Rule{ + ruleNoOpAuthenticatorModifyUpstreamGLOB := rule.Rule{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-noop/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "noop"}}, Authorizer: rule.Handler{Handler: "allow"}, @@ -267,41 +91,46 @@ func TestDecisionAPIGlob(t *testing.T) { } for k, tc := range []struct { - url string - code int - messages []string - rules []rule.Rule - transform func(r *http.Request) - authz string - d string + url string + code int + messages []string + rulesRegexp []rule.Rule + rulesGlob []rule.Rule + transform func(r *http.Request) + authz string + d string }{ { - d: "should fail because url does not exist in rule set", - url: ts.URL + "/decisions" + "/invalid", - rules: []rule.Rule{}, - code: http.StatusNotFound, + d: "should fail because url does not exist in rule set", + url: ts.URL + "/decisions" + "/invalid", + rulesRegexp: []rule.Rule{}, + rulesGlob: []rule.Rule{}, + code: http.StatusNotFound, }, { - d: "should fail because url does exist but is matched by two rules", - url: ts.URL + "/decisions" + "/authn-noop/1234", - rules: []rule.Rule{ruleNoOpAuthenticator, ruleNoOpAuthenticator}, - code: http.StatusInternalServerError, + d: "should fail because url does exist but is matched by two rulesRegexp", + url: ts.URL + "/decisions" + "/authn-noop/1234", + rulesRegexp: []rule.Rule{ruleNoOpAuthenticator, ruleNoOpAuthenticator}, + rulesGlob: []rule.Rule{ruleNoOpAuthenticatorGLOB, ruleNoOpAuthenticatorGLOB}, + code: http.StatusInternalServerError, }, { - d: "should pass", - url: ts.URL + "/decisions" + "/authn-noop/1234", - rules: []rule.Rule{ruleNoOpAuthenticator}, - code: http.StatusOK, + d: "should pass", + url: ts.URL + "/decisions" + "/authn-noop/1234", + rulesRegexp: []rule.Rule{ruleNoOpAuthenticator}, + rulesGlob: []rule.Rule{ruleNoOpAuthenticatorGLOB}, + code: http.StatusOK, transform: func(r *http.Request) { r.Header.Add("Authorization", "bearer token") }, authz: "bearer token", }, { - d: "should pass", - url: ts.URL + "/decisions" + "/strip-path/authn-noop/1234", - rules: []rule.Rule{ruleNoOpAuthenticatorModifyUpstream}, - code: http.StatusOK, + d: "should pass", + url: ts.URL + "/decisions" + "/strip-path/authn-noop/1234", + rulesRegexp: []rule.Rule{ruleNoOpAuthenticatorModifyUpstream}, + rulesGlob: []rule.Rule{ruleNoOpAuthenticatorModifyUpstreamGLOB}, + code: http.StatusOK, transform: func(r *http.Request) { r.Header.Add("Authorization", "bearer token") }, @@ -310,7 +139,12 @@ func TestDecisionAPIGlob(t *testing.T) { { d: "should fail because no authorizer was configured", url: ts.URL + "/decisions" + "/authn-anon/authz-none/cred-none/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-none/cred-none/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-none/cred-none/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Upstream: rule.Upstream{URL: ""}, @@ -323,7 +157,13 @@ func TestDecisionAPIGlob(t *testing.T) { { d: "should fail because no mutator was configured", url: ts.URL + "/decisions" + "/authn-anon/authz-allow/cred-none/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-none/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Upstream: rule.Upstream{URL: ""}, + }}, + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-none/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "allow"}, @@ -334,7 +174,14 @@ func TestDecisionAPIGlob(t *testing.T) { { d: "should pass with anonymous and everything else set to noop", url: ts.URL + "/decisions" + "/authn-anon/authz-allow/cred-noop/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "allow"}, @@ -347,7 +194,14 @@ func TestDecisionAPIGlob(t *testing.T) { { d: "should fail when authorizer fails", url: ts.URL + "/decisions" + "/authn-anon/authz-deny/cred-noop/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "deny"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "deny"}, @@ -359,7 +213,12 @@ func TestDecisionAPIGlob(t *testing.T) { { d: "should fail when authenticator fails", url: ts.URL + "/decisions" + "/authn-broken/authz-none/cred-none/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-broken/authz-none/cred-none/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "unauthorized"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-broken/authz-none/cred-none/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "unauthorized"}}, Upstream: rule.Upstream{URL: ""}, @@ -369,7 +228,14 @@ func TestDecisionAPIGlob(t *testing.T) { { d: "should fail when mutator fails", url: ts.URL + "/decisions" + "/authn-anonymous/authz-allow/cred-broken/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "broken"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "allow"}, @@ -381,7 +247,14 @@ func TestDecisionAPIGlob(t *testing.T) { { d: "should fail when one of the mutators fails", url: ts.URL + "/decisions" + "/authn-anonymous/authz-allow/cred-broken/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}, {Handler: "broken"}}, + Upstream: rule.Upstream{URL: ""}, + }}, + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "allow"}, @@ -393,7 +266,15 @@ func TestDecisionAPIGlob(t *testing.T) { { d: "should fail when authorizer fails and send www_authenticate as defined in the rule", url: ts.URL + "/decisions" + "/authn-anon/authz-deny/cred-noop/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "deny"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: ""}, + Errors: []rule.ErrorHandler{{Handler: "www_authenticate"}}, + }}, + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "deny"}, @@ -405,21 +286,29 @@ func TestDecisionAPIGlob(t *testing.T) { }, } { t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { - reg.RuleRepository().(*rule.RepositoryMemory).WithRules(tc.rules) - require.NoError(t, reg.RuleRepository().SetMatchingStrategy(context.Background(), configuration.Glob)) + testFunc := func(strategy configuration.MatchingStrategy) { + require.NoError(t, reg.RuleRepository().SetMatchingStrategy(context.Background(), strategy)) + req, err := http.NewRequest("GET", tc.url, nil) + require.NoError(t, err) + if tc.transform != nil { + tc.transform(req) + } - req, err := http.NewRequest("GET", tc.url, nil) - require.NoError(t, err) - if tc.transform != nil { - tc.transform(req) - } + res, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer res.Body.Close() - res, err := http.DefaultClient.Do(req) - require.NoError(t, err) - defer res.Body.Close() - - assert.Equal(t, tc.authz, res.Header.Get("Authorization")) - assert.Equal(t, tc.code, res.StatusCode) + assert.Equal(t, tc.authz, res.Header.Get("Authorization")) + assert.Equal(t, tc.code, res.StatusCode) + } + t.Run("regexp", func(t *testing.T) { + reg.RuleRepository().(*rule.RepositoryMemory).WithRules(tc.rulesRegexp) + testFunc(configuration.Regexp) + }) + t.Run("glob", func(t *testing.T) { + reg.RuleRepository().(*rule.RepositoryMemory).WithRules(tc.rulesRegexp) + testFunc(configuration.Glob) + }) }) } } diff --git a/api/rule_test.go b/api/rule_test.go index c7ff41d782..4e7a2559b3 100644 --- a/api/rule_test.go +++ b/api/rule_test.go @@ -24,7 +24,6 @@ import ( "bytes" "context" "encoding/json" - "net/http" "net/http/httptest" "net/url" "testing" @@ -32,11 +31,12 @@ import ( "github.com/ory/oathkeeper/driver/configuration" "github.com/ory/oathkeeper/x" + "github.com/ory/x/pointerx" + "github.com/ory/oathkeeper/internal" "github.com/ory/oathkeeper/internal/httpclient/client" sdkrule "github.com/ory/oathkeeper/internal/httpclient/client/api" "github.com/ory/oathkeeper/rule" - "github.com/ory/x/pointerx" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -59,7 +59,7 @@ func TestHandler(t *testing.T) { Schemes: []string{u.Scheme}, }) - rules := []rule.Rule{ + rulesRegexp := []rule.Rule{ { ID: "foo1", Match: &rule.Match{ @@ -93,45 +93,7 @@ func TestHandler(t *testing.T) { }, }, } - - reg.RuleRepository().(*rule.RepositoryMemory).WithRules(rules) - - t.Run("case=create a new rule", func(t *testing.T) { - results, err := cl.API.ListRules(sdkrule.NewListRulesParams().WithLimit(pointerx.Int64(10))) - require.NoError(t, err) - require.Len(t, results.Payload, 2) - assert.True(t, results.Payload[0].ID != results.Payload[1].ID) - - result, err := cl.API.GetRule(sdkrule.NewGetRuleParams().WithID(rules[1].ID)) - require.NoError(t, err) - - var b bytes.Buffer - var ruleResult rule.Rule - require.NoError(t, json.NewEncoder(&b).Encode(result.Payload)) - require.NoError(t, json.NewDecoder(&b).Decode(&ruleResult)) - - assert.EqualValues(t, rules[1], ruleResult) - }) -} - -func TestHandlerGlob(t *testing.T) { - conf := internal.NewConfigurationWithDefaults() - reg := internal.NewRegistry(conf).WithBrokenPipelineMutator() - - router := x.NewAPIRouter() - reg.RuleHandler().SetRoutes(router) - server := httptest.NewServer(router) - - u, err := url.ParseRequestURI(server.URL) - require.NoError(t, err) - - cl := client.NewHTTPClientWithConfig(nil, &client.TransportConfig{ - Host: u.Host, - BasePath: u.Path, - Schemes: []string{u.Scheme}, - }) - - rules := []rule.Rule{ + rulesGlob := []rule.Rule{ { ID: "foo1", Match: &rule.Match{ @@ -166,28 +128,31 @@ func TestHandlerGlob(t *testing.T) { }, } - reg.RuleRepository().(*rule.RepositoryMemory).WithRules(rules) - require.NoError(t, reg.RuleRepository().SetMatchingStrategy(context.Background(), configuration.Glob)) - t.Run("case=create a new rule", func(t *testing.T) { - results, err := cl.API.ListRules(sdkrule.NewListRulesParams().WithLimit(pointerx.Int64(10))) - require.NoError(t, err) - require.Len(t, results.Payload, 2) - assert.True(t, results.Payload[0].ID != results.Payload[1].ID) - - result, err := cl.API.GetRule(sdkrule.NewGetRuleParams().WithID(rules[1].ID)) - require.NoError(t, err) - - var b bytes.Buffer - var ruleResult rule.Rule - require.NoError(t, json.NewEncoder(&b).Encode(result.Payload)) - require.NoError(t, json.NewDecoder(&b).Decode(&ruleResult)) - assert.EqualValues(t, rules[1], ruleResult) - - u, err := url.Parse("https://localhost:34/baz") - require.NoError(t, err) - matchedRule, err := reg.RuleRepository().(*rule.RepositoryMemory).Match(context.Background(), http.MethodGet, u) - require.NoError(t, err) - assert.Equal(t, ruleResult.ID, matchedRule.ID) + testFunc := func(strategy configuration.MatchingStrategy, rules []rule.Rule) { + reg.RuleRepository().(*rule.RepositoryMemory).WithRules(rules) + require.NoError(t, reg.RuleRepository().SetMatchingStrategy(context.Background(), strategy)) + results, err := cl.API.ListRules(sdkrule.NewListRulesParams().WithLimit(pointerx.Int64(10))) + require.NoError(t, err) + require.Len(t, results.Payload, 2) + assert.True(t, results.Payload[0].ID != results.Payload[1].ID) + + result, err := cl.API.GetRule(sdkrule.NewGetRuleParams().WithID(rules[1].ID)) + require.NoError(t, err) + + var b bytes.Buffer + var ruleResult rule.Rule + require.NoError(t, json.NewEncoder(&b).Encode(result.Payload)) + require.NoError(t, json.NewDecoder(&b).Decode(&ruleResult)) + + assert.EqualValues(t, rules[1], ruleResult) + } + t.Run("regexp", func(t *testing.T) { + testFunc(configuration.Regexp, rulesRegexp) + }) + t.Run("glob", func(t *testing.T) { + testFunc(configuration.Glob, rulesGlob) + }) + }) } diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index d8202ec3b8..eab4cb8887 100644 --- a/proxy/proxy_test.go +++ b/proxy/proxy_test.go @@ -83,6 +83,20 @@ func TestProxy(t *testing.T) { Mutators: []rule.Handler{{Handler: "noop"}}, Upstream: rule.Upstream{URL: backend.URL, StripPath: "/strip-path/", PreserveHost: true}, } + ruleNoOpAuthenticatorGlob := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "noop"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: backend.URL}, + } + ruleNoOpAuthenticatorModifyUpstreamGlob := rule.Rule{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "noop"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: backend.URL, StripPath: "/strip-path/", PreserveHost: true}, + } // acceptRuleStripHost := rule.Rule{MatchesMethods: []string{"GET"}, MatchesURLCompiled: mustCompileRegex(t, proxy.URL+"/users/<[0-9]+>"), Mode: "pass_through_accept", Upstream: rule.Upstream{URLParsed: u, StripPath: "/users/", PreserveHost: true}} // acceptRuleStripHostWithoutTrailing := rule.Rule{MatchesMethods: []string{"GET"}, MatchesURLCompiled: mustCompileRegex(t, proxy.URL+"/users/<[0-9]+>"), Mode: "pass_through_accept", Upstream: rule.Upstream{URLParsed: u, StripPath: "/users", PreserveHost: true}} @@ -90,30 +104,34 @@ func TestProxy(t *testing.T) { // denyRule := rule.Rule{MatchesMethods: []string{"GET"}, MatchesURLCompiled: mustCompileRegex(t, proxy.URL+"/users/<[0-9]+>"), Mode: "pass_through_deny", Upstream: rule.Upstream{URLParsed: u}} for k, tc := range []struct { - url string - code int - messages []string - rules []rule.Rule - transform func(r *http.Request) - d string + url string + code int + messages []string + rulesRegexp []rule.Rule + rulesGlob []rule.Rule + transform func(r *http.Request) + d string }{ { - d: "should fail because url does not exist in rule set", - url: ts.URL + "/invalid", - rules: []rule.Rule{}, - code: http.StatusNotFound, + d: "should fail because url does not exist in rule set", + url: ts.URL + "/invalid", + rulesRegexp: []rule.Rule{}, + rulesGlob: []rule.Rule{}, + code: http.StatusNotFound, }, { - d: "should fail because url does exist but is matched by two rules", - url: ts.URL + "/authn-noop/1234", - rules: []rule.Rule{ruleNoOpAuthenticator, ruleNoOpAuthenticator}, - code: http.StatusInternalServerError, + d: "should fail because url does exist but is matched by two rules", + url: ts.URL + "/authn-noop/1234", + rulesRegexp: []rule.Rule{ruleNoOpAuthenticator, ruleNoOpAuthenticator}, + rulesGlob: []rule.Rule{ruleNoOpAuthenticatorGlob, ruleNoOpAuthenticatorGlob}, + code: http.StatusInternalServerError, }, { - d: "should pass", - url: ts.URL + "/authn-noop/1234", - rules: []rule.Rule{ruleNoOpAuthenticator}, - code: http.StatusOK, + d: "should pass", + url: ts.URL + "/authn-noop/1234", + rulesRegexp: []rule.Rule{ruleNoOpAuthenticator}, + rulesGlob: []rule.Rule{ruleNoOpAuthenticatorGlob}, + code: http.StatusOK, transform: func(r *http.Request) { r.Header.Add("Authorization", "bearer token") }, @@ -124,10 +142,11 @@ func TestProxy(t *testing.T) { }, }, { - d: "should pass", - url: ts.URL + "/strip-path/authn-noop/1234", - rules: []rule.Rule{ruleNoOpAuthenticatorModifyUpstream}, - code: http.StatusOK, + d: "should pass", + url: ts.URL + "/strip-path/authn-noop/1234", + rulesRegexp: []rule.Rule{ruleNoOpAuthenticatorModifyUpstream}, + rulesGlob: []rule.Rule{ruleNoOpAuthenticatorModifyUpstreamGlob}, + code: http.StatusOK, transform: func(r *http.Request) { r.Header.Add("Authorization", "bearer token") }, @@ -140,11 +159,16 @@ func TestProxy(t *testing.T) { { d: "should fail because no authorizer was configured", url: ts.URL + "/authn-anon/authz-none/cred-none/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-none/cred-none/<[0-9]+>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Upstream: rule.Upstream{URL: backend.URL}, }}, + rulesGlob: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-none/cred-none/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, transform: func(r *http.Request) { r.Header.Add("Authorization", "bearer token") }, @@ -153,24 +177,37 @@ func TestProxy(t *testing.T) { { d: "should fail because no credentials issuer was configured", url: ts.URL + "/authn-anon/authz-allow/cred-none/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-none/<[0-9]+>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "allow"}, Upstream: rule.Upstream{URL: backend.URL}, }}, + rulesGlob: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-none/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, code: http.StatusInternalServerError, }, { d: "should pass with anonymous and everything else set to noop", url: ts.URL + "/authn-anon/authz-allow/cred-noop/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]+>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "allow"}, Mutators: []rule.Handler{{Handler: "noop"}}, Upstream: rule.Upstream{URL: backend.URL}, }}, + rulesGlob: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]*>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, code: http.StatusOK, messages: []string{ "authorization=", @@ -181,247 +218,34 @@ func TestProxy(t *testing.T) { { d: "should fail when authorizer fails", url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "deny"}, Mutators: []rule.Handler{{Handler: "noop"}}, Upstream: rule.Upstream{URL: backend.URL}, }}, - code: http.StatusForbidden, - }, - { - d: "should fail when authorizer fails and send www_authenticate as defined in the rule", - url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"}, + rulesGlob: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "deny"}, Mutators: []rule.Handler{{Handler: "noop"}}, Upstream: rule.Upstream{URL: backend.URL}, - Errors: []rule.ErrorHandler{{Handler: "www_authenticate"}}, - }}, - code: http.StatusUnauthorized, - }, - { - d: "should fail when authenticator fails", - url: ts.URL + "/authn-broken/authz-none/cred-none/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-broken/authz-none/cred-none/<[0-9]+>"}, - Authenticators: []rule.Handler{{Handler: "unauthorized"}}, - Upstream: rule.Upstream{URL: backend.URL}, }}, - code: http.StatusUnauthorized, - }, - { - d: "should fail because no mutator was configured", - url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Authorizer: rule.Handler{Handler: "allow"}, - Upstream: rule.Upstream{URL: backend.URL}, - }}, - code: http.StatusInternalServerError, + code: http.StatusForbidden, }, { - d: "should fail when one of the mutators fails", + d: "should fail when authorizer fails and send www_authenticate as defined in the rule", url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Authorizer: rule.Handler{Handler: "allow"}, - Mutators: []rule.Handler{{Handler: "noop"}, {Handler: "broken"}}, - Upstream: rule.Upstream{URL: backend.URL}, - }}, - code: http.StatusInternalServerError, - }, - { - d: "should fail when credentials issuer fails", - url: ts.URL + "/authn-anonymous/authz-allow/cred-broken/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]+>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Authorizer: rule.Handler{Handler: "allow"}, - Mutators: []rule.Handler{{Handler: "broken"}}, - Upstream: rule.Upstream{URL: backend.URL}, - }}, - code: http.StatusInternalServerError, - }, - } { - t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { - reg.RuleRepository().(*rule.RepositoryMemory).WithRules(tc.rules) - - req, err := http.NewRequest("GET", tc.url, nil) - require.NoError(t, err) - if tc.transform != nil { - tc.transform(req) - } - - res, err := http.DefaultClient.Do(req) - require.NoError(t, err) - - greeting, err := ioutil.ReadAll(res.Body) - require.NoError(t, res.Body.Close()) - require.NoError(t, err) - - assert.Equal(t, tc.code, res.StatusCode, "%s", res.Body) - for _, m := range tc.messages { - assert.True(t, strings.Contains(string(greeting), m), `Value "%s" not found in message: -%s -proxy_url=%s -backend_url=%s -`, m, greeting, ts.URL, backend.URL) - } - }) - } -} - -func TestProxy_GlobMatchingEngine(t *testing.T) { - backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - fmt.Fprint(w, "authorization="+r.Header.Get("Authorization")+"\n") - fmt.Fprint(w, "host="+r.Host+"\n") - fmt.Fprint(w, "url="+r.URL.String()) - })) - defer backend.Close() - - viper.Set(configuration.ViperKeyAccessRuleMatchingStrategy, configuration.Glob) - - conf := internal.NewConfigurationWithDefaults() - reg := internal.NewRegistry(conf).WithBrokenPipelineMutator() - - d := reg.Proxy() - ts := httptest.NewServer(&httputil.ReverseProxy{Director: d.Director, Transport: d}) - defer ts.Close() - - viper.Set(configuration.ViperKeyAuthenticatorNoopIsEnabled, true) - viper.Set(configuration.ViperKeyAuthenticatorUnauthorizedIsEnabled, true) - viper.Set(configuration.ViperKeyAuthenticatorAnonymousIsEnabled, true) - viper.Set(configuration.ViperKeyAuthorizerAllowIsEnabled, true) - viper.Set(configuration.ViperKeyAuthorizerDenyIsEnabled, true) - viper.Set(configuration.ViperKeyMutatorNoopIsEnabled, true) - viper.Set(configuration.ViperKeyErrorsWWWAuthenticateIsEnabled, true) - - ruleNoOpAuthenticator := rule.Rule{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-noop/<[0-9]*>"}, - Authenticators: []rule.Handler{{Handler: "noop"}}, - Authorizer: rule.Handler{Handler: "allow"}, - Mutators: []rule.Handler{{Handler: "noop"}}, - Upstream: rule.Upstream{URL: backend.URL}, - } - ruleNoOpAuthenticatorModifyUpstream := rule.Rule{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/strip-path/authn-noop/<[0-9]*>"}, - Authenticators: []rule.Handler{{Handler: "noop"}}, - Authorizer: rule.Handler{Handler: "allow"}, - Mutators: []rule.Handler{{Handler: "noop"}}, - Upstream: rule.Upstream{URL: backend.URL, StripPath: "/strip-path/", PreserveHost: true}, - } - - for k, tc := range []struct { - url string - code int - messages []string - rules []rule.Rule - transform func(r *http.Request) - d string - }{ - { - d: "should fail because url does not exist in rule set", - url: ts.URL + "/invalid", - rules: []rule.Rule{}, - code: http.StatusNotFound, - }, - { - d: "should fail because url does exist but is matched by two rules", - url: ts.URL + "/authn-noop/1234", - rules: []rule.Rule{ruleNoOpAuthenticator, ruleNoOpAuthenticator}, - code: http.StatusInternalServerError, - }, - { - d: "should pass", - url: ts.URL + "/authn-noop/1234", - rules: []rule.Rule{ruleNoOpAuthenticator}, - code: http.StatusOK, - transform: func(r *http.Request) { - r.Header.Add("Authorization", "bearer token") - }, - messages: []string{ - "authorization=bearer token", - "url=/authn-noop/1234", - "host=" + urlx.ParseOrPanic(backend.URL).Host, - }, - }, - { - d: "should pass", - url: ts.URL + "/strip-path/authn-noop/1234", - rules: []rule.Rule{ruleNoOpAuthenticatorModifyUpstream}, - code: http.StatusOK, - transform: func(r *http.Request) { - r.Header.Add("Authorization", "bearer token") - }, - messages: []string{ - "authorization=bearer token", - "url=/authn-noop/1234", - "host=" + urlx.ParseOrPanic(ts.URL).Host, - }, - }, - { - d: "should fail because no authorizer was configured", - url: ts.URL + "/authn-anon/authz-none/cred-none/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-none/cred-none/<[0-9]*>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Upstream: rule.Upstream{URL: backend.URL}, - }}, - transform: func(r *http.Request) { - r.Header.Add("Authorization", "bearer token") - }, - code: http.StatusUnauthorized, - }, - { - d: "should fail because no credentials issuer was configured", - url: ts.URL + "/authn-anon/authz-allow/cred-none/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-none/<[0-9]*>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Authorizer: rule.Handler{Handler: "allow"}, - Upstream: rule.Upstream{URL: backend.URL}, - }}, - code: http.StatusInternalServerError, - }, - { - d: "should pass with anonymous and everything else set to noop", - url: ts.URL + "/authn-anon/authz-allow/cred-noop/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-allow/cred-noop/<[0-9]*>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, - Authorizer: rule.Handler{Handler: "allow"}, - Mutators: []rule.Handler{{Handler: "noop"}}, - Upstream: rule.Upstream{URL: backend.URL}, - }}, - code: http.StatusOK, - messages: []string{ - "authorization=", - "url=/authn-anon/authz-allow/cred-noop/1234", - "host=" + urlx.ParseOrPanic(backend.URL).Host, - }, - }, - { - d: "should fail when authorizer fails", - url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234", - rules: []rule.Rule{{ - Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, - Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "deny"}, Mutators: []rule.Handler{{Handler: "noop"}}, Upstream: rule.Upstream{URL: backend.URL}, + Errors: []rule.ErrorHandler{{Handler: "www_authenticate"}}, }}, - code: http.StatusForbidden, - }, - { - d: "should fail when authorizer fails and send www_authenticate as defined in the rule", - url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234", - rules: []rule.Rule{{ + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "deny"}, @@ -434,7 +258,12 @@ func TestProxy_GlobMatchingEngine(t *testing.T) { { d: "should fail when authenticator fails", url: ts.URL + "/authn-broken/authz-none/cred-none/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-broken/authz-none/cred-none/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "unauthorized"}}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-broken/authz-none/cred-none/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "unauthorized"}}, Upstream: rule.Upstream{URL: backend.URL}, @@ -444,7 +273,13 @@ func TestProxy_GlobMatchingEngine(t *testing.T) { { d: "should fail because no mutator was configured", url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "allow"}, @@ -455,7 +290,14 @@ func TestProxy_GlobMatchingEngine(t *testing.T) { { d: "should fail when one of the mutators fails", url: ts.URL + "/authn-anon/authz-deny/cred-noop/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "noop"}, {Handler: "broken"}}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anon/authz-deny/cred-noop/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "allow"}, @@ -467,7 +309,14 @@ func TestProxy_GlobMatchingEngine(t *testing.T) { { d: "should fail when credentials issuer fails", url: ts.URL + "/authn-anonymous/authz-allow/cred-broken/1234", - rules: []rule.Rule{{ + rulesRegexp: []rule.Rule{{ + Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]+>"}, + Authenticators: []rule.Handler{{Handler: "anonymous"}}, + Authorizer: rule.Handler{Handler: "allow"}, + Mutators: []rule.Handler{{Handler: "broken"}}, + Upstream: rule.Upstream{URL: backend.URL}, + }}, + rulesGlob: []rule.Rule{{ Match: &rule.Match{Methods: []string{"GET"}, URL: ts.URL + "/authn-anonymous/authz-allow/cred-broken/<[0-9]*>"}, Authenticators: []rule.Handler{{Handler: "anonymous"}}, Authorizer: rule.Handler{Handler: "allow"}, @@ -478,30 +327,41 @@ func TestProxy_GlobMatchingEngine(t *testing.T) { }, } { t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { - reg.RuleRepository().(*rule.RepositoryMemory).WithRules(tc.rules) - require.NoError(t, reg.RuleRepository().SetMatchingStrategy(context.Background(), configuration.Glob)) + testFunc := func(strategy configuration.MatchingStrategy, rules []rule.Rule) { + reg.RuleRepository().(*rule.RepositoryMemory).WithRules(rules) + require.NoError(t, reg.RuleRepository().SetMatchingStrategy(context.Background(), strategy)) - req, err := http.NewRequest("GET", tc.url, nil) - require.NoError(t, err) - if tc.transform != nil { - tc.transform(req) - } + req, err := http.NewRequest("GET", tc.url, nil) + require.NoError(t, err) + if tc.transform != nil { + tc.transform(req) + } - res, err := http.DefaultClient.Do(req) - require.NoError(t, err) + res, err := http.DefaultClient.Do(req) + require.NoError(t, err) - greeting, err := ioutil.ReadAll(res.Body) - require.NoError(t, res.Body.Close()) - require.NoError(t, err) + greeting, err := ioutil.ReadAll(res.Body) + require.NoError(t, res.Body.Close()) + require.NoError(t, err) - assert.Equal(t, tc.code, res.StatusCode, "%s", res.Body) - for _, m := range tc.messages { - assert.True(t, strings.Contains(string(greeting), m), `Value "%s" not found in message: + assert.Equal(t, tc.code, res.StatusCode, "%s", res.Body) + for _, m := range tc.messages { + assert.True(t, strings.Contains(string(greeting), m), `Value "%s" not found in message: %s proxy_url=%s backend_url=%s `, m, greeting, ts.URL, backend.URL) + } + } + + t.Run("regexp", func(t *testing.T) { + testFunc(configuration.Regexp, tc.rulesRegexp) + }) + t.Run("glob", func(t *testing.T) { + testFunc(configuration.Glob, tc.rulesGlob) + }) + }) } } diff --git a/rule/rule_test.go b/rule/rule_test.go index 27ed34c83f..c9abb1b06f 100644 --- a/rule/rule_test.go +++ b/rule/rule_test.go @@ -37,52 +37,24 @@ func mustParse(t *testing.T, u string) *url.URL { } func TestRule(t *testing.T) { - r := &Rule{ - Match: &Match{ - Methods: []string{"DELETE"}, - URL: "https://localhost/users/<[0-9]+>", - }, - } - - var tests = []struct { - method string - url string - expectedMatch bool - expectedErr error - }{ + rules := []Rule{ { - method: "DELETE", - url: "https://localhost/users/1234", - expectedMatch: true, - expectedErr: nil, + Match: &Match{ + Methods: []string{"DELETE"}, + URL: "https://localhost/users/<[0-9]+>", + }, }, { - method: "DELETE", - url: "https://localhost/users/1234?key=value&key1=value1", - expectedMatch: true, - expectedErr: nil, + Match: &Match{ + Methods: []string{"DELETE"}, + URL: "https://localhost/users/<[[:digit:]]*>", + }, }, { - method: "DELETE", - url: "https://localhost/users/abcd", - expectedMatch: false, - expectedErr: nil, - }, - } - for ind, tcase := range tests { - t.Run(string(ind), func(t *testing.T) { - matched, err := r.IsMatching(configuration.Regexp, tcase.method, mustParse(t, tcase.url)) - assert.Equal(t, tcase.expectedMatch, matched) - assert.Equal(t, tcase.expectedErr, err) - }) - } -} - -func TestRule1(t *testing.T) { - r := &Rule{ - Match: &Match{ - Methods: []string{"DELETE"}, - URL: "https://localhost/users/<[[:digit:]]*>", + Match: &Match{ + Methods: []string{"DELETE"}, + URL: "https://localhost/users/<[0-9]*>", + }, }, } @@ -113,14 +85,25 @@ func TestRule1(t *testing.T) { } for ind, tcase := range tests { t.Run(string(ind), func(t *testing.T) { - matched, err := r.IsMatching(configuration.Regexp, tcase.method, mustParse(t, tcase.url)) - assert.Equal(t, tcase.expectedMatch, matched) - assert.Equal(t, tcase.expectedErr, err) + testFunc := func(rule Rule, strategy configuration.MatchingStrategy) { + matched, err := rule.IsMatching(strategy, tcase.method, mustParse(t, tcase.url)) + assert.Equal(t, tcase.expectedMatch, matched) + assert.Equal(t, tcase.expectedErr, err) + } + t.Run("rule0", func(t *testing.T) { + testFunc(rules[0], configuration.Regexp) + }) + t.Run("rule1", func(t *testing.T) { + testFunc(rules[1], configuration.Regexp) + }) + t.Run("rule2", func(t *testing.T) { + testFunc(rules[2], configuration.Glob) + }) }) } } -func TestRule2(t *testing.T) { +func TestRule1(t *testing.T) { r := &Rule{ Match: &Match{ Methods: []string{"DELETE"}, @@ -161,45 +144,3 @@ func TestRule2(t *testing.T) { }) } } - -func TestRuleGlob(t *testing.T) { - r := &Rule{ - Match: &Match{ - Methods: []string{"DELETE"}, - URL: "https://localhost/users/<[0-9]*>", - }, - } - - var tests = []struct { - method string - url string - expectedMatch bool - expectedErr error - }{ - { - method: "DELETE", - url: "https://localhost/users/1234", - expectedMatch: true, - expectedErr: nil, - }, - { - method: "DELETE", - url: "https://localhost/users/1234?key=value&key1=value1", - expectedMatch: true, - expectedErr: nil, - }, - { - method: "DELETE", - url: "https://localhost/users/abcd", - expectedMatch: false, - expectedErr: nil, - }, - } - for ind, tcase := range tests { - t.Run(string(ind), func(t *testing.T) { - matched, err := r.IsMatching(configuration.Glob, tcase.method, mustParse(t, tcase.url)) - assert.Equal(t, tcase.expectedMatch, matched) - assert.Equal(t, tcase.expectedErr, err) - }) - } -} From 7d153d42b73afcf5068791606557d14d40ec0c03 Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Sun, 2 Feb 2020 19:17:25 +0100 Subject: [PATCH 14/15] refactor: go mod tidy Signed-off-by: Aynur Zulkarnaev --- go.mod | 8 +------- go.sum | 5 +++++ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index fa843bc36d..9716e0c660 100644 --- a/go.mod +++ b/go.mod @@ -36,13 +36,10 @@ require ( github.com/ory/gojsonschema v1.2.0 github.com/ory/graceful v0.1.1 github.com/ory/herodot v0.6.2 - github.com/ory/ladon v1.0.1 + github.com/ory/ladon v1.1.0 github.com/ory/sdk/swagutil v0.0.0-20200202121523-307941feee4b github.com/ory/viper v1.5.7 github.com/ory/x v0.0.93 - github.com/ory/ladon v1.1.0 - github.com/ory/viper v1.5.6 - github.com/ory/x v0.0.87 github.com/pborman/uuid v1.2.0 github.com/pelletier/go-toml v1.6.0 // indirect github.com/phayes/freeport v0.0.0-20180830031419-95f893ade6f2 @@ -63,9 +60,6 @@ require ( golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45 golang.org/x/tools v0.0.0-20191224055732-dd894d0a8a40 gopkg.in/square/go-jose.v2 v2.3.1 - golang.org/x/tools v0.0.0-20191026034945-b2104f82a97d - gopkg.in/square/go-jose.v2 v2.3.0 - gopkg.in/yaml.v2 v2.2.7 // indirect ) // Fix for https://github.com/golang/lint/issues/436 diff --git a/go.sum b/go.sum index a29707b5a7..89f36af130 100644 --- a/go.sum +++ b/go.sum @@ -556,6 +556,7 @@ github.com/ory/graceful v0.1.1 h1:zx+8tDObLPrG+7Tc8jKYlXsqWnLtOQA1IZ/FAAKHMXU= github.com/ory/graceful v0.1.1/go.mod h1:zqu70l95WrKHF4AZ6tXHvAqAvpY6M7g6ttaAVcMm7KU= github.com/ory/herodot v0.6.2 h1:zOb5MsuMn7AH9/Ewc/EK83yqcNViK1m1l3C2UuP3RcA= github.com/ory/herodot v0.6.2/go.mod h1:3BOneqcyBsVybCPAJoi92KN2BpJHcmDqAMcAAaJiJow= +github.com/ory/ladon v1.0.1/go.mod h1:1VhCA2mBtaMhRUS6VS0d9qrNVDQnFXqSRb5D0NvQUPY= github.com/ory/ladon v1.1.0 h1:6tgazU2J3Z3odPs1f0qn729kRXCAtlJROliuWUHedV0= github.com/ory/ladon v1.1.0/go.mod h1:25bNc/Glx/8xCH7MbItDxjvviAmFQ+aYxb1V1SE5wlg= github.com/ory/pagination v0.0.1/go.mod h1:d1ToRROAUleriPhmb2dYbhANhhLwZ8s395m2yJCDFh8= @@ -568,6 +569,7 @@ github.com/ory/sdk/swagutil v0.0.0-20200202121523-307941feee4b/go.mod h1:Ufg1eAy github.com/ory/viper v1.5.6/go.mod h1:TYmpFpKLxjQwvT4f0QPpkOn4sDXU1kDgAwJpgLYiQ28= github.com/ory/viper v1.5.7 h1:VeXfcBgTG3SCMlw4hmXkazXPZbyvBTdUjS7Dxm3gOjQ= github.com/ory/viper v1.5.7/go.mod h1:+Mfm2gCDqtYRn5gVaLsBtXACO59zITETZQ/jQwc9SZo= +github.com/ory/x v0.0.87/go.mod h1:wrnJRjIfYXFY/AUiuUlcIUpLBDxFtWc+8x6toAeLZXU= github.com/ory/x v0.0.88/go.mod h1:wrnJRjIfYXFY/AUiuUlcIUpLBDxFtWc+8x6toAeLZXU= github.com/ory/x v0.0.91 h1:4sySRGI1dExt3FpvXcnenpagoM6oQeEvboQ53/tcY9g= github.com/ory/x v0.0.91/go.mod h1:lfcTaGXpTZs7IEQAW00r9EtTCOxD//SiP5uWtNiz31g= @@ -883,8 +885,10 @@ golang.org/x/tools v0.0.0-20190614205625-5aca471b1d59/go.mod h1:/rFqwRUd4F7ZHNgw golang.org/x/tools v0.0.0-20190617190820-da514acc4774/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20190628153133-6cdbf07be9d0/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20190711191110-9a621aea19f8/go.mod h1:jcCCGcm9btYwXyDqrUWc6MKQKKGJCWEQ3AfLSRIbEuI= +golang.org/x/tools v0.0.0-20191026034945-b2104f82a97d/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20191224055732-dd894d0a8a40 h1:UyP2XDSgSc8ldYCxAK735zQxeH3Gd81sK7Iy7AoaVxk= golang.org/x/tools v0.0.0-20191224055732-dd894d0a8a40/go.mod h1:TB2adYChydJhpapKDTa4BR/hXlZSLoq2Wpct/0txZ28= +golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/api v0.4.0/go.mod h1:8k5glujaEP+g9n7WNsDg8QP6cUVNI86fCNMcbazEtwE= google.golang.org/api v0.7.0/go.mod h1:WtwebWUNSVBH/HAw79HIFXZNqEvBhG+Ra+ax0hx3E3M= @@ -925,6 +929,7 @@ gopkg.in/mail.v2 v2.0.0-20180731213649-a0242b2233b4/go.mod h1:htwXN1Qh09vZJ1NVKx gopkg.in/resty.v1 v1.12.0/go.mod h1:mDo4pnntr5jdWRML875a/NmxYqAlA73dVijT2AXvQQo= gopkg.in/square/go-jose.v2 v2.1.9/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI= gopkg.in/square/go-jose.v2 v2.2.2/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI= +gopkg.in/square/go-jose.v2 v2.3.0/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI= gopkg.in/square/go-jose.v2 v2.3.1 h1:SK5KegNXmKmqE342YYN2qPHEnUYeoMiXXl1poUlI+o4= gopkg.in/square/go-jose.v2 v2.3.1/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76mk0e1AI= gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7/go.mod h1:dt/ZhP58zS4L8KSrWDmTeBkI65Dw0HsyUHuEVlX15mw= From 7de92ebf3917975a8ceebb8e6d766bf49cb87cb2 Mon Sep 17 00:00:00 2001 From: Aynur Zulkarnaev Date: Tue, 4 Feb 2020 11:54:13 +0100 Subject: [PATCH 15/15] rule: fix code review comments Signed-off-by: Aynur Zulkarnaev --- README.md | 3 +- rule/{engine_glob.py.go => engine_glob.go} | 12 ++- rule/engine_regexp.go | 12 ++- rule/matcher_test.go | 87 ++++++++-------------- rule/matching_engine.go | 7 +- 5 files changed, 56 insertions(+), 65 deletions(-) rename rule/{engine_glob.py.go => engine_glob.go} (89%) diff --git a/README.md b/README.md index 4772fd7ea6..3e8a233bd2 100644 --- a/README.md +++ b/README.md @@ -4,10 +4,8 @@ Chat | Forums | Newsletter

- Guide | API Docs | Guide | - API Docs | Code Docs

Support this project! @@ -257,6 +255,7 @@ The Guide is available [here](https://www.ory.sh/docs/oathkeeper/). ### HTTP API documentation + The HTTP API is documented [here](https://www.ory.sh/docs/oathkeeper/sdk/api). diff --git a/rule/engine_glob.py.go b/rule/engine_glob.go similarity index 89% rename from rule/engine_glob.py.go rename to rule/engine_glob.go index 0d249d76f1..a0c4693a00 100644 --- a/rule/engine_glob.py.go +++ b/rule/engine_glob.go @@ -2,18 +2,19 @@ package rule import ( "bytes" - "hash/crc32" + "hash/crc64" "github.com/gobwas/glob" ) type globMatchingEngine struct { compiled glob.Glob - checksum uint32 + checksum uint64 + table *crc64.Table } // Checksum of a saved pattern. -func (ge *globMatchingEngine) Checksum() uint32 { +func (ge *globMatchingEngine) Checksum() uint64 { return ge.checksum } @@ -31,7 +32,10 @@ func (ge *globMatchingEngine) ReplaceAllString(_, _, _ string) (string, error) { } func (ge *globMatchingEngine) compile(pattern string) error { - if checksum := crc32.ChecksumIEEE([]byte(pattern)); checksum != ge.checksum { + if ge.table == nil { + ge.table = crc64.MakeTable(polynomial) + } + if checksum := crc64.Checksum([]byte(pattern), ge.table); checksum != ge.checksum { compiled, err := compileGlob(pattern, '<', '>') if err != nil { return err diff --git a/rule/engine_regexp.go b/rule/engine_regexp.go index 833783b777..4ecc02d450 100644 --- a/rule/engine_regexp.go +++ b/rule/engine_regexp.go @@ -1,7 +1,7 @@ package rule import ( - "hash/crc32" + "hash/crc64" "github.com/dlclark/regexp2" "github.com/ory/ladon/compiler" @@ -9,11 +9,15 @@ import ( type regexpMatchingEngine struct { compiled *regexp2.Regexp - checksum uint32 + checksum uint64 + table *crc64.Table } func (re *regexpMatchingEngine) compile(pattern string) error { - if checksum := crc32.ChecksumIEEE([]byte(pattern)); checksum != re.checksum { + if re.table == nil { + re.table = crc64.MakeTable(polynomial) + } + if checksum := crc64.Checksum([]byte(pattern), re.table); checksum != re.checksum { compiled, err := compiler.CompileRegex(pattern, '<', '>') if err != nil { return err @@ -25,7 +29,7 @@ func (re *regexpMatchingEngine) compile(pattern string) error { } // Checksum of a saved pattern. -func (re *regexpMatchingEngine) Checksum() uint32 { +func (re *regexpMatchingEngine) Checksum() uint64 { return re.checksum } diff --git a/rule/matcher_test.go b/rule/matcher_test.go index cf44829b62..86d02c97a6 100644 --- a/rule/matcher_test.go +++ b/rule/matcher_test.go @@ -68,6 +68,36 @@ var testRules = []Rule{ }, } +var testRulesGlob = []Rule{ + { + ID: "foo1", + Match: &Match{URL: "https://localhost:1234/<{foo*,bar*}>", Methods: []string{"POST"}}, + Description: "Create users rule", + Authorizer: Handler{Handler: "allow", Config: []byte(`{"type":"any"}`)}, + Authenticators: []Handler{{Handler: "anonymous", Config: []byte(`{"name":"anonymous1"}`)}}, + Mutators: []Handler{{Handler: "id_token", Config: []byte(`{"issuer":"anything"}`)}}, + Upstream: Upstream{URL: "http://localhost:1235/", StripPath: "/bar", PreserveHost: true}, + }, + { + ID: "foo2", + Match: &Match{URL: "https://localhost:34/<{baz*,bar*}>", Methods: []string{"GET"}}, + Description: "Get users rule", + Authorizer: Handler{Handler: "deny", Config: []byte(`{"type":"any"}`)}, + Authenticators: []Handler{{Handler: "oauth2_introspection", Config: []byte(`{"name":"anonymous1"}`)}}, + Mutators: []Handler{{Handler: "id_token", Config: []byte(`{"issuer":"anything"}`)}}, + Upstream: Upstream{URL: "http://localhost:333/", StripPath: "/foo", PreserveHost: false}, + }, + { + ID: "foo3", + Match: &Match{URL: "https://localhost:343/<{baz*,bar*}>", Methods: []string{"GET"}}, + Description: "Get users rule", + Authorizer: Handler{Handler: "deny"}, + Authenticators: []Handler{{Handler: "oauth2_introspection"}}, + Mutators: []Handler{{Handler: "id_token"}}, + Upstream: Upstream{URL: "http://localhost:3333/", StripPath: "/foo", PreserveHost: false}, + }, +} + func TestMatcher(t *testing.T) { type m interface { Matcher @@ -87,7 +117,7 @@ func TestMatcher(t *testing.T) { for name, matcher := range map[string]m{ "memory": NewRepositoryMemory(new(mockRepositoryRegistry)), } { - t.Run(fmt.Sprintf("matcher=%s", name), func(t *testing.T) { + t.Run(fmt.Sprintf("regexp matcher=%s", name), func(t *testing.T) { t.Run("case=empty", func(t *testing.T) { testMatcher(t, matcher, "GET", "https://localhost:34/baz", true, nil) testMatcher(t, matcher, "POST", "https://localhost:1234/foo", true, nil) @@ -118,60 +148,9 @@ func TestMatcher(t *testing.T) { testMatcher(t, matcher, "DELETE", "https://localhost:1234/foo", true, nil) }) }) - } -} - -var testRulesGlob = []Rule{ - { - ID: "foo1", - Match: &Match{URL: "https://localhost:1234/<{foo*,bar*}>", Methods: []string{"POST"}}, - Description: "Create users rule", - Authorizer: Handler{Handler: "allow", Config: []byte(`{"type":"any"}`)}, - Authenticators: []Handler{{Handler: "anonymous", Config: []byte(`{"name":"anonymous1"}`)}}, - Mutators: []Handler{{Handler: "id_token", Config: []byte(`{"issuer":"anything"}`)}}, - Upstream: Upstream{URL: "http://localhost:1235/", StripPath: "/bar", PreserveHost: true}, - }, - { - ID: "foo2", - Match: &Match{URL: "https://localhost:34/<{baz*,bar*}>", Methods: []string{"GET"}}, - Description: "Get users rule", - Authorizer: Handler{Handler: "deny", Config: []byte(`{"type":"any"}`)}, - Authenticators: []Handler{{Handler: "oauth2_introspection", Config: []byte(`{"name":"anonymous1"}`)}}, - Mutators: []Handler{{Handler: "id_token", Config: []byte(`{"issuer":"anything"}`)}}, - Upstream: Upstream{URL: "http://localhost:333/", StripPath: "/foo", PreserveHost: false}, - }, - { - ID: "foo3", - Match: &Match{URL: "https://localhost:343/<{baz*,bar*}>", Methods: []string{"GET"}}, - Description: "Get users rule", - Authorizer: Handler{Handler: "deny"}, - Authenticators: []Handler{{Handler: "oauth2_introspection"}}, - Mutators: []Handler{{Handler: "id_token"}}, - Upstream: Upstream{URL: "http://localhost:3333/", StripPath: "/foo", PreserveHost: false}, - }, -} - -func TestMatcherGlob(t *testing.T) { - type m interface { - Matcher - Repository - } - - var testMatcher = func(t *testing.T, matcher Matcher, method string, url string, expectErr bool, expect *Rule) { - r, err := matcher.Match(context.Background(), method, mustParseURL(t, url)) - if expectErr { - require.Error(t, err) - } else { - require.NoError(t, err) - assert.EqualValues(t, *expect, *r) - } - } - - for name, matcher := range map[string]m{ - "memory": NewRepositoryMemory(new(mockRepositoryRegistry)), - } { - t.Run(fmt.Sprintf("matcher=%s", name), func(t *testing.T) { + t.Run(fmt.Sprintf("glob matcher=%s", name), func(t *testing.T) { require.NoError(t, matcher.SetMatchingStrategy(context.Background(), configuration.Glob)) + require.NoError(t, matcher.Set(context.Background(), []Rule{})) t.Run("case=empty", func(t *testing.T) { testMatcher(t, matcher, "GET", "https://localhost:34/baz", true, nil) testMatcher(t, matcher, "POST", "https://localhost:1234/foo", true, nil) diff --git a/rule/matching_engine.go b/rule/matching_engine.go index 28d1de6bdf..1acc3344fa 100644 --- a/rule/matching_engine.go +++ b/rule/matching_engine.go @@ -1,9 +1,14 @@ package rule import ( + "hash/crc64" + "github.com/pkg/errors" ) +// polynomial for crc64 table which is used for checking crc64 checksum +const polynomial = crc64.ECMA + // common errors for MatchingEngine. var ( ErrUnbalancedPattern = errors.New("unbalanced pattern") @@ -15,5 +20,5 @@ var ( type MatchingEngine interface { IsMatching(pattern, matchAgainst string) (bool, error) ReplaceAllString(pattern, input, replacement string) (string, error) - Checksum() uint32 + Checksum() uint64 }