From 6a68d01ff583cf92ce2272fd522a80144458d286 Mon Sep 17 00:00:00 2001 From: James Alseth Date: Sun, 7 Jan 2024 14:20:29 -0800 Subject: [PATCH] feat: Raise error when problematic use of the if keyword is encountered Using the if keyword without also using the contains keyword makes the rule name in the OPA AST an emptry string, which causes conftest to inadvertently skip over tests leading to inaccurate results. https://github.com/open-policy-agent/opa/issues/6509 Signed-off-by: James Alseth --- .gitignore | 6 +- internal/testing/memfs/memfs.go | 75 ++++++++++++++++++++++++ policy/engine.go | 33 +++++++++-- policy/engine_test.go | 51 ++++++++++++++++ tests/problematic-if/data.yaml | 0 tests/problematic-if/policy/invalid.rego | 7 +++ tests/problematic-if/policy/valid.rego | 8 +++ tests/problematic-if/test.bats | 17 ++++++ 8 files changed, 192 insertions(+), 5 deletions(-) create mode 100644 internal/testing/memfs/memfs.go create mode 100644 tests/problematic-if/data.yaml create mode 100644 tests/problematic-if/policy/invalid.rego create mode 100644 tests/problematic-if/policy/valid.rego create mode 100644 tests/problematic-if/test.bats diff --git a/.gitignore b/.gitignore index 2c534d8803..a8da11a32a 100644 --- a/.gitignore +++ b/.gitignore @@ -6,5 +6,9 @@ site # IDEs .vscode .DS* - .idea + +# NodeJS files/dirs created when installing bats +node_modules/ +package.json +package-lock.json diff --git a/internal/testing/memfs/memfs.go b/internal/testing/memfs/memfs.go new file mode 100644 index 0000000000..dab0d2eb02 --- /dev/null +++ b/internal/testing/memfs/memfs.go @@ -0,0 +1,75 @@ +// Package memfs implements the fs.FS interface. +package memfs + +import ( + "fmt" + "io" + "io/fs" + "time" +) + +type FS struct { + files map[string][]byte +} + +func New(files map[string][]byte) *FS { + return &FS{files: files} +} + +func (fs *FS) Open(name string) (fs.File, error) { + data, ok := fs.files[name] + if !ok { + return nil, fmt.Errorf("file not found: %s", name) + } + return &File{name: name, data: data}, nil +} + +type File struct { + name string + data []byte +} + +func (f *File) Stat() (fs.FileInfo, error) { + return &FileInfo{ + name: f.name, + size: int64(len(f.data)), + }, nil +} + +func (f *File) Read(out []byte) (int, error) { + n := copy(out, f.data) + return n, io.EOF +} + +func (f *File) Close() error { + return nil +} + +type FileInfo struct { + name string + size int64 +} + +func (fi *FileInfo) Name() string { + return fi.name +} + +func (fi *FileInfo) Size() int64 { + return fi.size +} + +func (fi *FileInfo) Mode() fs.FileMode { + return fs.FileMode(0644) +} + +func (fi *FileInfo) ModTime() time.Time { + return time.Now() +} + +func (fi *FileInfo) IsDir() bool { + return false +} + +func (fi *FileInfo) Sys() any { + return nil +} diff --git a/policy/engine.go b/policy/engine.go index 47d18ecb5a..a19c216a29 100644 --- a/policy/engine.go +++ b/policy/engine.go @@ -39,6 +39,11 @@ type compilerOptions struct { capabilities *ast.Capabilities } +var ( + warningRegex = regexp.MustCompile("^warn(_[a-zA-Z0-9]+)*$") + failureRegex = regexp.MustCompile("^(deny|violation)(_[a-zA-Z0-9]+)*$") +) + func newCompilerOptions(strict bool, capabilities string) (compilerOptions, error) { c := ast.CapabilitiesForThisVersion() if capabilities != "" { @@ -81,6 +86,10 @@ func Load(policyPaths []string, c compilerOptions) (*Engine, error) { return nil, fmt.Errorf("get compiler: %w", compiler.Errors) } + if err := problematicIf(modules); err != nil { + return nil, fmt.Errorf("rule is using 'if' keyword without 'contains' keyword: %w", err) + } + policyContents := make(map[string]string, len(modules)) for path, module := range policies.Modules { path = filepath.Clean(path) @@ -90,7 +99,7 @@ func Load(policyPaths []string, c compilerOptions) (*Engine, error) { } engine := Engine{ - modules: policies.ParsedModules(), + modules: modules, compiler: compiler, policies: policyContents, } @@ -107,7 +116,6 @@ func LoadWithData(policyPaths []string, dataPaths []string, capabilities string, engine := &Engine{} if len(policyPaths) > 0 { - var err error engine, err = Load(policyPaths, compilerOptions) if err != nil { return nil, fmt.Errorf("loading policies: %w", err) @@ -519,15 +527,32 @@ func (e *Engine) query(ctx context.Context, input interface{}, query string) (ou } func isWarning(rule string) bool { - warningRegex := regexp.MustCompile("^warn(_[a-zA-Z0-9]+)*$") return warningRegex.MatchString(rule) } func isFailure(rule string) bool { - failureRegex := regexp.MustCompile("^(deny|violation)(_[a-zA-Z0-9]+)*$") return failureRegex.MatchString(rule) } +func problematicIf(modules map[string]*ast.Module) error { + // https://github.com/open-policy-agent/opa/issues/6509 + for _, module := range modules { + for _, rule := range module.Rules { + if rule.Head == nil || rule.Head.Value == nil || len(rule.Head.Reference) == 0 { + continue + } + refName := rule.Head.Reference[0].Value.String() + if isFailure(refName) || isWarning(refName) { + // Value being "true" here indicates usage of "if" without "contains". + if rule.Head.Value.String() == "true" { + return fmt.Errorf("rule in %s at line %d", module.Package.Loc().File, rule.Head.Location.Row) + } + } + } + } + return nil +} + func contains(collection []string, item string) bool { for _, value := range collection { if strings.EqualFold(value, item) { diff --git a/policy/engine_test.go b/policy/engine_test.go index 0c0e778f22..b98e00ef78 100644 --- a/policy/engine_test.go +++ b/policy/engine_test.go @@ -4,8 +4,10 @@ import ( "context" "testing" + "github.com/open-policy-agent/conftest/internal/testing/memfs" "github.com/open-policy-agent/conftest/parser" "github.com/open-policy-agent/opa/ast" + "github.com/open-policy-agent/opa/loader" ) func TestException(t *testing.T) { @@ -313,3 +315,52 @@ deny[{"msg": msg}] { }) } } + +func TestProblematicIf(t *testing.T) { + testCases := []struct { + desc string + body string + wantErr bool + }{ + { + desc: "No rules", + body: "", + }, + { + desc: "Rule not using if statement", + body: "deny[msg] {\n 1 == 1\nmsg := \"foo\"\n}\n", + }, + { + desc: "Unrelated rule using problematic if", + body: "import future.keywords.if\nunrelated[msg] if {\n 1 == 1\nmsg := \"foo\"\n}\n", + }, + { + desc: "Rule using if without contains", + body: "import future.keywords.if\ndeny[msg] if {\n 1 == 1\nmsg := \"foo\"\n}\n", + wantErr: true, + }, + { + desc: "Rule using if with contains", + body: "import future.keywords.if\nimport future.keywords.contains\ndeny contains msg if {\n 1 == 1\nmsg := \"foo\"\n}\n", + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + files := map[string][]byte{ + "policy.rego": []byte("package main\n\n" + tc.body), + } + fs := memfs.New(files) + l := loader.NewFileLoader().WithFS(fs) + + pols, err := l.All([]string{"policy.rego"}) + if err != nil { + t.Fatalf("Load policies: %v", err) + } + err = problematicIf(pols.ParsedModules()) + if gotErr := err != nil; gotErr != tc.wantErr { + t.Errorf("problematicIf = %v, want %v", gotErr, tc.wantErr) + } + }) + } +} diff --git a/tests/problematic-if/data.yaml b/tests/problematic-if/data.yaml new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/problematic-if/policy/invalid.rego b/tests/problematic-if/policy/invalid.rego new file mode 100644 index 0000000000..2d04e6d935 --- /dev/null +++ b/tests/problematic-if/policy/invalid.rego @@ -0,0 +1,7 @@ +package main + +import future.keywords.if + +deny[msg] if { + msg := "foo" +} diff --git a/tests/problematic-if/policy/valid.rego b/tests/problematic-if/policy/valid.rego new file mode 100644 index 0000000000..c36142cff3 --- /dev/null +++ b/tests/problematic-if/policy/valid.rego @@ -0,0 +1,8 @@ +package main + +import future.keywords.contains +import future.keywords.if + +deny contains msg if { + msg := "foo" +} diff --git a/tests/problematic-if/test.bats b/tests/problematic-if/test.bats new file mode 100644 index 0000000000..3d5923a41d --- /dev/null +++ b/tests/problematic-if/test.bats @@ -0,0 +1,17 @@ +#!/usr/bin/env bats + +@test "Test works as expected using contains and if" { + run $CONFTEST test --policy=policy/valid.rego data.yaml + + [ "$status" -eq 1 ] + echo $output + [[ "$output" =~ "1 test, 0 passed, 0 warnings, 1 failure, 0 exceptions" ]] +} + +@test "Error is raised when if is used without contains" { + run $CONFTEST test --policy=policy/invalid.rego data.yaml + + [ "$status" -eq 1 ] + echo $output + [[ "$output" =~ "'if' keyword without 'contains' keyword" ]] +}