core
core copied to clipboard
opnsense
IDS: API 'settings/get' response incomplete
Important notices
Before you add a new report, we ask you kindly to acknowledge the following:
- [x] I have read the contributing guide lines at https://github.com/opnsense/core/blob/master/CONTRIBUTING.md
- [x] I am convinced that my issue is new after having checked both open and closed issues at https://github.com/opnsense/core/issues?q=is%3Aissue
Describe the bug
I am currently working on implementing the IDS API as Ansible modules.
The issue I'm running into is:
In all other API endpoints the get (api/ids/settings/get) returns the data of all sub-endpoints. This is very useful as external integrations need this information for linking config-objects.
To Reproduce
Steps to reproduce the behavior:
- Log-in on your Firewall
- Open
https://<YOUR-FW>/api/ids/settings/get - Open
https://<YOUR-FW>/api/monit/settings/get - Compare the responses
Expected behavior
Examples:
-
As expected - monit
Response:
{"monit": { "general": {...}, "alert": {...}, "service": {...}, "test": {...} }} -
IDS
{"ids": { "general": {...} }} -
IDS expected
{"ids": { "general": {...}, "rules": {...}, "policies": {...}, "userDefinedRules": {...}, "files": {...}, "fileTags": {...} }}
Describe alternatives you considered
I am aware that one COULD implement it the same way the WebUI does.
Example:
- Call
searchPolicyto get a list of UUIDs - Call
getPolicyFOR EACH ITEM to get the details (maybe even in async)
But this can get time-intensive as every HTTP request takes a few hundred MS. This may not sound like much, but it does not scale well.
Also: In my opinion an API should share the same behavior between endpoints.
Environment
OPNsense 23.7.10_1-amd64 FreeBSD 13.2-RELEASE-p7 OpenSSL 1.1.1w Common KVM processor (6 cores, 6 threads) [PVE] 8GB RAM
It's more likely other get() actions return less content in the future as these actions are intended to be used for the non nested items. Returning all for a form offering only a selection of data is far from optimal, which is why https://github.com/opnsense/core/issues/4123 exists.
When trying to build ACL's for partial applications returning all content on get() is also a (minor) security concern, although these situations currently don't exist (and should safeguard the setter as well). We could consider adding an endpoint specifically for integration purposes at some point.
@AdSchellevis Thank you for the quick response and important information.
It might be good if it would be mentioned in the documentation when such API-endpoints are known-to-be-deprecated. This would save a lot of time and work for projects that implement your APIs (:
Specific endpoints for 'external' integrations might make sense - at least for pulling all entries of one kind.
As the searchX + getX combination is very UI-specific.
Example:
-
api/ids/settings/searchPolicy{ "rows": [ { "uuid": "1d587b04-ebcb-41bd-9a19-e144c88b679e", "enabled": "1", "prio": "0", "description": "test1" }, { "uuid": "e1cdc774-053f-4236-9835-929a001ea1ff", "enabled": "1", "prio": "0", "description": "" } ], "rowCount": 2, "total": 2, "current": 1 } -
api/ids/settings/getPolicy/<UUID>{ "policy": { "enabled": "1", "prio": "0", "action": { "disable": { "value": "Disabled", "selected": 0 }, "alert": { "value": "Alert", "selected": 0 }, "drop": { "value": "Drop", "selected": 0 } }, "rulesets": { "01cb5c91-14e7-4e9d-898f-ca96fa9f3aee": { "value": "drop.rules", "selected": 0 } }, "content": { "affected_product.Any": { "value": "Any", "selected": 0 }, "attack_target.Any": { "value": "Any", "selected": 0 }, "classtype.misc-attack": { "value": "misc-attack", "selected": 0 }, "deployment.Perimeter": { "value": "Perimeter", "selected": 0 }, "signature_severity.Minor": { "value": "Minor", "selected": 0 }, "tag.Dshield": { "value": "Dshield", "selected": 0 } }, "new_action": { "default": { "value": "Default", "selected": 0 }, "alert": { "value": "Alert", "selected": 1 }, "drop": { "value": "Drop", "selected": 0 }, "disable": { "value": "Disable", "selected": 0 } }, "description": "test1" } }
This totally makes sense for the UI - as the row-view only needs very limited information and the 'detail' view needs them all. But if you want/need to get the details of all entries - one needs perform loads of API-calls.
@ansibleguy Well, the endpoints are not being deprecated at all, but contents may change (as with all api's in all products). The intend of this specific endpoint might have been misunderstood, which I can understand, but realistically this issue isn't really solvable from the documentation. Often our ticket system helps in finding some longer standing issues which are not pressing, but we expect to change at some point.
Please keep in mind our API's are developed for our ui in the first place, by making sure these endpoints are actively being used we also automatically increase test coverage, which is more problematic for specific endpoints. Our general rule of thumb is to prevent adding endpoints without consumers on our end as issues on these can not be replicated from the ui.
As the code behind all of this is quite consistent, I don't mind adding one endpoint in the base class which does expose the "raw" model (which is what get() is currently doing), we should just think of a proper name to avoid more confusion.
If your product actively uses the current get in multiple places, it might make sense to implement this new endpoint first and postpone changing the getters on our end for a bit longer to ease the transition on your end. There's no real rush in implementing https://github.com/opnsense/core/issues/4123, but it is very likely new components don't automatically include all data when get is being called.
@AdSchellevis Thank you for the response.
Deprecated may be the wrong word. But I would think such a major change of the response of an API endpoint should be transparent as it might break existing integrations.
I understand.
The current implementation of my 'product' uses the get endpoint for most modules - yes.
I would appreciate it, if there would be another endpoint that could be used.
I get the point that 'access' to the raw model might not be the cleanest approach .
In my opinion it would also be enough to enable the getX endpoints to list all existing entries. (by making the UUID optional?)
Example:
- query one:
api/ids/settings/getPolicy/<UUID> - query all:
api/ids/settings/getPolicy/orapi/ids/settings/getPolicyAll/
Just brainstorming - I'm not sure what design would make more sense.
The "context aware" ones will be difficult to implement in a generic way other than what's already available, in most cases you can just use the implemented getItem() and searchItem(). The search actions (as GET) without parameters will already return all available items for that section by the way, which is more or less the same as your example above.
When seeking a way to fetch all items from a model (with possibly more containers inside), the current get usually works, but has a high risk of future breakage. That's something we can solve at some point as the pattern doesn't need any context.
The only problem I got with the GET searchX endpoint, is that they do only return few attributes of an item.
That totally makes sense if they are rendered in an UI, but if an external tool needs/wants the full information of all items, many consecutive GET getX calls need to be performed.
Just wanted to inform you that this is not optimal from an 'outside perspective'. I can make it work either way with some 'workarounds' on the client-side.. (;
Could you tell me when it's planned that the get endpoints are changed? (https://github.com/opnsense/core/issues/4123)
Just so I can plan the migration.
nothing is planned yet, so we do have time to look for alternatives which works for both. Maybe it's also an option to change the default behavior of the searchBase() action to return all fields when none is specified and change the callers. In most cases it's not problematic to return all fields per record as the set itself is limited anyway.
@ansibleguy returning all fields for a search endpoint is likely easy to change for most of them, as they use searchBase(). I've changed the IDS ones so you can easily try this on your end as well, you need https://github.com/opnsense/core/commit/2d45b78f744059089078d56b3c108765b2d23608 and https://github.com/opnsense/core/commit/fb2a9b839159333f3b9b918310c93dfa144b673f, next you can query all policies using:
https://x.x.x.x/api/ids/settings/searchPolicy
To install using opnsense-patch, use:
opnsense-patch 2d45b78 fb2a9b8
If this helps you out, I don't mind changing the endpoints in core to return all fields available.
Sorry for the delay..
Your patch looks promising.
If it's that easy to implement this on your side, in my opinion - it would make sense to implement it. This could immensely simplify the API usage for external integrations.
For the record:
Before:
{"rows":[{"uuid":"978afa0f-6491-421a-bc6a-d709611d1552","enabled":"1","prio":"2","description":"ANSIBLE_TEST_1_1"}],"rowCount":1,"total":1,"current":1}
After applying 8855fd8:
{"rows":[{"uuid":"978afa0f-6491-421a-bc6a-d709611d1552","enabled":"1","prio":"2","action":"Alert,Drop","rulesets":"drop.rules","content":"Minor,Dshield","new_action":"Alert","description":"ANSIBLE_TEST_1_1"}],"rowCount":1,"total":1,"current":1}
The only drawback I could think of, is that each client will have to fetch & process (a little) more data on the OPNSense WebUI table views. This could be noticeable if the table entry-count is set to a few hundred. I tested it with searchinstalledrules and noticed a little more lag. (mainly JS)
@ansibleguy in some cases returning all might not be the best idea, but in most cases the extra fields don't matter that much. Let's assess this per case, if you miss data on a controller, just open a ticket for us to remove the static field choices.
@fichtner let's close indeed.