fix: Prevent any users from approving custom policy sets

[bgalkows]

what

• Propagates policy set names to custom policy sets properly instead of defaulting all custom checks to an undefined set called "Custom"

why

• Currently, using custom policy tools with Atlantis inadvertently allows any user to successfully execute atlantis approve_policies on the policy check
• Policy set naming is used to map user permissions for who can approve what, and as a result all users are able to approve the default name which has no restrictions specified
• For the small community of Atlantis folks leveraging Terraform policy tools other than the integrated Conftest, this change is extremely important
  • Any user being able to bypass any custom policy set is a critical flaw in a controlled actuation workflow

tests

• I have tested these changes by reproducing the bug where any user can approve the policy set and then confirming it is not possible with this change added
• I've actually used a fork with this tweak added for a while and never noticed unintended consequences

references

• Bugs:
  • [Bug] unable to unmarshal conftest output or Anyone able to approve Atlantis policy failures  #4243
  • Anyone can approve custom policy exceptions #4962
• Other
  • Wish I'd seen these sooner! I contributed the custom policy check logic in v0.26.0 - feat: Add option allowing usage of custom policy check tools  #3765

[dimisjim]

@bgalkows Amazing, thanks a lot for the contribution. Let me know if I can help in any way, so that this gets merged and released asap!

[bgalkows]

Hey all, from what I can tell the failing unit test is erroring after it can't find Conftest installed in the GitHub runner environment. I see this matching error output in the logs:

atlantis/server/controllers/events/events_controller_e2e_test.go

Lines 1874 to 1886 in d99c66a

	// Will fail test if conftest isn't in path
	func ensureRunningConftest(t *testing.T) {
	// use `conftest` command instead `conftest$version`, so tests may fail on the environment cause the output logs may become change by version.
	t.Logf("conftest check may fail depends on conftest version. please use latest stable conftest.")
	_, err := exec.LookPath(conftestCommand)
	if err != nil {
	t.Logf(`%s must be installed to run this test
	- on local, please install conftest command or run 'make docker/test-all'
	- on CI, please check testing-env docker image contains conftest command. see testing/Dockerfile
	`, conftestCommand)
	t.FailNow()
	}
	}

Would anyone be able to help resolve this? testing/Dockerfile does have commands to install Conftest.

And @dimisjim, any input is appreciated if you're interested! Feel free to add commits or create your own PR if that's the easier

[dimisjim]

@bgalkows Would this still fix the issue with approvals (should only mark the policy as approved if an owner approves it), even if the policy_check run step under the custom workflow definition in the repo's atlantis.yaml contains the --no-fail flag? This is necessary to have, according to my tests, alongside -o table flag, so that policy is treated as a custom one and I don't get: unable to unmarshal conftest output.

My conftest cmd looks like: conftest test </path/to/inputs.json> -p <path/to/my/rego/policy> -o table --no-fail

[bgalkows]

@bgalkows Would this still fix the issue with approvals (should only mark the policy as approved if an owner approves it), even if the policy_check run step under the custom workflow definition in the repo's atlantis.yaml contains the --no-fail flag? This is necessary to have, according to my tests, alongside -o table flag, so that policy is treated as a custom one and I don't get: unable to unmarshal conftest output.

My conftest cmd looks like: conftest test </path/to/inputs.json> -p <path/to/my/rego/policy> -o table --no-fail

@dimisjim The custom policy checks are intended for non-Conftest policy tooling, so i've never actually used a custom check which runs Conftest. If you're using Conftest and getting unable to unmarshal conftest output, that would be related to a different bug as I understand it. Is it perhaps that the -o table flag is unsupported by Atlantis, confounding the output parsing?

To answer your question though, yes - this change will fix any user being able to approve the custom policy check regardless of what the run step actually contains.

[dimisjim]

Is it perhaps that the -o table flag is unsupported by Atlantis, confounding the output parsing?

actually -o table is the only output type that works for me.

The reason why I am using a custom policy, but still conftest is to include more than one input into the rego policy, basically following this example: https://www.runatlantis.io/docs/policy-checking#running-policy-check-against-terraform-source-code In my case it's not to test against terraform code directly but rather inject an input concerning multiple custom workflows, taken by a pre-workflow script.

[dimisjim]

@jamengual , @X-Guardian , @nitrocode, @chenrui333 Could you help out?

I would like to test this before merging. Is there any atlantis image I could use built from this PR?

[nitrocode]

For your own container

1. Merge your feature branch to your fork's main branch (looks like you already did)
2. Cut a release from your fork

This will create a container and upload it to your fork's ghcr and allow you to test that container e2e in your deployment.

[dimisjim]

@nitrocode, @bgalkows

I can confirm that it works with my setup too, after forking the repo, basing it on v0.33.0 and cherry picking the changes of this branch 🚀

[X-Guardian]

The policy check tests in events_controller_e2e_test.go are failing on this change and need fixing.

[github-actions]

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

[waqaskayani]

Waiting for this to be merged, much needed. 🙏

[github-actions]

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

[dimisjim]

bump

[bruceoutdoors]

bump

[jamengual]

the test will need to pass for this pr to be merged.

[dimisjim]

Can you check this one here please and let me know if this is fine in terms of tests additions and fixes? #5915

Thanks 🙏

[github-actions]

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

[dimisjim]

This can now be closed as this one is merged: #5915