METRON-363 Fix Cisco ASA Parser #276

kylerichardson · 2016-09-27T01:19:46Z

I've rewritten the ASA parser which can be extended, as needed, to new ASA message types by editing the bundled asa patterns file and the static map used for grok patterns in the code. I've also tried to make it easier to deploy the asa topology by including zookeeper config files and creating the kafka topic during metron install. Sample data is also included for integration testing.

cestella · 2016-09-27T03:19:20Z

Have you run this up in vagrant yet?

danieljue · 2016-09-27T19:02:59Z

FYI the PR for METRON-451 is failing at the same place.

kylerichardson · 2016-09-28T01:01:03Z

I've tested in a slimmed down single node vm (no sensors) but not in vagrant. I'm a bit limited in my resources at the moment so would appreciate if someone could validate in quick-dev as a sanity check.

kylerichardson · 2016-09-28T02:05:39Z

Currently my branch doesn't have build_utils. Going to rebase and see if that fixes the CI build.

kylerichardson · 2016-09-28T12:20:53Z

Testing

It occurs to me I haven't outlined how to test or how I tested this code (apologies, this is my first PR).

All my testing was performed on a single node vm (no sensors). This should mimic the quick-dev environment (unfortunately, I haven't had much luck with vagrant due to my primary OS being Windows).

Test Steps

Deploy single node vm using metron_full_install ansible playbook (I can provide my host and group_vars if anyone is interested)
Stop unused parsers
monit stop pcap-parser
monit stop yaf-parser
monit stop bro-parser
monit stop snort-parser
Install elasticsearch head
/usr/share/elasticsearch/bin/plugin install mobz/elasticsearch-head
Start the asa parser topology
start_parser_topology.sh -k node1:6667 -z node1:2181 -s asa
Use the console producer to load raw asa events into kafka
/usr/hdp/current/kafka-broker/bin/kafka-console-producer.sh --broker-list node1:6667 --topic asa < asa_raw.txt
For test data I used the sample data provided for integration testing and raw data collected from one of my devices.
Verify events in elasticsearch
Using the head plugin, I could browse the asa_index_* index and see the enriched events

Future enhancements

I could not add the asa* indexes to kibana. I believe an elasticsearch template is required. I'll be working on that as a future PR.
Minor bug in one of the ansible roles (metron_common). The logic to verify the jars exist is done remotely and should be done locally. I'll submit a separate JIRA and PR for this fix.

nickwallen

Good stuff, Kyle! Very easy to read. Have some suggestions, but job well done.

nickwallen · 2016-09-28T12:42:34Z

metron-platform/metron-integration-test/src/main/sample/data/asa/raw/asa_raw

+<166>Jan  5 08:52:35 10.22.8.216 %ASA-6-302014: Teardown TCP connection 212805593 for outside:10.22.8.223/59614(LOCAL\user.name) to inside:10.22.8.78/8102 duration 0:00:07 bytes 3433 TCP FINs (user.name)
+<174>Jan  5 14:52:35 10.22.8.212 %ASA-6-302013: Built inbound TCP connection 76245503 for outside:10.22.8.233/54209 (10.22.8.233/54209) to inside:198.111.72.238/443 (198.111.72.238/443) (user.name)
+<166>Jan  5 08:52:35 10.22.8.216 %ASA-6-302013: Built inbound TCP connection 212806031 for outside:10.22.8.17/58633 (10.22.8.17/58633)(LOCAL\user.name) to inside:10.22.8.12/389 (10.22.8.12/389) (user.name)
+<142>Jan  5 08:52:35 10.22.8.201 %ASA-6-302014: Teardown TCP connection 488168292 for DMZ-Inside:10.22.8.51/51231 to Inside-Trunk:10.22.8.174/40004 duration 0:00:00 bytes 2103 TCP FINs


Has this data been scrubbed? I just want to make sure that none of it is proprietary.

I took the existing test data found in .../sample/data/SampleInput/AsaOutput and added to it data from some of my test devices. The data I added has been scrubbed/anonymized.

nickwallen · 2016-09-28T12:44:52Z

metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/asa/BasicAsaParser.java

+
+    @Override
+    public void init() {
+        asaGrok = new Grok();


What features would we need to add to the GrokParser to implement ASA parsing with the GrokParser? Would be nice to learn from your experience to enhance the GrokParser.

At the moment, the GrokParser is limited to a single complied pattern that needs to match every incoming message. This works well for logs where the format is always the same (e.g. proxy logs, http access logs); however, falls short for devices with many different message types using different formats. ASAs are a good example of this as are standard Unix/Linux syslog messages.

I'm not sure what the best approach would be to solve this issue, but, ideally the GrokParser would be able to parse disparate message types based on an input pattern file (or files). I played around with the pattern discovery feature of Grok when developing this parser. It works pretty well and could be an option but seemed to slow down overall processing. That's why I ultimately landed on a static map of possible ASA message patterns.

I suppose another way would be to allow the user to specify as part of the configuration (1) a base message pattern (e.g. syslog) which should always match and then (2) an inner message pattern map which finds the best match and returns the results.

What do you think?

Yes, that is a good idea. We can keep that in mind in enhancing the GrokParser.

nickwallen · 2016-09-28T12:57:40Z

...n-platform/metron-parsers/src/main/java/org/apache/metron/parsers/utils/FieldValidators.java

+            return false;
+    }
+
+    public static boolean isValidIpAddr(String ipAddress) {


Does your isValidIpAddr function do something different than org.apache.commons.validator.routines.InetAddressValidator?

Yeah, agreed. I'd just do InetAddressValidator.getInstance().isValidInet6Address(ip) || InetAddressValidator.getInstance().isValidInet4Address(ip) here.

No my isValidIpAddr function is probably more rudimentary than org.apache.commons.validator.routines.InetAddressValidator. I actually switched to using the InetAddressValidator in the latest version of the code, so this is just an unused function that I will remove.

nickwallen · 2016-09-28T13:03:02Z

metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/asa/BasicAsaParser.java

+
+            String src_ip = (String) messageJson.get("src_ip");
+            if (src_ip != null && ipValidator.isValid(src_ip))
+                metronJson.put(Constants.Fields.SRC_ADDR.getName(), src_ip);


What if there is not a valid source IP? Is that OK? Or should we blow-up in that scenario? Same goes for the other core fields; src_port, dst_addr, protocol, etc.

I've been thinking about this one a bit. In the case where there is no source IP (or other core fields) in the original message, there shouldn't be an error as many message types may not contain all or any of these fields. However, in the case where the source IP (or other field) exists but is invalid, there is either a parsing issue (e.g. pattern is incorrect) or the original message is malformed. At that point, I think logging a warning would be appropriate for later followup. Are there other options to explore beyond logging? Is there a way to force that particular message into the parser_invalid or parser_error topics?

It turns out the ipValidator function isn't adding any benefit here. The grok pattern being used on the raw message is already checking that it's a valid IP address (IPv4 or IPv6). Given that I'm going to simply remove that validation from the code as redundant.

nickwallen · 2016-09-28T13:04:52Z

metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/asa/BasicAsaParser.java

+            LOG.trace("[Metron] Final normalized message: " + metronJson.toString());
+
+        } catch (Exception e) {
+            e.printStackTrace();


Probably better to do a LOG.error and grab the e.getMessage() along with other contextual information. Need to make it easy to understand why it blew up.

Agreed. I'll correct it.

nickwallen · 2016-09-28T13:12:07Z

metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/asa/BasicAsaParser.java

+        String syslogPattern = "%{CISCO_TAGGED_SYSLOG}";
+        JSONObject metronJson = new JSONObject();
+        List<JSONObject> messages = new ArrayList<>();
+        try {


It would make sense to break this logic up into two methods each with its own try-catch block. The first method parses the syslog portion. The second parses the syslog 'message' based on the given 'ciscotag'.

In each 'catch' we want to provide as much contextual information about what went wrong as we can. For example, if it throws an exception when parsing the syslog 'message' portion, then the error message should log the 'ciscotag' so we have more information to troubleshoot with.

Breaking this logic up into two methods each with its own try-catch block allows you to provide greater context in each failure scenario.

Agreed. I'll refactor.

nickwallen · 2016-09-28T13:27:03Z

metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/utils/SyslogUtils.java

+import java.time.ZonedDateTime;
+import java.time.format.DateTimeFormatter;
+
+public class SyslogUtils {


Do you have unit tests for these methods? Would be good to add specifically for the SyslogUtils methods.

nickwallen · 2016-09-28T13:30:15Z

metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/utils/SyslogUtils.java

+
+    public static long parseTimestampToEpochMillis(String logTimestamp) {
+        if (logTimestamp.length() < 20) {
+            ZonedDateTime now = ZonedDateTime.now(ZoneOffset.UTC);


Is a Syslog timestamp always UTC? More importantly do ASAs follow the Syslog standard, if so? ;)

Of course you're right, the timestamp will not always be in UTC. ASA logs consumed via syslog (either raw off the wire or through another syslog server) will generally follow the syslog standard.

There are a number of possibilities to explore here. If we assume that we will be collecting the raw syslog from the ASAs off the wire, the timestamp will not include the timezone/offset. This code assumes the device is logging in UTC, which, to your point, is probably a bad assumption. I made this assumption because it seems to me we would want all of the timestamps indexed to be in the same timezone and the easiest way to accomplish that would be to normalize all of the telemetry data to UTC.

Question for the team. How are other parsers handling timezone? Are they passing through the device timezone?

The way I'm thinking of solving this is by adding a configuration option to the parser to specify the device timezone. (This would require that all ASAs put through the parser we configured to the same timezone though.) I would then convert the timestamp to UTC prior to writing it into the metron normalized JSON message.

Any feedback or other ideas on solving this one?

I think your idea will work just fine for now. Allow the user to configure the input timezone and the output timezone should be in UTC.

As part of a future enhancement, maybe we can allow the user to define a map as part of the configuration. This maps the value of some indicator field to a timezone. For example, based on something like %ASA-6-302013 the parser will choose the appropriate input timezone.

nickwallen · 2016-09-28T13:31:14Z

metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/utils/SyslogUtils.java

+            ZonedDateTime now = ZonedDateTime.now(ZoneOffset.UTC);
+            int year = now.getYear();
+            if (now.getDayOfYear() == 1 && !now.getMonth().toString().substring(0,3).equals(logTimestamp.substring(0,3).toUpperCase()))
+                year--;


This logic stands out to me as not totally obvious. Would be good to comment why you are doing this. Under what conditions do we need to year-- and why?

Sure. I see how this is not entirely obvious. I'm trying to solve an edge case here where a message comes in for parsing without a year in the timestamp on January 1st but the message was actually generated on the device on December 31st. I'll add in some comments for clarity.

Gotcha. So anything that comes in on the first day of the year, with a month that is not January, will be backdated.

If something comes in on the 2nd day of the year, with a month of December, it will NOT be backdated. The period of time that we are willing to backdate, is effectively 1 day currently.

Maybe that time period needs to be configurable. The user defines the period of time, 1 day, 2 days, 1 week, after the beginning of the year in which messages can possibly be backdated.

Are there certain conditions under which the logic should blow-up and error? What if we are going to backdate a message where the month is July? Should we just do that or should we error?

Sorry for the late comment, but I perceive a small problem here.

Using criteria "now == day#1 in UTC" has an unknown relationship to the incoming data, depending on the source time zone. You might be 24 hrs ahead of the source, or you might be one second ahead. So if there might be a delay in obtaining the logs, due perhaps to a half-day network failure at Metron's end, you will backdate logs from some sources but not from others, depending on their geographic location. This argument is not alleviated by changing from "day Initial code for a website #1" to "day Initial code for a website #1 or replace opensoc-streaming version 0.4BETA with 0.6BETA 8e7a6b4ad9febbc… #2", the specifics just change somewhat. Also, log ingest can be delayed for months rather than days, and we want to be able to accommodate replay or historical ingest. I would suggest instead of using the current criterion (now.getDayOfYear() < "configurable.number.of.days" && now.getMonth() != logTimeStamp.getMonth()) that we use the criterion that the logTimestamp calculated with "this year" is significantly in the future from "now". I suggest "more than 4 days in the future" is a good criterion; that leaves 1 day for NTP error on the logging side, 1 day for NTP error on the Metron side, 1 day for possible leap year artifacts, and 1 day for Murphy's Law. With this rule, year-less logs can be correctly ingested from up to one year (less 4 days) in the past.

For the stated goal, that's sufficient. We don't need to make the "future limit" configurable. However, there is also the use case of ingesting year-less data more than a year old. The only way to enable that, that I can see, would be configuring the source with a year (or an age) along with a timezone. Doesn't work for on-going sources, but would work for a source that ingests a chunk of history of known age.

BTW, is the ZonedDateTime.parse() method robust if we accidentally hand it a FEB 29 date from a non-leap year? Since we are trying to synthesize the "year" information, it could happen.

I like the idea of checking how far the date in the current year would be in the future and basing the back date decision on that. Let me work on that.

cestella

I love it! Thanks for the contribution. Just a few comments, but great job.

cestella · 2016-09-28T13:38:53Z

metron-platform/metron-common/src/main/java/org/apache/metron/common/Constants.java

    ,DST_PORT("ip_dst_port")
    ,PROTOCOL("protocol")
    ,TIMESTAMP("timestamp")
+    ,ORIGINAL("original_string")


I particularly like this one. We should refactor the GrokParser to use it as a follow-on.

I agree. I think we've needed something like this for a while. it will help standardize our parsing. great job noticing this. I am definitely +1 on refactoring grok to look like this

cestella · 2016-09-28T13:39:42Z

metron-deployment/roles/metron_kafka_topics/defaults/main.yml

  - { topic: "bro",         num_partitions: 1, replication_factor: 1, retention_gb: 10 }
  - { topic: "yaf",         num_partitions: 1, replication_factor: 1, retention_gb: 10 }
  - { topic: "snort",       num_partitions: 1, replication_factor: 1, retention_gb: 10 }
+  - { topic: "asa", num_partitions: 1, replication_factor: 1, retention_gb: 10 }


should we create the kafka topic if we aren't starting the sensor as part of the default set of sensors? Shouldn't we handle this like squid, where we have the user create the topic if they set up the sensor?

That makes sense. I was thinking about building out the monit scripts, etc to make this as easy as possible for the user to deploy out-of-the-box, but that's a future PR. Is that something that would be valuable to folks? Either way, I can remove this from the current PR.

cestella · 2016-09-28T13:41:55Z

...n-platform/metron-parsers/src/main/java/org/apache/metron/parsers/utils/FieldValidators.java

+            return false;
+    }
+
+    public static boolean isValidIpAddr(String ipAddress) {


Yeah, agreed. I'd just do InetAddressValidator.getInstance().isValidInet6Address(ip) || InetAddressValidator.getInstance().isValidInet4Address(ip) here.

cestella · 2016-09-28T13:45:25Z

...-platform/metron-parsers/src/test/java/org/apache/metron/parsers/asa/BasicAsaParserTest.java

+    }
+
+    @Test
+    public void testCISCOFW106023() {


Can we have test-cases covering situations where:

IP's are malformed

IP's are IPv6

Data is garbage

nickwallen · 2016-09-28T13:52:48Z

metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/asa/BasicAsaParser.java

+        List<JSONObject> messages = new ArrayList<>();
+        try {
+            String logLine = new String(rawMessage, "UTF-8");
+            LOG.trace("[Metron] Started parsing raw message: " + logLine);


I am not sure all the LOG.trace is necessary. At the very least you should use SLF4J's parameterized messages to avoid the cost of parameter construction. SLF4J FAQ

I thought I already commented on this, but Github seems to have lost it. Sorry if this ends up being a dup.

I'd like to leave the additional logging in for assistance in debugging when adding new ASA message types to the parser in the future. I've updated the code to use the SLF4J parameterized messages as suggested.

kylerichardson · 2016-09-29T00:41:37Z

@nickwallen @cestella Thanks very much for the feedback! Much appreciated. I'll get started on these changes and respond to your questions as soon as I can.

kylerichardson · 2016-10-11T19:40:28Z

Not entirely sure why the CI build failed.

The error was:

testExample1(org.apache.metron.profiler.integration.ProfilerIntegrationTest)  Time elapsed: 35.546 sec  <<< FAILURE!
java.lang.AssertionError: expected:<1950.0> but was:<390.0>
    at org.junit.Assert.fail(Assert.java:88)
    at org.junit.Assert.failNotEquals(Assert.java:834)
    at org.junit.Assert.assertEquals(Assert.java:553)
    at org.junit.Assert.assertEquals(Assert.java:683)
    at org.apache.metron.profiler.integration.ProfilerIntegrationTest.testExample1(ProfilerIntegrationTest.java:140)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:483)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:252)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:141)
    at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:112)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:483)
    at org.apache.maven.surefire.util.ReflectionUtils.invokeMethodWithArray(ReflectionUtils.java:189)
    at org.apache.maven.surefire.booter.ProviderFactory$ProviderProxy.invoke(ProviderFactory.java:165)
    at org.apache.maven.surefire.booter.ProviderFactory.invokeProvider(ProviderFactory.java:85)
    at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:115)
    at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:75)

Slightly earlier in the log:

106738 [Curator-Framework-0] ERROR o.a.c.ConnectionState - Connection timed out for connection string (127.0.0.1:51857) and timeout (15000) / elapsed (18872)
org.apache.curator.CuratorConnectionLossException: KeeperErrorCode = ConnectionLoss
    at org.apache.curator.ConnectionState.checkTimeouts(ConnectionState.java:197) [metron-common-0.2.1BETA.jar:?]
    at org.apache.curator.ConnectionState.getZooKeeper(ConnectionState.java:87) [metron-common-0.2.1BETA.jar:?]
    at org.apache.curator.CuratorZookeeperClient.getZooKeeper(CuratorZookeeperClient.java:115) [metron-common-0.2.1BETA.jar:?]
    at org.apache.curator.framework.imps.CuratorFrameworkImpl.performBackgroundOperation(CuratorFrameworkImpl.java:806) [metron-common-0.2.1BETA.jar:?]
    at org.apache.curator.framework.imps.CuratorFrameworkImpl.backgroundOperationsLoop(CuratorFrameworkImpl.java:792) [metron-common-0.2.1BETA.jar:?]
    at org.apache.curator.framework.imps.CuratorFrameworkImpl.access$300(CuratorFrameworkImpl.java:62) [metron-common-0.2.1BETA.jar:?]
    at org.apache.curator.framework.imps.CuratorFrameworkImpl$4.call(CuratorFrameworkImpl.java:257) [metron-common-0.2.1BETA.jar:?]
    at java.util.concurrent.FutureTask.run(FutureTask.java:266) [?:1.8.0_31]
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) [?:1.8.0_31]
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) [?:1.8.0_31]
    at java.lang.Thread.run(Thread.java:745) [?:1.8.0_31]

I'm thinking this is similar to @ottobackwards issue on #303. Could it be anything else or should I just try a close and re-open?

nickwallen · 2016-10-11T21:15:43Z

I would close and re-open. As our test suite has expanded and is more demanding, at certain times Travis will fail the build when there is not really a problem. We need to figure out how to fix this problem, but right now I'd try a reboot.

kylerichardson · 2016-10-12T12:29:05Z

Thanks. Looks like re-opening did the trick.

I've done my best to incorporate everyone's feedback into this version. Re-tested in single node vm successfully.

nickwallen · 2016-10-12T12:50:21Z

@kylerichardson When you say "tested in single node vm", what do you mean exactly? Do you not use the Vagrant deployment mechanism at metron-deployment/vagrant/quick-dev-platform or metron-deployment/vagrant/full-dev-platform to create a single node VM for testing?

nickwallen · 2016-10-12T13:04:15Z

metron-platform/metron-parsers/src/main/config/zookeeper/parsers/asa.json

+  "parserClassName": "org.apache.metron.parsers.asa.BasicAsaParser",
+  "sensorTopic": "asa",
+  "parserConfig": {
+    "deviceTimeZone": "UTC-05:00"


Should we make the default UTC?

Yes, absolutely. I'll remove it. I left this in as an example. If no deviceTimeZone is provided, the code will default to UTC.

kylerichardson · 2016-10-12T13:06:11Z

@nickwallen Apologies, I should have been more specific. I tested using the same steps provided earlier in the PR. That said, my "single node vm" testing is not done with vagrant. Currently I'm not able to successfully use the quick dev environment based on my setup (e.g. Windows). I'm working to remedy that.

For "single node vm" testing, I actually run two vms, one Fedora host which I do development on and use to run the ansible deployment and a second Centos 6 (base install from snapshot) host which I deploy Metron onto.

For testing this PR, I deployed Metron without the sensors to by Centos 6 vm for testing and ran through the steps provided above.

nickwallen

Almost there. Looking really good. Just a few small issues with the tests, mainly.

nickwallen · 2016-10-12T13:08:21Z

metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/utils/SyslogUtils.java

+            return convertToEpochMillis(logTimestamp, DateTimeFormatter.ISO_OFFSET_DATE_TIME);
+
+        else
+            throw new ParseException(String.format("Unsupported date format: '%s'", logTimestamp));


Just curious, any reason we're using a checked exception here? In other places we're just using run time exceptions. The ParseException that you created is used only for this, I believe.

Not a big deal either way.

My thought here was that there may be some situations where we want to handle a parsing error without blowing up and sending the message to the error queue. It was a bit of "future proofing" on my part I suppose.

For consistency, would it be better to revert to using a RuntimeException?

nickwallen · 2016-10-12T13:10:06Z

...-platform/metron-parsers/src/test/java/org/apache/metron/parsers/asa/BasicAsaParserTest.java

+        try {
+            JSONObject asaJson = asaParser.parse(rawMessage.getBytes()).get(0);
+        } catch (RuntimeException e) {
+            assertTrue(true);


I don't think this test will ever fail. You can get rid of the try/catch and just change the annotation. That way the test will fail if a RunTimeException is not thrown.

@Test (expected = RunTimeException.class)

In this case, I'd prefer using JUnit's Rules to test this. RuntimeExceptions are fairly generic, and Rules would allow the specific exception message to be verified.

e.g.

TestThing testThing = new TestThing(); thrown.expect(NotFoundException.class); thrown.expectMessage(startsWith("some Message"));

https://github.com/junit-team/junit4/wiki/exception-testing

nickwallen · 2016-10-12T13:12:04Z

...n-platform/metron-parsers/src/test/java/org/apache/metron/parsers/utils/SyslogUtilsTest.java

+
+import static org.junit.Assert.*;
+
+public class SyslogUtilsTest {


A tricky, but necessary, part of your timestamp logic is rolling the year backwards in certain cases. Are there specific tests that hit on that? Maybe I am missing them. We need to make sure we cover all the edges on those scenarios.

Agreed. There currently isn't test coverage for that logic.

I was trying to avoid having to add a dependency on a Clock object but it may be the only way to throughly test this code.

Wouldn't it suffice to Mock the ZonedDateTime.now() call?

Yes, in theory that is.

I tried doing just that with PowerMock and Mockito; unfortunately, it seems there is a bug in the underlying javassist library that doesn't play well with the new java.time classes.

The issue is reportedly fixed in Javassist 3.20.0-GA; however, it doesn't appear that PowerMock has updated to this version.

Reference: JASSIST-246 and powermock/powermock#557

Bummer. Sorry, I have no advice. Any Mockito experts out there?

nickwallen · 2016-10-12T13:13:23Z

...n-platform/metron-parsers/src/test/java/org/apache/metron/parsers/utils/SyslogUtilsTest.java

+    }
+
+    private long getParsedEpochMillis(String originalTimestamp) {
+        try {


There is no need to try/catch here. Just have the method throw ParseException. Any of your test methods can also throw ParseException. This simplifies the logic and JUnit will fail the test if a ParseException is thrown.

nickwallen · 2016-10-12T13:32:36Z

metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/asa/BasicAsaParser.java

+        try {
+            asaGrok.addPatternFromReader(new InputStreamReader(patternStream));
+        } catch (GrokException e) {
+            LOG.error("[Metron] Failed to load grok patterns from jar", e);


I think we would want to throw a runtime exception after the LOG. That is, if the GrokParser itself is a good guide for expected behavior.

nickwallen · 2016-10-12T13:35:58Z

metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/asa/BasicAsaParser.java

+        asaGrok = new Grok();
+        InputStream patternStream = this.getClass().getClassLoader().getResourceAsStream("patterns/asa");
+        try {
+            asaGrok.addPatternFromReader(new InputStreamReader(patternStream));


Just calling this out for other's awareness, but this will only load the Grok pattern from the classpath. The GrokParser seems to work much harder looking for a pattern.

Maybe that is what we want in this case, since changing the Grok pattern might not play well with the rest of the Java code involved with Kyle's parser.

My two cents...

In this case, the parser code is tightly coupled to the pattern file, which is why I chose to load it from the classpath.

I could see a couple of other options:
(1) Add a parser config option to the full path of the pattern file
(2) Wait for #308 to be merged and add the patterns as config options

The problem I see with option 2 is that the pattern file used for the ASAs has a lot of lines and might be a bit ugly in the parser config.

With either of these options, another parser config option would need to be added to hold the CISCOTAG to pattern name mapping.

nickwallen

Looking very good. Almost there. Just a few changes with the tests.

kylerichardson · 2016-10-12T15:20:59Z

Thanks for bearing with me. I really appreciate the feedback and direction. I should be able to get these changes in later tonight after I finish up my "day job" :).

mattf-apache · 2016-10-13T16:01:52Z

I added a comment above, to SyslogUtils.java line 36, which the system did not email to the list, probably because I immediately edited it to fix a format error. @kylerichardson please consider it. Thanks.

kylerichardson · 2016-10-19T12:53:34Z

Whew, got the CI build to finally pass. All integration and unit tests are passing. I've also re-testing in the single node vm environment I described above.

kylerichardson · 2016-10-25T18:39:34Z

Any other feedback or suggestions for me?

james-sirota · 2016-10-27T19:14:48Z

Testing this in production this week on production hardware. Will have feedback in the next few days

james-sirota · 2016-10-29T15:34:22Z

metron-platform/metron-parsers/src/main/resources/patterns/asa

 LOGLEVEL ([A|a]lert|ALERT|[T|t]race|TRACE|[D|d]ebug|DEBUG|[N|n]otice|NOTICE|[I|i]nfo|INFO|[W|w]arn?(?:ing)?|WARN?(?:ING)?|[E|e]rr?(?:or)?|ERR?(?:OR)?|[C|c]rit?(?:ical)?|CRIT?(?:ICAL)?|[F|f]atal|FATAL|[S|s]evere|SEVERE|EMERG(?:ENCY)?|[Ee]merg(?:ency)?)

 #== Cisco ASA ==
-CISCO_TAGGED_SYSLOG ^<%{POSINT:syslog_pri}>%{CISCOTIMESTAMP:timestamp}( %{SYSLOGHOST:sysloghost})? ?:? %%{CISCOTAG:ciscotag}:


Do we really need to put the entire logstash patterns file here? i think we can just put the ASA-specific lines

The ASA patterns build off of several of the more generic patterns referenced earlier in the file; however, I should be able to reduce it down to just the ones being used.

james-sirota · 2016-10-31T04:10:50Z

Still testing...bare with me

kylerichardson · 2016-10-31T18:27:59Z

@james-sirota No worries. Thanks for testing!

james-sirota · 2016-10-31T23:01:58Z

+1. Great job. Any more revisions you want to make to this? Or are we good to commit?

Summary of changes: - Complete rewrite of ASA parser including new test suite - ZK configurations for ease of topology deployment (parser and enrichment) - Add field constant for original_string in metron-common - Minor changes to ASA patterns file for (1) Syslog severity/facility capture (2) Interface capture on CISCOFW106006_106007_106010 - Updates to various POMs to allow easier validation of logging during unit testing (1) Exclusions for slf4j-log4j12 on various dependencies for metron-parsers and metron-integration-test (2) Explicit dependency on slf4j-api for metron-parsers (3) Test dependency on slf4j-simple for metron-parsers

Includes the following: - Static map for ASA message patterns (vs pattern discovery) - Minor changes to ASA patterns file - Broke out common syslog parsing elements - Broke out reusable field validations

Includes the following: - Extend BasicParser - Handle both types of syslog timestamps (with and without year) - Include integration test and supporting sample data

Changes include: (1) New/additional unit tests (2) Reworked Syslog Timestamp (no year) logic (3) Enhanced error checking and logging (introduced new ParseException)

kylerichardson · 2016-11-01T01:26:37Z

Rebased against master to incorporate the global junit version change. Should be good to go now pending Travis.

Thanks again to everyone for all of the suggestions, feedback, and testing.

kylerichardson · 2016-11-01T17:17:38Z

Ok, need some helping figuring out why the CI build keeps failing...

I get several of these at the end of the log:

Running org.apache.metron.parsers.integration.JSONMapIntegrationTest
2016-11-01 15:54:52 FATAL KafkaServer:116 - [Kafka Server 0], Fatal error during KafkaServer startup. Prepare to shutdown
kafka.common.KafkaException: Socket server failed to bind to localhost:6667: Address already in use.

and prior to that I see:

Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 8.64 sec <<< FAILURE! - in org.apache.metron.parsers.integration.YafIntegrationTest
test(org.apache.metron.parsers.integration.YafIntegrationTest)  Time elapsed: 8.637 sec  <<< ERROR!
java.lang.NoClassDefFoundError: org/slf4j/event/LoggingEvent

This occurred for both of the CI builds since I rebased to the latest master. Any ideas?

kylerichardson · 2016-11-02T13:08:21Z

A big thank you to @ottobackwards for helping to troubleshoot the CI build fails. This should be good to go now.

nickwallen · 2016-11-02T13:20:36Z

+1 Great contribution

kylerichardson force-pushed the METRON-363 branch from 1bcfc5d to 04a936d Compare September 28, 2016 02:42

nickwallen suggested changes Sep 28, 2016

View reviewed changes

cestella suggested changes Sep 28, 2016

View reviewed changes

nickwallen reviewed Sep 28, 2016

View reviewed changes

kylerichardson closed this Oct 11, 2016

kylerichardson reopened this Oct 12, 2016

nickwallen reviewed Oct 12, 2016

View reviewed changes

nickwallen suggested changes Oct 12, 2016

View reviewed changes

kylerichardson mentioned this pull request Oct 14, 2016

Metron-498 Grok patterns are now read from zookeeper parser config property "grokPattern" #308

Closed

kylerichardson closed this Oct 18, 2016

kylerichardson reopened this Oct 18, 2016

kylerichardson closed this Oct 19, 2016

kylerichardson reopened this Oct 19, 2016

james-sirota reviewed Oct 29, 2016

View reviewed changes

kylerichardson added 12 commits October 31, 2016 20:55

METRON-363 Reworked parser to handle nulls and field validation

6fea1f2

Includes the following: - Static map for ASA message patterns (vs pattern discovery) - Minor changes to ASA patterns file - Broke out common syslog parsing elements - Broke out reusable field validations

METRON-363 Add integration test and sample data

0f50a95

Includes the following: - Extend BasicParser - Handle both types of syslog timestamps (with and without year) - Include integration test and supporting sample data

METRON-363 Add license and kafka topic

e8ae469

METRON-363 Adjust log level

72623da

METRON-363 Enhance logging, remove unused code

f5fdafa

METRON-363 Refactored and enhanced based on feedback

e51def4

Changes include: (1) New/additional unit tests (2) Reworked Syslog Timestamp (no year) logic (3) Enhanced error checking and logging (introduced new ParseException)

METRON-363 Default to UTC in zookeeper config

957876f

METRON-363 Update tests

5f49d45

METRON-363 Refactor to add Clock dependency for testing

7556703

METRON-363 Add tests for back dating RFC3164 timestamps

dcaa9bb

METRON-363 Update metron-parsers to use global junit version

12cd31e

kylerichardson force-pushed the METRON-363 branch from db86866 to 12cd31e Compare November 1, 2016 01:25

kylerichardson closed this Nov 1, 2016

kylerichardson reopened this Nov 1, 2016

METRON-363 Exclude slf4j-log4j12 from multiple poms

7481c5d

asfgit closed this in 5418a6a Nov 3, 2016


		import static org.junit.Assert.*;

		public class SyslogUtilsTest {

METRON-363 Fix Cisco ASA Parser #276

METRON-363 Fix Cisco ASA Parser #276

Uh oh!

Conversation

kylerichardson commented Sep 27, 2016

Uh oh!

cestella commented Sep 27, 2016

Uh oh!

danieljue commented Sep 27, 2016

Uh oh!

kylerichardson commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kylerichardson commented Sep 28, 2016

Uh oh!

kylerichardson commented Sep 28, 2016

Uh oh!

nickwallen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickwallen Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickwallen Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattf-apache Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cestella left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kylerichardson commented Sep 28, 2016 •

edited

Loading

nickwallen Sep 28, 2016 •

edited

Loading

nickwallen Sep 28, 2016 •

edited

Loading

mattf-apache Oct 13, 2016 •

edited

Loading