Rerun TCP protocol should be more structured and only log warnings if the incoming connection is from a Rerun SDK #5883
Labels
📺 re_viewer
affects re_viewer itself
Milestone
Comments
5 tasks
|
@emilk In case you haven't started, I have already created this fix. |
emilk
added a commit
that referenced
this issue
May 14, 2024
<!-- Open the PR up as a draft until you feel it is ready for a proper review. Do not make PR:s from your own `main` branch, as that makes it difficult for reviewers to add their own fixes. Add any improvements to the branch as new commits to make it easier for reviewers to follow the progress. All commits will be squashed to a single commit once the PR is merged into `main`. Make sure you mention any issues that this PR closes in the description, as well as any other related issues. To get an auto-generated PR description you can put "copilot:summary" or "copilot:walkthrough" anywhere. --> ### What Now we send a special header in the TCP connection before sending the protocol version. It helps the server recognize if the client is a rerun client or something else. **Edit** Have changed the approach to send the header _after_ the protocol version. See the discussion in comments. * Fixes #5883 ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Using examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6253?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6253?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! - [PR Build Summary](https://build.rerun.io/pr/6253) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) To run all checks from `main`, comment on the PR with `@rerun-bot full-check`. --------- Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
|
I messed up unfortunately and by accident removed part of this fix last-minute from 0.16 which as of the time of writing is in the process of being released by our CI job. Fortunately this affects only the log warning level, but we'll have to re-add this for 0.16.1 (if exists) or 0.17.0. Sorry! 😭 |
1 task
Wumpf
pushed a commit
that referenced
this issue
May 17, 2024
* Closes #5883 * [x] I know how checkboxes work
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently, the Rerun TCP protocol immediately starts with a version number.
Unfortunately, our port, 9876 is fairly low-entropy and likely to show up in the wild from non-Rerun sources or as part of security probes. This means we are quite likely to see incoming connections from non-Rerun sources.
Currently we Warn on protocol version mismatch:
However, sometimes these incoming connections are outside of our control. The more principled approach would be to ALWAYS start with a rerun-unique header and then follow that with the version number. This way if the expected header isn't there we can ignore the connection and not produce the warning.
The text was updated successfully, but these errors were encountered: