ROA Overlapping INVALID ROA and VALID ROA fix (#1470)

* feat: early return if not found or valid and Unit tests

* feat: order

* feat: add a sort for deterministic changes

* feat: remove unused import

* feat: add IT

* feat: rename

* feat: clarify the logic

* feat: return nullable in a method that can return null

* feat: rename class and add a comment

whois-api/src/main/java/net/ripe/db/whois/api/rest/search/RpkiRoaMessageGenerator.java

@@ -13,6 +13,7 @@
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.stereotype.Component;
 
+import javax.annotation.Nullable;
 import java.util.Map;
 
 
@@ -47,6 +48,7 @@ private boolean canProceed(final Parameters parameters){
         return isEnabled && (parameters.getRoaCheck()!= null && parameters.getRoaCheck());
     }
 
+    @Nullable
     private RpslMessage validateRoa(final RpslObject rpslObject){
         final Map.Entry<Roa, ValidationStatus> invalidRpkiRoa = whoisRoaChecker.validateAndGetInvalidRoa(rpslObject);
 
@@ -57,7 +59,8 @@ private RpslMessage validateRoa(final RpslObject rpslObject){
         return switch (invalidRpkiRoa.getValue()) {
             case INVALID_ORIGIN -> new RpslMessage(QueryMessages.roaRouteOriginConflicts(rpslObject.getType().getName(), invalidRpkiRoa.getKey().getMaxLength(), invalidRpkiRoa.getKey().getAsn()));
             case INVALID_PREFIX_LENGTH -> new RpslMessage(QueryMessages.roaRoutePrefixLengthConflicts(rpslObject.getType().getName(), invalidRpkiRoa.getKey().getMaxLength(), invalidRpkiRoa.getKey().getAsn()));
-            default -> new RpslMessage(QueryMessages.roaRouteConflicts(rpslObject.getType().getName(), invalidRpkiRoa.getKey().getMaxLength(), invalidRpkiRoa.getKey().getAsn()));
+            case INVALID_PREFIX_AND_ORIGIN -> new RpslMessage(QueryMessages.roaRouteConflicts(rpslObject.getType().getName(), invalidRpkiRoa.getKey().getMaxLength(), invalidRpkiRoa.getKey().getAsn()));
+            default -> null;
         };
     }
 }

whois-api/src/test/java/net/ripe/db/whois/api/rest/WhoisSearchServiceTestIntegration.java

@@ -2007,6 +2007,27 @@ public void search_route_roa_origin_and_prefix_length_mismatch_validation_enable
                 "either remove this route: object or update or delete the ROA.\n"));
     }
 
+    @Test
+    public void search_route_one_invalid_one_valid_roas_makes_it_valid() {
+        databaseHelper.addObject(RpslObject.parse("" +
+                "route:           92.38.45.0/24\n" +
+                "descr:           Ripe test allocation\n" +
+                "origin:          AS61979\n" +
+                "admin-c:         TP1-TEST\n" +
+                "mnt-by:          OWNER-MNT\n" +
+                "mnt-lower:       OWNER-MNT\n" +
+                "source:          TEST-NONAUTH\n"));
+        ipTreeUpdater.rebuild();
+
+        final WhoisResources whoisResources = RestTest.target(getPort(), "whois/search.xml?query-string=92.38.45.0/24AS61979&roa-check=true&flags=no-referenced")
+                .request()
+                .get(WhoisResources.class);
+
+        assertThat(whoisResources.getWhoisObjects(), hasSize(1));
+
+        assertThat(whoisResources.getWhoisObjects().get(0).getObjectMessages().getMessages(), hasSize(0));
+    }
+
     @Test
     public void search_route6_roa_mismatch_less_specific_as_xml_strings() {
         databaseHelper.addObject(RpslObject.parse("" +

whois-commons/src/main/java/net/ripe/db/whois/common/rpki/RpkiRoaChecker.java → whois-commons/src/main/java/net/ripe/db/whois/common/rpki/AbstractRpkiRoaChecker.java

@@ -17,11 +17,11 @@
 import static net.ripe.db.whois.common.rpki.ValidationStatus.VALID;
 import static net.ripe.db.whois.common.rpsl.ObjectType.ROUTE;
 
-public abstract class RpkiRoaChecker {
+public abstract class AbstractRpkiRoaChecker {
 
     private final RpkiService rpkiService;
 
-    public RpkiRoaChecker(final RpkiService rpkiService) {
+    public AbstractRpkiRoaChecker(final RpkiService rpkiService) {
         this.rpkiService = rpkiService;
     }
 

whois-commons/src/main/java/net/ripe/db/whois/common/rpki/ValidationStatus.java

@@ -6,5 +6,6 @@ public enum ValidationStatus {
     VALID,
     INVALID,
     INVALID_ORIGIN,
-    INVALID_PREFIX_LENGTH
+    INVALID_PREFIX_LENGTH,
+    INVALID_PREFIX_AND_ORIGIN
 }

whois-commons/src/main/java/net/ripe/db/whois/common/rpki/WhoisRoaChecker.java

@@ -11,45 +11,63 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.function.Predicate;
+import java.util.stream.Stream;
 
-import static net.ripe.db.whois.common.rpki.ValidationStatus.INVALID;
 import static net.ripe.db.whois.common.rpki.ValidationStatus.INVALID_ORIGIN;
+import static net.ripe.db.whois.common.rpki.ValidationStatus.INVALID_PREFIX_AND_ORIGIN;
 import static net.ripe.db.whois.common.rpki.ValidationStatus.INVALID_PREFIX_LENGTH;
+import static net.ripe.db.whois.common.rpki.ValidationStatus.NOT_FOUND;
 import static net.ripe.db.whois.common.rpki.ValidationStatus.VALID;
 
 @Component
-public class WhoisRoaChecker extends RpkiRoaChecker {
+public class WhoisRoaChecker extends AbstractRpkiRoaChecker {
     public WhoisRoaChecker(final RpkiService rpkiService) {
         super(rpkiService);
     }
 
     public Map.Entry<Roa, ValidationStatus> validateAndGetInvalidRoa(final RpslObject route){
-        final Optional<Map.Entry<Roa, ValidationStatus>> roaStatusMap = validateRoas(route)
+        /* This method prioritize VALID roas over INVALID roas. So in case of overlap the VALID ROA will se used.
+         This is a common behaviour related to roas */
+        final Map<Roa, ValidationStatus> roasStatus = validateRoas(route);
+        final Optional<Map.Entry<Roa, ValidationStatus>> roaStatusMap = roasStatus
                 .entrySet()
                 .stream()
-                .filter(entry -> INVALID.equals(entry.getValue()) || INVALID_PREFIX_LENGTH.equals(entry.getValue()) || INVALID_ORIGIN.equals(entry.getValue()))
-                .findFirst();
+                .filter(getValidOrNotFoundRoas()).findFirst()
+                .or(() -> getInvalidRoas(roasStatus).findFirst());
 
         if (roaStatusMap.isEmpty()){
             return null;
         }
         return roaStatusMap.get();
     }
+
     @Override
     protected ValidationStatus validate(final RpslObject route, final Roa roa, final IpInterval<?> prefix) {
         final List<ValidationStatus> invalidStatus = Lists.newArrayList();
-        final long nonAuthAsn = Asn.parse(route.getValueForAttribute(AttributeType.ORIGIN).toString()).asBigInteger().longValue();
+        final long routeAsn = Asn.parse(route.getValueForAttribute(AttributeType.ORIGIN).toString()).asBigInteger().longValue();
         if (prefix.getPrefixLength() > roa.getMaxLength()){
             invalidStatus.add(INVALID_PREFIX_LENGTH);
         }
 
-        if (nonAuthAsn != 0 && nonAuthAsn != roa.getAsn()){
+        if (routeAsn == 0 || routeAsn != roa.getAsn()){
             invalidStatus.add(INVALID_ORIGIN);
         }
 
         if (invalidStatus.isEmpty()){
             return VALID;
         }
-        return invalidStatus.size() == 1 ? invalidStatus.get(0) : INVALID;
+        return invalidStatus.size() == 1 ? invalidStatus.get(0) : INVALID_PREFIX_AND_ORIGIN;
+    }
+
+    private Predicate<Map.Entry<Roa, ValidationStatus>> getValidOrNotFoundRoas() {
+        return entry -> NOT_FOUND.equals(entry.getValue()) || VALID.equals(entry.getValue());
+    }
+
+    private Stream<Map.Entry<Roa, ValidationStatus>> getInvalidRoas(Map<Roa, ValidationStatus> roasStatus) {
+        return roasStatus.entrySet()
+                .stream()
+                .filter(other -> INVALID_PREFIX_AND_ORIGIN.equals(other.getValue()) || INVALID_PREFIX_LENGTH.equals(other.getValue()) || INVALID_ORIGIN.equals(other.getValue()))
+                .sorted((o1, o2) -> o1.getKey().getPrefix().compareTo(o2.getKey().getPrefix()));
     }
 }

whois-commons/src/test/java/net/ripe/db/whois/common/rpki/WhoisRoaCheckerTest.java

@@ -0,0 +1,131 @@
+package net.ripe.db.whois.common.rpki;
+
+import com.google.common.collect.Sets;
+import net.ripe.db.whois.common.rpsl.RpslObject;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+import java.util.Map;
+
+import static net.ripe.db.whois.common.rpki.ValidationStatus.VALID;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.when;
+
+@ExtendWith(MockitoExtension.class)
+public class WhoisRoaCheckerTest {
+
+    @Mock
+    RpkiService rpkiService;
+
+    @InjectMocks
+    WhoisRoaChecker whoisRoaChecker;
+
+    @Test
+    public void exact_match_valid() {
+        when(rpkiService.findRoas(eq("10.0.0.0/8"))).thenReturn(Sets.newHashSet(new Roa("AS1", 8, "10.0.0.0/8", "ripe")));
+        final Map.Entry<Roa, ValidationStatus> result = whoisRoaChecker.validateAndGetInvalidRoa(RpslObject.parse(
+                "route: 10.0.0.0/8\n" +
+                        "origin: AS1"
+        ));
+        assertThat(result.getValue(), is(ValidationStatus.VALID));
+    }
+
+    @Test
+    public void different_origin_invalid() {
+        when(rpkiService.findRoas(eq("10.0.0.0/8"))).thenReturn(Sets.newHashSet(new Roa("AS1", 8, "10.0.0.0/8", "ripe")));
+        final Map.Entry<Roa, ValidationStatus> result = whoisRoaChecker.validateAndGetInvalidRoa(RpslObject.parse(
+                "route: 10.0.0.0/8\n" +
+                        "origin: AS2"
+        ));
+        assertThat(result.getValue(), is(ValidationStatus.INVALID_ORIGIN));
+    }
+
+    @Test
+    public void asn0_invalid() {
+        when(rpkiService.findRoas(eq("10.0.0.0/8"))).thenReturn(Sets.newHashSet(new Roa("AS1", 8, "10.0.0.0/8", "ripe")));
+        final Map.Entry<Roa, ValidationStatus> result = whoisRoaChecker.validateAndGetInvalidRoa(RpslObject.parse(
+                "route: 10.0.0.0/8\n" +
+                        "origin: AS0"
+        ));
+        assertThat(result.getValue(), is(ValidationStatus.INVALID_ORIGIN));
+    }
+
+    @Test
+    public void more_specific_roa_prefix_lower_length_invalid() {
+        when(rpkiService.findRoas(eq("10.0.0.0/16"))).thenReturn(Sets.newHashSet(new Roa("AS1", 8, "10.0.0.0/8", "ripe")));
+        final Map.Entry<Roa, ValidationStatus> result = whoisRoaChecker.validateAndGetInvalidRoa(RpslObject.parse(
+                "route: 10.0.0.0/16\n" +
+                        "origin: AS1"
+        ));
+        assertThat(result.getValue(), is(ValidationStatus.INVALID_PREFIX_LENGTH));
+    }
+
+    @Test
+    public void more_specific_roa_allowed_by_same_length_valid() {
+        when(rpkiService.findRoas(eq("10.0.0.0/16"))).thenReturn(Sets.newHashSet(new Roa("AS1", 16, "10.0.0.0/8", "ripe")));
+        final Map.Entry<Roa, ValidationStatus> result = whoisRoaChecker.validateAndGetInvalidRoa(RpslObject.parse(
+                "route: 10.0.0.0/16\n" +
+                        "origin: AS1"
+        ));
+        assertThat(result.getValue(), is(VALID));
+    }
+
+    @Test
+    public void more_specific_roa_allowed_by_max_length_valid() {
+        when(rpkiService.findRoas(eq("10.0.0.0/16"))).thenReturn(Sets.newHashSet(new Roa("AS1", 24, "10.0.0.0/8", "ripe")));
+        final Map.Entry<Roa, ValidationStatus> result = whoisRoaChecker.validateAndGetInvalidRoa(RpslObject.parse(
+                "route: 10.0.0.0/16\n" +
+                        "origin: AS1"
+        ));
+        assertThat(result.getValue(), is(VALID));
+    }
+
+    @Test
+    public void more_specific_roa_prefix_and_different_origin_invalid() {
+        when(rpkiService.findRoas(eq("10.0.0.0/16"))).thenReturn(Sets.newHashSet(new Roa("AS1", 8, "10.0.0.0/8", "ripe")));
+        final Map.Entry<Roa, ValidationStatus> result = whoisRoaChecker.validateAndGetInvalidRoa(RpslObject.parse(
+                "route: 10.0.0.0/16\n" +
+                        "origin: AS2"
+        ));
+        assertThat(result.getValue(), is(ValidationStatus.INVALID_PREFIX_AND_ORIGIN));
+    }
+
+    @Test
+    public void less_specific_roa_prefix_valid() {
+        when(rpkiService.findRoas(eq("10.0.0.0/8"))).thenReturn(Sets.newHashSet(new Roa("AS1", 8, "10.0.0.0/16", "ripe")));
+        final Map.Entry<Roa, ValidationStatus> result = whoisRoaChecker.validateAndGetInvalidRoa(RpslObject.parse(
+                "route: 10.0.0.0/8\n" +
+                        "origin: AS1"
+        ));
+        assertThat(result.getValue(), is(ValidationStatus.VALID));
+    }
+
+    @Test
+    public void overlap_valid_invalid_roa_then_valid() {
+        final Roa nonMatchingRoa = new Roa("AS44546", 24, "92.38.0.0/17", "ripe");
+        final Roa matchingRoa = new Roa("AS61979", 24, "92.38.44.0/22", "ripe");
+        when(rpkiService.findRoas(eq("92.38.45.0/24"))).thenReturn(Sets.newHashSet(nonMatchingRoa, matchingRoa));
+        final Map.Entry<Roa, ValidationStatus> result = whoisRoaChecker.validateAndGetInvalidRoa(RpslObject.parse(
+                "route: 92.38.45.0/24\n" +
+                        "origin: AS61979"
+        ));
+        assertThat(result.getValue(), is(ValidationStatus.VALID));
+    }
+
+    @Test
+    public void overlap_two_invalid_roas_then_invalid() {
+        final Roa nonMatchingRoa = new Roa("AS44546", 24, "92.38.0.0/17", "ripe");
+        final Roa matchingRoa = new Roa("AS61979", 8, "92.38.44.0/22", "ripe");
+        when(rpkiService.findRoas(eq("92.38.45.0/24"))).thenReturn(Sets.newHashSet(nonMatchingRoa, matchingRoa));
+        final Map.Entry<Roa, ValidationStatus> result = whoisRoaChecker.validateAndGetInvalidRoa(RpslObject.parse(
+                "route: 92.38.45.0/24\n" +
+                        "origin: AS61979"
+        ));
+        assertThat(result.getValue(), is(ValidationStatus.INVALID_ORIGIN));
+    }
+}

whois-commons/src/test/resources/rpki/roas.json

@@ -26,6 +26,8 @@
     { "asn": "AS1964", "prefix": "206.48.0.0/16", "maxLength": 16, "ta": "arin" },
     { "asn": "AS6505", "prefix": "193.4.0.0/16", "maxLength": 16, "ta": "arin" },
     { "asn": "AS7505", "prefix": "200.4.0.0/23", "maxLength": 23, "ta": "arin" },
-    { "asn": "AS6505", "prefix": "176.223.250.0/23", "maxLength": 23, "ta": "ripe" }
+    { "asn": "AS6505", "prefix": "176.223.250.0/23", "maxLength": 23, "ta": "ripe" },
+    { "asn": "AS44546", "prefix": "92.38.0.0/17", "maxLength": 24, "ta": "arin" },
+    { "asn": "AS61979", "prefix": "92.38.44.0/22", "maxLength": 24, "ta": "ripe" }
   ]
 }