WIP: Merging of API and Manager to IntelMQ project #2428

gethvi · 2023-11-22T12:53:16Z

UPDATED.

Work in progress.

This PR merges API and Manager to the main IntelMQ repository.

Pros:

Users only need to install one piece of software.
No need to juggle with versions of three components.
We only need to make sure that the code works together within each commit.
Less packaging.
Easier implementation when pieces of code are aware of each other and you can include/import them.
Users can disable API and/or Manager easily.
One unified configuration file.

Cons:

The additional code and requirements take few more kB of disk space.

Updated project structure:

intelmq
├── app
│   ├── api       # <-- updated sources of intelmq-api
│   └── webgui    # <-- updated sources of intelmq-manager
├── bin
├── bots
├── etc
├── lib
└── tests
    └── app
        └── api   # <-- updated tests of intelmq-api

This PR adds intelmq.yaml configuration file with server block and adds new options, example:

server:
  host: 127.0.0.1
  port: 8080
  workers: 3
  debug: false
  enable_webgui: true
  session_store: /var/lib/intelmq/server/sessions.sqlite
  access_log: /var/log/intelmq/access.log
  allowed_path: /var/lib/intelmq/bots/
  intelmq_ctl_cmd:
    - /usr/bin/intelmqctl

This PR changes of Manager (webgui):

Mako templates changed to jinja templates.
Serves the website using FastAPI (used for the API as well).
All links are dynamically generated.
Links changed from /management.html to nicer /management etc.
Removes rendering of login form from HTML site when session_store is disabled.
Removes problem with double slash URL.

This PR integrates intelmq-add-user into one consistent CLI:

intelmq
intelmq server start --debug --port 8080 --host 127.0.0.1
intelmq server adduser --username intelmq --password secret

It is possible to run either:

production ready gunicorn server with intelmq server start
development uvicorn server with intelmq server start --debug.

Cleans up and updates intelmqsetup from code no longer needed.

Changes positions.conf file path to /var/lib/intelmq/server/positions.json. This file is hardly ever edited manually by the user, no need to put it in etc.

intelmq/api/router.py

…ation options, fixes double slash issue.

…er, hides login button when session_store is null, fixes double slash issue.

…hecks.

….json.

kamil-certat

Hey, I definitely support merging all projects, I think we're almost there (for me).

I have the following general questions/concerns

Confusing names

You have introduced the names "app" and "server" into the code, which I think can be confusing. It looks like a very core part to me (especially with the 'intelmq server start' command - it looks more like starting the botnet to me), but in our case the "very core" are bots, and I'd like to avoid that. Also, the "app" is in one place, then "serve" in others, and they don't map to each other - which makes it harder to find the right part of the code if you don't have the inside knowledge. I spent a long time thinking about how to solve that. What do you think of the name "web"? We can use it instead of "app" and "server" everywhere: in directory structures and commands, and it would fit better and say better what this piece of code is about.

Renaming the IntelMQ Manager

I see you have introduced the term "WebUI" into the code instead of IntelMQ Manager. It's functionally correct, IntelMQ Manager is the web user interface, but my experience says that every renaming and difference in names used by users and developers makes the onboarding harder for both, and just leads to more time spent on explanations and reviews (I had a few projects struggling with this...). Not to mention the Google/Microsoft culture of renaming tools every month ;). I don't see any benefit in changing the code to webui, it's exactly the same as the established name of IntelMQ Manager. Could we keep the name "Manager" in the code as well?

Broken support for non-Docker deployments and customisation

The one-line startup command is a great thing, and also the right way to work with Docker-like deployments where we communicate with an external reverse proxy via IP:PORT sockets. However, IntelMQ still supports (and is the primary method for) VM-native deployments. There are a few things to keep in mind:

We need to provide SystemD services to run our web service (you started this),
we need to provide a sample reverse proxy configuration and not rely on an external one (the config will then be installed by DEB package and/or manually tuned) - this is not provided (historically we have chosen Apache, so let's stick with it as the default),
The communication between the service and the proxy should go through a Unix socker - it's an important thing, in a dockerised environment you limit who can talk to whom through different bridges (networks). In a VM-native environment, the network stack is (to simplify a bit) one for all on the machine, and so the privileges set on the unix socket are the way to say who can use the connection.
There are a lot of SystemD unit configuration files in the contrib/ directory, and this is a mix of new and old (no longer working).

Please review the unit files and apache config to make them work again: Apache handles external connections (under /intelmq/... for API and /intelmq-manager for the Manager) -> proxy them to the socket -> socket is handled by FastAPI. I can help with testing.

We also need to allow administrators to change the Gunicorn configuration in production deployments. There are three ways to do this: 1) mention using the Gunicorn Env variable to do this, 2) let proxy all configs through our CLI (I really don't want to do this), 3) document running web application without our command line

API path changes

These are big & breaking changes that shouldn't be done in a mature software without careful consideration. Changing Manager paths is fine for me, but API can be used without it.

Lack of documentation - but this should rather go in a separate PR, this one is big enough :)

Changes positions.conf file path to /var/lib/intelmq/server/positions.json. This file is hardly ever edited manually by the user, no need to put it in etc.

I support this change, but also have to challenge your assumption: any kind of automated deployment that modifies bots has to modify it - otherwise IntelMQ Manager won't work. So in my setup, the Ansible scripts generate default locations and modify them without any interaction with the manager :)

kamil-certat · 2024-03-20T15:14:58Z

contrib/app/api/intelmq-api.service

+User=www-data
+Group=www-data
+RuntimeDirectory=gunicorn
+WorkingDirectory=/usr/lib/python3/dist-packages/intelmq_api/


Here should be a valid path to the IntelMQ package (if installed from deb)

Sorry, I forgot to delete this file.

kamil-certat · 2024-03-20T15:15:20Z

contrib/app/api/intelmq-api.service

+Group=www-data
+RuntimeDirectory=gunicorn
+WorkingDirectory=/usr/lib/python3/dist-packages/intelmq_api/
+ExecStart=/usr/bin/gunicorn intelmq_api.main:app --workers 4 --worker-class uvicorn.workers.UvicornWorker --bind unix:intelmq_api.sock


This should be updated to use new running method

Also forgot to delete this, it is replaced by intelmq/debian/systemd/intelmq.service.

kamil-certat · 2024-03-20T15:15:34Z

contrib/app/api/intelmq-api.socket

+Description=The socket to handle IntelMQ API requests
+
+[Socket]
+ListenStream=/usr/lib/python3/dist-packages/intelmq_api/intelmq_api.sock


This path should be updated

kamil-certat · 2024-03-20T15:17:00Z

contrib/app/webgui/manager-apache.conf

+#
+# SPDX-License-Identifier: CC0-1.0
+
+Alias /intelmq-manager /usr/share/intelmq_manager/html/


Is this path still valid?

kamil-certat · 2024-03-20T15:20:26Z

debian/systemd/intelmq.service

@@ -0,0 +1,17 @@
+[Unit]


Should it replace intelmq-api.service? Please also name it rather intelmq-web to say clear, that it's not the botnet service

kamil-certat · 2024-03-21T09:24:04Z

intelmq/app/server.py

+
+
+app.add_middleware(CORSMiddleware, allow_origins=config.allow_origins, allow_methods=("GET", "POST"))
+app.include_router(api_router, prefix="/api/v1")


❌ This is a breaking change in the API path. We currently use the v1/api, and as so, please do not silently change paths. It then breaks any API usage we don't control.

My argument is that while technically the API is publicly available, it is not public per se, currently the endpoints are not documented for the outside use nor do we explicitly say "this is a public API, feel free to use it". As it stands right now it is more of an internal API used for the Manager only. As such I don't think we need to "support" it for other usage.
It's kind of like if you reverse engineer an internal API of some sort, you can't expect the developer to keep you in mind and don't change the API because of you.

kamil-certat · 2024-03-21T09:35:01Z

intelmq/bin/intelmqsetup.py

❌ I definitely disagree with removing support for webserver/reverse proxy configuration, as well as deprecating disabling installing API and/or IntelMQ Manager.

Please note, IntelMQ is still used in many VM-native deployments. The reverse proxy configuration is there a basic requirement. This is not needed in Docker-based deployments, and because of that it should be the optional behaviour of the intelmqsetup. Removing this support creates a big headache for admins of current IntelMQ deployments (including me), and drops the ability to automatically update them.

After some private discussion: I agree that - as a principle - modifying Apache/nginx in the setup script goes a little beyond the scope of the IntelMQ itself, and it would be right to let the example Apache config be only in the documentation.

As it's breaking change for existing deployments and may require some manual intervention, I'm not sure how to handle it within DEB packages.

kamil-certat · 2024-03-21T09:36:16Z

scripts/intelmq-api-setup-systemd

@@ -0,0 +1,64 @@
+#!/bin/bash


This script will most probably not work with the changes

kamil-certat · 2024-03-21T09:37:50Z

intelmq/app/config.py

+
+    debug: bool = False
+
+    def __init__(self):


NIT: This loader looks like a little overkill that could mostly be replaced by a loop ;)

intelmq/__init__.py

gethvi · 2024-03-21T11:53:54Z

Confusing names

You have introduced the names "app" and "server" into the code, which I think can be confusing. It looks like a very core part to me (especially with the 'intelmq server start' command - it looks more like starting the botnet to me), but in our case the "very core" are bots, and I'd like to avoid that. Also, the "app" is in one place, then "serve" in others, and they don't map to each other - which makes it harder to find the right part of the code if you don't have the inside knowledge. I spent a long time thinking about how to solve that. What do you think of the name "web"? We can use it instead of "app" and "server" everywhere: in directory structures and commands, and it would fit better and say better what this piece of code is about.

I see. The logic was:

app is an instance of FastAPI Application and it consists of two separate parts (FastAPI routers really): api and webgui where loading the webgui (old Manager) is optional.
server was ment to mean the Uvicorn/Gunicorn instance of whichever one runs the app

But I can see that the code might not fully reflect this logic right now.

Renaming the IntelMQ Manager

I see you have introduced the term "WebUI" into the code instead of IntelMQ Manager. It's functionally correct, IntelMQ Manager is the web user interface, but my experience says that every renaming and difference in names used by users and developers makes the onboarding harder for both, and just leads to more time spent on explanations and reviews (I had a few projects struggling with this...). Not to mention the Google/Microsoft culture of renaming tools every month ;). I don't see any benefit in changing the code to webui, it's exactly the same as the established name of IntelMQ Manager. Could we keep the name "Manager" in the code as well?

The thought behind this change is that now the IntelMQ Manager is not a standalone tool anymore, it doesn't really need it's own name because it is just a part of IntelMQ. The user/administrator does not come in contact with the name Manager anymore. The only place where they come in contact with it is in the config which now provides optional boolean enable_webgui. From a new users perspective I find this way more self explaining than enable_manager.

Anyone slightly experienced with IntelMQ should be able to put two and two together and figure out that webgui probably means the only web interface IntelMQ ever had - the Manager.

The website itself will have just IntelMQ in it's title now.

Technically we are not "renaming" the tool, we are merging it to another bigger tool. From now on it is all just "IntelMQ". And as a part of the bigger tools, the code serves the purpose of providing webgui. :)

gethvi marked this pull request as draft November 22, 2023 12:53

gethvi force-pushed the merge-projects branch 2 times, most recently from a17e213 to 5a16e32 Compare November 22, 2023 13:01

github-advanced-security bot found potential problems Nov 22, 2023

View reviewed changes

intelmq/api/router.py Fixed Show fixed Hide fixed

intelmq/api/router.py Fixed Show fixed Hide fixed

gethvi force-pushed the merge-projects branch 7 times, most recently from ab1c04a to 82d442e Compare November 22, 2023 15:27

gethvi force-pushed the merge-projects branch 19 times, most recently from 5823270 to 36ca4d4 Compare December 13, 2023 19:46

gethvi force-pushed the merge-projects branch 15 times, most recently from 5876e69 to b5b861e Compare February 1, 2024 17:20

gethvi added this to the 3.4.0 milestone Mar 1, 2024

gethvi force-pushed the merge-projects branch from b5b861e to 17fe30b Compare March 4, 2024 10:14

gethvi marked this pull request as ready for review March 14, 2024 09:26

gethvi added 9 commits March 14, 2024 10:27

Adds intelmq-api sources. Just copypasted.

5c136fb

Adds intelmq-manager sources. Just copypasted.

977bf1c

Changes to the API: changes configuration to YAML, adds more configur…

a99aae4

…ation options, fixes double slash issue.

Changes to the WebGUI: changes to jinja2 templates, uses FastAPI serv…

100fe6d

…er, hides login button when session_store is null, fixes double slash issue.

Fixes dependencies, python packaging, licenses, typos and codespell c…

0f12e36

…hecks.

Adds consistent CLI for running the server and adding users.

cb328a6

Updates intelmqsetup and remove unnecessary code.

aef5110

Updates debian packaging.

6194ee0

Changes positions.conf file path to /var/lib/intelmq/server/positions…

7a610e3

….json.

gethvi force-pushed the merge-projects branch from 17fe30b to 7a610e3 Compare March 14, 2024 09:27

kamil-certat requested changes Mar 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Merging of API and Manager to IntelMQ project #2428

WIP: Merging of API and Manager to IntelMQ project #2428

gethvi commented Nov 22, 2023 •

edited

kamil-certat left a comment

kamil-certat Mar 20, 2024

gethvi Mar 21, 2024

kamil-certat Mar 20, 2024

gethvi Mar 21, 2024

kamil-certat Mar 20, 2024

kamil-certat Mar 20, 2024

kamil-certat Mar 20, 2024

kamil-certat Mar 21, 2024

gethvi Mar 21, 2024

kamil-certat Mar 21, 2024

kamil-certat Mar 21, 2024

kamil-certat Mar 21, 2024

kamil-certat Mar 21, 2024

gethvi commented Mar 21, 2024 •

edited



		app.add_middleware(CORSMiddleware, allow_origins=config.allow_origins, allow_methods=("GET", "POST"))
		app.include_router(api_router, prefix="/api/v1")

WIP: Merging of API and Manager to IntelMQ project #2428

Are you sure you want to change the base?

WIP: Merging of API and Manager to IntelMQ project #2428

Conversation

gethvi commented Nov 22, 2023 • edited

kamil-certat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gethvi commented Mar 21, 2024 • edited

gethvi commented Nov 22, 2023 •

edited

gethvi commented Mar 21, 2024 •

edited