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

Jump to bottom

draft encryption support for hookshot #2979

Closed
wants to merge 2 commits into from
Closed

draft encryption support for hookshot #2979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94abf2d
draft encryption support for hookshot
HarHarLinks Nov 1, 2023
9905309
amend docs
HarHarLinks Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/configuring-playbook-bridge-hookshot.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ Other configuration options are available via the `matrix_hookshot_configuration

Finally, run the playbook (see [installing](installing.md)).

### End-to-bridge endcryption
Copy link

Choose a reason for hiding this comment

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

Typo: endcryption -> encryption


1. Enable by setting `matrix_hookshot_experimental_encryption_enabled: true`

If the crypto store has become corrupted, reset it by running `ansible-playbook -i inventory/hosts setup.yml -K --tags=reset-hookshot-encryption`.

## Usage

Expand Down
2 changes: 1 addition & 1 deletion group_vars/matrix_servers
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3358,7 +3358,7 @@ ntfy_visitor_request_limit_exempt_hosts_hostnames_auto: |
#
######################################################################

redis_enabled: "{{ matrix_synapse_workers_enabled }}"
redis_enabled: "{{ matrix_synapse_workers_enabled or matrix_hookshot_experimental_encryption_enabled }}"

redis_identifier: matrix-redis

Expand Down
4 changes: 4 additions & 0 deletions roles/custom/matrix-bridge-hookshot/defaults/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ matrix_hookshot_public_endpoint: /hookshot
matrix_hookshot_appservice_port: 9993
matrix_hookshot_appservice_endpoint: "{{ matrix_hookshot_public_endpoint }}/_matrix/app"

# Controls whether the experimental end-to-bridge encryption support is enabled.
# This requires that support is also enabled in the homeserver, see the hookshot docs.
matrix_hookshot_experimental_encryption_enabled: false

# Controls whether metrics are enabled in the bridge configuration.
# Enabling them is usually enough for a local (in-container) Prometheus to consume them.
# If metrics need to be consumed by another (external) Prometheus server, consider exposing them via `matrix_hookshot_metrics_proxying_enabled`.
Expand Down
6 changes: 6 additions & 0 deletions roles/custom/matrix-bridge-hookshot/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@
- when: matrix_hookshot_enabled | bool
ansible.builtin.include_tasks: "{{ role_path }}/tasks/inject_into_nginx_proxy.yml"

- tags:
- reset-hookshot-encryption
block:
- when: matrix_hookshot_enabled | bool
ansible.builtin.include_tasks: "{{ role_path }}/tasks/reset_encryption.yml"

- tags:
- setup-all
- setup-hookshot
Expand Down
12 changes: 12 additions & 0 deletions roles/custom/matrix-bridge-hookshot/tasks/reset_encryptioon.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
- name: Resetting Hookshot's crypto store
ansible.builtin.command:
cmd: |
{{ devture_systemd_docker_base_host_command_docker }} run
--rm
--name={{ matrix_hookshot_container_url }}-reset-crypto
Copy link
Owner

Choose a reason for hiding this comment

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

It's quite unfortunate that the container-name (and what we call _ident in our roles) is called matrix_hookshot_container_url here, making you think it's http://something-something -- a value unsuitable for a container name.

In actuality, matrix_hookshot_container_url is just matrix-hookshot - a container name / identifier.

--user={{ matrix_user_uid }}:{{ matrix_user_gid }}
--cap-drop=ALL
-v {{ matrix_hookshot_base_path }}/config.yml:/config.yml
{{ matrix_hookshot_docker_image }} yarn start:resetcrypto
changed_when: false
8 changes: 8 additions & 0 deletions roles/custom/matrix-bridge-hookshot/templates/config.yml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,14 @@ metrics:
# (Optional) Prometheus metrics support
#
enabled: {{ matrix_hookshot_metrics_enabled | to_json }}
{% if matrix_hookshot_experimental_encryption_enabled %}
queue:
monolithic: true
port: 6379
host: matrix-redis
Comment on lines +113 to +114
Copy link
Owner

Choose a reason for hiding this comment

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

These assumptions should not be hardcoded and should use variables (matrix_hookshot_queue_host and matrix_hookshot_queue_port).

The port could default to 6379, since that's quite standard for Redis.

The host should default to empty. Perhaps you can only enable the queue (if statement) if the host is non-empty. Example: {% if matrix_hookshot_queue_host %}

The matrix_hookshot_queue_host and matrix_hookshot_queue_port variables can be set to the correct values (matrix-redis, ..) via group_vars/matrix_servers - the file we use for wiring the different roles together. It's better to explicitly wire things via this central file, instead of having roles randomly assume things about other roles or each into each other's variables.

matrix_hookshot_queue_host would be defined as such in group_vars/matrix_servers:

matrix_hookshot_queue_host: "{{ redis_identifier if matrix_hookshot_experimental_encryption_enabled and redis_enabled else '' }}"

This: provides sane defaults for these variables, potentially overrides them via the group vars file.. and still allows people to use external Redis instances, if they want to.

experimentalEncryption:
storagePath: /data/encryption
Comment on lines +115 to +116
Copy link
Owner

Choose a reason for hiding this comment

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

This should be out of the if that deals with the queue stuff, since one may wish to enable the queue without enabling experimental encryption.

Experimental encryption does probably require the queue configuration, but I suppose Hookshot checks for this by itself. You may add a check in validate_config.yml if you're feeling like it though.

experimentalEncryption settings should be dependant on {% if matrix_hookshot_experimental_encryption_enabled %}.

{% endif %}
logging:
# (Optional) Logging settings. You can have a severity debug,info,warn,error
#
Expand Down
6 changes: 6 additions & 0 deletions roles/custom/matrix-bridge-hookshot/templates/registration.yml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,9 @@ namespaces:
sender_localpart: hookshot
url: "http://{{ matrix_hookshot_container_url }}:{{ matrix_hookshot_appservice_port }}" # This should match the bridge.port in your config file
rate_limited: false

{% if matrix_hookshot_experimental_encryption_enabled %}
de.sorunome.msc2409.push_ephemeral: true
push_ephemeral: true
org.matrix.msc3202: true
{% endif %}
8 changes: 7 additions & 1 deletion roles/custom/matrix-bridge-hookshot/templates/systemd/matrix-hookshot.service.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Environment="HOME={{ devture_systemd_docker_base_systemd_unit_home_path }}"
ExecStartPre=-{{ devture_systemd_docker_base_host_command_docker }} kill {{ matrix_hookshot_container_url }}
ExecStartPre=-{{ devture_systemd_docker_base_host_command_docker }} rm {{ matrix_hookshot_container_url }}

ExecStart={{ devture_systemd_docker_base_host_command_docker }} run --rm --name {{ matrix_hookshot_container_url }} \
ExecStartPre={{ devture_systemd_docker_base_host_command_docker }} create --rm --name {{ matrix_hookshot_container_url }} \
--log-driver=none \
--user={{ matrix_user_uid }}:{{ matrix_user_gid }} \
--cap-drop=ALL \
Expand All @@ -30,6 +30,12 @@ ExecStart={{ devture_systemd_docker_base_host_command_docker }} run --rm --name
{% endfor %}
{{ matrix_hookshot_docker_image }}

{% if matrix_hookshot_experimental_encryption_enabled %}
ExecStartPre={{ devture_systemd_docker_base_host_command_docker }} network connect matrix-redis {{ matrix_hookshot_container_url }}
{% endif %}
Comment on lines +33 to +35
Copy link
Owner

Choose a reason for hiding this comment

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

As discussed before, it's not great for roles to make assumptions about other roles.

The assumption that this role makes there is that when encryption is enabled, Redis will also be enabled via nothing else but matrix-redis.

It's better to redo this by:

  • adding a new matrix_hookshot_container_additional_networks variable, which can connect the Hookshot container to any arbitrary additional network. In some roles, we even split this into multiple lists of networks (multiple variables) in defaults/main.yml:
    • matrix_hookshot_container_additional_networks: "{{ matrix_hookshot_container_additional_networks_auto + matrix_hookshot_container_additional_networks_custom }}"
    • matrix_hookshot_container_additional_networks_auto: []
    • matrix_hookshot_container_additional_networks_custom: []
    • With the above, matrix_hookshot_container_additional_networks_auto is supposed to be managed by the playbook, while matrix_hookshot_container_additional_networks_custom is supposed to be managed by the user
  • matrix_hookshot_container_additional_networks_auto may be redefined in group_vars/matrix_servers to inject redis_container_network (equal to matrix-redis) into the list of networks under certain conditions (if matrix_hookshot_queue_host == redis_identifier and redis_enabled). You can find various examples of _additional_networks redefinitions in group_vars/matrix_servers
  • redoing this systemd .service file, so that it loops over the list of networks (matrix_hookshot_container_additional_networks) and connects to them all

With this, the role is configurable and does not hardcode anything about matrix-redis. All wiring happens in group_vars/matrix_servers and people can do their own network wiring or use external Redis if they need to.


ExecStart={{ devture_systemd_docker_base_host_command_docker }} start --attach {{ matrix_hookshot_container_url }}

ExecStop=-{{ devture_systemd_docker_base_host_command_docker }} kill {{ matrix_hookshot_container_url }}
ExecStop=-{{ devture_systemd_docker_base_host_command_docker }} rm {{ matrix_hookshot_container_url }}
Restart=always
Expand Down