Skip to content

Commit 097b889

Browse files
jonesshajonessha
authored
CIDR bug fixes tied to matching patterns (#131)
There were two bugs with CIDR: When an IP pattern was present and the incoming value was also an IP, we would convert the IP to a hex string before performing matching. Problem with this was that if there was also a rule present that used anything-but on this same field, the anything-but would always be satisfied, since the anything-but does not specify our internal hex format. When both a CIDR pattern and a numeric pattern were present and the incoming value was an IP, we would attempt to convert to a numeric value first, which would fail, and cause us to skip over CIDR matching and go straight to String matching. * Fixing two bugs with CIDR matching * Fixing indentation
1 parent 41572f6 commit 097b889

4 files changed

Lines changed: 222 additions & 24 deletions

File tree

‎pom.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
<groupId>software.amazon.event.ruler</groupId>
2121
<artifactId>event-ruler</artifactId>
2222
<name>Event Ruler</name>
23-
<version>1.6.0</version>
23+
<version>1.7.0</version>
2424
<description>Event Ruler is a Java library that allows matching Rules to Events. An event is a list of fields,
2525
which may be given as name/value pairs or as a JSON object. A rule associates event field names with lists of
2626
possible values. There are two reasons to use Ruler: 1/ It's fast; the time it takes to match Events doesn't

‎src/main/software/amazon/event/ruler/ByteMachine.java‎

Lines changed: 40 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -88,33 +88,41 @@ class ByteMachine {
8888
// Multiple different next-namestate steps can result from processing a single field value, for example
8989
// "foot" matches "foot" exactly, "foo" as a prefix, and "hand" as an anything-but. So, this
9090
// method returns a list.
91-
Set<NameStateWithPattern> transitionOn(String valString) {
91+
Set<NameStateWithPattern> transitionOn(final String valString) {
9292

9393
// not thread-safe, but this is only used in the scope of this method on one thread
9494
final Set<NameStateWithPattern> transitionTo = new HashSet<>();
95-
boolean fieldValueIsNumeric = false;
96-
if (hasNumeric.get() > 0) {
97-
try {
98-
final double numerically = JavaDoubleParser.parseDouble(valString);
99-
valString = ComparableNumber.generate(numerically);
100-
fieldValueIsNumeric = true;
101-
} catch (Exception e) {
102-
// no-op, couldn't treat this as a sensible number
103-
}
104-
} else if (hasIP.get() > 0) {
95+
96+
// Do CIDR matching if there is at least one IP pattern, then move on to NUMERIC or STRING matching below.
97+
if (hasIP.get() > 0) {
10598
try {
106-
// we'll try both the quoted and unquoted version to help people whose events aren't
107-
// in JSON
99+
// we'll try both the quoted and unquoted version to help people whose events aren't in JSON
100+
String ipString;
108101
if (valString.startsWith("\"") && valString.endsWith("\"")) {
109-
valString = CIDR.ipToString(valString.substring(1, valString.length() -1));
102+
ipString = CIDR.ipToString(valString.substring(1, valString.length() -1));
110103
} else {
111-
valString = CIDR.ipToString(valString);
104+
ipString = CIDR.ipToString(valString);
112105
}
106+
doTransitionOn(ipString, transitionTo, TransitionValueType.CIDR);
113107
} catch (IllegalArgumentException e) {
114108
// no-op, couldn't treat this as an IP address
115109
}
116110
}
117-
doTransitionOn(valString, transitionTo, fieldValueIsNumeric);
111+
112+
// Do just one of NUMERIC or STRING matching. Do NUMERIC if machine has numeric patterns and provided value
113+
// is a number. Otherwise, do STRING. We'll still get the correct behavior even if there are both numeric and
114+
// string patterns present as no value can satisfy both a numeric and a string pattern. This is due to string
115+
// patterns starting/ending with a double quotation, where as numeric patterns never do.
116+
if (hasNumeric.get() > 0) {
117+
try {
118+
final double numerically = JavaDoubleParser.parseDouble(valString);
119+
doTransitionOn(ComparableNumber.generate(numerically), transitionTo, TransitionValueType.NUMERIC);
120+
return transitionTo;
121+
} catch (Exception e) {
122+
// no-op, couldn't treat this as a sensible number
123+
}
124+
}
125+
doTransitionOn(valString, transitionTo, TransitionValueType.STRING);
118126
return transitionTo;
119127
}
120128

@@ -454,7 +462,7 @@ private void checkAndDeleteStateAlongTraversedPath(
454462
}
455463

456464
private void doTransitionOn(final String valString, final Set<NameStateWithPattern> transitionTo,
457-
boolean fieldValueIsNumeric) {
465+
TransitionValueType valueType) {
458466
final Map<NameState, List<Patterns>> failedAnythingButs = new HashMap<>();
459467
final byte[] val = valString.getBytes(StandardCharsets.UTF_8);
460468

@@ -490,7 +498,7 @@ private void doTransitionOn(final String valString, final Set<NameStateWithPatte
490498
break;
491499
case NUMERIC_EQ:
492500
// only matches at last character
493-
if (fieldValueIsNumeric && valIndex == (val.length - 1)) {
501+
if (valueType == TransitionValueType.NUMERIC && valIndex == (val.length - 1)) {
494502
transitionTo.add(new NameStateWithPattern(match.getNextNameState(), match.getPattern()));
495503
}
496504
break;
@@ -510,15 +518,17 @@ private void doTransitionOn(final String valString, final Set<NameStateWithPatte
510518
case NUMERIC_RANGE:
511519
// as soon as you see the match, you've matched
512520
Range range = (Range) match.getPattern();
513-
if ((fieldValueIsNumeric && !range.isCIDR) || (!fieldValueIsNumeric && range.isCIDR)) {
521+
if ((valueType == TransitionValueType.NUMERIC && !range.isCIDR) ||
522+
(valueType != TransitionValueType.NUMERIC && range.isCIDR)) {
514523
transitionTo.add(new NameStateWithPattern(match.getNextNameState(), match.getPattern()));
515524
}
516525
break;
517526

518527
case ANYTHING_BUT:
519528
AnythingBut anythingBut = (AnythingBut) match.getPattern();
520529
// only applies if at last character
521-
if (valIndex == (val.length - 1) && anythingBut.isNumeric() == fieldValueIsNumeric) {
530+
if (valIndex == (val.length - 1) &&
531+
anythingBut.isNumeric() == (valueType == TransitionValueType.NUMERIC)) {
522532
addToAnythingButsMap(failedAnythingButs, match.getNextNameState(), match.getPattern());
523533
}
524534
break;
@@ -546,8 +556,9 @@ private void doTransitionOn(final String valString, final Set<NameStateWithPatte
546556
}
547557

548558
// This may look like premature optimization, but the first "if" here yields roughly 10x performance
549-
// improvement.
550-
if (!anythingButs.isEmpty()) {
559+
// improvement. We exclude CIDR because the value will have been transformed, causing the anythingBut to always
560+
// match. Wait for NUMERIC or STRING matching to harvest anythingBut matches.
561+
if (!anythingButs.isEmpty() && valueType != TransitionValueType.CIDR) {
551562
for (Map.Entry<NameState, List<Patterns>> entry : anythingButs.entrySet()) {
552563
boolean failedAnythingButsContainsKey = failedAnythingButs.containsKey(entry.getKey());
553564
for (Patterns pattern : entry.getValue()) {
@@ -1835,7 +1846,7 @@ private static ByteTransition getTransitionForMultiBytes(SingleByteTransition tr
18351846
return coalesce(candidates);
18361847
}
18371848

1838-
private static void addTransitionNextState(ByteState state, InputCharacter character, InputCharacter[] characters,
1849+
private void addTransitionNextState(ByteState state, InputCharacter character, InputCharacter[] characters,
18391850
int currentIndex, ByteState prevState, Patterns pattern,
18401851
ByteState nextState, NameState nameState) {
18411852
if (isWildcard(character)) {
@@ -2088,4 +2099,10 @@ public String toString() {
20882099
", anythingButs=" + anythingButs +
20892100
'}';
20902101
}
2102+
2103+
enum TransitionValueType {
2104+
NUMERIC,
2105+
CIDR,
2106+
STRING
2107+
};
20912108
}

‎src/test/software/amazon/event/ruler/ACMachineTest.java‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2308,4 +2308,95 @@ public void testInitialSharedNameStateAlreadyExistsWithNonLeadingValue() throws
23082308
assertEquals(1, matches.size());
23092309
assertTrue(matches.contains("rule2"));
23102310
}
2311+
2312+
@Test
2313+
public void testCIDRRuleWithMatchingAnythingButRule() throws Exception {
2314+
String rule1 = "{\"ip\": [{\"anything-but\": \"10.0.1.200\"}]}";
2315+
String rule2 = "{\"ip\": [\"10.0.1.200\"]}";
2316+
2317+
Machine machine = new Machine();
2318+
machine.addRule("rule1", rule1);
2319+
machine.addRule("rule2", rule2);
2320+
2321+
String event = "{" +
2322+
"\"ip\": \"10.0.1.200\"" +
2323+
"}";
2324+
2325+
List<String> matches = machine.rulesForJSONEvent(event);
2326+
assertEquals(1, matches.size());
2327+
assertTrue(matches.contains("rule2"));
2328+
}
2329+
2330+
@Test
2331+
public void testCIDRRuleWithMatchingAnythingButPrefixRule() throws Exception {
2332+
String rule1 = "{\"ip\": [{\"anything-but\": {\"prefix\": \"10.0.\"}}]}";
2333+
String rule2 = "{\"ip\": [\"10.0.1.200\"]}";
2334+
2335+
Machine machine = new Machine();
2336+
machine.addRule("rule1", rule1);
2337+
machine.addRule("rule2", rule2);
2338+
2339+
String event = "{" +
2340+
"\"ip\": \"10.0.1.200\"" +
2341+
"}";
2342+
2343+
List<String> matches = machine.rulesForJSONEvent(event);
2344+
assertEquals(1, matches.size());
2345+
assertTrue(matches.contains("rule2"));
2346+
}
2347+
2348+
@Test
2349+
public void testCIDRRuleWithMatchingAnythingButSuffixRule() throws Exception {
2350+
String rule1 = "{\"ip\": [{\"anything-but\": {\"suffix\": \"1.200\"}}]}";
2351+
String rule2 = "{\"ip\": [\"10.0.1.200\"]}";
2352+
2353+
Machine machine = new Machine();
2354+
machine.addRule("rule1", rule1);
2355+
machine.addRule("rule2", rule2);
2356+
2357+
String event = "{" +
2358+
"\"ip\": \"10.0.1.200\"" +
2359+
"}";
2360+
2361+
List<String> matches = machine.rulesForJSONEvent(event);
2362+
assertEquals(1, matches.size());
2363+
assertTrue(matches.contains("rule2"));
2364+
}
2365+
2366+
@Test
2367+
public void testCIDRRuleWithMatchingAnythingButEqualsIgnoreCaseRule() throws Exception {
2368+
String rule1 = "{\"ip\": [{\"anything-but\": {\"equals-ignore-case\": \"10.0.1.200\"}}]}";
2369+
String rule2 = "{\"ip\": [\"10.0.1.200\"]}";
2370+
2371+
Machine machine = new Machine();
2372+
machine.addRule("rule1", rule1);
2373+
machine.addRule("rule2", rule2);
2374+
2375+
String event = "{" +
2376+
"\"ip\": \"10.0.1.200\"" +
2377+
"}";
2378+
2379+
List<String> matches = machine.rulesForJSONEvent(event);
2380+
assertEquals(1, matches.size());
2381+
assertTrue(matches.contains("rule2"));
2382+
}
2383+
2384+
@Test
2385+
public void testCIDRRuleWithNumericRule() throws Exception {
2386+
String rule1 = "{\"ip\": [{\"numeric\": [\">\", 0, \"<=\", 5]}]}";
2387+
String rule2 = "{\"ip\": [\"10.0.1.200\"]}";
2388+
2389+
Machine machine = new Machine();
2390+
machine.addRule("rule1", rule1);
2391+
machine.addRule("rule2", rule2);
2392+
2393+
String event = "{" +
2394+
"\"ip\": \"10.0.1.200\"" +
2395+
"}";
2396+
2397+
List<String> matches = machine.rulesForJSONEvent(event);
2398+
assertEquals(1, matches.size());
2399+
assertTrue(matches.contains("rule2"));
2400+
}
2401+
23112402
}

‎src/test/software/amazon/event/ruler/MachineTest.java‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2168,6 +2168,96 @@ public void testInitialSharedNameStateAlreadyExistsWithNonLeadingValue() throws
21682168
assertTrue(matches.contains("rule2"));
21692169
}
21702170

2171+
@Test
2172+
public void testCIDRRuleWithMatchingAnythingButRule() throws Exception {
2173+
String rule1 = "{\"ip\": [{\"anything-but\": \"10.0.1.200\"}]}";
2174+
String rule2 = "{\"ip\": [\"10.0.1.200\"]}";
2175+
2176+
Machine machine = new Machine();
2177+
machine.addRule("rule1", rule1);
2178+
machine.addRule("rule2", rule2);
2179+
2180+
String event = "{" +
2181+
"\"ip\": \"10.0.1.200\"" +
2182+
"}";
2183+
2184+
List<String> matches = machine.rulesForEvent(event);
2185+
assertEquals(1, matches.size());
2186+
assertTrue(matches.contains("rule2"));
2187+
}
2188+
2189+
@Test
2190+
public void testCIDRRuleWithMatchingAnythingButPrefixRule() throws Exception {
2191+
String rule1 = "{\"ip\": [{\"anything-but\": {\"prefix\": \"10.0.\"}}]}";
2192+
String rule2 = "{\"ip\": [\"10.0.1.200\"]}";
2193+
2194+
Machine machine = new Machine();
2195+
machine.addRule("rule1", rule1);
2196+
machine.addRule("rule2", rule2);
2197+
2198+
String event = "{" +
2199+
"\"ip\": \"10.0.1.200\"" +
2200+
"}";
2201+
2202+
List<String> matches = machine.rulesForEvent(event);
2203+
assertEquals(1, matches.size());
2204+
assertTrue(matches.contains("rule2"));
2205+
}
2206+
2207+
@Test
2208+
public void testCIDRRuleWithMatchingAnythingButSuffixRule() throws Exception {
2209+
String rule1 = "{\"ip\": [{\"anything-but\": {\"suffix\": \"1.200\"}}]}";
2210+
String rule2 = "{\"ip\": [\"10.0.1.200\"]}";
2211+
2212+
Machine machine = new Machine();
2213+
machine.addRule("rule1", rule1);
2214+
machine.addRule("rule2", rule2);
2215+
2216+
String event = "{" +
2217+
"\"ip\": \"10.0.1.200\"" +
2218+
"}";
2219+
2220+
List<String> matches = machine.rulesForEvent(event);
2221+
assertEquals(1, matches.size());
2222+
assertTrue(matches.contains("rule2"));
2223+
}
2224+
2225+
@Test
2226+
public void testCIDRRuleWithMatchingAnythingButEqualsIgnoreCaseRule() throws Exception {
2227+
String rule1 = "{\"ip\": [{\"anything-but\": {\"equals-ignore-case\": \"10.0.1.200\"}}]}";
2228+
String rule2 = "{\"ip\": [\"10.0.1.200\"]}";
2229+
2230+
Machine machine = new Machine();
2231+
machine.addRule("rule1", rule1);
2232+
machine.addRule("rule2", rule2);
2233+
2234+
String event = "{" +
2235+
"\"ip\": \"10.0.1.200\"" +
2236+
"}";
2237+
2238+
List<String> matches = machine.rulesForEvent(event);
2239+
assertEquals(1, matches.size());
2240+
assertTrue(matches.contains("rule2"));
2241+
}
2242+
2243+
@Test
2244+
public void testCIDRRuleWithNumericRule() throws Exception {
2245+
String rule1 = "{\"ip\": [{\"numeric\": [\">\", 0, \"<=\", 5]}]}";
2246+
String rule2 = "{\"ip\": [\"10.0.1.200\"]}";
2247+
2248+
Machine machine = new Machine();
2249+
machine.addRule("rule1", rule1);
2250+
machine.addRule("rule2", rule2);
2251+
2252+
String event = "{" +
2253+
"\"ip\": \"10.0.1.200\"" +
2254+
"}";
2255+
2256+
List<String> matches = machine.rulesForEvent(event);
2257+
assertEquals(1, matches.size());
2258+
assertTrue(matches.contains("rule2"));
2259+
}
2260+
21712261
@Test
21722262
public void testApproxSizeForSimplestPossibleMachine() throws Exception {
21732263
String rule1 = "{ \"a\" : [ 1 ] }";

0 commit comments

Comments
 (0)