Fix firewall rules #204

SantiagoTorres · 2018-04-17T20:44:17Z

Please fill in the fields below to submit a pull request. The more information
that is provided, the better.

Fixes issue #:

None, this issue was brought up from conversations with potential integrators

Description of the changes being introduced by the pull request:

The current behavior of the artifact rules throw exceptions when an element in the filtered queue doesn't match the target. For example, if a file foo is being checked for a MODIFY rule then a VerificationError will be raised if foo is indeed not modified. This is not how firewalls behave.

Instead, foo should just remain in the queue and expected to be matched by other rules later down the line. The only rule that should throw an error is a DISALLOW rule when artifacts are matched.

Please verify and check that the pull request fulfills the following
requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

This is still somewhat of a WIP branch, as I'd like to have some feedback from @vladimir-v-diaz and @lukpueh (if possible)

coveralls · 2018-04-17T20:46:33Z

Coverage decreased (-0.06%) to 99.942% when pulling 119570f on fix-firewall-rules into faf64e0 on develop.

coveralls · 2018-04-17T20:46:33Z

Coverage remained the same at 100.0% when pulling 9e2b76a on fix-firewall-rules into c87b400 on develop.

SantiagoTorres · 2018-04-17T20:46:45Z

FYI: these failures appear to be due to the set operator using a different PYTHONHASHSEED

vladimir-v-diaz · 2018-04-17T20:50:39Z

@JustinCappos requested that @awwad review more in-toto pull requests, so I'll add Sebastien to the list of reviewers. For now, I need to focus on fixing Windows-related issues for the TUF project.

SantiagoTorres · 2018-04-17T20:57:20Z

@JustinCappos requested that @awwad review more in-toto pull requests, so I'll add Sebastien to the list of reviewers. For now, I need to focus on fixing Windows-related issues for the TUF project.

Noted, and thanks!

@awwad welcome aboard!

- The pinned version number was wrong - Added mention to the obsolete settings removal.

The verification behavior didn't behave as a firewall, as described in the documentation. Instead, it used to throw a verification exception if sctrict matching didn't happen. Update the behavior of the in_toto_verify routines, as well as the specific subroutines to match the expected behavior.

Verifylib was changed in the parent commit. Update the tests to match this behavior by, mostly, removing the expected exception raises and adding "DISALLOW *" rules where necessary.

During the test of the modify rules, it is possible that the returned queues are in different order than they came in (this is due to the converstion to a set and back). Sort the queues before comparing the two queues to ensure the contents are compared, and not their order. Likewise, declare the elements in artifact queues alfabetically to make sure the comparison stands.

awwad · 2018-04-26T14:29:23Z

So if I understand correctly, with this PR merged, if you have some rule set wherein rule 1 reads: MATCH arty WITH PRODUCTS FROM step1, rules 2 through 5 have nothing to do with arty, and rule 6 is DISALLOW *, then the error you get when arty exists and has a different hash than that listed for arty in the step1.link products is an error about arty being disallowed by rule 6, right?

awwad · 2018-04-26T14:37:00Z

Also, with this construction (rule verifier functions adjust the input to remove what they've matched), none of the hash mismatch info gets reported anywhere -- neither trickles up in an exception that's caught somewhere nor logged. Is that okay in this context? Is that going to be regrettable eventually?

In any case, the PR seems sane.

test_fail_hash_not_eual -> test_fail_hash_not_equal Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

SantiagoTorres · 2018-05-03T21:13:31Z

wow, sorry for some reason I missed this PR review.

So if I understand correctly, with this PR merged, if you have some rule set wherein rule 1 reads: MATCH arty WITH PRODUCTS FROM step1, rules 2 through 5 have nothing to do with arty, and rule 6 is DISALLOW *, then the error you get when arty exists and has a different hash than that listed for arty in the step1.link products is an error about arty being disallowed by rule 6, right?

Yes, this is pretty much it. We're trying to have DISALLOW be the only rule that fails, where the others match and process. That's the behavior we had described in the specification.

Also, with this construction (rule verifier functions adjust the input to remove what they've matched), none of the hash mismatch info gets reported anywhere -- neither trickles up in an exception that's caught somewhere nor logged. Is that okay in this context? Is that going to be regrettable eventually?

We were worried about this too, but it's a shared issue on firewall-like semantics. I don't think we can make much about it (yet).

In any case, the PR seems sane.

Thanks a lot! Merging :)

Update code documentation for verifylib functions related to rule verification, to align with changes introduced by in-toto#204.

Modified the rule verification error messages to be more specific about which artifacts failed to match which given rules. Also restructured error workflow to only allow the DISALLOW rule the power to fail overall rule verification, with the other rules only able to remove artifacts from queues on success or leave the queue unchanged on failure, in alignment with in-toto#204.

Update code documentation for verifylib functions related to rule verification, to align with changes introduced by in-toto#204.

Modified the rule verification error messages to be more specific about which artifacts failed to match which given rules. Also restructured error workflow to only allow the DISALLOW rule the power to fail overall rule verification, with the other rules only able to remove artifacts from queues on success or leave the queue unchanged on failure, in alignment with in-toto#204.

# This is the 1st commit message: Updated rule verification error messages and error workflow Modified the rule verification error messages to be more specific about which artifacts failed to match which given rules. Also restructured error workflow to only allow the DISALLOW rule the power to fail overall rule verification, with the other rules only able to remove artifacts from queues on success or leave the queue unchanged on failure, in alignment with in-toto#204. # The commit message in-toto#2 will be skipped: # MAINT: update license to apache 2.0 # The commit message in-toto#3 will be skipped: # Adopt license change in setup.py # # Update setup.py to adopt recent license change # from MIT to Apache-2.0 # The commit message in-toto#4 will be skipped: # Adopt license change in debian/copyright # # Update debian/copyright to adopt recent license change # from MIT to Apache-2.0

Modified the rule verification error messages to be more specific about which artifacts failed to match which given rules. Also restructured error workflow to only allow the DISALLOW rule the power to fail overall rule verification, with the other rules only able to remove artifacts from queues on success or leave the queue unchanged on failure, in alignment with in-toto#204.

This commit includes several changes to `verify_item_rules` and the from there called `verify_*_rule` functions (plus corresponding tests): - Harmonization of artifact queue modification Before, `verify_item_rules` used a material queue, a product queue and an artifact queue to keep track of the artifacts, consumed in the course of verifying all rules (of a type of an item). These queues were modified in different places (in different functions passed to by reference) and then re-assigned to each other, sometimes as copy, sometimes as reference, which led to unexpected states of queues. This commit removes the materials and products queue (the generic artifacts queue is enough for keeping track of consumed artifacts). It further focuses all artifact queue updates in a single place in `verify_item_rules` and changes the `verify_*_rule` functions to return the consumed artifacts instead of the updated queue, which also aligns with the pseudo code in the specification. This change should make the rule processing a lot more transparent to the reader. - Additional code simplifications This commit also makes better use of set operations in `verify_*_rule` functions and removes clutter like - redundant rule unpacking code - unnecessary `if/else`s related to "materials" and "products" - unnecessarily long variable names the `source_` prefix only makes sense in the scope of the match rule, where there is also a destination. The commit also updates and simplifies a lot of related code comments and docstrings. - Full adoption of in-toto#204 Remove raised exceptions in `verify_match_rule` and `verify_delete_rule`, where they should just not consume the corresponding artifacts. - Rethink tests Some of them were still tailored to pre-in-toto#204 logic. This commit makes them more meaningful (and table driven). - Adopt queue traceback There is only one queue now.

Modified the rule verification error messages to be more specific about which artifacts failed to match which given rules. Also restructured error workflow to only allow the DISALLOW rule the power to fail overall rule verification, with the other rules only able to remove artifacts from queues on success or leave the queue unchanged on failure, in alignment with in-toto#204.

SantiagoTorres requested review from lukpueh and vladimir-v-diaz April 17, 2018 20:44

vladimir-v-diaz requested a review from awwad April 17, 2018 20:51

SantiagoTorres force-pushed the fix-firewall-rules branch from 119570f to 3aa9329 Compare April 18, 2018 16:55

SantiagoTorres added 4 commits April 18, 2018 12:55

FIX: CHANGELOG: address changelog review comments

57138e1

- The pinned version number was wrong - Added mention to the obsolete settings removal.

FIX:test_verifylib: update test to match verifylib

5f22763

Verifylib was changed in the parent commit. Update the tests to match this behavior by, mostly, removing the expected exception raises and adding "DISALLOW *" rules where necessary.

Minor: typo fix in test func name

9e2b76a

test_fail_hash_not_eual -> test_fail_hash_not_equal Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad approved these changes Apr 27, 2018

View reviewed changes

SantiagoTorres merged commit ae8ddcd into develop May 3, 2018

lukpueh added a commit to lukpueh/in-toto that referenced this pull request Oct 24, 2018

Update verify_*_rules docstrings

a613af9

Update code documentation for verifylib functions related to rule verification, to align with changes introduced by in-toto#204.

This was referenced Oct 24, 2018

Update verify_*_rules docstrings #238

Merged

Update artifact rules descriptions and pseudo code in-toto/specification#11

Merged

michizhou mentioned this pull request Nov 8, 2018

Updated rule verification error messages and error workflow #242

Closed

3 tasks

michizhou mentioned this pull request Nov 8, 2018

Better error messages during verification of rules #228

Closed

michizhou pushed a commit to michizhou/in-toto that referenced this pull request Nov 8, 2018

Update verify_*_rules docstrings

18559e3

Update code documentation for verifylib functions related to rule verification, to align with changes introduced by in-toto#204.

michizhou mentioned this pull request Nov 8, 2018

Updated rule verification error messages and error workflow #243

Merged

3 tasks

lukpueh mentioned this pull request Mar 11, 2019

Major rewrite of artifact rule verification #262

Merged

3 tasks

lukpueh mentioned this pull request Apr 12, 2019

Document "artifact rule" rationale in-toto/specification#4

Open

SantiagoTorres deleted the fix-firewall-rules branch October 31, 2019 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix firewall rules #204

Fix firewall rules #204

SantiagoTorres commented Apr 17, 2018

coveralls commented Apr 17, 2018

coveralls commented Apr 17, 2018 •

edited

Loading

SantiagoTorres commented Apr 17, 2018

vladimir-v-diaz commented Apr 17, 2018

SantiagoTorres commented Apr 17, 2018

awwad commented Apr 26, 2018 •

edited

Loading

awwad commented Apr 26, 2018

SantiagoTorres commented May 3, 2018

Fix firewall rules #204

Fix firewall rules #204

Conversation

SantiagoTorres commented Apr 17, 2018

coveralls commented Apr 17, 2018

coveralls commented Apr 17, 2018 • edited Loading

SantiagoTorres commented Apr 17, 2018

vladimir-v-diaz commented Apr 17, 2018

SantiagoTorres commented Apr 17, 2018

awwad commented Apr 26, 2018 • edited Loading

awwad commented Apr 26, 2018

SantiagoTorres commented May 3, 2018

coveralls commented Apr 17, 2018 •

edited

Loading

awwad commented Apr 26, 2018 •

edited

Loading