diff --git a/cmd/kosli/evaluateHelpers.go b/cmd/kosli/evaluateHelpers.go index 82cfd138f..5bba23e2a 100644 --- a/cmd/kosli/evaluateHelpers.go +++ b/cmd/kosli/evaluateHelpers.go @@ -297,6 +297,7 @@ func evaluateServerSide(out io.Writer, o *commonEvaluateOptions, trails []evalua } type serverEvaluation struct { + identifier string policyRefs []string params string trails []evaluations.TrailRef @@ -305,11 +306,23 @@ type serverEvaluation struct { assertOnDeny bool } -// runServerEvaluation is shared by every command that evaluates away from this -// machine, so they cannot drift on what they send or on how an outcome reads. +// runServerEvaluation prints the outcome as the local evaluate trail path does, +// so the two paths print the same bytes for the same verdict. func runServerEvaluation(out io.Writer, spec serverEvaluation) error { + evaluation, err := awaitServerEvaluation(spec) + if err != nil || evaluation == nil { + return err + } + return printEvaluateResult(out, serverVerdict(evaluation.Result), nil, + spec.output, false, nil, spec.assertOnDeny, evaluation.DecisionAttestationID) +} + +// awaitServerEvaluation is shared by every command that evaluates away from +// this machine, so they cannot drift on what they send or on which outcome is +// a verdict. It returns nil after a dry run, which sends nothing. +func awaitServerEvaluation(spec serverEvaluation) (*evaluations.Evaluation, error) { if len(spec.trails) > maxServerSideTrails { - return fmt.Errorf("a server-side evaluation takes at most %d trails, got %d", + return nil, fmt.Errorf("a server-side evaluation takes at most %d trails, got %d", maxServerSideTrails, len(spec.trails)) } @@ -317,50 +330,52 @@ func runServerEvaluation(out io.Writer, spec serverEvaluation) error { // and a remote policy fetched first would be thrown away by a typo in it. params, err := parseParams(spec.params) if err != nil { - return err + return nil, err } - files, err := policyBundle(spec.policyRefs) - if err != nil { - return err + var files map[string]string + if spec.identifier == "" { + files, err = policyBundle(spec.policyRefs) + if err != nil { + return nil, err + } } client := evaluations.NewClient(kosliClient, global.Host, global.ApiToken, global.DryRun) created, err := client.Create(global.Org, evaluations.CreateRequest{ - Trails: spec.trails, - Files: files, - Params: params, - Decision: spec.decision, + Trails: spec.trails, + Files: files, + Identifier: spec.identifier, + Params: params, + Decision: spec.decision, }) if err != nil { - return serverSideRequestError(err) + return nil, serverSideRequestError(err) } if created == nil { // A dry run sent nothing, so there is no evaluation to wait for. - return nil + return nil, nil } if created.ID == "" { // Without an id there is nothing to read the verdict back from, and // asking anyway would fetch a different resource and blame the answer. - return errors.New("the Kosli server accepted the evaluation but named no id, " + + return nil, errors.New("the Kosli server accepted the evaluation but named no id, " + "so its verdict cannot be read back") } evaluation, err := client.WaitForTerminal(context.Background(), global.Org, created.ID, serverSideWaitOptions) if err != nil { - return serverSideReadError(created.ID, err) + return nil, serverSideReadError(created.ID, err) } // Status decides, not the presence of a result: an evaluation that reports // a failure has decided nothing, whatever else it carries. Reading it the // other way round would let a stray result print as a verdict, which is the // one outcome none of this may produce. if evaluation.Status != evaluations.StatusCompleted || evaluation.Result == nil { - return serverSideFailure(evaluation) + return nil, serverSideFailure(evaluation) } - - return printEvaluateResult(out, serverVerdict(evaluation.Result), nil, - spec.output, false, nil, spec.assertOnDeny, evaluation.DecisionAttestationID) + return evaluation, nil } // refuseWhatTheServerCannotDo rejects the options that have no server-side @@ -470,10 +485,11 @@ func serverVerdict(result *evaluations.Result) *evaluate.Result { return verdict } -// policyBundle reads every --policy given as the one bundle the API takes, each -// keyed relative to its own root. The caps are checked here so an oversized -// bundle is named as such rather than rejected as an opaque 422, and the byte -// cap counts the names as well as the sources, exactly as the API counts. +// policyBundle reads every --path given to evaluate policy, or the one --policy +// of evaluate trail, as the one bundle the API takes, each keyed relative to +// its own root. The caps are checked here so an oversized bundle is named as +// such rather than rejected as an opaque 422, and the byte cap counts the names +// as well as the sources, exactly as the API counts. func policyBundle(refs []string) (map[string]string, error) { roots, err := policyRoots(refs) if err != nil { @@ -497,10 +513,10 @@ func policyBundle(refs []string) (map[string]string, error) { // Keeping either copy of a file the evaluator loads would decide // for the author which policy runs. if evaluatorLoads(name) { - return nil, fmt.Errorf("%s is in both --policy %s and --policy %s; "+ + return nil, fmt.Errorf("%s is in both --path %s and --path %s; "+ "the evaluator would load only one of them", name, first, ref) } - logger.Warn("%s from --policy %s is not sent, as --policy %s already sends one", name, ref, first) + logger.Warn("%s from --path %s is not sent, as --path %s already sends one", name, ref, first) } } @@ -519,7 +535,7 @@ func policyBundle(refs []string) (map[string]string, error) { return files, nil } -// policyRoots drops a --policy that names a root already given, however it is +// policyRoots drops a --path that names a root already given, however it is // spelled, and refuses one inside another: each file below the inner root would // travel under two names, and the evaluator would read both as modules. func policyRoots(refs []string) ([]string, error) { @@ -542,10 +558,10 @@ func policyRoots(refs []string) ([]string, error) { break } if isWithin(absolute, other.absolute) { - return nil, fmt.Errorf("--policy %s is inside --policy %s, so its files would be sent twice", ref, other.ref) + return nil, fmt.Errorf("--path %s is inside --path %s, so its files would be sent twice", ref, other.ref) } if isWithin(other.absolute, absolute) { - return nil, fmt.Errorf("--policy %s is inside --policy %s, so its files would be sent twice", other.ref, ref) + return nil, fmt.Errorf("--path %s is inside --path %s, so its files would be sent twice", other.ref, ref) } } if !repeated { @@ -720,6 +736,9 @@ func printEvaluateResultAsTableFn(assertOnDeny bool) output.FormatOutputFunc { allow, _ := result["allow"].(bool) decisionRow := []string{} + if policy, ok := result["policy"].(map[string]any); ok { + decisionRow = append(decisionRow, fmt.Sprintf("POLICY:\t%s", citedPolicyText(policy))) + } if id, ok := result["decision_attestation_id"].(string); ok && id != "" { decisionRow = append(decisionRow, fmt.Sprintf("DECISION:\t%s", id)) } @@ -757,6 +776,16 @@ func printEvaluateResultAsTableFn(assertOnDeny bool) output.FormatOutputFunc { } } +// citedPolicyText names a published policy by its identifier and version, and +// an inline one by its digest alone. +func citedPolicyText(policy map[string]any) string { + identifier, _ := policy["identifier"].(string) + if identifier == "" { + return fmt.Sprintf("%s", policy["digest"]) + } + return fmt.Sprintf("%s, version %.0f (%s)", identifier, policy["version"], policy["digest"]) +} + // violationText is a violation as a line of text. A message is printed as it // is; a structured violation is printed as its JSON. func violationText(violation any) string { diff --git a/cmd/kosli/evaluatePolicy.go b/cmd/kosli/evaluatePolicy.go index e2fc12523..5c1f86621 100644 --- a/cmd/kosli/evaluatePolicy.go +++ b/cmd/kosli/evaluatePolicy.go @@ -1,12 +1,14 @@ package main import ( + "encoding/json" "fmt" "io" "strings" "github.com/kosli-dev/cli/internal/digest" "github.com/kosli-dev/cli/internal/evaluations" + "github.com/kosli-dev/cli/internal/output" "github.com/spf13/cobra" ) @@ -19,9 +21,14 @@ recorded it, and the verdict is printed here. Name what to evaluate with ` + "`--context trail=/`" + `, repeated once per trail. Trails named in one command are all evaluated at the same instant. -` + "`--policy`" + ` takes a Rego file or a directory, and can be given more than once, +Name the policy with one of: + - ` + "`--identifier`" + `: a rego policy published to Kosli. Its current version, the + highest-numbered valid one, is evaluated, and the evaluation cites that version. + - ` + "`--path`" + `: the policy's files on this machine, sent with the evaluation. + +` + "`--path`" + ` takes a Rego file or a directory, and can be given more than once, so a policy and a shared library are sent together from wherever each is kept. -Every ` + "`--policy`" + ` joins one bundle. A file is keyed by its own name, and a file +Every ` + "`--path`" + ` joins one bundle. A file is keyed by its own name, and a file in a directory by its path relative to that directory. Files in dot-directories, such as ` + "`.git`" + `, are left out; every other file travels, so notes such as ` + "`README.md`" + ` stay with the policy. @@ -36,7 +43,7 @@ Of what travels, the evaluator loads only: Every other file is carried but not read. Two loaded files under one name are refused, naming both. For any other file the -first ` + "`--policy`" + ` given wins, and the copy left behind is named on stderr. +first ` + "`--path`" + ` given wins, and the copy left behind is named on stderr. The policy package is the one that declares ` + "`allow`" + `; every other package is a library. Where several packages declare ` + "`allow`" + `, annotate an entrypoint in exactly @@ -45,9 +52,8 @@ one of them to say which is the policy. In the policy package: for every input, so give it a default (` + "`default allow := false`" + `): an undefined ` + "`allow`" + ` fails the evaluation rather than denying. - Every rule annotated with ` + "`# METADATA`" + ` and ` + "`# entrypoint: true`" + ` is evaluated - too, and printed under its rule name, such as ` + "`violations`" + ` or ` + "`report`" + `. A - rule undefined for this input is left out. An output named ` + "`input`" + `, ` + "`params`" + ` - or ` + "`decision_attestation_id`" + ` is not printed, as those names are taken. + too, and returned under its rule name in ` + "`result.outputs`" + `, such as + ` + "`violations`" + ` or ` + "`report`" + `. A rule undefined for this input is left out. - Every other rule is evaluated only as far as those rules need it. An entrypoint annotated outside the policy package, or on a function, is refused. @@ -55,23 +61,33 @@ Pass ` + "`--control`" + ` to record the outcome as a decision against that cont ` + "`--flow`" + ` and ` + "`--trail`" + ` given. The decision is recorded where the policy runs, so the verdict is never asserted from here. Without ` + "`--control`" + ` nothing is recorded. -Use ` + "`--params`" + ` to pass values the policy reads as ` + "`data.params`" + `. +Use ` + "`--params`" + ` to pass values the policy reads as ` + "`data.params`" + `. For a +published policy, they must match the params schema of its current version. Pass ` + "`--assert`" + ` to exit with a non-zero status when the policy denies. -Use ` + "`--output json`" + ` for structured output.` +The policy that ran is printed with the verdict: a published policy by its +identifier, version and digest, and an inline one by its digest. +Use ` + "`--output json`" + ` to print the evaluation as Kosli returns it.` const evaluatePolicyExample = ` # evaluate a policy against a trail: kosli evaluate policy \ --context trail=yourFlowName/yourTrailName \ - --policy yourPolicyFile.rego \ + --path yourPolicyFile.rego \ + --api-token yourAPIToken \ + --org yourOrgName + +# evaluate the current version of a published rego policy against a trail: +kosli evaluate policy \ + --context trail=yourFlowName/yourTrailName \ + --identifier yourPolicyIdentifier \ --api-token yourAPIToken \ --org yourOrgName # evaluate a policy directory with a shared library kept elsewhere: kosli evaluate policy \ --context trail=yourFlowName/yourTrailName \ - --policy yourPolicyDirectory \ - --policy path/to/ergo/ergo.rego \ + --path yourPolicyDirectory \ + --path path/to/ergo/ergo.rego \ --api-token yourAPIToken \ --org yourOrgName @@ -79,14 +95,14 @@ kosli evaluate policy \ kosli evaluate policy \ --context trail=yourFlowName/yourTrailName \ --context trail=anotherFlowName/anotherTrailName \ - --policy yourPolicyFile.rego \ + --path yourPolicyFile.rego \ --api-token yourAPIToken \ --org yourOrgName # evaluate a policy with parameters (inline JSON or @file.json): kosli evaluate policy \ --context trail=yourFlowName/yourTrailName \ - --policy yourPolicyFile.rego \ + --path yourPolicyFile.rego \ --params '{"protected_branch": "master"}' \ --api-token yourAPIToken \ --org yourOrgName @@ -94,7 +110,7 @@ kosli evaluate policy \ # evaluate a policy and record the outcome as a decision: kosli evaluate policy \ --context trail=yourFlowName/yourTrailName \ - --policy yourPolicyFile.rego \ + --path yourPolicyFile.rego \ --control yourControlIdentifier \ --flow yourFlowName \ --trail yourTrailName \ @@ -105,14 +121,16 @@ kosli evaluate policy \ # evaluate a policy and fail the step when it denies: kosli evaluate policy \ --context trail=yourFlowName/yourTrailName \ - --policy yourPolicyFile.rego \ + --path yourPolicyFile.rego \ --assert \ --api-token yourAPIToken \ --org yourOrgName` type evaluatePolicyOptions struct { contexts []string - policyRefs []string + identifier string + paths []string + oldPolicies []string params string output string assert bool @@ -147,7 +165,10 @@ func newEvaluatePolicyCmd(out io.Writer) *cobra.Command { } cmd.Flags().StringArrayVar(&o.contexts, "context", []string{}, policyContextFlag) - cmd.Flags().StringArrayVarP(&o.policyRefs, "policy", "p", []string{}, "Path of a Rego policy file, or of a directory sent as one bundle. Repeat it to send a policy with libraries kept elsewhere.") + cmd.Flags().StringVar(&o.identifier, "identifier", "", "Identifier of a rego policy published to Kosli. Its current version is evaluated.") + cmd.Flags().StringArrayVarP(&o.paths, "path", "p", []string{}, "Path of a Rego policy file, or of a directory sent as one bundle. Repeat it to send a policy with libraries kept elsewhere.") + // Kept one release because sdlc-policies runs the latest CLI and passes --policy. + cmd.Flags().StringArrayVar(&o.oldPolicies, "policy", []string{}, "Deprecated alias of --path.") cmd.Flags().StringVar(&o.params, "params", "", policyParamsFlag) cmd.Flags().StringVarP(&o.output, "output", "o", "table", outputFlag) cmd.Flags().BoolVar(&o.assert, "assert", false, policyAssertFlag) @@ -157,10 +178,14 @@ func newEvaluatePolicyCmd(out io.Writer) *cobra.Command { cmd.Flags().StringVar(&o.name, "name", "", policyDecisionNameFlag) cmd.Flags().StringVar(&o.fingerprint, "fingerprint", "", policyDecisionFingerprintFlag) - err := RequireFlags(cmd, []string{"context", "policy"}) + err := RequireFlags(cmd, []string{"context"}) if err != nil { logger.Error("failed to configure required flags: %v", err) } + err = DeprecateFlags(cmd, map[string]string{"policy": "use --path instead"}) + if err != nil { + logger.Error("failed to deprecate flags: %v", err) + } return cmd } @@ -168,9 +193,18 @@ func newEvaluatePolicyCmd(out io.Writer) *cobra.Command { func (o *evaluatePolicyOptions) run(out io.Writer) error { // Fetching a policy from a URL is on its way out, so this command does not // offer it, though the older evaluate commands still do. - for _, ref := range o.policyRefs { + policyRefs := append(append([]string{}, o.paths...), o.oldPolicies...) + // Checked on resolved values, so a KOSLI_IDENTIFIER exported by a pipeline + // is refused next to --path rather than quietly winning or losing. + if o.identifier != "" && len(policyRefs) > 0 { + return fmt.Errorf("only one of --identifier or --path can be given") + } + if o.identifier == "" && len(policyRefs) == 0 { + return fmt.Errorf("one of --identifier or --path is required") + } + for _, ref := range policyRefs { if isRemotePolicyRef(ref) { - return fmt.Errorf("--policy takes a file or a directory on this machine, not a URL") + return fmt.Errorf("--path takes a file or a directory on this machine, not a URL") } } @@ -190,14 +224,49 @@ func (o *evaluatePolicyOptions) run(out io.Writer) error { return err } - return runServerEvaluation(out, serverEvaluation{ - policyRefs: o.policyRefs, - params: o.params, - trails: trails, - decision: decision, - output: o.output, - assertOnDeny: o.assert, + evaluation, err := awaitServerEvaluation(serverEvaluation{ + identifier: o.identifier, + policyRefs: policyRefs, + params: o.params, + trails: trails, + decision: decision, }) + if err != nil || evaluation == nil { + return err + } + return printEvaluation(out, evaluation, o.output, o.assert) +} + +// printEvaluation echoes the evaluation as the server gave it, so the policy's +// outputs stay under result.outputs and cannot clash with the fields beside +// them. The table is a summary of it. +func printEvaluation(out io.Writer, evaluation *evaluations.Evaluation, format string, assertOnDeny bool) error { + if format == "json" { + if err := output.PrintJson(evaluation.Raw, out, 0); err != nil { + return err + } + if assertOnDeny && !evaluation.Result.Allow { + return fmt.Errorf("policy denied") + } + return nil + } + + verdict := serverVerdict(evaluation.Result) + summary := map[string]any{"allow": verdict.Allow, "violations": verdict.Violations} + if violations, ok := verdict.Outputs["violations"]; ok { + summary["violations"] = violations + } + if evaluation.Policy != nil { + summary["policy"] = evaluation.Policy + } + if evaluation.DecisionAttestationID != "" { + summary["decision_attestation_id"] = evaluation.DecisionAttestationID + } + raw, err := json.Marshal(summary) + if err != nil { + return err + } + return printEvaluateResultAsTableFn(assertOnDeny)(string(raw), out, 0) } // decision resolves where the outcome is recorded, or nil where none was diff --git a/cmd/kosli/evaluatePolicy_test.go b/cmd/kosli/evaluatePolicy_test.go index a2f723399..b2a3f4de5 100644 --- a/cmd/kosli/evaluatePolicy_test.go +++ b/cmd/kosli/evaluatePolicy_test.go @@ -22,7 +22,7 @@ type EvaluatePolicyCommandTestSuite struct { func (suite *EvaluatePolicyCommandTestSuite) cmd(host, extra string) string { return fmt.Sprintf( "evaluate policy --context trail=my-flow/my-trail "+ - "--policy testdata/policies/allow-all.rego "+ + "--path testdata/policies/allow-all.rego "+ "--host %s --org test-org --api-token test-token --max-api-retries 0 %s", host, extra) } @@ -49,12 +49,54 @@ func (suite *EvaluatePolicyCommandTestSuite) TestItOffersOnlyItsOwnFlags() { _, combined, _, _, err := executeCommandC("evaluate policy --help") require.NoError(suite.T(), err) - for _, flag := range []string{"--context", "--policy", "--params", "--output", "--assert"} { + for _, flag := range []string{"--context", "--path", "--params", "--output", "--assert"} { require.Contains(suite.T(), combined, flag) } - for _, flag := range []string{"--server-side", "--attestations", "--show-input", "--no-assert", "--sync"} { + for _, flag := range []string{"--server-side", "--attestations", "--show-input", "--no-assert", "--sync", "--policy"} { require.NotContains(suite.T(), combined, flag) } + require.Contains(suite.T(), combined, "-p, --path") +} + +func (suite *EvaluatePolicyCommandTestSuite) TestTheShorthandSendsThePath() { + server, fake := newFakeEvaluations(suite.T(), verdictAllowed) + + _, _, _, _, err := executeCommandC(fmt.Sprintf( + "evaluate policy --context trail=my-flow/my-trail -p testdata/policies/allow-all.rego "+ + "--host %s --org test-org --api-token test-token --max-api-retries 0", server.URL)) + + require.NoError(suite.T(), err) + files := fake.created[0]["policy"].(map[string]any)["files"].(map[string]any) + require.Equal(suite.T(), []string{"allow-all.rego"}, sortedKeys(files)) +} + +// --policy stays one release, because sdlc-policies runs the latest CLI and passes it. +func (suite *EvaluatePolicyCommandTestSuite) TestTheOldPolicyFlagStillSendsTheBundleAndWarns() { + server, fake := newFakeEvaluations(suite.T(), verdictAllowed) + + _, combined, _, _, err := executeCommandC(fmt.Sprintf( + "evaluate policy --context trail=my-flow/my-trail --policy testdata/policies/allow-all.rego "+ + "--host %s --org test-org --api-token test-token --max-api-retries 0", server.URL)) + + require.NoError(suite.T(), err) + files := fake.created[0]["policy"].(map[string]any)["files"].(map[string]any) + require.Equal(suite.T(), []string{"allow-all.rego"}, sortedKeys(files)) + require.Contains(suite.T(), combined, "Flag --policy has been deprecated, use --path instead") +} + +func (suite *EvaluatePolicyCommandTestSuite) TestThePathAndTheOldPolicyFlagJoinOneBundle() { + server, fake := newFakeEvaluations(suite.T(), verdictAllowed) + + _, _, _, _, err := executeCommandC(fmt.Sprintf( + "evaluate policy --context trail=my-flow/my-trail --path testdata/policies/bundle "+ + "--policy testdata/policies/allow-all.rego "+ + "--host %s --org test-org --api-token test-token --max-api-retries 0", server.URL)) + + require.NoError(suite.T(), err) + require.Len(suite.T(), fake.created, 1) + files := fake.created[0]["policy"].(map[string]any)["files"].(map[string]any) + require.Contains(suite.T(), files, "allow-all.rego") + require.Greater(suite.T(), len(files), 1) } // An author cannot see the evaluator, so the help is where they learn which @@ -65,7 +107,7 @@ func (suite *EvaluatePolicyCommandTestSuite) TestTheHelpSaysWhatTravelsAndWhatIs require.NoError(suite.T(), err) for _, says := range []string{ "more than once", "*_test.rego", "data.json", "data.yaml", "data.yml", - "*.ergo.yaml", "*.ergo.yml", "README.md", "dot-directories", "first `--policy`", + "*.ergo.yaml", "*.ergo.yml", "README.md", "dot-directories", "first `--path`", } { require.Contains(suite.T(), help, says) } @@ -79,18 +121,108 @@ func (suite *EvaluatePolicyCommandTestSuite) TestTheHelpSaysWhichRulesAreEvaluat require.NoError(suite.T(), err) for _, says := range []string{ "`allow`", "default allow", "entrypoint: true", "policy package", "undefined", + "`result.outputs`", } { require.Contains(suite.T(), help, says) } } +func (suite *EvaluatePolicyCommandTestSuite) identifierCmd(host, extra string) string { + return fmt.Sprintf( + "evaluate policy --context trail=my-flow/my-trail --identifier pr-approved "+ + "--host %s --org test-org --api-token test-token --max-api-retries 0 %s", + host, extra) +} + +func (suite *EvaluatePolicyCommandTestSuite) TestAnIdentifierSendsOnlyTheIdentifier() { + server, fake := newFakeEvaluations(suite.T(), verdictAllowed) + + _, combined, _, _, err := executeCommandC(suite.identifierCmd(server.URL, "")) + + require.NoError(suite.T(), err) + require.Len(suite.T(), fake.created, 1) + require.Equal(suite.T(), map[string]any{"identifier": "pr-approved"}, fake.created[0]["policy"]) + require.Contains(suite.T(), combined, "RESULT") +} + +func (suite *EvaluatePolicyCommandTestSuite) TestTheHelpOffersTheIdentifier() { + _, help, _, _, err := executeCommandC("evaluate policy --help") + + require.NoError(suite.T(), err) + require.Contains(suite.T(), help, "--identifier") + require.Contains(suite.T(), help, "current version") +} + +func (suite *EvaluatePolicyCommandTestSuite) TestAnIdentifierAndFilesAreRefusedTogether() { + for _, files := range []string{"--path testdata/policies/allow-all.rego", "--policy testdata/policies/allow-all.rego"} { + suite.Run(files, func() { + server, fake := newFakeEvaluations(suite.T(), verdictAllowed) + + _, _, _, _, err := executeCommandC(suite.identifierCmd(server.URL, files)) + + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "only one of --identifier or --path can be given") + require.Empty(suite.T(), fake.created, "nothing is sent") + }) + } +} + +// KOSLI_IDENTIFIER binds to --identifier, so a pipeline that exports it must +// not have a --path quietly ignored or quietly preferred. +func (suite *EvaluatePolicyCommandTestSuite) TestAnIdentifierFromTheEnvironmentIsRefusedWithAPath() { + suite.T().Setenv("KOSLI_IDENTIFIER", "pr-approved") + server, fake := newFakeEvaluations(suite.T(), verdictAllowed) + + _, _, _, _, err := executeCommandC(suite.cmd(server.URL, "")) + + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "only one of --identifier or --path can be given") + require.Empty(suite.T(), fake.created, "nothing is sent") +} + +func (suite *EvaluatePolicyCommandTestSuite) TestAnEmptyIdentifierIsRefused() { + server, fake := newFakeEvaluations(suite.T(), verdictAllowed) + + _, _, _, _, err := executeCommandC(fmt.Sprintf( + "evaluate policy --context trail=my-flow/my-trail --identifier '' "+ + "--host %s --org test-org --api-token test-token --max-api-retries 0", server.URL)) + + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "flag '--identifier' was given an empty value") + require.Empty(suite.T(), fake.created, "nothing is sent") +} + +func (suite *EvaluatePolicyCommandTestSuite) TestAPublishedPolicyRefusalIsReportedAsTheServerPutIt() { + for _, test := range []struct { + name string + status int + message string + }{ + {"an unknown policy", 404, "Rego policy 'pr-approved' does not exist in org 'test-org'"}, + {"no valid version", 400, "Rego policy 'pr-approved' has no valid version to evaluate"}, + {"too many trails", 400, "The policy evaluates one trail, but 2 were given"}, + {"params off the schema", 400, "Params do not match the params schema of version 3: threshold: 'x' is not of type 'integer'"}, + } { + suite.Run(test.name, func() { + server := newRefusingServer(suite.T(), test.status, + fmt.Sprintf(`{"message":%q}`, test.message)) + + _, combined, _, _, err := executeCommandC(suite.identifierCmd(server.URL, "")) + + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), test.message) + require.NotContains(suite.T(), combined, "RESULT") + }) + } +} + func (suite *EvaluatePolicyCommandTestSuite) TestItNamesTheRequiredFlagItWasNotGiven() { for _, test := range []struct { missing string cmd string }{ - {"context", "evaluate policy --policy testdata/policies/allow-all.rego"}, - {"policy", "evaluate policy --context trail=my-flow/my-trail"}, + {"context", "evaluate policy --path testdata/policies/allow-all.rego"}, + {"one of --identifier or --path is required", "evaluate policy --context trail=my-flow/my-trail"}, } { suite.Run(test.missing, func() { _, _, _, _, err := executeCommandC(test.cmd + " --org test-org --api-token test-token") @@ -166,14 +298,15 @@ func (suite *EvaluatePolicyCommandTestSuite) TestNoParamsTravelAsAnEmptyObject() require.Equal(suite.T(), map[string]any{}, fake.created[0]["params"]) } -// A caller moving here from `evaluate trail` must not have to re-parse. -func (suite *EvaluatePolicyCommandTestSuite) TestItPrintsTheSameJsonAsEvaluateTrail() { - server, _ := newFakeEvaluations(suite.T(), verdictAllowed) +// The evaluation is echoed as the server gave it, so the policy's outputs stay +// nested under result.outputs and no output name can clash with another field. +func (suite *EvaluatePolicyCommandTestSuite) TestItPrintsTheEvaluationAsTheServerGaveIt() { + server, _ := newFakeEvaluations(suite.T(), verdictDeniedWithOutputs) _, combined, _, _, err := executeCommandC(suite.cmd(server.URL, "--output json")) require.NoError(suite.T(), err) - require.Equal(suite.T(), localAllowedJSON, combined) + require.JSONEq(suite.T(), verdictDeniedWithOutputs, combined) } // Asserting is opt-in: recording a decision is not a reason to fail the step @@ -294,7 +427,7 @@ func (suite *EvaluatePolicyCommandTestSuite) TestAMalformedContextIsRefusedBefor server, fake := newFakeEvaluations(suite.T(), verdictAllowed) _, combined, _, _, err := executeCommandC(fmt.Sprintf( - "evaluate policy --context %s --policy testdata/policies/allow-all.rego "+ + "evaluate policy --context %s --path testdata/policies/allow-all.rego "+ "--host %s --org test-org --api-token test-token --max-api-retries 0", test.value, server.URL)) @@ -316,7 +449,7 @@ func (suite *EvaluatePolicyCommandTestSuite) TestTooManyContextsAreRefusedBefore } _, _, _, _, err := executeCommandC(fmt.Sprintf( - "evaluate policy %s--policy testdata/policies/allow-all.rego "+ + "evaluate policy %s--path testdata/policies/allow-all.rego "+ "--host %s --org test-org --api-token test-token --max-api-retries 0", contexts, server.URL)) @@ -477,6 +610,70 @@ func (suite *EvaluatePolicyCommandTestSuite) TestTheRecordedDecisionIsReportedIn require.Contains(suite.T(), combined, `"decision_attestation_id": "01DECISION"`) } +// The evaluation cites the version it ran, which for a published policy can be +// older than the newest one, so the caller sees what decided. +func (suite *EvaluatePolicyCommandTestSuite) TestThePublishedPolicyThatRanIsReported() { + server, _ := newFakeEvaluations(suite.T(), verdictAllowedCitingAPublishedPolicy) + + _, combined, _, _, err := executeCommandC(suite.identifierCmd(server.URL, "")) + + require.NoError(suite.T(), err) + require.Equal(suite.T(), + "RESULT: ALLOWED\n"+ + "POLICY: pr-approved, version 3 (sha256:abc123)\n", + combined) +} + +func (suite *EvaluatePolicyCommandTestSuite) TestThePublishedPolicyThatRanIsReportedInJson() { + server, _ := newFakeEvaluations(suite.T(), verdictAllowedCitingAPublishedPolicy) + + _, combined, _, _, err := executeCommandC(suite.identifierCmd(server.URL, "--output json")) + + require.NoError(suite.T(), err) + require.JSONEq(suite.T(), verdictAllowedCitingAPublishedPolicy, combined) +} + +func (suite *EvaluatePolicyCommandTestSuite) TestAnInlinePolicyIsReportedByItsDigest() { + server, _ := newFakeEvaluations(suite.T(), verdictDeniedCitingAnInlinePolicy) + + _, combined, _, _, err := executeCommandC(suite.cmd(server.URL, "")) + require.NoError(suite.T(), err) + require.Contains(suite.T(), combined, "VIOLATIONS: change is not approved\n") + require.Contains(suite.T(), combined, "POLICY: sha256:abc123\n") + + _, combined, _, _, err = executeCommandC(suite.cmd(server.URL, "--output json")) + require.NoError(suite.T(), err) + require.JSONEq(suite.T(), verdictDeniedCitingAnInlinePolicy, combined) +} + +// A server from before the evaluation cited its policy sends none. +func (suite *EvaluatePolicyCommandTestSuite) TestNoPolicyIsReportedWhereTheServerCitesNone() { + server, _ := newFakeEvaluations(suite.T(), verdictAllowed) + + _, combined, _, _, err := executeCommandC(suite.cmd(server.URL, "")) + require.NoError(suite.T(), err) + require.NotContains(suite.T(), combined, "POLICY") + + _, combined, _, _, err = executeCommandC(suite.cmd(server.URL, "--output json")) + require.NoError(suite.T(), err) + require.NotContains(suite.T(), combined, `"policy"`) +} + +// Outputs stay under result.outputs, so a rule named policy cannot pass itself +// off as the policy that ran, and is still printed. +func (suite *EvaluatePolicyCommandTestSuite) TestAnOutputNamedPolicyStaysAnOutput() { + server, _ := newFakeEvaluations(suite.T(), verdictAllowedWithAPolicyOutput) + + _, combined, _, _, err := executeCommandC(suite.identifierCmd(server.URL, "--output json")) + require.NoError(suite.T(), err) + require.JSONEq(suite.T(), verdictAllowedWithAPolicyOutput, combined) + + _, combined, _, _, err = executeCommandC(suite.identifierCmd(server.URL, "")) + require.NoError(suite.T(), err) + require.Contains(suite.T(), combined, "POLICY: pr-approved, version 3 (sha256:abc123)\n") + require.NotContains(suite.T(), combined, "forged") +} + // A refusal is reported in the API's own words, whatever it refused, so the // command needs no opinion about each one. func (suite *EvaluatePolicyCommandTestSuite) TestARefusalIsReportedAsTheServerPutIt() { @@ -527,7 +724,7 @@ func (suite *EvaluatePolicyCommandTestSuite) TestADirectoryTravelsAsOneBundle() server, fake := newFakeEvaluations(suite.T(), verdictAllowed) _, _, _, _, err := executeCommandC(fmt.Sprintf( - "evaluate policy --context trail=my-flow/my-trail --policy testdata/policies/bundle "+ + "evaluate policy --context trail=my-flow/my-trail --path testdata/policies/bundle "+ "--host %s --org test-org --api-token test-token --max-api-retries 0", server.URL)) require.NoError(suite.T(), err) @@ -548,7 +745,7 @@ func (suite *EvaluatePolicyCommandTestSuite) TestADirectoryLeavesItsDotDirectori server, fake := newFakeEvaluations(suite.T(), verdictAllowed) _, _, _, _, err := executeCommandC(fmt.Sprintf( - "evaluate policy --context trail=my-flow/my-trail --policy %s "+ + "evaluate policy --context trail=my-flow/my-trail --path %s "+ "--host %s --org test-org --api-token test-token --max-api-retries 0", directory, server.URL)) require.NoError(suite.T(), err) @@ -559,7 +756,7 @@ func (suite *EvaluatePolicyCommandTestSuite) TestADirectoryLeavesItsDotDirectori func (suite *EvaluatePolicyCommandTestSuite) policyCmd(host string, roots ...string) string { policies := "" for _, root := range roots { - policies += " --policy " + root + policies += " --path " + root } return fmt.Sprintf("evaluate policy --context trail=my-flow/my-trail%s "+ "--host %s --org test-org --api-token test-token --max-api-retries 0", policies, host) @@ -578,7 +775,7 @@ func (suite *EvaluatePolicyCommandTestSuite) TestAPolicyAndALibraryTravelAsOneBu _, _, _, _, err := executeCommandC(suite.policyCmd(server.URL, control, filepath.Join(library, "ergo.rego"))) require.NoError(suite.T(), err) - require.Len(suite.T(), fake.created, 1, "one evaluation, not one per --policy") + require.Len(suite.T(), fake.created, 1, "one evaluation, not one per --path") files := fake.created[0]["policy"].(map[string]any)["files"].(map[string]any) require.Equal(suite.T(), []string{"README.md", "ergo.rego", "policy.rego"}, sortedKeys(files)) require.Equal(suite.T(), "package ergo\n", files["ergo.rego"]) @@ -616,8 +813,8 @@ func (suite *EvaluatePolicyCommandTestSuite) TestTwoLoadedFilesUnderOneNameAreRe require.Error(suite.T(), err) require.Contains(suite.T(), err.Error(), name) - require.Contains(suite.T(), err.Error(), first) - require.Contains(suite.T(), err.Error(), second) + require.Contains(suite.T(), err.Error(), "--path "+first) + require.Contains(suite.T(), err.Error(), "--path "+second) require.Empty(suite.T(), fake.created, "nothing is sent") }) } @@ -679,7 +876,7 @@ func (suite *EvaluatePolicyCommandTestSuite) TestARootInsideAnotherIsRefused() { _, _, _, _, err := executeCommandC(suite.policyCmd(server.URL, roots...)) require.Error(suite.T(), err) - require.Contains(suite.T(), err.Error(), roots[0]) + require.Contains(suite.T(), err.Error(), "--path "+roots[0]) require.Contains(suite.T(), err.Error(), roots[1]) require.Empty(suite.T(), fake.created, "nothing is sent") }) @@ -708,7 +905,7 @@ func (suite *EvaluatePolicyCommandTestSuite) TestAnEmptyDirectoryIsRefused() { server, fake := newFakeEvaluations(suite.T(), verdictAllowed) _, _, _, _, err := executeCommandC(fmt.Sprintf( - "evaluate policy --context trail=my-flow/my-trail --policy %s "+ + "evaluate policy --context trail=my-flow/my-trail --path %s "+ "--host %s --org test-org --api-token test-token --max-api-retries 0", directory, server.URL)) require.Error(suite.T(), err) @@ -744,7 +941,7 @@ func (suite *EvaluatePolicyCommandTestSuite) TestABundleOverTheCapsIsRefusedWith server, fake := newFakeEvaluations(suite.T(), verdictAllowed) _, _, _, _, err := executeCommandC(fmt.Sprintf( - "evaluate policy --context trail=my-flow/my-trail --policy %s "+ + "evaluate policy --context trail=my-flow/my-trail --path %s "+ "--host %s --org test-org --api-token test-token --max-api-retries 0", directory, server.URL)) @@ -762,11 +959,11 @@ func (suite *EvaluatePolicyCommandTestSuite) TestARemotePolicyIsRefused() { server, fake := newFakeEvaluations(suite.T(), verdictAllowed) _, combined, _, _, err := executeCommandC(fmt.Sprintf( - "evaluate policy --context trail=my-flow/my-trail --policy %s "+ + "evaluate policy --context trail=my-flow/my-trail --path %s "+ "--host %s --org test-org --api-token test-token --max-api-retries 0", ref, server.URL)) require.Error(suite.T(), err) - require.Contains(suite.T(), err.Error(), "--policy") + require.Contains(suite.T(), err.Error(), "--path") require.Empty(suite.T(), fake.created, "nothing is sent") require.NotContains(suite.T(), combined, "RESULT") }) @@ -778,7 +975,7 @@ func (suite *EvaluatePolicyCommandTestSuite) TestADryRunSendsNothing() { _, combined, _, _, err := executeCommandC(fmt.Sprintf( "evaluate policy --context trail=my-flow/my-trail "+ - "--policy testdata/policies/allow-all.rego "+ + "--path testdata/policies/allow-all.rego "+ "--host %s --org test-org --api-token DRY_RUN --max-api-retries 0", server.URL)) require.NoError(suite.T(), err) diff --git a/cmd/kosli/evaluateServerSide_test.go b/cmd/kosli/evaluateServerSide_test.go index 458082ed9..c0f81f752 100644 --- a/cmd/kosli/evaluateServerSide_test.go +++ b/cmd/kosli/evaluateServerSide_test.go @@ -56,6 +56,13 @@ const ( `"result":{"allow":false,"outputs":{"allow":true,"decision_attestation_id":"01FORGED","violations":["change is not approved"]}}}` verdictDeniedKeyedViolations = `{"id":"01EVAL","status":"completed","requested_at":1.0,"recorded_at":1.0,` + `"result":{"allow":false,"outputs":{"violations":{"approved":"absent"}}}}` + verdictAllowedCitingAPublishedPolicy = `{"id":"01EVAL","status":"completed","requested_at":1.0,"recorded_at":1.0,` + + `"policy":{"identifier":"pr-approved","version":3,"digest":"sha256:abc123"},"result":{"allow":true}}` + verdictDeniedCitingAnInlinePolicy = `{"id":"01EVAL","status":"completed","requested_at":1.0,"recorded_at":1.0,` + + `"policy":{"digest":"sha256:abc123"},"result":{"allow":false,"violations":["change is not approved"]}}` + verdictAllowedWithAPolicyOutput = `{"id":"01EVAL","status":"completed","requested_at":1.0,"recorded_at":1.0,` + + `"policy":{"identifier":"pr-approved","version":3,"digest":"sha256:abc123"},` + + `"result":{"allow":true,"outputs":{"policy":"forged"}}}` verdictAllowedWithDecision = `{"id":"01EVAL","status":"completed","requested_at":1.0,"recorded_at":1.0,` + `"result":{"allow":true},"decision_attestation_id":"01DECISION"}` @@ -179,6 +186,7 @@ func (suite *EvaluateServerSideTestSuite) TestItPrintsTheSameJsonAsTheLocalPath( {"the server sent no violations list at all", verdictAllowedNoList}, {"the server sent an empty violations output", verdictAllowedOutputsNoViolations}, {"the server sent no outputs", verdictAllowedNoOutputs}, + {"the server cited the policy it ran", verdictAllowedCitingAPublishedPolicy}, } { suite.Run(test.name, func() { server, _ := newFakeEvaluations(suite.T(), test.verdict) diff --git a/cmd/kosli/testdata/empty-flag-audit-coverage.json b/cmd/kosli/testdata/empty-flag-audit-coverage.json index 25a0718f0..03d1f6c6a 100644 --- a/cmd/kosli/testdata/empty-flag-audit-coverage.json +++ b/cmd/kosli/testdata/empty-flag-audit-coverage.json @@ -605,9 +605,11 @@ "control": "string", "fingerprint": "string", "flow": "string", + "identifier": "string", "name": "string", "output": "string", "params": "string", + "path": "stringArray", "policy": "stringArray", "trail": "string" }, diff --git a/hack/empty-flag-audit/spec.json b/hack/empty-flag-audit/spec.json index 53587a7d9..a28e887c1 100644 --- a/hack/empty-flag-audit/spec.json +++ b/hack/empty-flag-audit/spec.json @@ -4918,7 +4918,7 @@ "args": [], "flags": { "context": "trail={flow}/{trail}", - "policy": "cmd/kosli/testdata/policies/allow-all.rego" + "path": "cmd/kosli/testdata/policies/allow-all.rego" }, "baseline_ok": false, "error": "the evaluation is created and run away from this machine, and the server the audit runs against does not offer it", @@ -4929,9 +4929,11 @@ "control", "fingerprint", "flow", + "identifier", "name", "output", "params", + "path", "policy", "trail" ], @@ -4941,9 +4943,11 @@ "control": "{control}", "fingerprint": "1bef738d0bb1e690500f99a5b57d958caf3a5eb3e00d9012e1f4369fc6812e01", "flow": "{flow}", + "identifier": "pr-approved", "name": "{name}", "output": "json", "params": "{}", + "path": "cmd/kosli/testdata/policies/allow-all.rego", "policy": "cmd/kosli/testdata/policies/allow-all.rego", "trail": "{trail}" }, diff --git a/internal/evaluations/client.go b/internal/evaluations/client.go index c7c04d6af..41e253283 100644 --- a/internal/evaluations/client.go +++ b/internal/evaluations/client.go @@ -32,10 +32,13 @@ type TrailRef struct { // single instant, which is why several trails belong in one request rather // than in one request each. type CreateRequest struct { - Trails []TrailRef - Files map[string]string - Params map[string]any - Decision *Decision + Trails []TrailRef + // Exactly one of Files and Identifier is set: Files sends the policy + // inline, Identifier names a published one whose current version runs. + Files map[string]string + Identifier string + Params map[string]any + Decision *Decision } // Decision is where an evaluation records its outcome, and against which @@ -70,17 +73,29 @@ type Failure struct { Message string `json:"message"` } +// PolicyCitation is the policy an evaluation ran. Identifier and Version are +// empty for a policy sent inline, which is cited by its digest alone. +type PolicyCitation struct { + Identifier string `json:"identifier,omitempty"` + Version int `json:"version,omitempty"` + Digest string `json:"digest"` +} + // Evaluation carries at most one of Result and Failure. Which one is present // follows from Status, so callers branch on Status rather than on presence. type Evaluation struct { - ID string `json:"id"` - Status string `json:"status"` - RequestedAt float64 `json:"requested_at"` - RecordedAt float64 `json:"recorded_at"` - Result *Result `json:"result"` - Failure *Failure `json:"error"` + ID string `json:"id"` + Status string `json:"status"` + RequestedAt float64 `json:"requested_at"` + RecordedAt float64 `json:"recorded_at"` + // Absent from a server that does not cite the policy it ran. + Policy *PolicyCitation `json:"policy"` + Result *Result `json:"result"` + Failure *Failure `json:"error"` // Empty until a decision that was asked for has been written. DecisionAttestationID string `json:"decision_attestation_id"` + // Raw is the body the server sent, so a caller can echo it unchanged. + Raw string `json:"-"` } // IsTerminal reports whether the evaluation has finished and will not change. @@ -129,7 +144,7 @@ func (c *Client) Create(org string, request CreateRequest) (*Evaluation, error) DryRun: c.dryRun, Payload: createPayload{ Context: createContext{Trails: request.Trails}, - Policy: inlinePolicy{Files: request.Files}, + Policy: createPolicy{Files: request.Files, Identifier: request.Identifier}, Params: params, Decision: request.Decision, }, @@ -148,7 +163,7 @@ func (c *Client) Create(org string, request CreateRequest) (*Evaluation, error) // to be stated exactly here rather than inherited from a caller's struct. type createPayload struct { Context createContext `json:"context"` - Policy inlinePolicy `json:"policy"` + Policy createPolicy `json:"policy"` Params map[string]any `json:"params"` Decision *Decision `json:"decision,omitempty"` } @@ -157,8 +172,10 @@ type createContext struct { Trails []TrailRef `json:"trails"` } -type inlinePolicy struct { - Files map[string]string `json:"files"` +// The server takes one of the two variants and refuses a policy with both. +type createPolicy struct { + Files map[string]string `json:"files,omitempty"` + Identifier string `json:"identifier,omitempty"` } func decodeEvaluation(body string) (*Evaluation, error) { @@ -166,5 +183,6 @@ func decodeEvaluation(body string) (*Evaluation, error) { if err := json.Unmarshal([]byte(body), &evaluation); err != nil { return nil, fmt.Errorf("failed to parse the evaluation: %w", err) } + evaluation.Raw = body return &evaluation, nil } diff --git a/internal/evaluations/client_test.go b/internal/evaluations/client_test.go index 5e8aec5b7..3bcd32610 100644 --- a/internal/evaluations/client_test.go +++ b/internal/evaluations/client_test.go @@ -107,6 +107,18 @@ func TestCreateSendsExactlyTheAgreedPayload(t *testing.T) { require.Equal(t, map[string]any{"threshold": float64(2)}, seen.body["params"]) } +func TestCreateSendsOnlyTheIdentifierOfAPublishedPolicy(t *testing.T) { + server, seen := newFakeServer(t, http.StatusCreated, createdBody) + request := aCreateRequest() + request.Files = nil + request.Identifier = "pr-approved" + + _, err := newTestClient(t, server.URL, false).Create("my-org", request) + require.NoError(t, err) + + require.Equal(t, map[string]any{"identifier": "pr-approved"}, seen.body["policy"]) +} + func TestCreateSendsEveryTrailInOneEvaluation(t *testing.T) { server, seen := newFakeServer(t, http.StatusCreated, createdBody) @@ -325,6 +337,31 @@ func TestGetReadsTheDecisionTheEvaluationWrote(t *testing.T) { require.Equal(t, "01DECISION", evaluation.DecisionAttestationID) } +func TestGetReadsThePolicyTheEvaluationRan(t *testing.T) { + server, _ := newFakeServer(t, http.StatusOK, + `{"id":"01JABCDEF","status":"completed","requested_at":1.0,"recorded_at":1.0,`+ + `"policy":{"identifier":"pr-approved","version":3,"digest":"sha256:abc123"},"result":{"allow":true}}`) + + evaluation, err := newTestClient(t, server.URL, false).Get("my-org", "01JABCDEF") + require.NoError(t, err) + + require.Equal(t, &PolicyCitation{Identifier: "pr-approved", Version: 3, Digest: "sha256:abc123"}, + evaluation.Policy) +} + +// The body is kept so a caller can echo the evaluation as the server gave it, +// fields this client does not know included. +func TestGetKeepsTheBodyAsTheServerGaveIt(t *testing.T) { + body := `{"id":"01JABCDEF","status":"completed","requested_at":1.0,"recorded_at":1.0,` + + `"result":{"allow":true},"added_later":"kept"}` + server, _ := newFakeServer(t, http.StatusOK, body) + + evaluation, err := newTestClient(t, server.URL, false).Get("my-org", "01JABCDEF") + require.NoError(t, err) + + require.Equal(t, body, evaluation.Raw) +} + // An evaluation asked for no decision names none. func TestGetReadsNoDecisionWhereNoneWasWritten(t *testing.T) { server, _ := newFakeServer(t, http.StatusOK,