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.

Sign up for GitHub

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

Internal Admin API and Draupnir Hjack Command Config #3389

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+50 −1
Open

Internal Admin API and Draupnir Hjack Command Config #3389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d3c031
Enable Internal Admin API Access separately from Public access.
FSG-Cat Jun 28, 2024
3a92b63
Add Config variable for Draupnir Hijack command
FSG-Cat Jun 28, 2024
9d24643
Apply suggestions from code review
FSG-Cat Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions group_vars/matrix_servers
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2747,6 +2747,8 @@ matrix_bot_draupnir_container_image_self_build: "{{ matrix_architecture not in [

matrix_bot_draupnir_container_network: "{{ matrix_addons_container_network }}"

matrix_bot_draupnir_admin_api_enabled: "{{ matrix_bot_draupnir_room_hijack_enabled }}"

matrix_bot_draupnir_container_additional_networks_auto: |-
{{
([] if matrix_addons_homeserver_container_network == '' else [matrix_addons_homeserver_container_network])
Expand Down Expand Up @@ -4307,6 +4309,7 @@ matrix_synapse_container_labels_public_client_root_redirection_enabled: "{{ matr
matrix_synapse_container_labels_public_client_root_redirection_url: "{{ (('https://' if matrix_playbook_ssl_enabled else 'http://') + matrix_server_fqn_element) if matrix_client_element_enabled else '' }}"

matrix_synapse_container_labels_public_client_synapse_admin_api_enabled: "{{ matrix_synapse_admin_enabled }}"
matrix_synapse_container_labels_internal_client_synapse_admin_api_enabled: "{{ matrix_bot_draupnir_admin_api_enabled }}"
Copy link
Owner

Choose a reason for hiding this comment

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

The matrix_synapse_container_labels_internal_client_synapse_admin_api_enabled variable is written to here, but it's not defined in defaults/main.yml in the matrix-synapse role.

There should be a definition with some default value (false) there.


matrix_synapse_container_labels_public_federation_api_traefik_hostname: "{{ matrix_server_fqn_matrix_federation }}"
matrix_synapse_container_labels_public_federation_api_traefik_entrypoints: "{{ matrix_federation_traefik_entrypoint_name }}"
Expand Down Expand Up @@ -4466,6 +4469,7 @@ matrix_synapse_reverse_proxy_companion_container_labels_traefik_hostname: "{{ ma

matrix_synapse_reverse_proxy_companion_container_labels_public_client_synapse_client_api_enabled: "{{ matrix_synapse_container_labels_public_client_synapse_client_api_enabled }}"
matrix_synapse_reverse_proxy_companion_container_labels_public_client_synapse_admin_api_enabled: "{{ matrix_synapse_container_labels_public_client_synapse_admin_api_enabled }}"
matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_enabled: "{{ matrix_synapse_container_labels_internal_client_synapse_admin_api_enabled }}"
Copy link
Owner

Choose a reason for hiding this comment

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

Like matrix_synapse_container_labels_internal_client_synapse_admin_api_enabled, the matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_enabled variable is written to here, but it's not defined in defaults/main.yml in the matrix-synapse-reverse-proxy-companion role.

There should be a definition with some default value (false) there.


matrix_synapse_reverse_proxy_companion_container_labels_public_federation_api_traefik_entrypoints: "{{ matrix_synapse_container_labels_public_federation_api_traefik_entrypoints }}"
matrix_synapse_reverse_proxy_companion_container_labels_public_federation_api_traefik_tls: "{{ matrix_synapse_container_labels_public_federation_api_traefik_tls }}"
Expand Down
7 changes: 7 additions & 0 deletions roles/custom/matrix-bot-draupnir/defaults/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@ matrix_bot_draupnir_raw_homeserver_url: ""
# Its Exposed here because its common enough to be valid to expose.
matrix_bot_draupnir_disable_server_acl: "false"

# Used to control if the Synapse Admin API is exposed internally to the containers and therefore giving Draupnir Access.
matrix_bot_draupnir_admin_api_enabled: false

# Controls if the draupnir room hijack command is activated or not. This also automatically enables the internal admin API
# in the process of activation.
matrix_bot_draupnir_room_hijack_enabled: false

# Default configuration template which covers the generic use case.
# You can customize it by controlling the various variables inside it.
#
Expand Down
2 changes: 1 addition & 1 deletion roles/custom/matrix-bot-draupnir/templates/production.yaml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ admin:
# (with enough permissions) to "make" a user an admin.
#
# This only works if a local user with enough admin permissions is present in the room.
enableMakeRoomAdminCommand: false
enableMakeRoomAdminCommand: {{ matrix_bot_draupnir_room_hijack_enabled | to_json }}

# Misc options for command handling and commands
commands:
Expand Down
38 changes: 38 additions & 0 deletions roles/custom/matrix-synapse-reverse-proxy-companion/templates/labels.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,44 @@ traefik.http.routers.matrix-synapse-reverse-proxy-companion-public-client-synaps
############################################################
{% endif %}

{% if matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_enabled %}
Copy link
Owner

Choose a reason for hiding this comment

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

It seems like matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_enabled actually enables 2 things right now: /_synapse/client and /_synapse/admin.

For the "public" exposure of these, we have dedicated variables: matrix_synapse_reverse_proxy_companion_container_labels_public_client_synapse_client_api_enabled and matrix_synapse_reverse_proxy_companion_container_labels_public_client_synapse_admin_api_enabled.

So it's odd to have a single variable (matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_enabled) exposing both right now.

I suggest to create an additional variable (matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_client_api_enabled) and use that one to decide whether to expose /_synapse/client.

Of course, you can wire the new variable (matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_client_api_enabled) in group_vars/matrix_servers the same way (based on matrix_bot_draupnir_room_hijack_enabled).

I don't know if Draupnir room hijacking requires both /_synapse/admin and /_synapse/client though. If it only requires the former, then there's no need to enable the latter based on matrix_bot_draupnir_room_hijack_enabled.

You've added labels to matrix-synapse-reverse-proxy-companion, which handles the workers-being-enabled case.

Similar labels should be added to roles/custom/matrix-synapse/templates/labels.j2 as well.
Along with those, variables for each of these should be defined in the matrix-synapse role. Example:

  • matrix_synapse_container_labels_internal_client_synapse_admin_api_enabled
  • matrix_synapse_container_labels_internal_client_synapse_admin_api_traefik_rule
  • matrix_synapse_container_labels_internal_client_synapse_admin_api_entrypoints
  • matrix_synapse_container_labels_internal_client_synapse_client_api_enabled
  • matrix_synapse_container_labels_internal_client_synapse_client_api_traefik_rule
  • matrix_synapse_container_labels_internal_client_synapse_client_api_entrypoints

The matrix-synapse-reverse-proxy-companion role tends to inherit (via group_vars/matrix_overrides) various variable values from matrix-synapse (here is an example), so you can wire the new matrix-synapse-reverse-proxy-companion variables (enabled, entrypoints, etc.) the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should note that when it comes to this PR i am blindly taking the labels from the issue in question.

So like it makes sense that these flaws exist. And i need to thank you for this very thuroughly written review.

I will try to take a look at this during the week to see how much of this stuff i can fix on my own based on all this valuable feedback and trying to look at other roles more closely to understand how they are plumbed. (Primarily Synapse admin i think i will need to look closer at as i think it helps a lot.)

As for the exact API surface Draupnir needs for its admin commands i will come back with an answer on that front as i honestly dont know but i know i can find out.

Copy link
Owner

Choose a reason for hiding this comment

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

That's understandable! Thank you for looking into it!

############################################################
# #
# Internal Synapse Admin API (/_synapse/client) #
# #
############################################################

traefik.http.routers.matrix-synapse-reverse-proxy-companion-internal-client-synapse-client-api.rule=PathPrefix(`/_synapse/client`)
Copy link
Owner

Choose a reason for hiding this comment

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

It'd be better if the rule is coming from a variable (matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_traefik_rule), defined in default/main.yml in the matrix-synapse-reverse-proxy-companion role.

Here's an example:

matrix-docker-ansible-deploy/roles/custom/matrix-synapse-reverse-proxy-companion/defaults/main.yml

Line 78 in e000cbf

matrix_synapse_reverse_proxy_companion_container_labels_internal_client_api_traefik_rule: "PathPrefix(`{{ matrix_synapse_reverse_proxy_companion_container_labels_internal_client_api_traefik_path_prefix }}`)"

You may even consider adding a _priority variable (like this), getting its value from the matrix-synapse role. In the matrix-synapse role, a priority of 0 is a good default, I suppose.



traefik.http.routers.matrix-synapse-reverse-proxy-companion-internal-client-synapse-client-api.service=matrix-synapse-reverse-proxy-companion-client-api
traefik.http.routers.matrix-synapse-reverse-proxy-companion-internal-client-synapse-client-api.entrypoints=matrix-internal-matrix-client-api
Copy link
Owner

Choose a reason for hiding this comment

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

It'd be better if the entrypoints are coming from a variable (matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_traefik_entrypoints), defined in default/main.yml in the matrix-synapse-reverse-proxy-companion role.

Then, group_vars/matrix_servers can pass the correct internal entrypoint like this:

matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_traefik_entrypo: "{{ matrix_playbook_internal_matrix_client_api_traefik_entrypoint_name }}"

Here's an example:

matrix-docker-ansible-deploy/group_vars/matrix_servers

Line 4320 in e000cbf

matrix_synapse_container_labels_internal_client_api_traefik_entrypoints: "{{ matrix_playbook_internal_matrix_client_api_traefik_entrypoint_name }}"


############################################################
# #
# /Internal Synapse Admin API (/_synapse/client) #
# #
############################################################


############################################################
# #
# Internal Synapse Admin API (/_synapse/admin) #
# #
############################################################

traefik.http.routers.matrix-synapse-reverse-proxy-companion-internal-client-synapse-admin-api.rule=PathPrefix(`/_synapse/admin`)


traefik.http.routers.matrix-synapse-reverse-proxy-companion-internal-client-synapse-admin-api.service=matrix-synapse-reverse-proxy-companion-client-api
traefik.http.routers.matrix-synapse-reverse-proxy-companion-internal-client-synapse-admin-api.entrypoints=matrix-internal-matrix-client-api
Comment on lines +148 to +152
Copy link
Owner

Choose a reason for hiding this comment

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

Same as for /_synapse/client above, it'd be better if these are not using hardcoded values, but the values are coming from variables.


############################################################
# #
# /Internal Synapse Admin API (/_synapse/admin) #
# #
############################################################
{% endif %}

{% if matrix_synapse_reverse_proxy_companion_container_labels_public_federation_api_enabled %}
############################################################
Expand Down
Loading