Configure HTTP-based ARP information fetching from Palo Alto PAN-OS firewalls using management profiles #3147
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3147 +/- ##
==========================================
+ Coverage 60.33% 60.54% +0.21%
==========================================
Files 606 606
Lines 43700 43723 +23
Branches 48 48
==========================================
+ Hits 26366 26474 +108
+ Misses 17322 17237 -85
Partials 12 12 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This is shaping up really nicely!
I'm really glad you took the time to write a proper release notes entry, though I am not sure that we need to delve deeply into rationalizing why we are changing the palo alto API key config stuff. The bit about ARP being available only through the API I believe was explained in an earlier release note too.
You added a separate "make linter happy" commit, which should be unnecessary. These are formatting changes that your pre-commit hooks should have fixed for you in the original commits (you did install the pre-commit hooks, right?)
The "Fix unittests" commit is also probably best if squashed into the already existing commit that updates the tests.
jorund1
force-pushed
the
rest-api-management-profile
branch
9 times, most recently
from
d03a79d to
fe3a967
Compare
jorund1
force-pushed
the
rest-api-management-profile
branch
2 times, most recently
from
0cf36f4 to
c930c2f
Compare
jorund1
force-pushed
the
rest-api-management-profile
branch
from
c930c2f to
b0ef4b4
Compare
jorund1
force-pushed
the
rest-api-management-profile
branch
from
b0ef4b4 to
01dd43d
Compare
jorund1
changed the title
There was a problem hiding this comment.
Nice work, @jorund1. I'm approving this, but with minor typo and naming suggestions for some test.
I am curious though how this works in practice. I tried to test this with an actual HTTP(S) server locally, and the TLS setup fails because the request does not use SNI. This might work well against a real Palo Alto API implementation - but if we are to add proper TLS certificate verification, I would assume we can only do it by using a host name for the request to have something to validate the certificate on (right now, it uses the IP address directly and does not use SNI). I forget if we remembered to add an issue for future TLS verification, but I'm putting it on my TODO since I have to run to a meeting in 4 minutes.
Found it at #2895 |
jorund1
force-pushed
the
rest-api-management-profile
branch
2 times, most recently
from
c56ef5e to
05ec033
Compare
Prior to this commit, the netboxes handled by the PaloaltoArp ipdevpoll plugin used to be configured in the `ipdevpoll.conf` configuration file, but since the netboxes the plugin wants to handle (i.e. collect Arp information from) already should reside in the NAV database, this configuration is now instead done through the SeedDB tool by assigning a HTTP_API ManagementProfile (with `service` set `Palo Alto ARP` and api_key set to some secret API key) to the netboxes to be handled. The prior way to configure the netboxes handled by the PaloAltoArp plugin implicitly only allowed one API key per netbox (both enforced in code but also by the configuration syntax). With ManagementProfiles, it is perfectly possible to assign multiple profiles (e.g. configurations) of the same type but with different parameters (e.g. API keys) to the same netbox. Hence the new way to configure the netboxes allow many API keys per netbox. Thus the semantics of the plugin must change a little: For any given netbox, the plugin now assumes there may be multiple API keys, and uses the ARP results of first API key for which the _do_request method returns a successful response. IMPORTANT: This commit removes the ability to configure the netboxes handled by the PaloaltoArp plugin the NAV version 5.10 - 5.11 way through the `[paloaltoarp]` section in the `ipdevpoll.conf` configuration file.
lunkwill42
force-pushed
the
rest-api-management-profile
branch
from
05ec033 to
c5ab32e
Compare
|
Deprecate the
[paloaltoarp]section ofipdevpoll.confin favor of using management profiles to configure HTTP-based fetching of ARP information from Palo Alto PIO-OS firewalls, analogous to how configuration of SNMP-based fetching is done.The new management profile protocol to configure HTTP-based fetching has been given the name
HTTP APIHTTP REST APIfor now, but this name might be missing the mark since there really isn't anything REST-related about the management profile type.HTTP WITH API KEY, would be a more fitting name?Fixes #2894