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

Connection now supports UNIX sockets if host starts with a slash #400

Merged
merged 1 commit into from Jul 9, 2017

Conversation

Projects
None yet
2 participants
@schnusch
Contributor

schnusch commented Jun 24, 2017

  • for people using nzbget behind a reverse HTTP proxy, it does not occupy a port on the loopback interface and provides better security, because access from other users on the same machine is no longer possible over a port on the loopback interface, but is now controlled by file permissions
  • currently breaks CLI client but there are probably a couple more people out there who also basically never use it
@hugbug

This comment has been minimized.

Show comment
Hide comment
@hugbug

hugbug Jun 25, 2017

Member

Thanks for the contribution.

I think that's pretty useless ;) but nonetheless I can integrate this if you make it less intrusive. My goal is to minimise the amount of code needed for the feature; the code I need to support in years to come.

I suggest the following changes:

  • instead of new option ControlSocket (mis)use existing option ControlIP. For normal sockets it contains either IP or a DNS address. For file socket it will contain a path starting with / which can be used to detect the file socket mode. Maybe if we rename ControlIP to ControlAddress its name would be better suitable; anyway such renaming is a separate thing.
  • the only changes needed will be localised in Connection.cpp-unit. No changes to other units (Options.cpp, RemoteServer.cpp, BinRpc.cpp, WebServer.cpp). Some error messages printed by program maybe a little misleading (they will mention "port" instead of "file socket") but I think that's acceptable.
  • add a couple of lines to Bind in Connection.cpp to support the file socket mode (instead of new class). This will make the feature transparently usable from other program parts, which don't need to be aware of the new mode at all (they just pass ControlIP to Connection).

And of course you have to make CLI work; otherwise that's a half baked feature. Hopefully a couple of lines in Connect-method can make the client-side work.

Member

hugbug commented Jun 25, 2017

Thanks for the contribution.

I think that's pretty useless ;) but nonetheless I can integrate this if you make it less intrusive. My goal is to minimise the amount of code needed for the feature; the code I need to support in years to come.

I suggest the following changes:

  • instead of new option ControlSocket (mis)use existing option ControlIP. For normal sockets it contains either IP or a DNS address. For file socket it will contain a path starting with / which can be used to detect the file socket mode. Maybe if we rename ControlIP to ControlAddress its name would be better suitable; anyway such renaming is a separate thing.
  • the only changes needed will be localised in Connection.cpp-unit. No changes to other units (Options.cpp, RemoteServer.cpp, BinRpc.cpp, WebServer.cpp). Some error messages printed by program maybe a little misleading (they will mention "port" instead of "file socket") but I think that's acceptable.
  • add a couple of lines to Bind in Connection.cpp to support the file socket mode (instead of new class). This will make the feature transparently usable from other program parts, which don't need to be aware of the new mode at all (they just pass ControlIP to Connection).

And of course you have to make CLI work; otherwise that's a half baked feature. Hopefully a couple of lines in Connect-method can make the client-side work.

@schnusch schnusch changed the title from added server-side support for web interface on UNIX socket to Connection now supports UNIX sockets if host starts with a slash Jun 25, 2017

@schnusch

This comment has been minimized.

Show comment
Hide comment
@schnusch

schnusch Jun 25, 2017

Contributor

Yes, you are right, by simply reusing the value of ControlIP it all boils down to changes in Connection::Bind and Connection::DoConnect. I also added some code to remove the UNIX socket upon closing the listening connection in Connection::DoDisconnect and to avoid remote address resolution in Connection::GetRemoteAddr.
The CLI client also works. A side-effect that was introduced is that one should now also be able to connect to NNTP servers on local UNIX sockets.

Contributor

schnusch commented Jun 25, 2017

Yes, you are right, by simply reusing the value of ControlIP it all boils down to changes in Connection::Bind and Connection::DoConnect. I also added some code to remove the UNIX socket upon closing the listening connection in Connection::DoDisconnect and to avoid remote address resolution in Connection::GetRemoteAddr.
The CLI client also works. A side-effect that was introduced is that one should now also be able to connect to NNTP servers on local UNIX sockets.

@hugbug

This comment has been minimized.

Show comment
Hide comment
@hugbug

hugbug Jul 2, 2017

Member

Works perfectly.
Sorry for the delay of merging this. I need to publish v19.1 with fixes and can merge the PR (for v20.0) after that.

Member

hugbug commented Jul 2, 2017

Works perfectly.
Sorry for the delay of merging this. I need to publish v19.1 with fixes and can merge the PR (for v20.0) after that.

@hugbug hugbug merged commit 0ee9083 into nzbget:develop Jul 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

hugbug added a commit that referenced this pull request Jul 9, 2017

hugbug added a commit that referenced this pull request Oct 9, 2017

#400: support for file sockets (POSIX only)
Option "ControlIP" can be set to local file path to use file sockets instead of network sockets.

hugbug added a commit that referenced this pull request Oct 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment