This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Support registration_shared_secret
in a file
#13614
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Support setting the registration shared secret in a file, via a new `registration_shared_secret_path` configuration option. | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,11 +20,20 @@ | |
import hmac | ||
import logging | ||
import sys | ||
from typing import Callable, Optional | ||
from typing import Any, Callable, Optional | ||
|
||
import requests | ||
import yaml | ||
|
||
_CONFLICTING_SHARED_SECRET_OPTS_ERROR = """\ | ||
Conflicting options 'registration_shared_secret' and 'registration_shared_secret_path' | ||
are both defined in config file. | ||
""" | ||
Comment on lines
+28
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kind of a shame that we need to do the same checks that we have in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I'm not keen to drag in the whole config-parsing infrastructure |
||
|
||
_NO_SHARED_SECRET_OPTS_ERROR = """\ | ||
No 'registration_shared_secret' or 'registration_shared_secret_path' defined in config. | ||
""" | ||
|
||
|
||
def request_registration( | ||
user: str, | ||
|
@@ -213,9 +222,16 @@ def main() -> None: | |
|
||
if "config" in args and args.config: | ||
config = yaml.safe_load(args.config) | ||
secret = config.get("registration_shared_secret", None) | ||
secret = config.get("registration_shared_secret") | ||
secret_file = config.get("registration_shared_secret_path") | ||
if secret_file: | ||
if secret: | ||
print(_CONFLICTING_SHARED_SECRET_OPTS_ERROR, file=sys.stderr) | ||
sys.exit(1) | ||
secret = _read_file(secret_file, "registration_shared_secret_path").strip() | ||
|
||
if not secret: | ||
print("No 'registration_shared_secret' defined in config.") | ||
print(_NO_SHARED_SECRET_OPTS_ERROR, file=sys.stderr) | ||
sys.exit(1) | ||
else: | ||
secret = args.shared_secret | ||
|
@@ -229,5 +245,29 @@ def main() -> None: | |
) | ||
|
||
|
||
def _read_file(file_path: Any, config_path: str) -> str: | ||
"""Check the given file exists, and read it into a string | ||
|
||
If it does not, exit with an error indicating the problem | ||
|
||
Args: | ||
file_path: the file to be read | ||
config_path: where in the configuration file_path came from, so that a useful | ||
error can be emitted if it does not exist. | ||
Returns: | ||
content of the file. | ||
""" | ||
if not isinstance(file_path, str): | ||
print(f"{config_path} setting is not a string", file=sys.stderr) | ||
sys.exit(1) | ||
|
||
try: | ||
with open(file_path) as file_stream: | ||
return file_stream.read() | ||
except OSError as e: | ||
print(f"Error accessing file {file_path}", file=sys.stderr) | ||
sys.exit(1) | ||
|
||
|
||
if __name__ == "__main__": | ||
main() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels very similar to me to simply putting the
registration_shared_secret
option in a separate configuration file -- can you describe a bit more why this is useful?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's particularly useful in combination with #13615. With both PRs in place, I can have a k8s deployment which creates a homeserver.yaml with
registration_shared_secret_path
pointing to a non-existent file. Then Synapse will magically generate me a shared secret on first run (whichregister_new_matrix_user
can slurp).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will that cause the shared secret to be regenerated each time the container is rebuilt? I'm struggling to see why you would want to generate these on first run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That depends if you use a persistent volume to keep
registration_shared_secret_path
on. If you do, you'll keep the same secret between rebuilds. But it doesn't matter anyway: there is little benefit to using the same shared secret on each run, providedregister_new_matrix_user
can find the current secret. Indeed, maybe it's better to rotate it frequently.... well, because if you don't have synapse generate them, you have to generate them yourself with some annoying scripting.