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

issue #100: implementation for the restriction of interfaces vcontrold tries to bind to #101

Merged
merged 10 commits into from
Aug 27, 2022

Conversation

speters
Copy link
Member

@speters speters commented Aug 19, 2022

ATTN: this is is untested code, just for discussion of

#100

@speters
Copy link
Member Author

speters commented Aug 19, 2022

I only tested it very lazily

./vcontrold -g -n -x ../xml/300/vcontrold.xml -p 3322
...
[83194] Fri Aug 19 21:24:32 2022 : TCP socket 0.0.0.0:3322 opened
[83194] Fri Aug 19 21:24:32 2022 : Not started as root, username/groupname settings ignored
[83194] Fri Aug 19 21:24:35 2022 : Client connected 127.0.0.1:46544 (FD:5)

(connection on external and loopback interfaces possible)

./vcontrold -g -n -x ../xml/300/vcontrold.xml -b "localhost" -p 3322
...
[83351] Fri Aug 19 21:27:02 2022 : TCP socket localhost:3322 opened
[83351] Fri Aug 19 21:27:02 2022 : Not started as root, username/groupname settings ignored
[83351] Fri Aug 19 21:27:07 2022 : Client connected 127.0.0.1:36222 (FD:5)

(now no connection on external interface possible, localhost only)

./vcontrold -g -n -x ../xml/300/vcontrold.xml -b "8.8.8.8" -p 3322
...
[83491] Fri Aug 19 21:30:10 2022 :         Token: 2 Hexlen: 2, Unit: UT
socket error: could not open socket
./vcontrold -g -n -x ../xml/300/vcontrold.xml -b "" -p 3322
...
[83528] Fri Aug 19 21:30:52 2022 : getaddrinfo error: [Name or service not known]
[83528] Fri Aug 19 21:30:52 2022 : Could not start vcontrold on  port 3322
...
[83674] Fri Aug 19 21:31:48 2022 : TCP socket 0:3322 opened
[83674] Fri Aug 19 21:31:48 2022 : Not started as root, username/groupname settings ignored

(this is OK-ish, as 0.0.0.0 is 0 which stands for "all interfaces").

 ./vcontrold -g -n -x ../xml/300/vcontrold.xml -b "-1" -p 3322
...
[83751] Fri Aug 19 21:33:54 2022 : Could not start vcontrold on -1 port 3322

PS:
Those exit(1)statements on error in socket.c are questionable. Leave error handling to the caller would be better code IMO. But it's the way it was done, so I leave it for now.

@speters
Copy link
Member Author

speters commented Aug 19, 2022

I'm not happy with the naming of "bind" config parameter. Any better ideas?

Also: Which default? Safe/localhost only or keep-it-as-it-was/lazy/easy/all interfaces?

@l3u
Copy link
Collaborator

l3u commented Aug 20, 2022

What about "IP"? As a socket address consists of an IP and and port, I would call it IP and set it to "0.0.0.0". This way, we get the behavior we have now as the default, and it can easily set to e.g. "127.0.0.1" to only listen on local connections, or to e.g. "192.168.1.10" or such to provide the server in the LAN.

@hmueller01
Copy link
Collaborator

Why not having a "simple" netmask and call it like this?

@l3u
Copy link
Collaborator

l3u commented Aug 26, 2022

"listen" sounds nice to me :-)

@speters speters merged commit 68f3ba7 into master Aug 27, 2022
@speters speters deleted the test_bindtointerfaceoption_issue100 branch August 27, 2022 17:42
@hmueller01
Copy link
Collaborator

On my compiler I get a warning

vcontrold/src/xmlconfig.c:504:13: warning: expression result unused [-Wunused-value]
    cfgPtr->listenAddress;
    ~~~~~~  ^~~~~~~~~~~~~
1 warning generated.

Why is line 504

cfgPtr->listenAddress;

needed? I think it can be deleted.

@speters
Copy link
Member Author

speters commented Aug 27, 2022

Yes, you're right.

@hmueller01 hmueller01 mentioned this pull request Aug 27, 2022
@hmueller01
Copy link
Collaborator

I added the removal to #105 ...

@speters speters restored the test_bindtointerfaceoption_issue100 branch August 31, 2022 18:33
@speters speters deleted the test_bindtointerfaceoption_issue100 branch August 31, 2022 18:39
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.

3 participants