lib/discover: Don't leak relay-tokens to discovery #8762
Merged
Conversation
This file contains hidden or 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
|
The question in my mind is whether/why we want to pass on any query parameters whatsoever to discovery... |
|
As I understand the current logic, we do submit the query part of the relay server URL because it contains the relay server ID. ST clients wanting to dial another device via a relay address want to open a relay session in temporary mode. In order to perform relay server certificate validation, they need to know the ID. They can get that via the announced URL as part of the query. |
|
Good point, so that needs to be sent. But currently we just send the full URL as we got it from the relay pool endpoint (or manual config), which contains all kinds of stuff like who provides it etc. |
|
So we perhaps want to invert the logic (block vs allowlist)? Only include query parameters we explicitly want announced to discovery (the ID) and ignore everything else? |
Use a whitelist of allowed relay address parameters instead of filtering out the token
|
Something like this? I think the checks failing has nothing to do with the code. |
imsodin
previously approved these changes
Mar 2, 2023
|
And yes, the check is definitely not on this PR - probably some FS/OS limitation of the github actions runner (those jobs are new-ish - nice btw :) ). |
imsodin
approved these changes
Mar 4, 2023
calmh
added a commit
to imsodin/syncthing
that referenced
this pull request
Mar 10, 2023
* main: (46 commits) build: Update dependencies lib/api: Expose `blocksHash` in file info (syncthing#8810) gui, man, authors: Update docs, translations, and contributors lib/discover: Don't leak relay-tokens to discovery (syncthing#8762) gui, man, authors: Update docs, translations, and contributors gui: Add Croatian (hr) translation template (syncthing#8801) build(deps): bump github.com/quic-go/quic-go from 0.32.0 to 0.33.0 (syncthing#8800) cmd/stupgrades: Cache should apply to HEAD as well as GET build: Add more GitHub Actions gui: Remove non-existent customicons.css file reference (fixes syncthing#8797) (syncthing#8798) Only fail after chmod error if permissions differ (e.g. on config file) (syncthing#8771) gui, man, authors: Update docs, translations, and contributors build: Use Go 1.19.6 for Windows build: Update dependencies gui, man, authors: Update docs, translations, and contributors gui: Remove duplicate Spanish (Spain) translation (fixes syncthing#8781) (syncthing#8782) gui: Add xattr filter editor (fixes syncthing#8660) (syncthing#8734) gui: Switch to Weblate for translations (syncthing#8777) all: Use new Go 1.19 atomic types (syncthing#8772) gui, man, authors: Update docs, translations, and contributors ...
calmh
added a commit
to tomasz1986/syncthing
that referenced
this pull request
Mar 18, 2023
* main: (424 commits) gui, man, authors: Update docs, translations, and contributors lib/protocol: Cache expensive key operations (fixes syncthing#8599) (syncthing#8820) gui: Add Persian (fa) translation template (syncthing#8822) lib: Correctly handle encrypted trailer size (fixes syncthing#8556) (syncthing#8563) gui: Disable Restore Versions filters when no versioned files exist (fixes syncthing#5408) (syncthing#8539) build(deps): bump github.com/chmduquesne/rollinghash from 0.0.0-20180912150627-a60f8e7142b5 to 4.0.0+incompatible (syncthing#8804) build: Update dependencies (syncthing#8821) lib/api: Expose `blocksHash` in file info (syncthing#8810) gui, man, authors: Update docs, translations, and contributors lib/discover: Don't leak relay-tokens to discovery (syncthing#8762) gui, man, authors: Update docs, translations, and contributors gui: Add Croatian (hr) translation template (syncthing#8801) build(deps): bump github.com/quic-go/quic-go from 0.32.0 to 0.33.0 (syncthing#8800) cmd/stupgrades: Cache should apply to HEAD as well as GET build: Add more GitHub Actions gui: Remove non-existent customicons.css file reference (fixes syncthing#8797) (syncthing#8798) Only fail after chmod error if permissions differ (e.g. on config file) (syncthing#8771) gui, man, authors: Update docs, translations, and contributors build: Use Go 1.19.6 for Windows build: Update dependencies ...
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR aims to mitigate the token-disclosure described in #8561 (comment) by @GermanCoding: The client leaks the relay-token to discovery by announcing it as part of the
relay://...-URL.I tested this PR with network local and external clients (read: local and global discovery) but would appreciate it if somebody else could give it a go.