Default deny when accessing undefined key

[marcelmindemann]

Hi,
I am using Conftest: 0.62.0, OPA: 1.6.0 to implement Pull Request checks on several repositories holding configuration in our organization. The config files are mostly JSON or YAML and they are pretty dynamic in their schema and highly nested.

The big problem we've noticed with conftest is the robustness when it comes to accessing nonexistent keys from the input document(s), which could happens due to a typo or an unexpected schema of the input document. A policy that tries to access a nonexistent key like input.changes.before.groups.mygroup.members will evaluate to undefined via Rego's cascading undefined.

This means any policy which tries to access a nonexistent key or that simply contains a typo defaults to allow, with no way of noticing that you have a typo somewhere other than extensive unit testing. This becomes very high effort when your input documents are deeply nested.

I've read the following related issues:
open-policy-agent/opa#1857
open-policy-agent/opa#2345
open-policy-agent/opa#7602

Do I take it correctly that the current best workaround is using object.get()? While that works, it does make our policy code extremely verbose and unreadable. A lookup like

m := input.changes.before.groups.mygroup.members

blows up to six lines of

changes := object.get(input, "changes", {})
before := object.get(changes, "before", {})
...

I think this negates the best part of Rego, readable policies that read close to human language.

Another option would be to use a "default deny" strategy if the deny rule evaluates to undefined, but this seems to be impossible, as Rego only allows default on complete rules and conftest's convention is to use the partial rule deny contains msg. Therefore, I cannot do

default deny := ["default deny msg"] # error: conflicting rules data.play.deny found

Does someone have some guidance on the current best practice to use conftest in a robust way, e.g. not silently defaulting to allow when there is any typo/incorrect lookup in the policy?

[anderseknert]

Hi there! This is not specific to conftest, but the below applies to Rego in general.

I think this negates the best part of Rego

That's funny, because the idiomatic solution to work with undefined in Rego is commonly negation :) Which the not keyword helps with. You could either define your requirements on the input document as a series of incremental rules, like:

deny contains "no group members in input" if not input.changes.before.groups.mygroup.members
deny contains "no foo.bar in input" if not input.foo.bar
# ... and so on ...

Note that you also can negate more complex (complete) rules, e.g:

mygroup_is_valid if {
    # note that this fails if input.changes.before.groups.mygroup.members is undefined,
    # which is fine, as we negate the whole rule later anyway! This is often referred to as
    # "leaning in on undefined"
    num_members := count(input.changes.before.groups.mygroup.members)
    num_members > 0
    num_members < 11
}

deny contains "invalid group in input, must have 1-10 number of members" if not mygroup_is_valid

If you already have a defined JSON schema for the input document, you could alternatively use the json.verify_schema built-in function to verify the whole input document in a single call. This avoids "boilerplate rules" like the ones above, but that also means you can't understand the requirements just by looking at the policy, which is why using not is commonly considered the more idiomatic option.

A lookup like m := input.changes.before.groups.mygroup.members blows up to six lines

No, that's still a single line if the path is passed as an array:

m := object.get(input, ["changes", "before", "groups", "mygroup", "members"], "default value")

This is equivalent of writing:

default m := "default value"

m := input.changes.before.groups.mygroup.members

Where the latter is often preferred for top-level rule assignment, and the former for inside of rule bodies (where default is not allowed). But it doesn't matter too much which one you use as the result is the same using either method. I would recommend against using these when you don't want to fallback on a default value though! I.e. you'll sometimes see rules like this:

deny contains "no group members in input" if {
   m := object.get(input, ["changes", "before", "groups", "mygroup", "members"], {})
   m == {}
}

Which is really just a more convoluted version of:

deny contains "no group members in input" if not input.changes.before.groups.mygroup.members

that we saw above.

I hope that helps! If not I'm happy to answer any follow-up questions you may have.

[marcelmindemann]

Hello Anders,
thank you for taking the time to respond with such a helpful answer, I appreciate it a lot. You mentioned some good patterns I wasn't aware of. I will take the time to implement those in our organization.

I do have one remark though: We have also discovered a pattern similar to the one you call "leaning in on undefined", as it basically implements the "default deny" strategy we were looking for. However, this pattern makes it much harder to produce helpful error messages:

deny contains "invalid group in input, must have 1-10 number of members" if not mygroup_is_valid

is quite unspecific, and a user who changes a handful of groups in a PR then needs to manually check which group failed our check. It would be more helpful to produce an error message like the following:

sprintf("invalid group %s in input, must have 1-10 number of members", [group_name])

but alas, this throws us back to square one. group_name is of course derived from something like input.changes.before.groups.mygroup.name, which if undefined, cascades to make the msg and therefore the whole deny rule undefined.
Do you know a clever way around this problem?

[anderseknert]

this pattern makes it much harder to produce helpful error messages

That's not the fault of the pattern, but my limited understanding of your schema :) You can of course do iteration in your rule if it's a collection we're talking about. You might want to use a function rather than a rule though in order to pass each item for validation. The negation works the same. Assuming then that input.changes.before.groups is an object keyed by group name:

deny contains sprintf("invalid group %s in input, must have 1-10 number of members", [name]) if {
    some name, group in input.changes.before.groups

    not mygroup_is_valid(group)
}

mygroup_is_valid(group) if {
    num_members := count(group)
    num_members > 0
    num_members < 11
}

(you'll likely want to complement that with a few more rules that for example validates that there are any groups in the first place, if that's a requirement)

[marcelmindemann]

Thanks for having the patience to explain all of this to a Rego newbie :)

That's not the fault of the pattern, but my limited understanding of your schema

Your guess about our schema (input.changes.before.groups is an object keyed by group name) was correct. I believe my lack of understanding is more general than that though. Taking your last code bit as an example, I understand how that yields more readable error messages. It does introduce the direct lookup of some name, group in input.changes.before.groups inside the deny rule again, which means if we have a typo in input.changes.before.groups (or one of the other handful of lookups we do against input) the deny rule will be undefined and conftest yields exit code 0. The same happens if the schema of the input changes unexpectedly: suddenly all our PR checks will be green, when instead they should be red and say something like "invalid lookup".

To mitigate this, we would need to wrap all the lookups inside deny with object.get() again, which we would like to learn how to avoid. This is the flip-flopping we've been doing - either we have deny rules with helpful error messages, but those error messages could be undefined, which lead to default allow behaviour. Or we use the leaning in on undefined pattern, which negates the undefined, but because of that cannot produce templated error messages.

I assume the crux of the problem is that OPA treats undefined as false and therefore stops rule evaluation early? If we had a way of telling conftest "I am sure of my input schema, treat all lookups to input as defined", this would solve the problem. In this mode, conftest should panic early with something like

invalid lookup: input.changes.before.groups is undefined

So what we are trying to achieve is a different failure mode than the current one. Currently, conftest behaves like this:

• deny is false (no invalid groups found) => exit code 0
• deny is undefined (unexpected input schema) => exit code 0
• deny is defined (invalid groups found) => exit code 1, helpful templated error message

And we would need something like

• deny is false (no invalid groups found) => exit code 0
• deny is undefined (unexpected input schema) => exit code 1, generic error message
• deny is defined (invalid groups found) => exit code 1, helpful templated error message

As my understanding of the system deepens, I believe this is more of an issue with the Rego language than the conftest tool, correct?

[anderseknert]

An issue? That's one of Rego's superpowers 🙂 At least once you learn how to use it to your advantage.

It sounds like what you're asking for is compile time validation of the input schema? So that if you mistype what you call a lookup in the policy, it will be rejected? You can have that by extending OPA's type checker with your own schema. I'm not sure if conftest supports this or relies on OPA for that, but it doesn't matter much, as you'll normally only want to check this at a.g. "build time" anyway.

Providing a JSON schema to the type checker essentially allows you to say "my input has an attribute called foo and one called bar, and will have the compiler fail if your policy contains a reference like input.fool, as that's known not to be a valid input attribute.

Editors like VS Code + the OPA extension makes this pretty easy to integrate in your development workflow, as you can configure where your schemas are found and to have the check run whenever the document is saved. We're also looking into having the Regal language server use known schemas for auto-completions, so that the risk of typos is reduced even before they're made by only suggesting completion of paths known from schemas.

(As previously mentioned, you can also perform schema validation at runtime, which validates that the actual document used as input matches the same schema, thereby preventing typos or other mistakes that a client may do. This I've personally found less useful as the same thing can be done in Rego, but perhaps it's a good extra safety net while learning how to better navigate how Rego works in this regard.)

[marcelmindemann]

An issue? That's one of Rego's superpowers 🙂 At least once you learn how to use it to your advantage.

I sincerely would love to learn how to use this language feature to my advantage! From my current vantage point of a newcomer to the ecosystem, it presents itself as a hurdle to overcome rather than a superpower, but I do believe you when you say that this stems from my lacking understanding of the language. If you have any references or tutorials I can read that go into using this feature to my advantage, I would love to read them.

It sounds like what you're asking for is compile time validation of the input schema? So that if you mistype what you call a lookup in the policy, it will be rejected?

Not necessarily validation, rather runtime errors when assumption are not fulfilled. I guess I am trying to apply patterns I know from classical programming languages like Python/Go to a logic programming language like Rego. If I imagine that I implemented our Pull Request checks in Python, and I baked some assumptions about the schema of my data under test into my code, such as:

groups = input["changes"]["before"]["groups"]

then this would fail in any of the failure cases I mentioned (typo, unexpected input schema) with a KeyError and exit code 1. So I can rest assured that if I or the people who open PRs make any common human mistake, the PR check will be red and I will have a look at what's wrong.

However with Rego, the PR check will be green and silently allow a PR with "wrong" configuration to be merged and deployed. This is what I called "default allow" strategy in my last comment. It seems surprising to me that conftest/Rego would choose such a design for a language that has policy evaluation/compliance checking as its prime use case. Avoiding this "pitfall" seems to take extra effort and is not highlighted in the documentation of conftest or Rego.

Please don't get me wrong, I don't want to be one of these people to show up to an Open Source ecosystem and instantly criticize everything without understanding the reasons why things came to be. I really want to understand how to use Rego properly and if my approach to solving our use case with conftest is simply going at it the wrong way. I am happy to contribute to the documentation of conftest once I feel I have fully understood the issue. I believe I am not the only one with this confusion, looking at open-policy-agent/opa#1857.

Providing a JSON schema to the type checker essentially allows you to say "my input has an attribute called foo and one called bar, and will have the compiler fail if your policy contains a reference like input.fool, as that's known not to be a valid input attribute.

I have indeed looked at using the JSON schema validation feature of OPA, and on the surface it would solve this problem (despite requiring some extra effort of creating a schema file). Unfortunately Rego's current implementation of JSONschema does not support AdditionalProperties which means we would need to fully model our input JSON documents, which presents another problem: Aside other use cases, we are trying to validate plan.json outputs from Terraform, so modelling every resource's JSON representation in the schema file is simply impossible.

I think the official blog post that introduced JSONschema validation does a good job of explaining the pitfall:

The empty value returned is indistinguishable from a situation where the input did not violate the policy. This error is therefore causing the policy not to catch violating inputs appropriately.

So I am hopeful for the future, where support for AdditionalProperties is implemented! In the meanwhile, I suppose the pattern you explained in your first response of implementing manual schema checks in Rego is satisfactory for us!

[anderseknert]

All good! I enjoy the discussion, and FWIW, I agree that there isn't enough written about the fundamentals, why Rego works like it does and how to best wield its powers 🙂 While I think users who put in the time are going to see the benefits eventually, there is likely a large group of them who won't make it that far, and the better job we do communicating this, the more likely it is that they'll stick around. I'm hoping that I get some time for that this winter, and perhaps I'll steal some bits from this discussion if you don't mind.

I'm not sure it answer all of your questions, and let me know if there's something you feel like I missed, but I'll expand some on the "philosophy" aspect below as I think that works better to explain the "Rego way".

A longer read, but well worth the time, is this blog by @nevumx. While the topic is authorization, I think it's just as applicable to anything you might want to use Rego for.

This is what I called "default allow" strategy in my last comment. It seems surprising to me that conftest/Rego would choose such a design for a language that has policy evaluation/compliance checking as its prime use case.

There's no such strategy baked into Rego, but that would be for the policy author to decide on. Things like "allow" or "deny" mean nothing to OPA — they're just names. Besides of course their names, these rules are identical:

allow if "admin" in input.user.groups
deny if "admin" in input.user.groups

OPA has no way of knowing that "allow" likely is the more sensible name for the rule given the value assigned, or the conditions in the rule body. They're just complete or partial rules, functions, data, etc. The same is of course true for rules building sets (partial/multi-value rules), like deny in your example. Regal's policy bundle of ~18K lines of Rego doesn't contain a single rule named "allow" or "deny", as those aren't names that best describe that domain.

baked some assumptions about the schema of my data under test into my code

Right — in Rego, merely referencing something does not bake in assumptions of its presence at runtime (i.e. if will be defined or undefined). That's what allows writing concise but expressive declarative rules like:

users_with_email contains input.users[x].name if input.users[x].email

Which given an array of users in input collects all the names of those with an email attribute defined. Users where the email attribute — or even the name — is not provided (i.e. undefined) are simply not aggregated. If there are no users (if input.users is undefined or empty) — the logical conclusion is of course that there also are no users with emails (users_with_email evaluates to an empty set).

That's "leaning in on undefined", or at least how I like to think about it.

If we want to express that some attribute must be present, there are many ways to do that too, like using the every keyword.

Now, systems built on top of OPA and Rego, like Conftest or Gatekeeper, often do enforce some form of meaning, or a "contract" if you will, where those applications define the meaning of e.g. a set named deny and what it contains (if anything). Using a set to collect reasons for denial, and thus having an empty set mean "no reason to deny" is fairly common,
but as you have shown, that type of contract often requires more careful consideration about where evaluation may halt for whatever reason. As previously covered, an idiomatic approach to tackle that is to simply flip the conditions around via negation, and describe what is required rather than what mustn't be true. But there are many ways to define expectations and rules of any kind — that's what policy is after all :)

Conftest is a bit of an outlier with regard to "safety" I suppose, as conftest policies commonly validate user provided inputs (i.e. files on disk). The issue you're describing, where a user makes a typo in their cloud resource manifest (or whatever it may be) thus making it invalid, simply isn't an issue most policies have to consider. The Kubernetes API isn't going to send invalid resources to your OPA admission controller, and your Java application is always going to serialize class attributes predictably (as defined by annotations or whatnot) before presenting it as input to OPA. Users provide input data in those cases, but they can't modify the input format.

I don't normally use the JSON schema validation functions at runtime for that reason, but I would perhaps consider doing that if I knew I couldn't trust anything about the input a client provides. Another option would be to use a separate tool for validating resources against schemas before they're provided as input to conftest.

Providing schemas for the type checker prevents mistakes made in the policy, at compile time, and while it requires some upfront effort, is IMHO always worth it. We've talked some about having tools like Regal's language server provide options for generating schemas out of known input attributes found in policies. Perhaps that would be one way of reducing that upfront effort further. Regal already provides completion suggestions based on an example input.json file (if found in the workspace), and I've found it to be very effective for preventing typos before even the compiler has a say.

Finally, just as it hasn't been mentioned — unit tests are absolutely key for gaining confidence in expectations being met by a policy. While 100% test coverage is almost always a waste of time in languages like Java, it's both sensible and trivial to achieve in Rego. The rather recent addition of parameterized tests makes this even easier now than it used to be.

Rego's current implementation of JSONschema does not support AdditionalProperties

Yeah, that would be a good addition for sure. If someone is reading this and would like to pick that up... :)

One thing to be aware of, and I guess not obvious from the docs — even without additionalAttributes it's possible to define a very loose type contract for a given attribute in case you want it to be fully dynamic. For example, this schema input would have input.foo checked only as an object with no regard to what it contains:

{
  "type": "object",
  "properties": {
    "foo": {
      "type": "object"
    },
    "bar": {
      "type": "string"
    }
  }
}

The type checker would still rightly protest on typing input.baz:

1 error occurred: p.rego:3: rego_type_error: undefined ref: input.baz
	input.baz
	      ^
	      have: "baz"
	      want (one of): ["bar" "foo"]

But will not complain about a reference to input.foo.a.b.c.d.

What you can't currently do, and what additionalProperties would allow, is to define an object as built of both some static (known) and dynamic (unknown) properties.

[marcelmindemann]

Anders, thank you very much for your guidance and the in-depth explanations. I will carry this new knowledge into our organization and use it to improve our usage of conftest.

I'm hoping that I get some time for that this winter, and perhaps I'll steal some bits from this discussion if you don't mind.

I don't mind! Improving the docs in this aspect sounds like a fantastic idea.

Now, systems built on top of OPA and Rego, like Conftest or Gatekeeper, often do enforce some form of meaning, or a "contract" if you will, where those applications define the meaning of e.g. a set named deny and what it contains (if anything).

Yes, this is what my current understanding boils down to - I have understood that rules like deny or allow do not have special meaning in Rego, it's rather conftest that uses the convention of partial deny rules. I think this is where most of my confusion came from: Most of the examples in the Rego Policy Language Tour use allow rules, which implicitly use the "leaning in on undefined" pattern without explicitly mentioning it. When I read the Rego Language Tour and applied that knowledge to write conftest policies, I was not able to properly transfer my new skills to match conftest's conventions. Again, thanks a lot for highlighting this gap in my understanding.

Another option would be to use a separate tool for validating resources against schemas before they're provided as input to conftest.

I agree, that's one option we're currently investigating. Another option could be adding an (optional?) case for "unchecked" lookups into the Regal linter? It would have been very helpful for me in the beginning if the linter warned me that a lookup like input.changes.before.groups is not matched by an input validation rule such as

deny contains "no groups in input" if not input.changes.before.groups

but I can see how such a linter check would be highly annoying for people using Rego for use cases where the input schema is trusted (like the K8S API output example). Also, I understand that the linter would have a hard time identifying the input validation rule asdeny carries no special meaning in Rego :)

[anderseknert]

Thanks Marcel! I'll review the docs to see if I can find a good place to inject a summary on some of the things we covered here.

Also, I understand that the linter would have a hard time identifying the input validation rule asdeny carries no special meaning in Rego :)

Regal has a concept of "engines", where you can define a target system (like EOPA or Conftest). This was built so that Regal would recognize built-in functions that these systems may add on top of OPA. But there's nothing saying we couldn't also use it to infer behavior of policies, like Conftest's various warn and deny rule variations. Just so much to do, and so little time :)