webserver: implement support for Linux abstract namespace sockets#16613
webserver: implement support for Linux abstract namespace sockets#16613bjacquin wants to merge 3 commits intoPowerDNS:masterfrom
Conversation
07d56d5 to
50913ad
Compare
Pull Request Test Coverage Report for Build 20800973421Details
💛 - Coveralls |
|
I see that spell check is failing for this PR, I have tried a number of alternative to ignore |
|
Hi, I'm the @check-spelling developer. I'm curious what alternatives you tried. The goal is for you to load https://github.com/PowerDNS/pdns/actions/runs/19989803358/attempts/1#summary-57328879876 and use one of the |
docs/http-api/index.rst
Outdated
| * :ref:`setting-webserver-port`: Port to bind the webserver to (not relevant if :ref:`setting-webserver-address` is set to a UNIX domain socket or abstract namespace). | ||
| * :ref:`setting-webserver-allow-from`: Netmasks that are allowed to connect to the webserver (not relevant if :ref:`setting-webserver-address` is set to a UNIX domain socket or abstract namespace). |
There was a problem hiding this comment.
I'd reverse the logic and write
| * :ref:`setting-webserver-port`: Port to bind the webserver to (not relevant if :ref:`setting-webserver-address` is set to a UNIX domain socket or abstract namespace). | |
| * :ref:`setting-webserver-allow-from`: Netmasks that are allowed to connect to the webserver (not relevant if :ref:`setting-webserver-address` is set to a UNIX domain socket or abstract namespace). | |
| * :ref:`setting-webserver-port`: Port to bind the webserver to (only relevant if :ref:`setting-webserver-address` is set to an IP address). | |
| * :ref:`setting-webserver-allow-from`: Netmasks that are allowed to connect to the webserver (only relevant if :ref:`setting-webserver-address` is set to an IP address). |
docs/settings.rst
Outdated
|
|
||
| Webserver/API access is only allowed from these subnets. | ||
| Ignored if ``webserver-address`` is set to a UNIX domain socket. | ||
| Ignored if ``webserver-address`` is set to a UNIX domain socket or abstract namespace. |
There was a problem hiding this comment.
Similarly,
| Ignored if ``webserver-address`` is set to a UNIX domain socket or abstract namespace. | |
| Ignored unless ``webserver-address`` is set to an IP address. |
docs/settings.rst
Outdated
|
|
||
| The port where webserver/API will listen on. | ||
| Ignored if ``webserver-address`` is set to a UNIX domain socket. | ||
| Ignored if ``webserver-address`` is set to a UNIX domain socket or abstract namespace. |
There was a problem hiding this comment.
and
| Ignored if ``webserver-address`` is set to a UNIX domain socket or abstract namespace. | |
| Ignored unless ``webserver-address`` is set to an IP address. |
docs/http-api/index.rst
Outdated
|
|
||
| * :ref:`setting-webserver`: If set to anything but 'no', a webserver is launched. | ||
| * :ref:`setting-webserver-address`: IP address (or UNIX domain socket path, from version 5.0.0 onward) to bind the webserver to. Defaults to 127.0.0.1, which implies that only the local computer is able to connect to the nameserver! To allow remote hosts to connect, change to 0.0.0.0 or the physical IP address of your nameserver. | ||
| * :ref:`setting-webserver-address`: IP address, or UNIX domain socket path (from version 5.0.0 onward) or abstract namespace (Linux only) to bind the webserver to. Defaults to 127.0.0.1, which implies that only the local computer is able to connect to the nameserver! To allow remote hosts to connect, change to 0.0.0.0 or the physical IP address of your nameserver. |
There was a problem hiding this comment.
| * :ref:`setting-webserver-address`: IP address, or UNIX domain socket path (from version 5.0.0 onward) or abstract namespace (Linux only) to bind the webserver to. Defaults to 127.0.0.1, which implies that only the local computer is able to connect to the nameserver! To allow remote hosts to connect, change to 0.0.0.0 or the physical IP address of your nameserver. | |
| * :ref:`setting-webserver-address`: IP address, UNIX domain socket path (from version 5.0.0 onward) or abstract namespace (Linux only) to bind the webserver to. Defaults to 127.0.0.1, which implies that only the local computer is able to connect to the nameserver! To allow remote hosts to connect, change to 0.0.0.0 or the physical IP address of your nameserver. |
| } | ||
|
|
||
| createSocketAndBind(AF_UNIX, (struct sockaddr*)& local, sizeof(local)); | ||
| createSocketAndBind(AF_UNIX, (struct sockaddr*)& local, sizeof(local) - sizeof(local.sun_path) + fname.length()); |
There was a problem hiding this comment.
I am worried this change, which seems to be needed for abstract sockets, may break proper operation on regular unix sockets on some platforms.
So as a very minimum, this length computation (here and in SockaddrWrapper below) should be made conditional depending on whether this is an abstract socket or not.
There was a problem hiding this comment.
This patch is actually part of a prep patch before applying abstract socket patch. As of right now, before this patch, createSocketAndBind() always allocate sizeof(struct sockaddr_un) which on Linux is 110 bytes long, which is not necessary, leading to extra memory allocated. Man page specify that pathlen must not exceed sizeof(struct sockaddr_un)`. As of now, regardless of the size of the UNIX socket path, the following is performed:
bind(4, {sa_family=AF_UNIX, sun_path="/tmp/pdns.controlsocket"}, 110) = 0
This issue becomes only visible when using abstract socket and the client size would then need to connect to the socket specifying the exact same length as the socket name starts with zero.
There was a problem hiding this comment.
Man page specify that pathlen must not exceed sizeof(struct sockaddr_un)`.
Which manpage are you referring to here?
There was a problem hiding this comment.
Which manpage are you referring to here?
Oh sorry, I was referring to unix(7).
There was a problem hiding this comment.
Well, the copy of the manpage available here, says:
• The addrlen argument that describes the enclosing sockaddr_un struc‐
ture should have a value of at least:
offsetof(struct sockaddr_un, sun_path)+strlen(addr.sun_path)+1
or, more simply, addrlen can be specified as sizeof(struct sock‐
addr_un).
Even though *BSD and Linux will behave correctly if you pass the exact size needed, there is absolutely no guarantee that other TCP/IP implementations will, which is why portable code uses sizeof(struct sockaddr_un), and this is exactly what PowerDNS does here. Which is why I strongly believe that this computation change should only performed when we have verified that this is an abstract socket.
| return -1; | ||
|
|
||
| path.copy(ret->sun_path, sizeof(ret->sun_path), 0); | ||
| if (path.find("abns@", 0, 5) == 0) { |
There was a problem hiding this comment.
Some comments would not hurt here. Maybe
| if (path.find("abns@", 0, 5) == 0) { | |
| // If a Linux abstract socket is required, put its name in sun_path after a leading | |
| // NUL byte. | |
| if (path.find("abns@", 0, 5) == 0) { |
| return -1; | ||
|
|
||
| path.copy(ret->sun_path, sizeof(ret->sun_path), 0); | ||
| if (path.find("abns@", 0, 5) == 0) { |
There was a problem hiding this comment.
Also, find here does not guarantee the match will occur at the beginning of the string.
Why not simply use
| if (path.find("abns@", 0, 5) == 0) { | |
| if (path.substr(0, 5) == "abns@") { |
There was a problem hiding this comment.
I don't mind changing current implementation, but since find() returns the position at which the string was found, with the condition applied, we actually guarantee the abns@ will be found at the beginning of the string.
There was a problem hiding this comment.
but find will walk the whole string which is more expensive than just checking the beginning.
|
|
||
| path.copy(ret->sun_path, sizeof(ret->sun_path), 0); | ||
| if (path.find("abns@", 0, 5) == 0) { | ||
| path.copy(ret->sun_path + 1, path.length() -5, 5); |
There was a problem hiding this comment.
| path.copy(ret->sun_path + 1, path.length() -5, 5); | |
| path.copy(ret->sun_path + 1, std::string::npos, 5); |
would probably be simpler.
There was a problem hiding this comment.
In this particular case, std::string::npos returns -1 which would not allow to get socket size without abns@ prefixed remove unfortunately
There was a problem hiding this comment.
copy with the 2nd argument being npos is documented as copying up to the end of the string.
| @@ -796,7 +796,12 @@ int makeUNsockaddr(const std::string& path, struct sockaddr_un* ret) | |||
| if (path.length() >= sizeof(ret->sun_path)) | |||
There was a problem hiding this comment.
You might want to check for the abstract socket prefix earlier, so that, at this point, the length check could be path.length() - 5 /* prefix len */ >= sizeof(ret->sun_path) - 1 for abstract sockets.
i.e.
#ifdef __linux__
bool isLinuxAbstractSocket = path.substr(0, 5) == "abns@";
#else
bool isLinuxAbstractSocket{false};
#endif
if (isLinuxAbstractSocket) {
length check for abstract socket
ret->sun_path setup for abstract socket
}
else {
length check for regular unix socket
ret->sun_path setup for regular unix socket
}
ret is first initialized to zero, and verification is performed to ensure path length is not greater than size of sockaddr_un->sun_path, thus we can reduce amount of data copied to actual size of path. Signed-off-by: Bertrand Jacquin <bertrand@jacquin.bzh>
Returning sizeof(struct sockaddr_un) from SockaddrWrapper bind the
socket to full length of struct sockaddr_un
bind(4, {sa_family=AF_UNIX, sun_path="/tmp/pdns.controlsocket"}, 110) = 0
This change now return a size relative to the actual content of sun_path
as specified in unix(7).
bind(4, {sa_family=AF_UNIX, sun_path="/tmp/pdns.controlsocket"}, 25) = 0
Signed-off-by: Bertrand Jacquin <bertrand@jacquin.bzh>
Abstract namespace is a Linux specific feature to bind a UNIX domain
socket without the need for any filesystem access, which is particularly
interesting to allow a process running in a chroot or with limited
privileges to access the webserver socket over UNIX domain socket.
The main difference from regular UNIX domain socket is that the first
byte of sun_path is set to zero.
Abstract namespace are defined by prefixing value with `abns@`.
$ cat pdns.conf
..
webserver=yes
webserver-address=abns@pdns.http
$ curl -sv --abstract-unix-socket pdns.http http://localhost/metrics
* Trying :0...
* Established connection to localhost ( port 0) from port 0
* using HTTP/1.x
> GET /metrics HTTP/1.1
> Host: localhost
> User-Agent: curl/8.16.0
> Accept: */*
>
* Request completely sent off
< HTTP/1.1 200 OK
< Connection: close
< Content-Length: 15033
< Content-Type: text/plain; version=0.0.4
<
{ [15033 bytes data]
* shutting down connection #0
Signed-off-by: Bertrand Jacquin <bertrand@jacquin.bzh>
50913ad to
f2d785a
Compare
Short description
Checklist
I have: