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

Don't update an existing config file #401

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
3 changes: 3 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,8 @@ ENV SYDENT_DB_PATH=/data/sydent.db
WORKDIR /sydent
USER sydent:sydent
VOLUME ["/data"]

RUN python3 /sydent/scripts/generate-key >> /data/sydent.conf
Copy link
Member

Choose a reason for hiding this comment

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

Is there any risk here of adding keys to the file more than once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, since the file now won't get written to by Sydent if it exists, and running this script is the place where the file gets created.

One thing that might be of concern though - which I'd initially overlooked - is if this docker image gets used by more than one person, they'd be using the same keys! (Since the key gets generated while it's being built, whereas before it was generated when it was run). Do you think this is an issue?

Copy link
Member

Choose a reason for hiding this comment

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

Do you think this is an issue?

That's probably not good, yes. It also means that anyone would have access to the keys just by loading the image.

Does the same provider (e.g. matrix.org) need to use the same keys over time or would it be OK to rotate them for every release or something else?

Copy link
Member

Choose a reason for hiding this comment

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

yes, let's not hardcode the keys into the docker image.

Technically, I think it's ok for a server to use a different key each time it is started. I think this means that there is a much simpler option here: at startup, if there is no key in the config file, emit a warning and make up a key for the duration of that run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would it be ok for it to use a different key each time it started? Wouldn't that mean any verification messages sent before a restart could no longer be used?

Instead of using different keys for each run, the docker file could run a script which checked for the existance of the config file, if it doesn't exist (which it won't on the first run) generate keys and then start sydent, else just start sydent.

Copy link
Member

Choose a reason for hiding this comment

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

Why would it be ok for it to use a different key each time it started?

because, iirc, the key used to sign a given 3pid invite event is embedded in that event. Though I might be misremembering how all this works.

Copy link
Member

Choose a reason for hiding this comment

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

ok, looking harder, I think this is nonsense, so we better go with your suggestion :)

Copy link
Member

Choose a reason for hiding this comment

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

alternatively, I think it would be reasonable to require the user to do a manual step to generate the keys, and just refuse to start if they are not present.


EXPOSE 8090/tcp
CMD [ "python", "-m", "sydent.sydent" ]
10 changes: 7 additions & 3 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,13 @@ With the virtualenv activated, you can run Sydent using::

python -m sydent.sydent

This will create a configuration file in ``sydent.conf`` with some defaults. If a setting is
defined in both the ``[DEFAULT]`` section and another section in the configuration file,
then the value in the other section is used.
If this is the first time Sydent is run, then it will generate a configuration file in
``sydent.conf`` with some defaults and then stop. You must run the generate-key script
and update the config with this key before Sydent will start.

You should not write anything in the ``[DEFAULT]`` section. If a
setting is defined in both the ``[DEFAULT]`` section and another section in the configuration
file, then the value in the other section is used.

You'll most likely want to change the server name (``server.name``) and specify an email server
(look for the settings starting with ``email.``).
Expand Down
1 change: 1 addition & 0 deletions changelog.d/401.removal
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Don't update existing config files anymore. The generate-key script must now be run to create new keys.
Azrenbeth marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 3 additions & 0 deletions matrix_is_test/launcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
email.from = Sydent Validation <noreply@localhost>
email.smtpport = 9925
email.subject = Your Validation Token

[crypto]
ed25519.signingkey = ed25519 0 broXDusfghcDAamylh2RmOEHfPJCi4snha7NNCJKOao
"""


Expand Down
2 changes: 1 addition & 1 deletion scripts/casefold_db.py
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ def update_global_associations(
sys.exit(1)

sydent_config = SydentConfig()
sydent_config.parse_config_file(args.config_path)
sydent_config.parse_config_file(args.config_path, skip_logging_setup=True)

reactor = ResolvingMemoryReactorClock()
sydent = Sydent(sydent_config, reactor, False)
Expand Down
15 changes: 10 additions & 5 deletions scripts/generate-key
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,21 @@

# The signing key is generally used in "ed25519.signingkey" in the sydent config

import sys

import signedjson.key

signing_key = signedjson.key.generate_signing_key(0);
signing_key = signedjson.key.generate_signing_key(0)
sk_str = "%s %s %s" % (
signing_key.alg,
signing_key.version,
signedjson.key.encode_signing_key_base64(signing_key)
)
print ("signing key: %s " % sk_str)
pk_str = signedjson.key.encode_verify_key_base64(signing_key.verify_key)
print ("verify key: %s" % pk_str)

print(
"# A new key has been generated. To use it, update your sydent config file with the following: \n"
Azrenbeth marked this conversation as resolved.
Show resolved Hide resolved
"\n"
"[crypto]\n"
f"ed25519.signingkey = {sk_str}\n"
"\n"
f"# For reference, the public (verificiation) key is {pk_str}\n"
Azrenbeth marked this conversation as resolved.
Show resolved Hide resolved
)
48 changes: 48 additions & 0 deletions scripts/update-key
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#!/usr/bin/env python3
Azrenbeth marked this conversation as resolved.
Show resolved Hide resolved

# Example run
# ```
# $ ./scripts/update-key b251d15e720672cbbe73626447a8458ffa1ab1413925cd51ef990e652d48318A
# Update your sydent config file with the following:
#
# [crypto]
# ed25519.signingkey = ed25519 0 slHRXnIGcsu+c2JkR6hFj/oasUE5Jc1R75kOZS1IMYo
#
# For reference, the public (verificiation) key is 53eNltXamSKdvxIgL3Tq4KMrwgR/sQA18xvwvxEYc4o
# ````

# Use this to update the signing key used in sydent configurations. This doesn't
# change the key, only the way that it's encoded in the config file.
Azrenbeth marked this conversation as resolved.
Show resolved Hide resolved

import sys

import nacl
import signedjson.key

if len(sys.argv) != 2:
print("Usage: updated-key [hex encoded key]")
else:
signing_key_hex = sys.argv[1]

signing_key = nacl.signing.SigningKey(
signing_key_hex, encoder=nacl.encoding.HexEncoder
)
signing_key.version = "0"
signing_key.alg = signedjson.key.NACL_ED25519

signing_key_str = "%s %s %s" % (
signing_key.alg,
signing_key.version,
signedjson.key.encode_signing_key_base64(signing_key),
)

verify_key_str = signedjson.key.encode_verify_key_base64(signing_key.verify_key)

print(
"Update your sydent config file with the following: \n"
"\n"
"[crypto]\n"
f"ed25519.signingkey = {signing_key_str}\n"
"\n"
f"For reference, the public (verificiation) key is {verify_key_str}\n"
Azrenbeth marked this conversation as resolved.
Show resolved Hide resolved
)
Azrenbeth marked this conversation as resolved.
Show resolved Hide resolved
50 changes: 27 additions & 23 deletions sydent/config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,10 @@ def _parse_config(self, cfg: ConfigParser) -> bool:

:param cfg: the configuration to be parsed

:return: whether or not cfg has been altered. This method CAN
return True, but it *shouldn't* as this leads to altering the
config file.
:return: whether or not the config file needs updating. This method CAN
return True, but it *shouldn't*. Instead a ConfigError exception
should be raised. This is left in for the soon to be deprecated way
of generating config files.
"""
needs_saving = False
for section in self.config_sections:
Expand All @@ -200,55 +201,58 @@ def _parse_config(self, cfg: ConfigParser) -> bool:

return needs_saving

def parse_from_config_parser(self, cfg: ConfigParser) -> bool:
def _parse_from_config_parser(self, cfg: ConfigParser) -> bool:
"""
Parse the configuration from a ConfigParser object

:param cfg: the configuration to be parsed

:return: whether or not cfg has been altered. This method CAN
return True, but it *shouldn't* as this leads to altering the
config file.
:return: whether or not the config file needs updating. This method CAN
return True, but it *shouldn't*. Instead a ConfigError exception
should be raised. This is left in for the soon to be deprecated way
of generating config files.
"""
return self._parse_config(cfg)

def parse_config_file(self, config_file: str) -> None:
def parse_config_file(
self, config_file: str, skip_logging_setup: bool = False
) -> None:
"""
Parse the given config from a filepath, populating missing items and
sections. NOTE: this method also sets up logging.

:param config_file: the file to be parsed
"""
# If the config file doesn't exist, prepopulate the config object
# with the defaults, in the right section.
#
# Otherwise, we have to put the defaults in the DEFAULT section,
# to ensure that they don't override anyone's settings which are
# in their config file in the default section (which is likely,
# because sydent used to be braindead).
use_defaults = not os.path.exists(config_file)
# with the defaults, in the DEFAULT section.
new_config_file = not os.path.exists(config_file)
Azrenbeth marked this conversation as resolved.
Show resolved Hide resolved

cfg = ConfigParser()
for sect, entries in CONFIG_DEFAULTS.items():
cfg.add_section(sect)
for k, v in entries.items():
cfg.set(DEFAULTSECT if use_defaults else sect, k, v)
cfg.set(DEFAULTSECT if new_config_file else sect, k, v)

cfg.read(config_file)

# Logging is configured in cfg, but these options must be parsed first
# so that we can log while parsing the rest
setup_logging(cfg)

# TODO: Don't alter config file when starting Sydent so that
# it can be set to read-only
if not skip_logging_setup:
Azrenbeth marked this conversation as resolved.
Show resolved Hide resolved
setup_logging(cfg)

needs_saving = self.parse_from_config_parser(cfg)
needs_updating = self._parse_from_config_parser(cfg)

if needs_saving:
# Don't edit config file when starting Sydent unless it's the first run
if new_config_file:
fp = open(config_file, "w")
cfg.write(fp)
fp.close()
exit(0)

if needs_updating:
# A more specific log message should have been given earlier
logger.error("The config file needs updating")
exit(1)

def parse_config_dict(self, config_dict: Dict) -> None:
"""
Expand All @@ -274,7 +278,7 @@ def parse_config_dict(self, config_dict: Dict) -> None:
# This is only ever called by tests so don't configure logging
# as tests do this themselves

self.parse_from_config_parser(cfg)
self._parse_from_config_parser(cfg)


def setup_logging(cfg: ConfigParser) -> None:
Expand Down
28 changes: 9 additions & 19 deletions sydent/config/crypto.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,40 +33,30 @@ def parse_config(self, cfg: "ConfigParser") -> bool:
signing_key_str = cfg.get("crypto", "ed25519.signingkey")
signing_key_parts = signing_key_str.split(" ")

save_key = False

if signing_key_str == "":
logger.info(
"This server does not yet have an ed25519 signing key. "
"Creating one and saving it in the config file."
logger.warning(
"'ed25519.signingkey' cannot be blank. Please generate a new"
" signing key with the 'generate-key' script."
)

self.signing_key = signedjson.key.generate_signing_key("0")

save_key = True
return True
elif len(signing_key_parts) == 1:
# old format key
logger.info("Updating signing key format: brace yourselves")
logger.warning(
"Updating signing key format for this run. Please run the"
" 'update-key' script to speedup the next startup"
Azrenbeth marked this conversation as resolved.
Show resolved Hide resolved
)

self.signing_key = nacl.signing.SigningKey(
signing_key_str, encoder=nacl.encoding.HexEncoder
)
self.signing_key.version = "0"
self.signing_key.alg = signedjson.key.NACL_ED25519

save_key = True
else:
self.signing_key = signedjson.key.decode_signing_key_base64(
signing_key_parts[0], signing_key_parts[1], signing_key_parts[2]
)

if save_key:
signing_key_str = "%s %s %s" % (
self.signing_key.alg,
self.signing_key.version,
signedjson.key.encode_signing_key_base64(self.signing_key),
)
cfg.set("crypto", "ed25519.signingkey", signing_key_str)
return True
else:
return False
return False
2 changes: 1 addition & 1 deletion sydent/sydent.py
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ def run_gc():

if __name__ == "__main__":
sydent_config = SydentConfig()
sydent_config.parse_config_file(get_config_file_path())
sydent_config.parse_config_file(get_config_file_path(), skip_logging_setup=False)

syd = Sydent(sydent_config)
syd.run()
8 changes: 8 additions & 0 deletions tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@
-----END CERTIFICATE-----
"""

DEFAULT_SIGNING_KEY = "ed25519 0 broXDusfghcDAamylh2RmOEHfPJCi4snha7NNCJKOao"


def make_sydent(test_config={}):
"""Create a new sydent
Expand All @@ -68,6 +70,12 @@ def make_sydent(test_config={}):
else:
test_config["db"].setdefault("db.file", ":memory:")

# Set a value for the signingkey if it hasn't been set by the test
if "crypto" not in test_config:
test_config["crypto"] = {"ed25519.signingkey": DEFAULT_SIGNING_KEY}
elif "ed25519.signingkey" not in test_config["crypto"]:
test_config["crypto"] = {"ed25519.signingkey": DEFAULT_SIGNING_KEY}

reactor = ResolvingMemoryReactorClock()

sydent_config = SydentConfig()
Expand Down