Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix admin unix sockets #2787

Merged
merged 3 commits into from
Oct 8, 2021
Merged

Conversation

thatsmydoing
Copy link
Contributor

The admin_ws_unix and admin_wss_unix options were mistakenly being taken from the general section instead of the adminsection.

Additionally, admin_ws_unix did not actually set the LWS_SERVER_OPTION_UNIX_SOCK so it would not work anyway.

I decided to just factor out the whole websocket server creation so that any further changes here don't need to be repeated for each variation of the server.

This matches the example configuration file and all the other admin_*
options
This should keep all types of servers admin vs non-admin and secure vs
non-secure in parity.
@januscla
Copy link

januscla commented Oct 6, 2021

Thanks for your contribution, @thatsmydoing! Please make sure you sign our CLA, as it's a required step before we can merge this.

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting the issue! I think the refactoring you did is quite helpful, even though from a quick glance it will need fixing (see below).

char *password = NULL;
char *ciphers = NULL;

if (config_certs) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's correct. With this change, if the config file doesn't have a certs section, but the config file says a secure WebSockets server must be created, the attempt will not fail as it should, but will simply create a plain WebSocket server on the provided port instead, which is wrong.

Depending on the prefix, config_certs may need to be required and not optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's right. I initially did have a flag for secure or not but replaced it with just config_certs I forgot that it could be NULL by itself.

@thatsmydoing
Copy link
Contributor Author

I've fixed the issue and signed the CLA

@lminiero
Copy link
Member

lminiero commented Oct 6, 2021

Thanks for the quick fixes! I'll do a deeper review and some tests either later today or tomorrow morning, and in case there's no regression, I'll merge 👍

@lminiero
Copy link
Member

lminiero commented Oct 8, 2021

Looks good, merging ✌️

@lminiero lminiero merged commit 7d7bf7b into meetecho:master Oct 8, 2021
@thatsmydoing thatsmydoing deleted the fix-admin-unix-sockets branch March 1, 2022 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants