Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge group files endpoints #21653

Merged
merged 8 commits into from
Feb 1, 2024
Merged

Conversation

GGP1
Copy link
Member

@GGP1 GGP1 commented Jan 29, 2024

Related issue
Closes #20851

Description

Merges the /groups/:group_id/files/:file_name/json and /groups/:group_id/files/:file_name/xml endpoints into a single one that takes the raw parameter which defines whether the response content will be formatted in json or plain text. The response content-type is defined based on the file extension retrieved.

Logs/Alerts example

Get merged.mg in plain text

GET /groups/default/files/merged.mg?raw=true

#default
!228 ar.conf
restart-ossec0 - restart-ossec.sh - 0
restart-ossec0 - restart-ossec.cmd - 0
restart-wazuh0 - restart-ossec.sh - 0
restart-wazuh0 - restart-ossec.cmd - 0
restart-wazuh0 - restart-wazuh - 0
restart-wazuh0 - restart-wazuh.exe - 0
!76 agent.conf
<agent_config>

  <!-- Shared agent configuration here -->

</agent_config>
!28411 cis_apache2224_rcl.txt
...
Get merged.mg in JSON

GET /groups/default/files/merged.mg

{
    "data": [
        {
            "file_name": "ar.conf",
            "file_size": 228,
            "file_content": "restart-ossec0 - restart-ossec.sh - 0\nrestart-ossec0 - restart-ossec.cmd - 0\nrestart-wazuh0 - restart-ossec.sh - 0\nrestart-wazuh0 - restart-ossec.cmd - 0\nrestart-wazuh0 - restart-wazuh - 0\nrestart-wazuh0 - restart-wazuh.exe - 0\n"
        },
        {
            "file_name": "agent.conf",
            "file_size": 76,
            "file_content": "<agent_config>\n\n  <!-- Shared agent configuration here -->\n\n</agent_config>\n"
        },
        ...
 	]
}
Get file content using a configuration type

GET /groups/default/files/agent.conf?type=conf

{
    "data": {
        "total_affected_items": 1,
        "affected_items": [
            {
                "filters": {},
                "config": {}
            }
        ]
    },
    "error": 0
}

Failed checks are not related to the changes introduced.

@GGP1 GGP1 self-assigned this Jan 29, 2024
@GGP1 GGP1 linked an issue Jan 29, 2024 that may be closed by this pull request
@GGP1 GGP1 force-pushed the fix/20851-merge-group-files-formats branch from 1412554 to 586795c Compare January 29, 2024 18:15
EduLeon12
EduLeon12 previously approved these changes Jan 31, 2024
Copy link
Contributor

@EduLeon12 EduLeon12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @GGP1

LGTM!,

My only question is if it would be better for readibility purposes to change the raw default value to a bool False instead of `None.

@GGP1
Copy link
Member Author

GGP1 commented Jan 31, 2024

My only question is if it would be better for readibility purposes to change the raw default value to a bool False instead of `None.

I've seen both in the code, I don't think there's a standard but I agree that using False is slightly better so I modified it. Thanks!

@GGP1 GGP1 force-pushed the fix/20851-merge-group-files-formats branch from f84e142 to cedccbc Compare January 31, 2024 12:08
Copy link
Contributor

@Selutario Selutario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just some minor changes were requested. Also, I found a strange behaviour. I guess this is expected, but I would like to confirm.

I tried creating a test.json file. When the content-type returned is application/json, it is seen this way:

curl -k -X GET "https://localhost:55000/groups/default/files/test.json?raw=true" -H "Authorization: Bearer $TOKEN" -v

image

However, this does not happen when xml content is returned as application/xml:
image

Nor when the json file is returned as plain 🤔:
image

@@ -1360,9 +1361,9 @@ async def get_group_files(request, group_id: str, pretty: bool = False, wait_for
return web.json_response(data=data, status=200, dumps=prettify if pretty else dumps)


async def get_group_file_json(request, group_id: str, file_name: str, pretty: bool = False,
async def get_group_file(request, group_id: str, file_name: str, raw: bool = False, pretty: bool = False,
wait_for_complete: bool = False) -> web.Response:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wait_for_complete: bool = False) -> web.Response:
wait_for_complete: bool = False) -> web.Response | ConnexionResponse:

api/api/controllers/agent_controller.py Show resolved Hide resolved
response = ConnexionResponse(body=data["data"], mimetype='application/xml')

return response
return ConnexionResponse(body=data["data"], mimetype=mimetype)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return ConnexionResponse(body=data["data"], mimetype=mimetype)
return ConnexionResponse(body=data["data"], mimetype=mimetype)


# ![file_size] [file_name]
regex_header = re.compile(r"^!(\d+)\s*(.*)")
regex_comment = re.compile(r"^\s*#")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's worth skipping comments since the way to mark a comment could be different in each file inside the merged.mg. The agent.conf comments (for example) are still being included while those in rcl files are not.

The first line should be skipped though.

@@ -770,33 +820,33 @@ def get_agent_conf_multigroup(multigroup_id: str = None, offset: int = 0, limit:
return {'totalItems': len(data), 'items': cut_array(data, offset=offset, limit=limit)}


def get_file_conf(filename: str, group_id: str = None, type_conf: str = None, return_format: str = None) -> dict:
"""Return the configuration file as dictionary.
def get_file_conf(filename: str, group_id: str = None, type_conf: str = None, raw: bool = False) -> dict:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def get_file_conf(filename: str, group_id: str = None, type_conf: str = None, raw: bool = False) -> dict:
def get_file_conf(filename: str, group_id: str = None, type_conf: str = None, raw: bool = False) -> dict | str:


Raises
------
WazuhResourceNotFound(1710)
Group was not found.
WazuhError(1006)
agent.conf does not exist or there is a problem with the permissions.
The file does not exist or there is a problem with the permissions.
WazuhError(1104)
Invalid file type.

Returns
-------
dict
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dict
dict or str

@GGP1
Copy link
Member Author

GGP1 commented Feb 1, 2024

Looks good! Just some minor changes were requested. Also, I found a strange behaviour. I guess this is expected, but I would like to confirm.

I tried creating a test.json file. When the content-type returned is application/json, it is seen this way:

curl -k -X GET "https://localhost:55000/groups/default/files/test.json?raw=true" -H "Authorization: Bearer $TOKEN" -v

image

@Selutario This is expected, the content of the file is returned as a string, and if the Content-Type is json then it will be interpreted as a json string instead of an object.

In order to include all the content inside the string, new lines have to be literal and double quotes have to be escaped. The other content types do not behave the same because they don't need to escape or wrap anything (and thus the output is the actual file content).

@GGP1 GGP1 force-pushed the fix/20851-merge-group-files-formats branch from 02c18e5 to b4f9e3a Compare February 1, 2024 14:14
Copy link
Contributor

@Selutario Selutario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think we'll need to update the documentation too, at least the RBAC reference.

Please, open a PR for this @GGP1

@Selutario Selutario merged commit 9e41df9 into master Feb 1, 2024
51 of 53 checks passed
@Selutario Selutario deleted the fix/20851-merge-group-files-formats branch February 1, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

default group merged.mg error
3 participants