websocketserver: Allow the user to bind to a specific address #1279
Conversation
silasary
changed the title
src/Config.cpp
Outdated
| #define PARAM_ALERTS "alerts_enabled" | ||
| #define PARAM_AUTHREQUIRED "auth_required" | ||
| #define PARAM_PASSWORD "server_password" | ||
|
|
||
| #define CMDLINE_WEBSOCKET_PORT "websocket_port" | ||
| #define CMDLINE_WEBSOCKET_HOST "websocket_host" |
There was a problem hiding this comment.
I think that I would like this to use address instead of host. address would likely be less ambiguous.
There was a problem hiding this comment.
I used host because that's what websocketpp called it, but agreed, address is a better name.
src/Config.cpp
Outdated
| @@ -72,6 +74,8 @@ void Config::Load(json config) | |||
| AuthRequired = config[PARAM_AUTHREQUIRED]; | |||
| if (config.contains(PARAM_PASSWORD) && config[PARAM_PASSWORD].is_string()) | |||
| ServerPassword = config[PARAM_PASSWORD]; | |||
| if (config.contains(PARAM_HOST) && config[PARAM_HOST].is_string()) | |||
There was a problem hiding this comment.
Nitpick: Move this and other occurrences above the port override
| if (conf->Ipv4Only) { | ||
| if (conf->ServerHost != "") { | ||
| blog(LOG_INFO, "[WebSocketServer::Start] Locked to %s", conf->ServerHost); | ||
| _server.listen(conf->ServerHost, std::to_string(conf->ServerPort), errorCode); |
There was a problem hiding this comment.
I'm looking at the websocketpp documentation, and I'm not seeing a matching function for listen(). Do you know which function it's being implicitly cast to? I'm guessing listen(InternetProtocol const &internet_protocol, uint16_t port, lib::error_code &ec)
It may be safer in this specific situation to first create an InternetProtocol object, then pass it to the listen function.
There was a problem hiding this comment.
I'm using listen( std::string const &host, std::string const &service, lib::error_code &ec) docs here, no implicit cast.
As far as I could tell, InternetProtocol doesn't let you bind to a specific address.
|
Giving this a bump, as it has unresolved comments still. Looks like the feedback was acknowledged, but the requested changes have not been made yet. |
|
Thanks for the bump 😄 |
Description
This PR adds a config variable and command line argument for binding the websocket server to a specific address, rather than blindly listening on all interfaces.
This PR does not currently add a GUI element to configure this.
By default, OBS will continue listening on all addresses, this just adds the option to reduce the exposure for users who want or need it (changing the default appears to be the main reason 1cd12c1 was reverted).
I have chosen to allow binding of a specific address instead of a "localhost only" flag, as it's more flexible for users with multiple network adaptors.
No UI changes just yet, because I'm not sure how to best expose it in a UI, and feel like the users who want/need it are probably comfortable with hand-editing the config or using the CLI argument.
Motivation and Context
I am trying to use OSB websockets in an environment where listening on all addresses is a problem. This allows me to bind OBS to only one network adaptor.
This PR also addresses the security concerns of #907, allowing the user to bind to
127.0.0.1or::1.How Has This Been Tested?
Modified config file, tried connecting to the socket from local machine and remote machine.
Reset config file, then repeated test with command line argument.
Also conducted test without any config changes to ensure default was still
0.0.0.0Tested OS(s):
Windows 10, Windows 11
Types of changes
Checklist:
masteror arelease/*branch.