encode best practice in default rule behavior for DISALLOW *

[ultrasaurus]

Description of issue or feature request:
CNCF SIG Security assessment notes: "As a recommended best practice, we assume there is a DISALLOW * rule at the end of the rule list for each step." This would be more robust if it was encoded in the step, rather than required for users to specify.

Current behavior:
From the demo, users must specify DISALLOW * as part of the rule...

          "expected_materials": [["MATCH", "demo-project/*", "WITH", "PRODUCTS",
                                "FROM", "clone"], ["DISALLOW", "*"]],
          "expected_products": [["ALLOW", "demo-project/foo.py"], ["DISALLOW", "*"]],

Expected behavior:

You could consider including that as part of the rules "expected_materials and expected_products and make different keys to implement the rare use cases where this is would not be desirable.

[reza-curtmola]

I believe that DISALLOW * was implicitly included as part of rules, but it was changed a while ago.
There was a discussion back then, and I believe Santiago also pointed out some firewalls that don't have an implicit DISALLOW rule at the end of the ruleset.

[lukpueh]

IIUC the assessment note recommends a default ["DISALLOW", "*"] rule added to each rule list (when using in-toto tooling to create layouts?). This is different from having an implicit disallow everything that has not been consumed until now in the verification routine, as we had it prior to #140.

in-toto/specification#4 calls for documenting the rationale for all artifact rule related design decisions. Currently it only lists pointers to related GitHub issues and prs. We should have the discussion there.

[trishankatdatadog]

According to @SantiagoTorres, I was also responsible for this, although I can't remember having this discussion, and it is conducive for present purposes to forget about it now even if it happened...

In any case, @adityasaky and I were bitten by this today, and we highly recommend adding an implicit "DISALLOW *" pattern at the end of every step. At the very least, I will add it to my layout library as a default last step.

[JustinCappos]

poking this issue. Can we make a determination and move forward?

[trishankatdatadog]

If we DISALLOW * by default in the reference implementation to err on the side of safety by default, how would exceptions work? Would an explicit ALLOW * beforehand have the intended effect, or would the implicit DISALLOW * always prevail?

[reza-curtmola]

I would imagine that an explicit ALLOW rule on a subset would override a DISALLOW rule on a set that includes that subset.

…

On Fri, Dec 23, 2022 at 6:13 AM Trishank Karthik Kuppusamy < ***@***.***> wrote: If we DISALLOW * by default in the reference implementation to err on the side of safety by default, how would exceptions work? Would an explicit "ALLOW *beforehand have the intended effect, or would the implicitDISALLOW *` always prevail? — Reply to this email directly, view it on GitHub <#287 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADMU746LANH2FBJ4ONUNLI3WOWCORANCNFSM4HWUC5VQ> . You are receiving this because you commented.Message ID: ***@***.***>

[reza-curtmola]

I meant to say: I would imagine that an explicit ALLOW rule on a subset would override an implicit DISALLOW rule on a set that includes that subset.

…

On Fri, Dec 23, 2022 at 6:13 AM Trishank Karthik Kuppusamy < ***@***.***> wrote: If we DISALLOW * by default in the reference implementation to err on the side of safety by default, how would exceptions work? Would an explicit "ALLOW *beforehand have the intended effect, or would the implicitDISALLOW *` always prevail? — Reply to this email directly, view it on GitHub <#287 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADMU746LANH2FBJ4ONUNLI3WOWCORANCNFSM4HWUC5VQ> . You are receiving this because you commented.Message ID: ***@***.***>

[adityasaky]

Have we considered a reversal of #140? My understanding is we wanted to match firewall rules but I'm not convinced that's the best option for artifact rules. In the general sense, we want firewalls to let through anything that's not on a denylist but I can't see why that should be default behaviour for in-toto.

Perhaps in_toto_verify can provide a flag to not fail when there are unconsumed artifacts matching current behaviour. I also don't think this is a breaking change and it does not affect any layouts that have the explicit DISALLOW now. Edit: It'll break layouts that don't include DISALLOW * at the moment but that's probably a good thing.

[lukpueh]

I lean towards fixing this in artifact rules documentation, e.g. add a paragraph that strongly recommends adding DISALLOW * at the end of layout artifact rule lists (expected_materials or expected_products).

This is not what OP recommends, i.e. adding those rules per default (for whatever that means), nor what has been discussed above, i.e. assuming the rule exists per default (as prior to #140), but has the advantage that it won't break any existing users, and is also an opportunity to review this particular part of the documentation.

If breaking existing users is acceptable, we might want to make more drastic improvements to the verification language
(see notes about "Changes to verification language" from the Nov '22 in-toto community meeting).

[adityasaky]

@lukpueh do you think you can submit a patch to the spec (and here where we discuss artifact rules) adding this note about DISALLOW *?

[lukpueh]

Unfortunately, improving in-toto documentation is not a priority for me right now.

[adityasaky]

Gotcha, I'll try and sneak it into in-toto/specification#69 🤔

[adityasaky]

I emphasized an existing mention of this in the spec here: in-toto/specification@e709692. I'm not sure if we want to make these kinds of recommendations there but curious what others think.

[trishankatdatadog]

I still disagree that this is a documentation-level thing, but maybe we can have opinionated tools like for ITE-2 fix that instead.

[lukpueh]

I think we actually agree that this should be solved on a spec and implementation level, @trishankatdatadog. I was just trying to say that an isolated fix for disallow *, IMO, is not worth breaking existing clients, given that multiple concerns about the verification language have been raised.

[adityasaky]

Also added a note to artifact rules section in the readme: #530

[JustinCappos]

With the above documentation changes, I'm going to resolve this issue.