From 1e7cfaf92dfb4418fe58eaa18706381086410f6d Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Tue, 11 Aug 2026 11:15:27 +0200 Subject: [PATCH] refactor(policies): read the rego policies once at startup --- services/policies/README.md | 149 +++++++++++------ services/policies/pkg/engine/opa/engine.go | 66 +++++--- .../pkg/engine/opa/engine_bench_test.go | 71 ++++++++ .../policies/pkg/engine/opa/engine_test.go | 154 ++++++++++++++++++ .../pkg/engine/opa/example_policies_test.go | 94 +++++++++++ .../policies/pkg/engine/opa/rf_resource.go | 74 ++++----- .../pkg/engine/opa/rf_resource_test.go | 52 +++++- .../pkg/engine/opa/testdata/bench/proxy.rego | 14 ++ .../pkg/engine/opa/testdata/bench/utils.rego | 15 ++ .../pkg/engine/opa/testdata/rules/broken.rego | 3 + .../pkg/engine/opa/testdata/rules/denied.rego | 5 + .../engine/opa/testdata/rules/granted.rego | 5 + services/proxy/pkg/middleware/policies.go | 8 + .../proxy/pkg/middleware/policies_test.go | 80 ++++++++- 14 files changed, 679 insertions(+), 111 deletions(-) create mode 100644 services/policies/pkg/engine/opa/engine_bench_test.go create mode 100644 services/policies/pkg/engine/opa/engine_test.go create mode 100644 services/policies/pkg/engine/opa/example_policies_test.go create mode 100644 services/policies/pkg/engine/opa/testdata/bench/proxy.rego create mode 100644 services/policies/pkg/engine/opa/testdata/bench/utils.rego create mode 100644 services/policies/pkg/engine/opa/testdata/rules/broken.rego create mode 100644 services/policies/pkg/engine/opa/testdata/rules/denied.rego create mode 100644 services/policies/pkg/engine/opa/testdata/rules/granted.rego diff --git a/services/policies/README.md b/services/policies/README.md index e35038f61c..bc089b883e 100644 --- a/services/policies/README.md +++ b/services/policies/README.md @@ -1,34 +1,39 @@ # Policies -The policies service provides a new gRPC API which can be used to check whether a requested operation is allowed or not. To do so, Open Policy Agent (OPA) is used to define the set of rules of what is permitted and what is not. +The policies service provides a new gRPC API which can be used to check whether a requested operation is allowed or not. +To do so, Open Policy Agent (OPA) is used to define the set of rules of what is permitted and what is not. -Policies are written in the [rego query language](https://www.openpolicyagent.org/docs/latest/policy-language/). The location of the rego files can be configured via yaml, a configuration via environment variables is not possible. +Policies are written in the [rego query language](https://www.openpolicyagent.org/docs/latest/policy-language/). The +location of the rego files can be configured via yaml, a configuration via environment variables is not possible. ## General Information The policies service consists of the following modules: -* Proxy authorization (middleware) -* Event authorization (async post-processing) -* gRPC API (can be used by other services) +* Proxy authorization (middleware) +* Event authorization (async post-processing) +* gRPC API (can be used by other services) To configure the policies service, three environment variables need to be defined: -* `POLICIES_ENGINE_TIMEOUT` -* `POLICIES_POSTPROCESSING_QUERY` -* `PROXY_POLICIES_QUERY` +* `POLICIES_ENGINE_TIMEOUT` +* `POLICIES_POSTPROCESSING_QUERY` +* `PROXY_POLICIES_QUERY` -Note that each query setting defines the [Complete Rules](https://www.openpolicyagent.org/docs/latest/#complete-rules) variable defined in the rego rule set the corresponding step uses for the evaluation. If the variable is mistyped or not found, the evaluation defaults to deny. Individual query definitions can be defined for each module. +Note that each query setting defines the [Complete Rules](https://www.openpolicyagent.org/docs/latest/#complete-rules) +variable defined in the rego rule set the corresponding step uses for the evaluation. If the variable is mistyped or not +found, the evaluation defaults to deny. Individual query definitions can be defined for each module. -To activate the policies service for a module, it must be started with a yaml configuration that points to one or more rego files. Note that if the service is scaled horizontally, each instance should have access to the same rego files to avoid unpredictable results. If a file path has been configured but the file is not present or accessible, the evaluation defaults to deny. +To activate the policies service for a module, it must be started with a yaml configuration that points to at least one +rego file that contains the complete rule variable to be queried. Note that if the service is scaled horizontally, each +instance should have access to the same rego files to avoid unpredictable results. -When using async post-processing which is done via the postprocessing service, the value `policies` must be added to the `POSTPROCESSING_STEPS` configuration in postprocessing service in the order where the evaluation should take place. +The rego files are read once when the service starts. A changed rule set takes effect after a restart, and a configured +file that is missing or does not parse keeps the service from starting. -variable defined in the Rego rule set the corresponding step uses for the evaluation. If the variable is mistyped or not found, the evaluation defaults to deny. Individual query definitions can be defined for each module. - -To activate the policies service for a module, it must be started with a yaml configuration that points to at least one Rego file that contains the complete rule variable to be queried. Note that if the service is scaled horizontally, each instance should have access to the same Rego files to avoid unpredictable results. If a file path has been configured but the file it is not present or accessible, the evaluation defaults to deny. - -When using async post-processing via the postprocessing service, the value `policies` must be added to the `POSTPROCESSING_STEPS` configuration in the order in which the evaluation should take place. Example: First check if a file contains questionable content via policies. If it looks okay, continue to check for viruses. +When using async post-processing via the postprocessing service, the value `policies` must be added to the +`POSTPROCESSING_STEPS` configuration in the order in which the evaluation should take place. Example: First check if a +file contains questionable content via policies. If it looks okay, continue to check for viruses. For configuration examples, the [Example Policies](#example-policies) from below are used. @@ -36,39 +41,59 @@ For configuration examples, the [Example Policies](#example-policies) from below ### gRPC API -The gRPC API can be used by any other internal service. It can also be used for example by third parties to find out if an action is allowed or not. This layer is already used by the proxy middleware. There is no configuration necessary, because the query setting (complete rule variable) must be part of the request. +The gRPC API can be used by any other internal service. It can also be used for example by third parties to find out if +an action is allowed or not. This layer is already used by the proxy middleware. There is no configuration necessary, +because the query setting (complete rule variable) must be part of the request. ### Proxy Middleware -The proxy service already includes a middleware which uses the internal [gRPC API](#grpc-api) to evaluate the policies. Since the proxy is in heavy use and every HTTP request is processed here, only simple and quick decisions should be evaluated. More complex queries such as file content evaluation are _strongly_ discouraged. +The proxy service already includes a middleware which uses the internal [gRPC API](#grpc-api) to evaluate the policies. +Since the proxy is in heavy use and every HTTP request is processed here, only simple and quick decisions should be +evaluated. More complex queries such as file content evaluation are _strongly_ discouraged. -If the evaluation in the proxy results in a "denied" outcome, the response will return a `403 Permission Denied` with the following response body +The middleware only denies on the outcome of the policy, it makes no decision of its own. Where the file name is not +part of the request path, for example when a single shared resource is uploaded to by its id, the middleware stats the +resource to obtain it. If that stat fails, `input.resource.name` reaches the policy empty and the policy decides what +that means. Prefer policies that state which files are allowed over policies that list what is forbidden, a rule +matching on a specific extension does not match an empty name: + +```rego +granted = false if { + input.resource.name == "" +} +``` + +If the evaluation in the proxy results in a "denied" outcome, the response will return a `403 Permission Denied` with +the following response body ```json { - "error": - { - "code": "deniedByPolicy", - "message": "Operation denied due to security policies", - "innererror": - { - "date": "2023-09-19T13:22:20Z", - "filename": "File", - "method": "POST", - "path": "/dav/spaces/some-space-id/Folder/", - "request-id": "9CFCE925-F9D9-4F26-AB3B-2C1C40A9CD0C" - } + "error": { + "code": "deniedByPolicy", + "message": "Operation denied due to security policies", + "innererror": { + "date": "2023-09-19T13:22:20Z", + "filename": "File", + "method": "POST", + "path": "/dav/spaces/some-space-id/Folder/", + "request-id": "9CFCE925-F9D9-4F26-AB3B-2C1C40A9CD0C" } + } } ``` ### Event Service (Postprocessing) -This layer is event-based and part of the postprocessing service. Since processing at this point is asynchronous, the operations can also take longer and be more expensive, like evaluating the contents of a file. +This layer is event-based and part of the postprocessing service. Since processing at this point is asynchronous, the +operations can also take longer and be more expensive, like evaluating the contents of a file. ## Defining Policies to Evaluate -Each module can have as many policy files as needed for evaluation. Files can also include other files if necessary. To use policies, they have to be saved to a location that is accessible to the policies service. As a good starting point, take the config directory and use a subdirectory collecting all the `.rego` files, though any other directory can be defined. The config directory is already accessible by all services and usually is included in a xref:maintenance/b-r/backup.adoc[backup] plan. +Each module can have as many policy files as needed for evaluation. Files can also include other files if necessary. To +use policies, they have to be saved to a location that is accessible to the policies service. As a good starting point, +take the config directory and use a subdirectory collecting all the `.rego` files, though any other directory can be +defined. The config directory is already accessible by all services and usually is included in a xref: +maintenance/b-r/backup.adoc[backup] plan. If this is done, it's required to configure the policies service to use these files: @@ -83,7 +108,8 @@ policies: - your_path_to_policies/util.rego ``` -Once the references to policy files are configured correctly, the `_QUERY` configuration needs to be defined for the proxy middleware and for the events service. +Once the references to policy files are configured correctly, the `_QUERY` configuration needs to be defined for the +proxy middleware and for the events service. ## Setting the Query Configuration @@ -92,9 +118,11 @@ To define a value for the query evaluation, the following scheme is necessary: `data..` * The keyword `data` is mandatory and must be present. -* The `package-name` is defined in one .rego file like `package postprocessing`. It is not related to the filename. For more details, see the [packages](https://www.openpolicyagent.org/docs/latest/policy-language/#packages) documentation. +* The `package-name` is defined in one .rego file like `package postprocessing`. It is not related to the filename. For + more details, see the [packages](https://www.openpolicyagent.org/docs/latest/policy-language/#packages) documentation. * The `complete-rule-variable-name` is the variable providing the result of the evaluation. -* Exact one of the defined files, which is responsible for returning the evaluation result, must contain the combination of `` and ``. +* Exact one of the defined files, which is responsible for returning the evaluation result, must contain the combination + of `` and ``. ### Proxy @@ -126,29 +154,54 @@ The same can be achieved by setting the following environment variable: export POLICIES_POSTPROCESSING_QUERY=data.postprocessing.granted ``` -As soon as that query is configured, the postprocessing service must be informed to use the policies step by setting the environment variable: +As soon as that query is configured, the postprocessing service must be informed to use the policies step by setting the +environment variable: ```shell export POSTPROCESSING_STEPS=policies ``` -Note that additional steps can be configured and their position in the list defines the order of processing. For details see the postprocessing service documentation. +Note that additional steps can be configured and their position in the list defines the order of processing. For details +see the postprocessing service documentation. ## Rego Key Match -To identify available keys for OPA, you need to look at [engine.go](https://github.com/opencloud-eu/opencloud/blob/main/services/policies/pkg/engine/engine.go) and the [policies.swagger.json](https://github.com/opencloud-eu/opencloud/blob/master/protogen/gen/opencloud/services/policies/v0/policies.swagger.json) file. Note that which keys are available depends on from which module it is used. +To identify available keys for OPA, you need to look +at [engine.go](https://github.com/opencloud-eu/opencloud/blob/main/services/policies/pkg/engine/engine.go) and +the [policies.swagger.json](https://github.com/opencloud-eu/opencloud/blob/master/protogen/gen/opencloud/services/policies/v0/policies.swagger.json) +file. Note that which keys are available depends on from which module it is used. + +## Rego Extensions + +Besides the standard rego built-in functions, the following functions are added on top: + +| Function | Result | Description | +|----------------------------------------------------|---------------------|----------------------------------------------------------------------------------------------------------------------------------------------| +| `opencloud.mimetype.extensions("application/pdf")` | `[".pdf"]` | Lists the file extensions associated with a mimetype. See [Extend Mimetype File Extension Mapping](#extend-mimetype-file-extension-mapping). | +| `opencloud.mimetype.detect(bytes)` | `"application/pdf"` | Detects a mimetype from content. The list of known mimetypes is limited. | +| `opencloud.resource.download(input.resource.url)` | bytes | Downloads a resource. Available in the event service (postprocessing) where `input.resource.url` is set. | + +Rego has no byte type, so `opencloud.resource.download` hands the content to the policy as base64. +`opencloud.mimetype.detect` takes that value as it is, a policy working on the content itself has to run it through +`base64.decode` first. + +Note that `opencloud.resource.download` performs an HTTP request and holds the whole resource in memory. Use it in +postprocessing policies only, not in policies evaluated by the proxy middleware. ## Extend Mimetype File Extension Mapping -In the extended set of the rego query language, it is possible to get a list of associated file extensions based on a mimetype, for example `opencloud.mimetype.extensions("application/pdf")`. +In the extended set of the rego query language, it is possible to get a list of associated file extensions based on a +mimetype, for example `opencloud.mimetype.extensions("application/pdf")`. The list of mappings is restricted by default and is provided by the host system OpenCloud is installed on. -In order to extend this list, OpenCloud must be provided with the path to a custom `mime.types` file that maps mimetypes to extensions. -The location for the file must be accessible by all instances of the policy service. As a rule of thumb, use the directory where the OpenCloud configuration files are stored. -Note that existing mappings from the host are extended by the definitions from the mime types file, but not replaced. +In order to extend this list, OpenCloud must be provided with the path to a custom `mime.types` file that maps mimetypes +to extensions. The location for the file must be accessible by all instances of the policy service. As a rule of thumb, +use the directory where the OpenCloud configuration files are stored. Note that existing mappings from the host are +extended by the definitions from the mime types file, but not replaced. -The path to that file can be provided via a yaml configuration or an environment variable. Note to replace the `OC_CONFIG_DIR` string by an existing path. +The path to that file can be provided via a yaml configuration or an environment variable. Note to replace the +`OC_CONFIG_DIR` string by an existing path. ```shell export POLICIES_ENGINE_MIMES=OC_CONFIG_DIR/mime.types @@ -160,8 +213,12 @@ policies: mimes: OC_CONFIG_DIR/mime.types ``` -A good example of how such a file should be formatted can be found in the [Apache svn repository](https://svn.apache.org/repos/asf/httpd/httpd/trunk/docs/conf/mime.types). +A good example of how such a file should be formatted can be found in +the [Apache svn repository](https://svn.apache.org/repos/asf/httpd/httpd/trunk/docs/conf/mime.types). ## Example Policies -The policies service contains a set of preconfigured example policies. See the [devtools policie](https://github.com/opencloud-eu/opencloud/tree/main/devtools/deployments/service_policies/policies/) directory for details. The contained policies disallow OpenCloud to create certain file types, both via the proxy middleware and the events service via postprocessing. +The policies service contains a set of preconfigured example policies. See +the [devtools policie](https://github.com/opencloud-eu/opencloud/tree/main/devtools/deployments/service_policies/policies/) +directory for details. The contained policies disallow OpenCloud to create certain file types, both via the proxy +middleware and the events service via postprocessing. diff --git a/services/policies/pkg/engine/opa/engine.go b/services/policies/pkg/engine/opa/engine.go index 79f5187d51..102fc94c11 100644 --- a/services/policies/pkg/engine/opa/engine.go +++ b/services/policies/pkg/engine/opa/engine.go @@ -6,64 +6,86 @@ import ( "os" "time" + "github.com/open-policy-agent/opa/loader" "github.com/open-policy-agent/opa/rego" - "github.com/open-policy-agent/opa/topdown/print" + "github.com/opencloud-eu/reva/v2/pkg/rhttp" "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/policies/pkg/config" "github.com/opencloud-eu/opencloud/services/policies/pkg/engine" ) +// downloads go to the internal data gateway, its certificate is not verified. +const rHTTPInsecure = true + // OPA wraps open policy agent makes it possible to ask if an action is granted. type OPA struct { - printHook print.Hook - policies []string - timeout time.Duration - options []func(r *rego.Rego) + timeout time.Duration + policies *loader.Result + options []func(r *rego.Rego) } // NewOPA returns a ready to use opa engine. -func NewOPA(timeout time.Duration, logger log.Logger, conf config.Engine) (OPA, error) { +func NewOPA(timeout time.Duration, logger log.Logger, conf config.Engine) (*OPA, error) { var mtReader io.ReadCloser if conf.Mimes != "" { var err error mtReader, err = os.Open(conf.Mimes) if err != nil { - return OPA{}, err + return nil, err } - defer mtReader.Close() + defer func() { + _ = mtReader.Close() + }() } rfMimetypeExtensions, err := RFMimetypeExtensions(mtReader) if err != nil { - return OPA{}, err + return nil, err } - return OPA{ - policies: conf.Policies, - timeout: timeout, - printHook: logPrinter{logger: logger}, - options: []func(r *rego.Rego){ - RFMimetypeDetect, - RFResourceDownload, - rfMimetypeExtensions, - }, + policies, err := loader.NewFileLoader().WithProcessAnnotation(true).Filtered(conf.Policies, nil) + if err != nil { + return nil, err + } + + options := []func(r *rego.Rego){ + rego.EnablePrintStatements(true), + rego.PrintHook(logPrinter{logger: logger}), + RFMimetypeDetect, + RFResourceDownload(rhttp.GetHTTPClient(rhttp.Insecure(rHTTPInsecure))), + rfMimetypeExtensions, + } + + for _, module := range policies.ParsedModules() { + options = append(options, rego.ParsedModule(module)) + } + + return &OPA{ + timeout: timeout, + policies: policies, + options: options, }, nil } // Evaluate evaluates the opa policies and returns the result. -func (o OPA) Evaluate(ctx context.Context, qs string, env engine.Environment) (bool, error) { +func (o *OPA) Evaluate(ctx context.Context, qs string, env engine.Environment) (bool, error) { + // note that we use the caller's context here because having a timeout is optional and up to the caller, + // since this part only parses the rules, and the configured timeout ctx, cancel := context.WithTimeout(ctx, o.timeout) defer cancel() + store, err := o.policies.Store() + if err != nil { + return false, err + } + q, err := rego.New( append([]func(r *rego.Rego){ rego.Query(qs), - rego.Load(o.policies, nil), - rego.EnablePrintStatements(true), - rego.PrintHook(o.printHook), + rego.Store(store), }, o.options...)..., ).PrepareForEval(ctx) if err != nil { diff --git a/services/policies/pkg/engine/opa/engine_bench_test.go b/services/policies/pkg/engine/opa/engine_bench_test.go new file mode 100644 index 0000000000..9f919d59aa --- /dev/null +++ b/services/policies/pkg/engine/opa/engine_bench_test.go @@ -0,0 +1,71 @@ +package opa_test + +import ( + "context" + "path/filepath" + "testing" + "time" + + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/services/policies/pkg/config" + "github.com/opencloud-eu/opencloud/services/policies/pkg/engine" + "github.com/opencloud-eu/opencloud/services/policies/pkg/engine/opa" +) + +func benchEngine(tb testing.TB) engine.Engine { + tb.Helper() + + e, err := opa.NewOPA(10*time.Second, log.NopLogger(), config.Engine{Policies: []string{ + filepath.Join("testdata", "bench", "proxy.rego"), + filepath.Join("testdata", "bench", "utils.rego"), + }}) + if err != nil { + tb.Fatal(err) + } + + return e +} + +func benchEnvironment(name string) engine.Environment { + return engine.Environment{ + Stage: engine.StageHTTP, + Request: engine.Request{Method: "PUT", Path: "/remote.php/dav/files/alice/" + name}, + Resource: engine.Resource{Name: name}, + } +} + +func BenchmarkEvaluate(b *testing.B) { + e := benchEngine(b) + env := benchEnvironment("report.txt") + ctx := context.Background() + + b.ReportAllocs() + b.ResetTimer() + + for range b.N { + granted, err := e.Evaluate(ctx, "data.proxy.granted", env) + if err != nil { + b.Fatal(err) + } + if !granted { + b.Fatal("expected .txt to be granted") + } + } +} + +func BenchmarkEvaluateParallel(b *testing.B) { + e := benchEngine(b) + env := benchEnvironment("report.txt") + ctx := context.Background() + + b.ReportAllocs() + b.ResetTimer() + + b.RunParallel(func(pb *testing.PB) { + for pb.Next() { + if _, err := e.Evaluate(ctx, "data.proxy.granted", env); err != nil { + b.Fatal(err) + } + } + }) +} diff --git a/services/policies/pkg/engine/opa/engine_test.go b/services/policies/pkg/engine/opa/engine_test.go new file mode 100644 index 0000000000..e28325db60 --- /dev/null +++ b/services/policies/pkg/engine/opa/engine_test.go @@ -0,0 +1,154 @@ +package opa_test + +import ( + "bytes" + "context" + "image" + "image/png" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "sync/atomic" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/services/policies/pkg/config" + "github.com/opencloud-eu/opencloud/services/policies/pkg/engine" + "github.com/opencloud-eu/opencloud/services/policies/pkg/engine/opa" +) + +var _ = Describe("engine", func() { + newEngine := func(source string) (engine.Engine, string) { + path := filepath.Join(GinkgoT().TempDir(), "policy.rego") + Expect(os.WriteFile(path, []byte(source), 0o600)).To(Succeed()) + + e, err := opa.NewOPA(10*time.Second, log.NopLogger(), config.Engine{Policies: []string{path}}) + Expect(err).ToNot(HaveOccurred()) + + return e, path + } + + Context("when the policy files are read", func() { + var path string + + install := func(name string) { + source, err := os.ReadFile(filepath.Join("testdata", "rules", name)) + Expect(err).ToNot(HaveOccurred()) + Expect(os.WriteFile(path, source, 0o600)).To(Succeed()) + } + + start := func(policies ...string) engine.Engine { + e, err := opa.NewOPA(10*time.Second, log.NopLogger(), config.Engine{Policies: policies}) + Expect(err).ToNot(HaveOccurred()) + + return e + } + + decide := func(e engine.Engine) bool { + granted, err := e.Evaluate(context.Background(), "data.rules.granted", engine.Environment{}) + Expect(err).ToNot(HaveOccurred()) + + return granted + } + + BeforeEach(func() { + path = filepath.Join(GinkgoT().TempDir(), "rules.rego") + install("granted.rego") + }) + + It("keeps the rule set a rewrite replaced", func() { + e := start(path) + Expect(decide(e)).To(BeTrue()) + + install("denied.rego") + Expect(decide(e)).To(BeTrue(), "a rewrite only takes effect on restart") + }) + + It("keeps the rule set after the file disappeared", func() { + e := start(path) + Expect(os.Remove(path)).To(Succeed()) + + Expect(decide(e)).To(BeTrue()) + }) + + It("refuses to start on a broken policy", func() { + install("broken.rego") + + _, err := opa.NewOPA(10*time.Second, log.NopLogger(), config.Engine{Policies: []string{path}}) + Expect(err).To(HaveOccurred()) + }) + + It("refuses to start when a policy is missing", func() { + Expect(os.Remove(path)).To(Succeed()) + + _, err := opa.NewOPA(10*time.Second, log.NopLogger(), config.Engine{Policies: []string{path}}) + Expect(err).To(HaveOccurred()) + }) + + It("takes a directory as a policy path", func() { + dir := GinkgoT().TempDir() + source, err := os.ReadFile(filepath.Join("testdata", "rules", "granted.rego")) + Expect(err).ToNot(HaveOccurred()) + Expect(os.WriteFile(filepath.Join(dir, "policy.rego"), source, 0o600)).To(Succeed()) + + Expect(decide(start(dir))).To(BeTrue()) + }) + }) + + Context("across evaluations", func() { + It("carries no downloaded content into the next one", func() { + buf := new(bytes.Buffer) + Expect(png.Encode(buf, image.NewRGBA(image.Rect(0, 0, 1, 1)))).To(Succeed()) + + // same url, different content per request. + var served atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + body := []byte("plain text, not an image") + if served.Add(1) == 1 { + body = buf.Bytes() + } + + _, err := w.Write(body) + Expect(err).ToNot(HaveOccurred()) + })) + defer srv.Close() + + e, _ := newEngine("package isolation\n\nimport future.keywords.if\n\ndefault granted := true\n\ngranted = false if {\n body := opencloud.resource.download(input.resource.url)\n opencloud.mimetype.detect(body) == \"image/png\"\n}\n") + env := engine.Environment{Resource: engine.Resource{URL: srv.URL}} + + first, err := e.Evaluate(context.Background(), "data.isolation.granted", env) + Expect(err).ToNot(HaveOccurred()) + Expect(first).To(BeFalse(), "png is served first and has to be denied") + + second, err := e.Evaluate(context.Background(), "data.isolation.granted", env) + Expect(err).ToNot(HaveOccurred()) + Expect(second).To(BeTrue(), "text is served second, seeing the png again means the memo leaked") + + Expect(served.Load()).To(BeEquivalentTo(2), "the second evaluation has to fetch again") + }) + + It("judges each one by its own input", func() { + e, _ := newEngine("package isolation\n\nimport future.keywords.if\n\ndefault granted := true\n\ngranted = false if {\n endswith(input.resource.name, \".exe\")\n}\n") + + for _, tc := range []struct { + name string + want bool + }{ + {"notes.txt", true}, + {"virus.exe", false}, + {"notes.txt", true}, + {"virus.exe", false}, + } { + granted, err := e.Evaluate(context.Background(), "data.isolation.granted", engine.Environment{ + Resource: engine.Resource{Name: tc.name}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(granted).To(Equal(tc.want), "for %s", tc.name) + } + }) + }) +}) diff --git a/services/policies/pkg/engine/opa/example_policies_test.go b/services/policies/pkg/engine/opa/example_policies_test.go new file mode 100644 index 0000000000..958b88cc5b --- /dev/null +++ b/services/policies/pkg/engine/opa/example_policies_test.go @@ -0,0 +1,94 @@ +package opa_test + +import ( + "bytes" + "compress/gzip" + "context" + "image" + "image/png" + "net/http" + "net/http/httptest" + "path/filepath" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/services/policies/pkg/config" + "github.com/opencloud-eu/opencloud/services/policies/pkg/engine" + "github.com/opencloud-eu/opencloud/services/policies/pkg/engine/opa" +) + +const examplePolicyDir = "../../../../../devtools/deployments/service_policies/policies" + +func gzipped(payload string) []byte { + buf := new(bytes.Buffer) + w := gzip.NewWriter(buf) + _, err := w.Write([]byte(payload)) + Expect(err).ToNot(HaveOccurred()) + Expect(w.Close()).To(Succeed()) + + return buf.Bytes() +} + +var _ = Describe("the shipped example policies", func() { + var ( + e engine.Engine + body []byte + srv *httptest.Server + ) + + BeforeEach(func() { + var err error + e, err = opa.NewOPA(10*time.Second, log.NopLogger(), config.Engine{Policies: []string{ + filepath.Join(examplePolicyDir, "proxy.rego"), + filepath.Join(examplePolicyDir, "postprocessing.rego"), + filepath.Join(examplePolicyDir, "utils.rego"), + }}) + Expect(err).ToNot(HaveOccurred()) + + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, err := w.Write(body) + Expect(err).ToNot(HaveOccurred()) + })) + DeferCleanup(srv.Close) + }) + + DescribeTable("data.proxy.granted judges the upload by its name", + func(method, path, name string, expected bool) { + granted, err := e.Evaluate(context.Background(), "data.proxy.granted", engine.Environment{ + Stage: engine.StageHTTP, + Request: engine.Request{Method: method, Path: path}, + Resource: engine.Resource{Name: name}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(granted).To(Equal(expected)) + }, + Entry("allowed extension", http.MethodPut, "/remote.php/dav/files/alice/notes.txt", "notes.txt", true), + Entry("denied extension", http.MethodPut, "/remote.php/dav/files/alice/virus.exe", "virus.exe", false), + Entry("denied on tus post", http.MethodPost, "/data/upload", "virus.exe", false), + Entry("unrestricted path", http.MethodPut, "/graph/v1.0/me", "virus.exe", true), + Entry("unrestricted method", http.MethodGet, "/remote.php/dav/files/alice/virus.exe", "virus.exe", true), + ) + + DescribeTable("data.postprocessing.granted judges the upload by its content", + func(name string, content func() []byte, expected bool) { + body = content() + + granted, err := e.Evaluate(context.Background(), "data.postprocessing.granted", engine.Environment{ + Stage: engine.StagePP, + Resource: engine.Resource{Name: name, URL: srv.URL}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(granted).To(Equal(expected)) + }, + Entry("allowed mimetype", "image.png", func() []byte { + buf := new(bytes.Buffer) + Expect(png.Encode(buf, image.NewRGBA(image.Rect(0, 0, 1, 1)))).To(Succeed()) + return buf.Bytes() + }, true), + Entry("denied mimetype", "image.png", func() []byte { return gzipped("payload") }, false), + Entry("denied extension short circuits before the download", "virus.exe", func() []byte { return nil }, false), + ) +}) diff --git a/services/policies/pkg/engine/opa/rf_resource.go b/services/policies/pkg/engine/opa/rf_resource.go index 6307ce53d0..6724a1bbe2 100644 --- a/services/policies/pkg/engine/opa/rf_resource.go +++ b/services/policies/pkg/engine/opa/rf_resource.go @@ -8,53 +8,53 @@ import ( "github.com/open-policy-agent/opa/ast" "github.com/open-policy-agent/opa/rego" "github.com/open-policy-agent/opa/types" - "github.com/opencloud-eu/reva/v2/pkg/rhttp" ) // RFResourceDownload extends the rego dictionary with the possibility to download opencloud resources. // // Rego: `opencloud.resource.download("opencloud/path/0034892347349827")` // Result: bytes -var RFResourceDownload = rego.Function1( - ®o.Function{ - Name: "opencloud.resource.download", - Decl: types.NewFunction(types.Args(types.S), types.A), - Memoize: true, - Nondeterministic: true, - }, - func(_ rego.BuiltinContext, a *ast.Term) (*ast.Term, error) { - var url string +func RFResourceDownload(client *http.Client) func(*rego.Rego) { + return rego.Function1( + ®o.Function{ + Name: "opencloud.resource.download", + Decl: types.NewFunction(types.Args(types.S), types.A), + Memoize: true, + Nondeterministic: true, + }, + func(bctx rego.BuiltinContext, a *ast.Term) (*ast.Term, error) { + var url string - if err := ast.As(a.Value, &url); err != nil { - return nil, err - } + if err := ast.As(a.Value, &url); err != nil { + return nil, err + } - req, err := http.NewRequest(http.MethodGet, url, nil) - if err != nil { - return nil, err - } + req, err := http.NewRequestWithContext(bctx.Context, http.MethodGet, url, nil) + if err != nil { + return nil, err + } - client := rhttp.GetHTTPClient(rhttp.Insecure(true)) - res, err := client.Do(req) - if err != nil { - return nil, err - } - defer res.Body.Close() + res, err := client.Do(req) + if err != nil { + return nil, err + } + defer res.Body.Close() - if res.StatusCode != http.StatusOK { - return nil, fmt.Errorf("unexpected status code from Download %v", res.StatusCode) - } + if res.StatusCode != http.StatusOK { + return nil, fmt.Errorf("unexpected status code from Download %v", res.StatusCode) + } - buf := new(bytes.Buffer) - if _, err := buf.ReadFrom(res.Body); err != nil { - return nil, err - } + buf := new(bytes.Buffer) + if _, err := buf.ReadFrom(res.Body); err != nil { + return nil, err + } - v, err := ast.InterfaceToValue(buf.Bytes()) - if err != nil { - return nil, err - } + v, err := ast.InterfaceToValue(buf.Bytes()) + if err != nil { + return nil, err + } - return ast.NewTerm(v), nil - }, -) + return ast.NewTerm(v), nil + }, + ) +} diff --git a/services/policies/pkg/engine/opa/rf_resource_test.go b/services/policies/pkg/engine/opa/rf_resource_test.go index 237361aa9c..49b9ebedf7 100644 --- a/services/policies/pkg/engine/opa/rf_resource_test.go +++ b/services/policies/pkg/engine/opa/rf_resource_test.go @@ -5,11 +5,17 @@ import ( "encoding/base64" "net/http" "net/http/httptest" + "os" + "path/filepath" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/open-policy-agent/opa/rego" + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/services/policies/pkg/config" + "github.com/opencloud-eu/opencloud/services/policies/pkg/engine" "github.com/opencloud-eu/opencloud/services/policies/pkg/engine/opa" ) @@ -22,7 +28,7 @@ var _ = Describe("opa opencloud resource functions", func() { })) defer srv.Close() - r := rego.New(rego.Query(`opencloud.resource.download("`+srv.URL+`")`), opa.RFResourceDownload) + r := rego.New(rego.Query(`opencloud.resource.download("`+srv.URL+`")`), opa.RFResourceDownload(srv.Client())) rs, err := r.Eval(context.Background()) Expect(err).ToNot(HaveOccurred()) @@ -32,5 +38,49 @@ var _ = Describe("opa opencloud resource functions", func() { Expect(data).To(Equal(ts)) }) + + It("is cut by the engine timeout", func() { + const ( + timeout = 300 * time.Millisecond + holds = 3 * time.Second + ) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.(http.Flusher).Flush() + + select { + case <-time.After(holds): + case <-r.Context().Done(): + } + })) + defer srv.Close() + + path := filepath.Join(GinkgoT().TempDir(), "download.rego") + source := "package download\n\nimport future.keywords.if\n\ndefault granted := true\n\ngranted = false if {\n opencloud.resource.download(input.resource.url)\n}\n" + Expect(os.WriteFile(path, []byte(source), 0o600)).To(Succeed()) + + e, err := opa.NewOPA(timeout, log.NopLogger(), config.Engine{Policies: []string{path}}) + Expect(err).ToNot(HaveOccurred()) + + start := time.Now() + _, _ = e.Evaluate(context.Background(), "data.download.granted", engine.Environment{ + Resource: engine.Resource{URL: srv.URL}, + }) + Expect(time.Since(start)).To(BeNumerically("<", holds/2)) + }) + + It("stays undefined on a non ok response", func() { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + r := rego.New(rego.Query(`opencloud.resource.download("`+srv.URL+`")`), opa.RFResourceDownload(srv.Client())) + + rs, err := r.Eval(context.Background()) + Expect(err).ToNot(HaveOccurred()) + Expect(rs).To(BeEmpty()) + }) }) }) diff --git a/services/policies/pkg/engine/opa/testdata/bench/proxy.rego b/services/policies/pkg/engine/opa/testdata/bench/proxy.rego new file mode 100644 index 0000000000..20da7f0e86 --- /dev/null +++ b/services/policies/pkg/engine/opa/testdata/bench/proxy.rego @@ -0,0 +1,14 @@ +package proxy + +import future.keywords.if +import data.utils + +default granted := true + +granted = false if { + input.request.method == "PUT" + pathPrefixes := ["/dav", "/remote.php/webdav", "/remote.php/dav", "/webdav"] + restricted := pathPrefixes[_] + startswith(input.request.path, restricted) + not utils.is_extension_allowed(input.resource.name) +} diff --git a/services/policies/pkg/engine/opa/testdata/bench/utils.rego b/services/policies/pkg/engine/opa/testdata/bench/utils.rego new file mode 100644 index 0000000000..5cb16bf969 --- /dev/null +++ b/services/policies/pkg/engine/opa/testdata/bench/utils.rego @@ -0,0 +1,15 @@ +package utils + +ALLOWED_RESOURCE_EXTENSIONS := [ + ".apk", ".avi", ".bat", ".bmp", ".css", ".csv", ".doc", ".docm", ".docx", + ".docxf", ".dotx", ".eml", ".epub", ".htm", ".html", ".ipa", ".jar", ".java", + ".jpg", ".js", ".json", ".mp3", ".mp4", ".msg", ".odp", ".ods", ".odt", ".oform", + ".ots", ".ott", ".pdf", ".php", ".png", ".potm", ".potx", ".ppsm", ".ppsx", ".ppt", + ".pptm", ".pptx", ".py", ".rtf", ".sb3", ".sprite3", ".sql", ".svg", ".tif", ".tiff", + ".txt", ".xls", ".xlsm", ".xlsx", ".xltm", ".xltx", ".xml", ".zip", ".md" +] + +is_extension_allowed(identifier) { + extension := ALLOWED_RESOURCE_EXTENSIONS[_] + endswith(identifier, extension) +} diff --git a/services/policies/pkg/engine/opa/testdata/rules/broken.rego b/services/policies/pkg/engine/opa/testdata/rules/broken.rego new file mode 100644 index 0000000000..3f94210bea --- /dev/null +++ b/services/policies/pkg/engine/opa/testdata/rules/broken.rego @@ -0,0 +1,3 @@ +package rules + +this is not rego diff --git a/services/policies/pkg/engine/opa/testdata/rules/denied.rego b/services/policies/pkg/engine/opa/testdata/rules/denied.rego new file mode 100644 index 0000000000..cc7522e702 --- /dev/null +++ b/services/policies/pkg/engine/opa/testdata/rules/denied.rego @@ -0,0 +1,5 @@ +package rules + +import future.keywords.if + +default granted := false diff --git a/services/policies/pkg/engine/opa/testdata/rules/granted.rego b/services/policies/pkg/engine/opa/testdata/rules/granted.rego new file mode 100644 index 0000000000..ac7e48c276 --- /dev/null +++ b/services/policies/pkg/engine/opa/testdata/rules/granted.rego @@ -0,0 +1,5 @@ +package rules + +import future.keywords.if + +default granted := true diff --git a/services/proxy/pkg/middleware/policies.go b/services/proxy/pkg/middleware/policies.go index 963fc778e9..04aec89714 100644 --- a/services/proxy/pkg/middleware/policies.go +++ b/services/proxy/pkg/middleware/policies.go @@ -8,6 +8,7 @@ import ( "strings" "time" + rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/go-chi/chi/v5/middleware" "github.com/go-chi/render" @@ -113,6 +114,13 @@ func Policies(qs string, opts ...Option) func(next http.Handler) http.Handler { }, }) + // the name stays empty and the policy judges + if err != nil { + logger.Err(err).Msg("error stating the resource") + } else if code := sRes.GetStatus().GetCode(); code != rpc.Code_CODE_OK { + logger.Debug().Str("code", code.String()).Msg("unexpected status stating the resource") + } + resource.Name = sRes.GetInfo().GetName() } diff --git a/services/proxy/pkg/middleware/policies_test.go b/services/proxy/pkg/middleware/policies_test.go index 366c146b82..363d7ff9d1 100644 --- a/services/proxy/pkg/middleware/policies_test.go +++ b/services/proxy/pkg/middleware/policies_test.go @@ -11,17 +11,20 @@ import ( "testing" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" + provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" . "github.com/onsi/gomega" - pMessage "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/messages/policies/v0" - policiesPG "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/policies/v0" - "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/policies/v0/mocks" - "github.com/opencloud-eu/opencloud/services/proxy/pkg/middleware" - "github.com/opencloud-eu/opencloud/services/webdav/pkg/net" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" "github.com/stretchr/testify/mock" "go-micro.dev/v4/client" "google.golang.org/grpc" + + pMessage "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/messages/policies/v0" + policiesPG "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/policies/v0" + "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/policies/v0/mocks" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/middleware" + "github.com/opencloud-eu/opencloud/services/webdav/pkg/net" ) func TestPolicies_NoQuery_PassThrough(t *testing.T) { @@ -140,6 +143,73 @@ func TestPolicies_EvaluationEnvironment_Resource(t *testing.T) { } } +func TestPolicies_EvaluatesWithEmptyNameWhenStatFails(t *testing.T) { + const spaceRef = "/remote.php/dav/spaces/storage-id$space-id!opaque-id" + + for _, tc := range []struct { + name string + stat func(*cs3mocks.GatewayAPIClient) + }{ + { + name: "transport error", + stat: func(c *cs3mocks.GatewayAPIClient) { + c.On("Stat", mock.Anything, mock.Anything).Return(nil, errors.New("any")).Once() + }, + }, + { + name: "non ok status", + stat: func(c *cs3mocks.GatewayAPIClient) { + c.On("Stat", mock.Anything, mock.Anything).Return(&provider.StatResponse{ + Status: &rpc.Status{Code: rpc.Code_CODE_NOT_FOUND}, + }, nil).Once() + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + var g = NewWithT(t) + + policiesMiddleware, policiesProviderService, gatewayClient := prepare("any") + tc.stat(gatewayClient) + + policiesProviderService.On("Evaluate", mock.Anything, mock.Anything, mock.Anything).Return( + func(_ context.Context, in *policiesPG.EvaluateRequest, _ ...client.CallOption) (*policiesPG.EvaluateResponse, error) { + g.Expect(in.Environment.Resource.Name).To(BeEmpty()) + + return &policiesPG.EvaluateResponse{Result: false}, nil + }, + ).Once() + + responseRecorder := httptest.NewRecorder() + policiesMiddleware.ServeHTTP(responseRecorder, httptest.NewRequest(http.MethodPut, spaceRef, nil)) + + policiesProviderService.AssertCalled(t, "Evaluate", mock.Anything, mock.Anything, mock.Anything) + }) + } +} + +func TestPolicies_EvaluatesStattedName(t *testing.T) { + var g = NewWithT(t) + + policiesMiddleware, policiesProviderService, gatewayClient := prepare("any") + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(&provider.StatResponse{ + Status: &rpc.Status{Code: rpc.Code_CODE_OK}, + Info: &provider.ResourceInfo{Name: "statted-file-name.png"}, + }, nil).Once() + + policiesProviderService.On("Evaluate", mock.Anything, mock.Anything, mock.Anything).Return( + func(_ context.Context, in *policiesPG.EvaluateRequest, _ ...client.CallOption) (*policiesPG.EvaluateResponse, error) { + g.Expect(in.Environment.Resource.Name).To(Equal("statted-file-name.png")) + + return &policiesPG.EvaluateResponse{Result: true}, nil + }, + ).Once() + + responseRecorder := httptest.NewRecorder() + policiesMiddleware.ServeHTTP(responseRecorder, httptest.NewRequest(http.MethodPut, "/remote.php/dav/spaces/storage-id$space-id!opaque-id", nil)) + + g.Expect(responseRecorder.Code).To(Equal(http.StatusOK)) +} + func prepare(q string) (http.Handler, *mocks.PoliciesProviderService, *cs3mocks.GatewayAPIClient) { // mocked gatewaySelector