Palo Alto ARP table plugin #2613
Conversation
|
|
|
Kudos, SonarCloud Quality Gate passed!
|
Thanks a bunch for your much anticipated contribution, @Slenderman00 ! We seem to currently be having some unrelated issues with our CI test suite, which also causes the tests to fail for your PR (See #2617). We'll try to fix those issues in the master branch before evaluation your contribution properly. |
lunkwill42
force-pushed
the
masteruninett
branch
from
395c9f2
to
c4376c4
Compare
|
Rebased this on the latest master and ran a force-push. My apologies that it took so long to give this a proper look - summer vacation time snuck up on us really fast... |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2613 +/- ##
==========================================
+ Coverage 60.21% 60.23% +0.02%
==========================================
Files 601 602 +1
Lines 43981 44050 +69
==========================================
+ Hits 26481 26534 +53
- Misses 17500 17516 +16 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I've not done an in-depth review, but most of it looks ok.
No. 1 issue is that there is no test coverage. As you will see, CodeCov will reject this PR since it has less than 50% coverage, and also lowers the overall coverage number for the entire codebase.
Any thoughts on how this could reasonably tested?
|
for testing i was thinking of just feeding the class some xml data and then verifying the output. |
Sounds reasonable to me :) |
|
Kudos, SonarCloud Quality Gate passed!
|
|
Hi, May I ask you please to add this plugin to the next release ? We have been waiting for this plugin for long time. Thank you for your understanding |
|
Kudos, SonarCloud Quality Gate passed!
|
That would have been nice, but the tests don't work, and there are still issues with the code, so we won't have time to fix this on @Slenderman00's behalf for the next release. |
|
@lunkwill42 1137c1a fixes the tests. |
Thank you - we still won't have time to evaluate this properly for the next release, as this is the last day of our sprint. But we will be putting this at the top of our backlog for the next sprint (about 1 week after Easter, I believe). |
lunkwill42
force-pushed
the
masteruninett
branch
from
1137c1a
to
0e77f24
Compare
|
Instead of the eternal ping-pong of getting this PR into shape, I've rebased it on the current master and started working on completing it myself. It's still a work in progress, so I'm converting it to a draft PR for now. |
|
|
||
| class SslPolicy(client.BrowserLikePolicyForHTTPS): | ||
| def creatorForNetloc(self, hostname, port): | ||
| return ssl.CertificateOptions(verify=False) |
There was a problem hiding this comment.
I'm not accustomed to using the Twisted client libraries for this sort of thing. Does this in fact mean that we explicitly turn off TLS certificate verification? If so, is that really what we want when talking to a security-specific product? :)
There was a problem hiding this comment.
We discussed this at UiT last year. Since we are only extracting ARP data this should be fine as long as the keys and permissions on the Palo Alto device are configured correctly (To only allow for fetching ARP data). But yes we are explicitly turning of TLS certificate verification on all requests made by the plugin.
There was a problem hiding this comment.
It might be better to implement certificate pinning instead of this solution just in case someone manages to misconfigure their permissions.
There was a problem hiding this comment.
A MITM attack might also allow for data infiltration, certificate pinning is starting to seem like the best option.
There was a problem hiding this comment.
To get this PR done with, I would just make note of this fact in the docs. That way, you could submit a new PR with changes for certificate config options (i.e. switch verification on/off or point to a pinned certificate)
PyOpenSSL is *not* a requirement for the test suite only: It's required to make the PaloAlto ARP plugin work in production!
The address doesn't necessarily need to be an IP, it could also be a hostname, so the more generic `address` name is preferable.
We follow PEP-8: Class names should not begin with lower-case letters unless there is a really specific or necessary reason for it.
Even if we do want a broad exception handler, never use just `except`, as that will catch even low-level Python exceptions. Added a noqa tag and log the full exception with a slightly more information message.
We're expecting to switch to using towncrier at any moment in NAV, so might as well add a news fragment for this PR right away.
The intent is to fallback to look up using the sysname if the host isn't configured using an IP address.
I tried to increase the coverage ever so slightly, with the result that whereas CodeCov previously greenlighted my changes, it's now nixing them :P |
lunkwill42
dismissed
their stale review
Fixed my requests myself, someone else needs to review.
There was a problem hiding this comment.
Now all that is missing is @Slenderman00 testing this on real hardware?
|
What do you think, @Slenderman00, do our updates make sense to you? We're ready to merge it, but have no way of testing it against real gear. |
It looks really good, I should be able to test it on real hardware later today. |
|
I have now tested it on real hardware, everything seems to work fine. |
Then let's roll with this for NAV 5.10! 🎉 |
Implements a plugin that fetches ARP tables from Palo Alto network equipment
#2343