Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic policy composition #12

Merged
merged 6 commits into from Mar 24, 2022
Merged

Dynamic policy composition #12

merged 6 commits into from Mar 24, 2022

Conversation

anderseknert
Copy link
Member

@anderseknert anderseknert commented Mar 24, 2022

This adds dynamic routing / policy compositon based on the input.resource.type, mapping something like AWS::S3::Bucket to the data.aws.s3.bucket package, where the deny rule(s) will be evaluated, and the result aggregated into the decision. Moved and remodeled the policies and tests to work with this.

Since it's been a while since I worked a full day on policy authoring, here are some random thoughts and findings from this experience:

  • Metadata annotations are really nice! Thanks @johanfylling :)
  • However, opa inspect -a wasn't intuitive to me for this purpose. It still seems to want a bundle file, although I'm only interested in inspecting annotations in my rego files.
  • With policies mapped to a single resource type, being able to use JSON schema for input validation would be killer. But once again we're burnt by the "booleans are strings" fiasco.
  • Random idea around annotations - it would be nice to be able to reference the package name from inside an inherited annotation, particularly when using schemas, i.e, something like:
# METADATA
# scope: subpackages
# schemas:
#   - input: schema["${self.package.name}"]
package aws

That way we could apply schemas to all subpackages with a single declaration 馃く 馃殌

  • opa fmt is pretty annoying, in particular for test files where we construct lots of maps manually. These don't take line length into account, and sometimes makes a well structured map compressed into a single 300 chars long line 馃う
  • Wrt opa fmt, also got burnt by opa fmt indentation issues with with/as statements聽open-policy-agent/opa#4376
  • Created some test helpers for getting to know what failed in tests (by simply printing expectation vs reality), and those are definitely a keeper until we have that in OPA proper!
  • The new "future" keywords are very nice to use.

Signed-off-by: Anders Eknert <anders@eknert.com>
Signed-off-by: Anders Eknert <anders@eknert.com>
Signed-off-by: Anders Eknert <anders@eknert.com>
Signed-off-by: Anders Eknert <anders@eknert.com>
Signed-off-by: Anders Eknert <anders@eknert.com>
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple minor comments. I think we'll need to test out this composition model with the system type framework in DAS.

import future.keywords

deny[msg] {
input.resource.properties.SecurityGroupIngress[0].CidrIp == "0.0.0.0/0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there are multiple elements in the security group ingress collection? Should we use some instead?

some ingress in input.resource.properties.SecurityGroupIngress
ingress.CidrIp == "0.0.0.0/0"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, yeah I thought of this too when I read this today, and made a note to check later (I only moved this code). Will follow up on this after the PR.

policy/main.rego Outdated Show resolved Hide resolved
Signed-off-by: Anders Eknert <anders@eknert.com>
@anderseknert anderseknert merged commit aa10d86 into main Mar 24, 2022
@anderseknert anderseknert deleted the dynamic-policy-composition branch March 24, 2022 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants