[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[justinleet]

Github user justinleet commented on the issue:

https://github.com/apache/metron/pull/579
  
@ctramnitz Just noticed this, but could you change the name of the PR to 
match the Jira?

i.e.
> METRON-941 native PaloAlto parser corrupts message when having a comma in 
the payload



---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[ottobackwards]

Github user ottobackwards commented on the issue:

https://github.com/apache/metron/pull/579
  
after that I'll merge



---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[ottobackwards]

Github user ottobackwards commented on the issue:

https://github.com/apache/metron/pull/579
  
@ctramnitz one thing, since this is a regression, technically, we need to 
update the release notes / upgrade guide.
Can you add a note to the Upgrading.md about the removal of the Syslog 
-ness?


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[justinleet]

Github user justinleet commented on the issue:

https://github.com/apache/metron/pull/579
  
I'm +1, this is definitely a major improvement and I agree, getting it 
merged would be great. @simonellistonball Any comment as @ottobackwards asked, 
or are we good to pull this in?


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[simonellistonball]

Github user simonellistonball commented on the issue:

https://github.com/apache/metron/pull/579
  
I don't believe I have any further things to add. I'm +1 and keen to see 
this get in.


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[ctramnitz]

Github user ctramnitz commented on the issue:

https://github.com/apache/metron/pull/579
  
Any further comments? The old parser was plain broken. So even if there is 
room for improvement here, I would really like to see this merged.

Thanks!


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[ottobackwards]

Github user ottobackwards commented on the issue:

https://github.com/apache/metron/pull/579
  
Im +1 on this.  I would like to get comment from @simonellistonball et al 
on the change for syslog


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[ctramnitz]

Github user ctramnitz commented on the issue:

https://github.com/apache/metron/pull/579
  
8.0 log format is also working now

In the latest two commits I included the changed tests from @justinleet but 
changed the expected input from full syslog messages including syslog header 
into just the syslog message aka payload.
It is not safe to assume that the previously used syslog header (in old 
RFC3164 format) will be used by anyone. Until we have something generic to 
(pre-)parse syslog before it reaches the message parser I assumed the messages 
will be stripped off the syslog header for now.

This works nicely with the rsyslog config snippet above.

The PR should be ready for prime-time now. Please let me know if anything 
else needs to be changed.


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[ctramnitz]

Github user ctramnitz commented on the issue:

https://github.com/apache/metron/pull/579
  
For the syslog header vs message parser this future_use is not really 
important. This has other implications than just carrying arbitrary data in a 
field that is labeled to do something else.


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[justinleet]

Github user justinleet commented on the issue:

https://github.com/apache/metron/pull/579
  
from @ctramnitz on the PR I made against his branch.

> However, I'm not sure the result for is really as expected.
> It shouldn't be "<11>Jan 5 05:38:59 PAN1.exampleCustomer.com 1", but just 
"1". The rest is the Syslog header.
> 
> Is the PaloParser called against the entire syslog line or after the 
header has been stripped out?

Keeping in mind, that I'm definitely not an expert on this, here's what 
I've dug up.  If anybody has more expertise / insight, I'd be happy for the 
contribution.

Looking over it, the full log line. Check out 
metron-platform/metron-integration-test/src/main/sample/data/SampleInput/PaloaltoOutput

Looking at 
https://www.paloaltonetworks.com/content/dam/pan/en_US/assets/pdf/framemaker/60/pan-os/pan-os.pdf
 for the 6.0 fields, they are

```
FUTURE_USE
Receive Time
Serial Number
Type
Subtype
FUTURE_USE
Generated Time
Source IP
Destination IP
NAT Source IP
NAT Destination IP
Rule Name
Source User
Destination User
Application
Virtual System
Source Zone
Destination Zone
Ingress Interface
Egress Interface
LogForwarding Profile
FUTURE_USE
Session ID
Repeat Count
Source Port
Destination Port
NAT SourcePort
NAT Destination Port
Flags
Protocol
Action
Miscellaneous
Threat ID
Category
Severity
Direction
Sequence Number
Action Flags
Source Location
Destination Location
FUTURE_USE
Content Type
PCAP_id*
Filedigest*
Cloud*
```

The field we pull out as `palo_alto_domain` appears to be a `FUTURE_USE` 
field.  There also doesn't appear to something that would obviously correspond 
to the PaloAltoDomain field (unless I can't read, which has happened before).

The field we call `time_logged` also appears to be `FUTURE_USE`. There's 
also another FUTURE_USE, but I think I misread something because it didn't line 
up like I expected (and the specifics aren't actually super important).

It makes me question how reliable anything labelled FUTURE_USE is (although 
that doesn't stop us from looking at it and labelling it).


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[ctramnitz]

Github user ctramnitz commented on the issue:

https://github.com/apache/metron/pull/579
  
I think 
https://github.com/apache/metron/pull/579/commits/ccd99dda3c8a72408ae13eeaca078af1e345a36c#diff-e0385f97ebea64bab3a83bceef70bb4aR67
expected.put(BasicPaloAltoFirewallParser.PaloAltoDomain, "<11>Jan  5 
05:38:59 PAN1.exampleCustomer.com 1");
should be
expected.put(BasicPaloAltoFirewallParser.PaloAltoDomain, "1");

The rest is the syslog header, not the PA domain.

I'd suggest to strip the syslog header off the test data and assume it will 
also be stripped off on ingestion until we have a syslog-preparsing capability 
(i.e. https://issues.apache.org/jira/browse/METRON-1453).

I'm already doing this using rsyslog:
```
module(load="imudp")
module(load="omkafka")

template(name="msgonly" type="string"
 string="%msg:::drop-last-lf%"
)

ruleset(name="udp514"){
  if () then {
action(
  broker=[":6667"]
  confparam=["client.id=rsyslog", "compression.codec=snappy", 
"socket.keepalive.enable=true"]
  type="omkafka"
  topic="paloalto"
  template="msgonly"
  errorfile="/var/log/rsyslog_kafka_failures.log"
)
  }  
}
input(type="imudp" port="514" ruleset="udp514")
```


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[ottobackwards]

Github user ottobackwards commented on the issue:

https://github.com/apache/metron/pull/579
  
@ctramnitz thank you!  Let us know where you are at and if we can help


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[ctramnitz]

Github user ctramnitz commented on the issue:

https://github.com/apache/metron/pull/579
  
Rebasing to master to see where we are. If this comes back clean please 
don't merge yet, I want to add 8.0 log format first.


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[ottobackwards]

Github user ottobackwards commented on the issue:

https://github.com/apache/metron/pull/579
  
@ctramnitz is there any update on this PR?  Is there something we can do to 
help?


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[james-sirota]

Github user james-sirota commented on the issue:

https://github.com/apache/metron/pull/579
  
Hi we would like to get this into the next release.  @ctramnitz we'll be 
happy to help you fix it


---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[justinleet]

Github user justinleet commented on the issue:

https://github.com/apache/metron/pull/579
  
Just the start.  I got the previously existing test cases in a better 
shape, and @ctramnitz should be able to add on pretty easily.

I do want to echo what @mattf-horton said about the quality of work and the 
length of the cycle. We definitely appreciate getting fixes like this, and I 
wanted to make sure we got tests in, not just for the quality of your work, but 
so that we avoid regressions in the future.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[mattf-horton]

Github user mattf-horton commented on the issue:

https://github.com/apache/metron/pull/579
  
First, a note to @ctramnitz : Christian, please be patient with the long 
cycle on this.  It is not a reflection on your work, but rather a desire to 
make sure that work gets appropriate consideration.  Your work is very much 
appreciated.  We really do have a very strong bias in Apache toward test-driven 
development, and so we tend to ask that new work provide unit tests, even if 
those tests were lamentably lacking before the new work :-)

Anyway, thanks to fast work by @justinleet , METRON-962 is now in, and the 
PaloAlto parser can be unit tested.  Please rebase so it can be done.  Since 
the one file you changed has not been touched by recent commits, the rebase 
should be painless.  If you're not familiar with git rebase, ping me offline.

If my understanding of Justin's PR is correct, you can then just layer on 
his bf8ce62 patch, and have a good start on testability.  @justinleet is that 
correct?  Was it your intent that this patch will fully provide the unit 
testing capability, or is it just the start?  Thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[justinleet]

Github user justinleet commented on the issue:

https://github.com/apache/metron/pull/579
  
https://github.com/ctramnitz/metron/pull/1 opened, if anyone cares.  As 
noted, 
https://github.com/ctramnitz/metron/commit/bf8ce62cb5ed3846f288e4a7a606410ebbaf9d30
 is the relevant commit.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[justinleet]

Github user justinleet commented on the issue:

https://github.com/apache/metron/pull/579
  
I lied, it's a bit circular.  It depends on your PR's fixes, so let's go 
back to the original strategy of making a PR against your branch.  I'm more 
than happy to give up credit, since this is just moving where the test data 
lives and actually checking against it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[justinleet]

Github user justinleet commented on the issue:

https://github.com/apache/metron/pull/579
  
Actually, I'll just open a separate ticket/PR.  You'll still want to merge 
it in.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[justinleet]

Github user justinleet commented on the issue:

https://github.com/apache/metron/pull/579
  
I just kicked up https://github.com/apache/metron/pull/612, which 
theoretically fixes the unit tests (and simplifies them a lot).  Straight 
replacing the logs would work, but we might want versioned tests (which could 
either be done here, or as a follow on).

A proper review obviously has to be done there (and I still owe a bit on it 
myself), but if reasonably possible I'd prefer to get that done to enable unit 
testing here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[kylerichardson]

Github user kylerichardson commented on the issue:

https://github.com/apache/metron/pull/579
  
Given these unit tests have been broken for quite some time and @ctramnitz 
has thoroughly tested in his environment and that this is a relatively minor 
change, I'm okay to move it forward especially with a JIRA in to address these 
broken 'AbstractConfigTest' tests holistically in METRON-962.

+1 from me but would prefer a second to commit without tests.

@ctramnitz as discussed please upload public safe sample logs to the JIRA 
to assist with test development.

-Kyle

> On Jun 5, 2017, at 5:40 PM, Matt Foley  wrote:
> 
> @ctramnitz says (in email): "I didn’t touch unit tests because it was 
already broken before.
> We agreed to put this into a separate ticket (METRON-962) as it applies 
to other parsers as well.
> For now, METRON-941 fixes an actual bug (parsing is broken with the 
current code)."
> 
> @ottobackwards and @justinleet (principles on METRON-962), do you agree 
we should go ahead and commit this without a viable PaloAltoParser unit test? 
Or wait for METRON-962?
> 
> @kylerichardson , what is your opinion on the issue?
> Thanks.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
> 



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[ctramnitz]

Github user ctramnitz commented on the issue:

https://github.com/apache/metron/pull/579
  
I'm running this myself and actively parsing PaloAlto logs with this change 
for about 3 weeks. You can also see that the changes are fairly trivial and 
that structure and naming follows the vendors specifications.
I understand your concerns about the missing unit tests, but, as mentioned 
before, unit testing was completely non functional before, I didn't even 
remotely touch it. You basically have the choice of not merging this, resulting 
in having no unit test and a broken parser, or merging it with still no unit 
test but having a working parser. As I already discussed for METRON-962 I can 
provide sample logs for unit testing.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[mattf-horton]

Github user mattf-horton commented on the issue:

https://github.com/apache/metron/pull/579
  
Also, @ctramnitz , could you please describe in more detail the manual 
testing you did, which might make us feel better about a commit without unit 
test?  Thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[kylerichardson]

Github user kylerichardson commented on the issue:

https://github.com/apache/metron/pull/579
  
Thanks for the contribution! Would you mind having a look at the unit tests 
for this parser and updating them?

From a quick glance, the unit test appears to be defunct. It's using 
`AbstractConfigTest` to read in test data from a file that doesn't exist anyone.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] metron issue #579: METRON-941 fix PaloAltoParser

[ctramnitz]

Github user ctramnitz commented on the issue:

https://github.com/apache/metron/pull/579
  
I updated the mapping to use the Metron name right away and added some more 
handling for different versions of the log format. Any other feedback?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---