636504 - Add a new NSPR function that calls getaddrinfo with AI_PASSIVE for server applications (ipv6)

Status: NEW

(NSPR :: NSPR, enhancement)

Description

[Wan-Teh Chang]

The existing NSPR wrapper for getaddrinfo, PR_GetAddrInfoByName,
is designed for client applications, which open sockets for
outgoing connections.

Server applications, which open listening sockets, need to
call getaddrinfo with the AI_PASSIVE flag.  We should add either
a new NSPR function that calls getaddrinfo with AI_PASSIVE, or
a general-purpose PR_GetAddrInfo function that can be used by
both client and server applications.

Comment 1

[Kai Engert (:KaiE:)]

The intention of this bug may not be immediately obvious, so I'd like to add a high level explanation.

Some systems have only one IP stack - either IPv4, or IPv6.

Some of our code uses a server socket, and a client socket to connect to it. 
(For example, when using a loopback socket as the implementation for the NSPR pollable event mechanism, or, between the NSS test tools selfserv and tstclnt.)

We need a NSPR mechanism, that both client code and server code can call, and that will return a working IP address for the local system. The result could be either an IPv4 or an IPv6 address.

Both client and server code can then use the resulting IP address to connect to each other.


I just read the documentation for getaddrinfo and AI_PASSIVE.

If I understand correctly, when calling getaddrinfo with a NULL hostname parameter, the function will return a LIST of IP addresses.

How can we ensure that both client and server code will choose the same IP address to listen/connect?

Even if both server code and client code are implemented to always pick the first entry in the returned list, what happens if a bad operating system implementation sometimes returns "ip4/ip6" and sometimes "ip6/ip4"?


I guess there are multiple solutions to my worry.

(a)
The server is required to listen and multiple stacks, it must make sure to have an open server socket for each of the IP addresses returned.

Then the client will be able to connect, whatever IP address the client chooses to use.

Is this always *simply* doable for servers, or might multiple-listening cause problems, or unwanted additional complexity for servers?

If the answer might be similar to a "yes" in at least some configurations, I'd rather propose to use one of the following approaches, either (b) or (c).


(b)
The new NSPR function could always return only one single IP address, which is the preferred address. For scenarios where there are multiple stacks available, the function could use a hardcoded preference, like "prefer ipv6 if both are available". If necessary, this preference code could be done using platform specific #idefs


(c)
The function returns the full list of IP addresses obtained from the operating system, however, the NSPR function guarantees a stable ordering. It should sort the list using a given, NSPR defined priority, e.g. IPv6 addresses will first in the list, IPv4 later.


I personally have a tendency for simplicity, and towards (b).

I'd define a new function like:
  PR_GetPreferredLoopbackIPAddress(PRNetAddr *result);

Comment 2

[Kai Engert (:KaiE:)]

Attachment: Patch v1

This is a proposed implementation, to get the discussion going.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: v1 - ignoring whitespace - easier to review

Comment 4

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Comment 5

[Kai Engert (:KaiE:)]

Attachment: v2 - ignoring whitespace - easier to review

Comment 6

[Masatoshi Kimura [:emk]]

> +PR_IMPLEMENT(PRStatus) PR_GetPreferredLoopbackIPAddress(PRNetAddr *result,
> +    hints.ai_flags = AI_PASSIVE;
> +    rv = GETADDRINFO(NULL, service, &hints, &res);
If getaddrinfo is called with NULL nodename and the AI_PASSIVE flag,
it will return "0.0.0.0" (aka INADDR_ANY) for AF_INET and "::" for AF_INET6.
I don't think it's correct to use it as a loopback address (especially for
PR_NewPollableEvent implementation on non-Unix platforms).
If you do NOT pass AI_PASSIVE to getaddreinfo, it will returns
"127.0.0.1" for AF_INET "::1" for AF_INET6 (that is, loopback address).

This behavior is written in RFC 3493.
https://tools.ietf.org/html/rfc3493#page-25
>   If the AI_PASSIVE flag is specified, the returned address information
>   shall be suitable for use in binding a socket for accepting incoming
>   connections for the specified service (i.e., a call to bind()).  In
>   this case, if the nodename argument is null, then the IP address
>   portion of the socket address structure shall be set to INADDR_ANY
>   for an IPv4 address or IN6ADDR_ANY_INIT for an IPv6 address.  If the
>   AI_PASSIVE flag is not specified, the returned address information
>   shall be suitable for a call to connect() (for a connection-mode
>   protocol) or for a call to connect(), sendto() or sendmsg() (for a
>   connectionless protocol).  In this case, if the nodename argument is
>   null, then the IP address portion of the socket address structure
>   shall be set to the loopback address.  This flag is ignored if the
>   nodename argument is not null.

Comment 7

[Robert Relyea]

Comment on attachment 514898 [details] [diff] [review]
v2 - ignoring whitespace - easier to review

Review of attachment 514898 [details] [diff] [review]:
-----------------------------------------------------------------

1) I'm OK reviewing this outside of phabricator, but the patch does need to be tested against nss-try before we land it. Since it adds a new function, the patch will need an abi fixup record.
2) The only bug I found is in nspr.def. The new exported function needs to be added to a new section for the latest version of NSPR.
3) The rest are comments that should be answered, but I'm presuming the answers will indicate those parts of the patch are fine.

::: mozilla/nsprpub/pr/src/nspr.def
@@ +473,4 @@
>  ;+	global:
>  		PR_AssertCurrentThreadOwnsLock;
>  		PR_AssertCurrentThreadInMonitor;
> +                PR_GetPreferredLoopbackIPAddress;

This needs to change to NSPR_4.{current_version}

::: mozilla/security/nss/cmd/selfserv/selfserv.c
@@ +1501,3 @@
>  
> +    if (addr.inet.family == PR_AF_INET6) {
> +        listen_sock = PR_OpenTCPSocket(PR_AF_INET6);

We ran into an issue on Linux where if you are using the IPv6 emulator on an IPv4 socket, self serve would fail when it tried to set inheritable. I'm guessing that won't happen here because if you don't have a real IPv6 address, you'll get addr.inet.family == PR_AF_INET?

This happens if the OS supports INET6, but the IPaddress is IPv4.

::: mozilla/security/nss/cmd/tstclnt/tstclnt.c
@@ +684,5 @@
> +            status = PR_GetPreferredLoopbackIPAddress(&addr, "selfserv", portno);
> +            if (status != PR_FAILURE) {
> +                gotLoopbackIP = PR_TRUE;
> +            }
> +        }

why was this needed?

Comment 8

[Robert Relyea]

Have you seen my comments? The patch is pretty close but needs some work.

bob

Comment 9

[Kai Engert (:KaiE:)]

Thanks for your comments. I was on PTO for one week, and I have lots of priority work, so I'm not sure how soon I could follow up. If anyone wants to help contribute the necessary improvements, I'd appreciate it.

Comment 10

[Kai Engert (:KaiE:)]

For example, if you want to make the suggested changes yourself, I'm happy to review the delta.

Comment 11

[Robert Relyea]

Thanks Kai. I'll create an updated patch, but I'd like the answers to the questions I had in the review if you can recall them. Thanks,
bob

Comment 12

[Kai Engert (:KaiE:)]

(In reply to Robert Relyea from comment #11)

Thanks Kai. I'll create an updated patch, but I'd like the answers to the questions I had in the review if you can recall them. Thanks,
bob

I don't remember, but it seems past me has documented it in comment 1:

(In reply to Kai Engert (:KaiE:) from comment #1)

If I understand correctly, when calling getaddrinfo with a NULL hostname
parameter, the function will return a LIST of IP addresses.

How can we ensure that both client and server code will choose the same IP
address to listen/connect?

Even if both server code and client code are implemented to always pick the
first entry in the returned list, what happens if a bad operating system
implementation sometimes returns "ip4/ip6" and sometimes "ip6/ip4"?

and I said I preferred (b) at that time.

Comment 13

[Kai Engert (:KaiE:)]

Thanks :emk, I've just read your comment 6.
It sounds like we should drop the flag AI_PASSIVE, to really get a loopback IP (and not ANY).

Comment 14

[Kai Engert (:KaiE:)]

If we drop AI_PASSIVE, I think we're dropping the original intention that Wan-Teh had - it sounds like he wanted an API to get the best IP for public listening.

We're hijacking this bug for the loopback purpose. I think that's fine, but we might want to update the bug title, if we really drop AI_PASSIVE.

Comment 15

[Robert Relyea]

I've also realized that there are both nspr and nss patches combined into one here. Is there an nspr try server?

Comment 16

[Kai Engert (:KaiE:)]

You can use the nss try server for testing NSPR patches:
https://wiki.mozilla.org/NSS:TryServer#Enable_NSPR_changes_.28--nspr-patch.29

Comment 17

[Robert Relyea]

So, selfserv/tstclient should work even not against the loopback. Is there reason PR_GetPreferredLoopbackIPAddress() couldn't just be PR_GetPerferredIPAddress()?

bob

Comment 18

[Robert Relyea]

Thanks Kai!

Comment 19

[Kai Engert (:KaiE:)]

NSPR pollable events are implemented using a pair of sockets on some platforms, and that shouldn't use a public IP address.
So a loopback address is preferred for that.

Also, it looks like we want to override the parameter given to tstclnt/selfserv, but only if the generic localhost name is given, and tweak it to automatically use a specific IP stack - if necessary.

We probably shouldn't apply this automatic tweaking if any specific hostname other than localhost is given.

Also, when selfserv is asked to bind to localhost, it seems wrong to bind to ANY, which usually is a public IP.

For these reasons it might be best to implement the new API specifically for loopback.

Comment 20

[Robert Relyea]

I'm confused. selfserv and tstclnt are supposed to mimic actual server/client connections. Making it work only on the loopback/localhost case doesn't sound right to me.

bob

Comment 21

[Robert Relyea]

Attachment: Bug 636504 Add a new NSPR function that calls getaddrinfo with AI_PASSIVE for server applications (ipv6)

Kai's implementation of PR_GetPreferredLoopbackIPAddress(PRNetAddr *result);
and updates to tstclnt and selfserv to use it.

Depends on D141538

Comment 22

[Robert Relyea]

This phabricator patch passes treeherder tests. Note that the nspr portion is a separate patch file. Still need to decide what to do about the loopback.

bob

Comment 23

[Robert Relyea]

I've had the phabricator patch up for review. I need to have something checked in in the next week or so, so if this patch isn't good, I'll submit the downstream patch if #ifdef LINUX in them.

bob

Comment 24

[Kai Engert (:KaiE:)]

Sorry for the delay. I was deep down in other work.
I'm working on this now.

Comment 25

[Kai Engert (:KaiE:)]

(In reply to Robert Relyea from comment #22)

This phabricator patch passes treeherder tests. Note that the nspr portion is a separate patch file. Still need to decide what to do about the loopback.

Here is the link that you had forgotten:
https://treeherder.mozilla.org/jobs?repo=nss-try&revision=fa3904f98e0300f96b2da7ba5f085c3f9d1ff687

Comment 26

[Kai Engert (:KaiE:)]

Bob didn't make any changes to the patch, besides the nspr.def file, but that file actually needs a different version number.
I'm going back to the approach that Bob had originally suggested. I'll create an updated patch, and ask Bob for review.

After thinking about this bug more, I'm concluding that we have hijacked the wrong bug.

Wan-Teh's original request is completely different from the problem that Bob is trying to solve now (and I was trying to solve the same problem in 2011).

Wan-Teh wanted a function for server applications, which takes a hostname, and that resolves to an appropriate listen address. Wan-Teh wasn't trying to solve any problem like "which listen IP should be preferred".

But for our selfserv/tstclnt problem at hand, we want an easy solution, because selfserv cannot listen on two IP addresses. The easy solution is to make a decision, which IP address to prefer if more than one stack is available, to ensure that that tstsclnt can reliably connect to it.

Bob suggested to solve this decision making in general. In general means, for hostnames other than localhost. But that would require more work, because in order to reliably pick an IP address to listen on, we'd have to somehow sort a potential list of multiple IP addresses, in order to make a stable selection.

Therefore I suggest that we implement a solution that's specific to our need, and ignore Wan-Teh's original request (for now).

In our scenario, only if localhost or localhost.domain is the requested listen or target node, we go through a new API that makes a decision what the preferred API should be.

While thinking about that, I'm questioning that this makes much sense for a general purpose NSPR API.

Given that we're simply trying to work around a limitation in the selfserv code, it might make sense to simply duplicate the decision making code in both selfserv and tstclnt. This would avoid the complexity of introducing a new API, and having to make sure that our API makes sense in general.

I'll suggest a patch soon.

Comment 27

[Kai Engert (:KaiE:)]

I missed that the implementation of this function calls functions using internal NSPR macros, so we cannot move it to the NSS level.
Never mind, let's add the API.

We can simplify the API, it doesn't need the service name. It only needs the port as input, which will be added into the result addr.

The server will not use the returned address. It will only look at the returned family, and create a new socket of that family.

I'll remove the AI_PASSIVE flag.
man getaddrinfo says, if we don't give that flag, the returned socket will be suitable for a connect call, that's what we want for our use case.

The API can document that the returned address can be used directly by a client to connect to that address, and that a server should look at the family of the returned address to decide which socket to use.

To be closer to the name of other APIs, like PR GetAddrInfoByName, let's call the new one: PR_GetPrefLoopbackAddrInfo

Comment 28

[Kai Engert (:KaiE:)]

Bob, see the separate bugs I have filed, NSPR bug 1769293 and NSS bug 1769295.

Are you able to test that the patch actually works on a host that fails otherwise?

Comment 29

[Kai Engert (:KaiE:)]

I suggest that we move the discussion over to those new bugs.

The whole discussion here was offtopic for this bug here.

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has priority 'P2'.
:KaiE, could you have a look please?
For more information, please visit auto_nag documentation.