PAN: Support for rule-type in security rules #6291
Conversation
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #6291 +/- ##
============================================
+ Coverage 72.94% 72.96% +0.02%
+ Complexity 35081 35044 -37
============================================
Files 2832 2828 -4
Lines 142491 142200 -291
Branches 17106 17085 -21
============================================
- Hits 103933 103753 -180
+ Misses 30339 30233 -106
+ Partials 8219 8214 -5 |
There was a problem hiding this comment.
Reviewed 3 of 6 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ratulm and @sfraint)
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PaloAltoConfiguration.java, line 647 at r2 (raw file):
.isEmpty(); boolean toZoneInRuleTo = !Sets.intersection(rule.getTo(), ImmutableSet.of(toZoneName, CATCHALL_ZONE_NAME)).isEmpty();
I think what you have written is probably correct, but I think it is confusing.
-
If either condition is false, return false.
switch (firstNonNull(ruletype, UNIVERSAL)
case UNIVERSAL:
return true;
case INTRAZONE:
return fromZoneName.equals(toZoneName);
case INTERZONE:
return !fromZoneName.equals(toZoneName);
default:
throw missing statmeent or whatever.
Yes, PAN UI enforces that INTRAZONE => the from and to sets are equal, but I think we should not rely on that here. What if the text is human-generated?
If we want to rely on that here, we need to document it and enforce it sensibly (elsewhere? drop the rule in this case?)
projects/batfish/src/test/java/org/batfish/representation/palo_alto/PaloAltoConfigurationTest.java, line 651 at r2 (raw file):
rule.setRuleType(RuleType.INTRAZONE); assertTrue(securityRuleApplies("A", "A", rule));
This condition should not pass: A is not a valid toZone
projects/batfish/src/test/resources/org/batfish/grammar/palo_alto/testconfigs/rulebase-rule-type, line 6 at r2 (raw file):
Quoted 5 lines of code…
set rulebase security rules RULE2 from z1 set rulebase security rules RULE2 to z2 set rulebase security rules RULE2 source any set rulebase security rules RULE2 destination any set rulebase security rules RULE2 rule-type intrazone
this seems to violate the constraints?
projects/batfish/src/test/resources/org/batfish/grammar/palo_alto/testconfigs/rulebase-rule-type, line 24 at r2 (raw file):
set rulebase security rules RULE3 destination any set rulebase security rules RULE3 rule-type universal
add a "default" case where the value is null?
There was a problem hiding this comment.
Reviewed 5 of 5 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ratulm and @sfraint)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2844 at r3 (raw file):
Quoted 4 lines of code…
_mainConfiguration .getVirtualSystems() .values() .forEach(this::removeInvalidIntrazoneSecurityRules);
@sfraint - better place to do this? Extractor#getConfiguration seems like an odd choice vs, say, during conversion. Do we do similar things elsewhere?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sfraint)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2844 at r3 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
_mainConfiguration .getVirtualSystems() .values() .forEach(this::removeInvalidIntrazoneSecurityRules);@sfraint - better place to do this?
Extractor#getConfigurationseems like an odd choice vs, say, during conversion. Do we do similar things elsewhere?
happy to defer to @sfraint's take. the rationale for doing it here: the parser itself produces a "clean" configuration and is in line with how we treat some other instances of the box rejecting config lines.
|
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2844 at r3 (raw file):
The other concern I have is that there's nothing in the API contract that links |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sfraint)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sfraint)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2844 at r3 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
we treat some other instances of the box rejecting config lines.
Not sure what you mean? AFAIU, we reject a line in the parser only if it would have been rejected by the device when typed right there. That's not this -- the device would let you type these lines in, at least if therule-typewas after thefrom.
Can you point me at an example of what you're thinking of?The other concern I have is that there's nothing in the API contract that links
getConfigurationto "parsing is done". No guarantees about when or how many times it's called.
Some context on rejection via Spencer:
(We should test if the device behavior is based on whats current when it sees the rule-type command. So, entering from z1 and then rule-type intrazone will fail unless to z1 has been entered before. If that is the behavior, the cleanup discussion is moot.)
Your other concern is valid. Another place I considered was here:
I couldn't find other examples of such post-parsing clean ups but doing it during conversion "felt wrong."
|
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2844 at r3 (raw file): Previously, ratulm wrote…
We have parsers that call -- I'm not quite sure why conversion is the wrong time to do this. It feels like "commit time" errors are best mapped to conversion -- at least, it's definitely not something that is part of walking the parse tree, it's something that comes from post-processing the extracted data from the parse tree. Batfish-standard is to do this kind of thing during conversion; for example that's where we change undefined references into "match all" in Cisco. (That's also what Spencer suggested in the thread you linked.) I could see a design with a middle phase between parsing and conversion -- parsing is literally just extracting the text into objects, then something where we "normalize" or "validate" (and/or "concretize"?) the parsed data, and then conversion where we blindly create VI objects from the VS ones. That middle step would also be where we fill in every single null field with its defaults, for example. I could see why it's strange to propagate Right now we don't really have that separation tho. |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sfraint)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2848 at r3 (raw file):
} /** Remove intrazone rules that don't have identical From and To zones */
FYI you'll have to merge master. It looks like your prior PR(s) conflicted with this one.
There was a problem hiding this comment.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @dhalperi and @ratulm)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2844 at r3 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
We have parsers that call
processParseTreemore than once, too. (Juniper).--
I'm not quite sure why conversion is the wrong time to do this. It feels like "commit time" errors are best mapped to conversion -- at least, it's definitely not something that is part of walking the parse tree, it's something that comes from post-processing the extracted data from the parse tree.
Batfish-standard is to do this kind of thing during conversion; for example that's where we change undefined references into "match all" in Cisco. (That's also what Spencer suggested in the thread you linked.)
I could see a design with a middle phase between parsing and conversion -- parsing is literally just extracting the text into objects, then something where we "normalize" or "validate" (and/or "concretize"?) the parsed data, and then conversion where we blindly create VI objects from the VS ones.
That middle step would also be where we fill in every single null field with its defaults, for example. I could see why it's strange to propagate
firstNonNull(field, it's default)all over conversion code.Right now we don't really have that separation tho.
I'd say we should either:
1a. add this check in PaloAltoConfigurationBuilder
1b. (better version of 1a) add this check in a new pre-conversion step like @dhalperi suggested
2. add this check during conversion
Generally, I'd say for things PAN cli won't let you enter 1 seems appropriate, and for things you can enter but might be rejected during commit process 2 seems appropriate.
Since PAN enforces validation for rule-type when you enter it on the cli (not at commit time), I'm leaning slightly toward doing this in 1. (also I believe @ratulm confirmed rule-type comes after to / from in config dumps)
|
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2844 at r3 (raw file): Previously, sfraint (Spencer Fraint) wrote…
For 1, as long as we can do it in an |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ratulm and @sfraint)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhalperi and @ratulm)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2844 at r3 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
For 1, as long as we can do it in an
enter|exit_Rule-- the part that corresponds to CLI entry time -- then I'm down.
👍 I was thinking in the rule-type exit rule
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhalperi and @sfraint)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2844 at r3 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
👍 I was thinking in the
rule-typeexit rule
just so we are clear, we need checks here both 1) when rule-type is entered; and 2) after parsing is done. 2 is needed because the rule can be made invalid after the fact by changing its From/To zones.
i'll add 1 to parsing time and build on dan's other PR to 2.
|
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2844 at r3 (raw file): Previously, ratulm wrote…
My other PR is a discussion piece. Instead, you could prevent users from changing zones if rule type has been set to intrazone. If you want to stick to post-processing, just do it in conversion for now so we can move forward. That's what we do everywhere else in Batfish. |
There was a problem hiding this comment.
Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @dhalperi and @sfraint)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2844 at r3 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
My other PR is a discussion piece.
Instead, you could prevent users from changing zones if rule type has been set to intrazone.
If you want to stick to post-processing, just do it in conversion for now so we can move forward. That's what we do everywhere else in Batfish.
Did both during parsing and conversion, to get closer to device semantics (which checks during rule-type command and during commit, not during from/to commands).
If we prevented zone changes for intrazone rules, the following config will behave differently on device and batfish:
from = zone A
to = zone A
rule-type = intrazone
from = B
rule-type = interzone
on device: interzone rule from zone B -> A
on batfish: interzone rule from A -> A (because we rejected from = B)
There was a problem hiding this comment.
Reviewed 5 of 5 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ratulm)
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 2485 at r6 (raw file):
Intrazone security
Maybe something that indicates we skipped this directive?
Error: cannot set rule-type intrazone for a security rule with different ...
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PaloAltoConfiguration.java, line 706 at r6 (raw file):
Invalid
Skipping invalid... ?
There was a problem hiding this comment.
Reviewed 4 of 4 files at r7.
Reviewable status:complete! all files reviewed, all discussions resolved
No description provided.