Junos: warn if a standard community regex is used that can match longer strings

[dhalperi]

Junos community regexes are not implicitly truncated so can match longer
strings in many cases. This is risky, as unintended community matches can lead
to unexpected bad routing behaviors. Add a warning for these regexes.

Do not warn in common cases that do work even though the regex is not
truncated: as of modern Junos releases, the regex is not matched against
non-standard communities so 12345:3$, for example, is fine as no standard
community has 6 digits in the first half.

---

Stack:

• CompletionMetadata: minor parallelism and better logs #9168
• Junos: warn if a standard community regex is used that can match longer strings #9167 ⬅

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

[batfish-bot]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 80.55556% with 7 lines in your changes missing coverage. Please review.

Project coverage is 72.70%. Comparing base (de056d9) to head (9142de1).
Report is 59 commits behind head on master.

Files with missing lines	Patch %	Lines
...h/representation/juniper/RegexCommunityMember.java	87.09%	1 Missing and 3 partials ⚠️
...fish/grammar/flatjuniper/ConfigurationBuilder.java	25.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9167      +/-   ##
==========================================
- Coverage   72.70%   72.70%   -0.01%     
==========================================
  Files        3313     3313              
  Lines      169873   169904      +31     
  Branches    20007    20013       +6     
==========================================
+ Hits       123508   123528      +20     
- Misses      37207    37212       +5     
- Partials     9158     9164       +6     

Files with missing lines	Coverage Δ
...h/representation/juniper/JuniperConfiguration.java	86.06% <100.00%> (-0.03%)	⬇️
...fish/grammar/flatjuniper/ConfigurationBuilder.java	68.34% <25.00%> (-0.05%)	⬇️
...h/representation/juniper/RegexCommunityMember.java	86.48% <87.09%> (-13.52%)	⬇️

... and 2 files with indirect coverage changes

[millstein]

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @anothermattbrown, @dhalperi, and @nickgian)

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 22 at r1 (raw file):

  }

  public static Optional<String> isRiskyCommunityRegex(String junosRegex) {

Add a comment. Also the name sounds like it will return a boolean so consider renaming.

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 24 at r1 (raw file):

  public static Optional<String> isRiskyCommunityRegex(String junosRegex) {
    String javaRegex = communityRegexToJavaRegex(junosRegex);
    String u16 = "((0|[1-9][0-9]*)&<0-65535>)";

This string is also defined in CommunityVar so maybe it can be shared?

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 25 at r1 (raw file):

    String javaRegex = communityRegexToJavaRegex(junosRegex);
    String u16 = "((0|[1-9][0-9]*)&<0-65535>)";
    String regex = javaRegex.replaceAll("\\.", "[0-9]");

Why this replacement? I see that below you are intersecting (the automaton for) this regex with one that represents all (and only) valid communities, so won't that intersection have this same effect (and handle any other similar kinds of things)?

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 50 at r1 (raw file):

      examples.add(example.substring(2, example.length() - 2));
    }
    return Optional.of(String.join(" and ", examples));

Why return a string rather than just the list of examples (and then let callers do what they want with them)?

[nickgian]

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @anothermattbrown, @dhalperi, and @millstein)

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 18 at r1 (raw file):

    String out = regex;
    out = out.replace(":*", ":.*");
    out = out.replaceFirst("^\\*", ".*");

Since you are at it, would be useful to add a comment to this one too

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 27 at r1 (raw file):

    String regex = javaRegex.replaceAll("\\.", "[0-9]");

    Automaton valid = new RegExp("^^" + u16 + ":" + u16 + "$$").toAutomaton();

I tried looking here but couldn't figure it out; is the meaning of ^^ the usual ^?

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 41 at r1 (raw file):

    List<String> examples = new LinkedList<>();
    if (!exampleExtendingFront.minus(validClipped).isEmpty()) {
      String example = exampleExtendingFront.getShortestExample(true);

you can pull this out in another method since it is the same code as below.

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 50 at r1 (raw file):

Previously, millstein (Todd Millstein) wrote…

Why return a string rather than just the list of examples (and then let callers do what they want with them)?

+1, especially if we end up with other types of violations too.

---

projects/batfish/src/test/java/org/batfish/representation/juniper/RegexCommunityMemberTest.java line 21 at r1 (raw file):

      assertThat(maybeRisky, equalTo("not risky"));
    }
    for (String risky : new String[] {"123:4", "^123:4", "123:4$", "12345:5[12][34]"}) {

Is something like
community lst members 123:4 a case we will warn on? This will not be parsed as a regular expression community by Batfish, right?

(It's fine for this code to mark this as risky)

[millstein]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @anothermattbrown and @dhalperi)

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 27 at r1 (raw file):

Previously, nickgian (Nick Giannarakis) wrote…

I tried looking here but couldn't figure it out; is the meaning of ^^ the usual ^?

No, ^ is treated as just a regular character by the automaton library. This took me a bit to understand as well and is worth a comment. Dan is using two of them so that he can do the check that he wants to do. In particular this allows him to distinguish ^30: from 30: because after intersecting with the automaton for valid communities, they will effectively become ^^30: and ^.*30: [not exactly for that second one, but close enough], so the difference is clear. If you didn't have the double carets then they would end up being the same automaton after the intersection.

By the way we already use double carets elsewhere, in SymbolicAsPathRegex, for a somewhat related reason.

[millstein]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @anothermattbrown and @dhalperi)

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 22 at r1 (raw file):

Previously, millstein (Todd Millstein) wrote…

Add a comment. Also the name sounds like it will return a boolean so consider renaming.

(Though I see why an option is like a boolean, so that's ok if you keep the return type as is. But see later comment on that point.)

[millstein]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @anothermattbrown and @dhalperi)

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 27 at r1 (raw file):

Previously, millstein (Todd Millstein) wrote…

No, ^ is treated as just a regular character by the automaton library. This took me a bit to understand as well and is worth a comment. Dan is using two of them so that he can do the check that he wants to do. In particular this allows him to distinguish ^30: from 30: because after intersecting with the automaton for valid communities, they will effectively become ^^30: and ^.*30: [not exactly for that second one, but close enough], so the difference is clear. If you didn't have the double carets then they would end up being the same automaton after the intersection.

By the way we already use double carets elsewhere, in SymbolicAsPathRegex, for a somewhat related reason.

Whoops the last regex above should be ^^.*30:

[dhalperi]

Reviewable status: 2 of 8 files reviewed, 5 unresolved discussions (waiting on @anothermattbrown, @millstein, and @nickgian)

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 18 at r1 (raw file):

Previously, nickgian (Nick Giannarakis) wrote…

Since you are at it, would be useful to add a comment to this one too

Done.

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 22 at r1 (raw file):

Previously, millstein (Todd Millstein) wrote…

(Though I see why an option is like a boolean, so that's ok if you keep the return type as is. But see later comment on that point.)

Yeah moved it to List.

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 24 at r1 (raw file):

Previously, millstein (Todd Millstein) wrote…

This string is also defined in CommunityVar so maybe it can be shared?

That's more or less a coincidence in that this function is specific to how Juniper processes regexes and is "before" we get into VI land. Don't want to couple them as I expect them to diverge when we get deeper into Junos bugs.

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 25 at r1 (raw file):

Previously, millstein (Todd Millstein) wrote…

Why this replacement? I see that below you are intersecting (the automaton for) this regex with one that represents all (and only) valid communities, so won't that intersection have this same effect (and handle any other similar kinds of things)?

We have to do this change because the automaton library doesn't support ^$ correctly. Letting it stay . introduces collisions with the ^$. This adds false positives because the user's regex then appears to permit shorter-than-5-digit regexes. Consider that .....:..... as the input could be treated as ^1234:2345$.

See updated comment.

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 27 at r1 (raw file):

Previously, millstein (Todd Millstein) wrote…

Whoops the last regex above should be ^^.*30:

What Todd said.

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 41 at r1 (raw file):

Previously, nickgian (Nick Giannarakis) wrote…

you can pull this out in another method since it is the same code as below.

eh. It's the same amount of code, but yeah if I end up doing more and more (probably tackle extended communities next) I'm sure we'll refactor.

For now, don't want to over-perfect this code.

---

projects/batfish/src/main/java/org/batfish/representation/juniper/RegexCommunityMember.java line 50 at r1 (raw file):

Previously, nickgian (Nick Giannarakis) wrote…

+1, especially if we end up with other types of violations too.

This is not meant to be a super generic reusable function. It's specific to Junos semantics and kinda this use case. Even worse, if we have many types of violations the caller wouldn't know how to treat them differently -- so we'd have to change the type again.

That said, it made testing easier to return the list, so I did that. But I don't expect it to become reusable.

---

projects/batfish/src/test/java/org/batfish/representation/juniper/RegexCommunityMemberTest.java line 21 at r1 (raw file):

Previously, nickgian (Nick Giannarakis) wrote…

Is something like
community lst members 123:4 a case we will warn on? This will not be parsed as a regular expression community by Batfish, right?

(It's fine for this code to mark this as risky)

This function will warn on that, but you're right that it should never be called with that argument.

[nickgian]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @anothermattbrown and @millstein)

[dhalperi]

Dismissed @millstein and @nickgian from 5 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @anothermattbrown)