Don't update an existing config file #401

Azrenbeth · 2021-09-16T11:01:32Z

Followed by #402 and #404.

Currently, if you delete the signing key from the config file, a new one gets generated and the config file gets updated.

This causes two issues:

The config file cannot be set to be read-only.
You may want a minimal config file (with only the things you're overriding from the defaults) but this update will write all of the default values into your minimal config file as well.

How Sydent will now behave:

If there is no config file, generate a new one and then stop, with log message telling user to run the generate-key script.
If there is a config file and it has a base64 encoded signing key, run as normal.
If there is a config file and it has a hex encoded signing key, run as before but without updating the format of the signing key to base64 (instead log a warning telling the user to run the new update-key script) - note keys haven't been Hex encoded since 2015
If there is a config file and it has no signing key, exit with log message telling user to run the generate-key script

The idea is that in the future the user can/must instead run a generate-config script before the first run of Sydent, and there will be documentation on how the configuration works.

This isn't particularly a loss of backwards compatability, for people updating from previous versions:

If they have an existing config, the only change they'll experience is having to run the the generate-key script to make new keys instead of just being able to delete their value from the config.
If they don't have an existing config, they need to run the generate-key script before they can get Sydent to start. (Or generate-config script added in Add generate-config script and document config options #404)

clokep · 2021-09-20T18:25:41Z

Do we want to put upgrade notes for this somewhere? It seems like it would mostly be copying out of the description. Probably in the CHANGELOG.md file directly?

clokep · 2021-09-20T18:30:07Z

Dockerfile

@@ -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


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

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?

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?

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.

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.

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.

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

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.

scripts/update-key

sydent/config/crypto.py

clokep · 2021-09-21T12:54:06Z

I'm going to throw this back on the @matrix-org/synapse-core pile as I'm not sure I have enough context into how these keys are used to be confident in my review!

richvdh

a few thoughts on this!

scripts/generate-key

scripts/update-key

richvdh · 2021-09-23T13:37:50Z

Dockerfile

@@ -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


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.

scripts/update-key

scripts/generate-key

sydent/config/__init__.py

changelog.d/401.removal

- update changelog - multiline strings in script output - add --quiet option to generate-key

richvdh · 2021-09-27T13:17:51Z

@Azrenbeth I'm going to put this back in your court to do something about #401 (comment).

Azrenbeth added 6 commits September 16, 2021 11:25

Add script to update signing key format

53eabbf

Don't update an existing config file. Exit instead.

f0c79f9

Add return False to end of crypto parse_config

c9f2e5c

Update README and add better instructions to generate-key

adb132e

Add changelog

02cf85e

Fix silly typo

55ef37a

Azrenbeth mentioned this pull request Sep 16, 2021

Convert ConfigParser object to dict and prevent #287 #402

Open

Azrenbeth force-pushed the azren/read_only_config branch 2 times, most recently from 44fb10a to 55ef37a Compare September 16, 2021 16:34

Azrenbeth added 3 commits September 20, 2021 09:54

Remove incorrect assertion from readme

5306b44

Merge remote-tracking branch 'origin/main' into azren/read_only_config

e581b04

Apply casefolding fix from #405

d319e64

Azrenbeth marked this pull request as ready for review September 20, 2021 11:13

Azrenbeth requested a review from a team September 20, 2021 11:15

Azrenbeth mentioned this pull request Sep 20, 2021

Add generate-config script and document config options #404

Open

Azrenbeth marked this pull request as draft September 20, 2021 12:11

Azrenbeth removed the request for review from a team September 20, 2021 12:11

Update dockerfile to run generate-key before CMD

8df0080

Azrenbeth marked this pull request as ready for review September 20, 2021 12:42

Azrenbeth requested a review from a team September 20, 2021 12:43

clokep reviewed Sep 20, 2021

View reviewed changes

Azrenbeth added 2 commits September 21, 2021 13:35

Add suggestions from code review

b31fc0b

Move script description above example run

99b49a2

Azrenbeth requested a review from clokep September 21, 2021 12:36

clokep requested review from a team and removed request for clokep September 21, 2021 12:53

richvdh requested changes Sep 23, 2021

View reviewed changes

Apply suggestions from code review:

203e27e

- update changelog - multiline strings in script output - add --quiet option to generate-key

Azrenbeth added 2 commits September 27, 2021 12:31

Merge remote-tracking branch 'origin/main' into azren/read_only_config

3d98cac

Run isort

b73c975

Azrenbeth requested a review from richvdh September 27, 2021 11:36

richvdh removed their request for review September 27, 2021 13:17

Azrenbeth added 2 commits September 28, 2021 12:20

Merge remote-tracking branch 'origin/main' into azren/read_only_config

150009c

Fix mistake that occured while fixing merge conflicts

5863e95

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't update an existing config file #401

Don't update an existing config file #401

Azrenbeth commented Sep 16, 2021 •

edited

Loading

clokep commented Sep 20, 2021

clokep Sep 20, 2021

Azrenbeth Sep 21, 2021

clokep Sep 21, 2021

richvdh Sep 23, 2021

Azrenbeth Sep 27, 2021

richvdh Sep 27, 2021

richvdh Sep 27, 2021

richvdh Sep 27, 2021

clokep commented Sep 21, 2021

richvdh left a comment

richvdh Sep 23, 2021

richvdh commented Sep 27, 2021

Don't update an existing config file #401

Are you sure you want to change the base?

Don't update an existing config file #401

Conversation

Azrenbeth commented Sep 16, 2021 • edited Loading

clokep commented Sep 20, 2021

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

clokep commented Sep 21, 2021

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Sep 27, 2021

Azrenbeth commented Sep 16, 2021 •

edited

Loading