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

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

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

gethvi
Copy link
Contributor

@gethvi gethvi commented Nov 22, 2023

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

  1. 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

  1. 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.

  1. 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.

  1. Cleans up and updates intelmqsetup from code no longer needed.

  1. 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.

@gethvi gethvi marked this pull request as draft November 22, 2023 12:53
@gethvi gethvi force-pushed the merge-projects branch 2 times, most recently from a17e213 to 5a16e32 Compare November 22, 2023 13:01
intelmq/api/router.py Fixed Show fixed Hide fixed
intelmq/api/router.py Fixed Show fixed Hide fixed
@gethvi gethvi force-pushed the merge-projects branch 7 times, most recently from ab1c04a to 82d442e Compare November 22, 2023 15:27
@gethvi gethvi force-pushed the merge-projects branch 19 times, most recently from 5823270 to 36ca4d4 Compare December 13, 2023 19:46
@gethvi gethvi force-pushed the merge-projects branch 15 times, most recently from 5876e69 to b5b861e Compare February 1, 2024 17:20
@gethvi gethvi added this to the 3.4.0 milestone Mar 1, 2024
@gethvi gethvi marked this pull request as ready for review March 14, 2024 09:26
Copy link
Contributor

@kamil-certat kamil-certat left a comment

Choose a reason for hiding this comment

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

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

I have the following general questions/concerns

  1. 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.

  1. 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?

  1. 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:

  1. We need to provide SystemD services to run our web service (you started this),
  2. 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),
  3. 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

  1. 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.

  1. 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 :)

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I forgot to delete this file.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be updated to use new running method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Description=The socket to handle IntelMQ API requests

[Socket]
ListenStream=/usr/lib/python3/dist-packages/intelmq_api/intelmq_api.sock
Copy link
Contributor

Choose a reason for hiding this comment

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

This path should be updated

#
# SPDX-License-Identifier: CC0-1.0

Alias /intelmq-manager /usr/share/intelmq_manager/html/
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this path still valid?

@@ -0,0 +1,17 @@
[Unit]
Copy link
Contributor

Choose a reason for hiding this comment

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

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



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

Choose a reason for hiding this comment

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

❌ 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

❌ 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

This script will most probably not work with the changes


debug: bool = False

def __init__(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

intelmq/__init__.py Show resolved Hide resolved
@gethvi
Copy link
Contributor Author

gethvi commented Mar 21, 2024

  1. 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.

  1. 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. :)

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.

2 participants