908617 - Add tstclnt option to enforce the use of either IPv4 or IPv6

Status: RESOLVED FIXED

(NSS :: Tools, defect, P1)

Description

[Kai Engert (:KaiE:)]

I propose to add these command line parameters to the tstclnt tool:
  -4  Enforce to use an IPv4 destination address
  -6  Enforce to use an IPv6 destination address

Unfortunately a few of the build slaves used for NSS testing don't have connectivity to the public IPv6 internet addresses.

This is a problem, as one of the external hosts that we rely on for testing has been configured to be IPv6 enabled, and contains an IPv6 address in DNS, and tstclnt attempts to use it.

Therefore, I propose to use 
  tstclnt -4
as a temporary solution, until we no longer depend on the external host, or until we can get full IPv6 connectivity on all build slaves used for testing.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: patch v1

Comment 2

[Elio Maldonado]

Comment on attachment 794585 [details] [diff] [review]
patch v1

r+, with a minor language usage nitpick. English usage prefers the gerund is over the infinitive. 

Please change
+    fprintf(stderr, "%-20s Enforce to use an IPv4 destination address\n", "-4");
+    fprintf(stderr, "%-20s Enforce to use an IPv6 destination address\n", "-6");
to
+    fprintf(stderr, "%-20s Enforce using an IPv4 destination address\n", "-4");
+    fprintf(stderr, "%-20s Enforce using an IPv6 destination address\n", "-6");

The patch is otherwise fine.

Comment 3

[Kai Engert (:KaiE:)]

Checked in with change as requested.

https://hg.mozilla.org/projects/nss/rev/7ec8979863be

Comment 4

[Wan-Teh Chang]

Comment on attachment 794585 [details] [diff] [review]
patch v1

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

r=wtc. I suggest some changes. In particular, the change of
PR_AF_UNSPEC to PR_AF_INET or PR_AF_INET6 is the right way to
restrict the address family.

::: cmd/tstclnt/tstclnt.c
@@ +235,5 @@
>      fprintf(stderr, "%-20s Test -F allows 0=any (default), 1=only OCSP, 2=only CRL\n", "-M");
>      fprintf(stderr, "%-20s Restrict ciphers\n", "-c ciphers");
>      fprintf(stderr, "%-20s Print cipher values allowed for parameter -c and exit\n", "-Y");
> +    fprintf(stderr, "%-20s Enforce to use an IPv4 destination address\n", "-4");
> +    fprintf(stderr, "%-20s Enforce to use an IPv6 destination address\n", "-6");

Nit: maybe we should say -4 and -6 cannot both be specified.

@@ +990,2 @@
>  	addrInfo = PR_GetAddrInfoByName(host, PR_AF_UNSPEC, 
>  	                                PR_AI_ADDRCONFIG | PR_AI_NOCANONNAME);

The right way to restrict the address family is to change
the PR_AF_UNSPEC argument to
- PR_AF_INET if allowIPv6 is false, or
- PR_AF_INET6 if allowIPv4 is false.

Then you don't need to change the do-while loop below.

@@ +1000,5 @@
> +		    break;
> +		if (addr.raw.family == PR_AF_INET6 && allowIPv6)
> +		    break;
> +	    }
> +	} while (enumPtr);

Note: you can ignore this suggestion if you make my
suggested change above.

Nit: we can rewrite this loop so that we don't do the
null pointer check of enumPtr twice.

    for (;;) {
        enumPtr = PR_EnumerateAddrInfo(enumPtr, addrInfo, portno, &addr);
        if (enumPtr == NULL)
            break;
        if (addr.raw.family == PR_AF_INET && allowIPv4)
            break;
        if (addr.raw.family == PR_AF_INET6 && allowIPv6)
            break;
    }

::: tests/ocsp/ocsp.sh
@@ +52,5 @@
>  ocsp_stapling()
>  {
>    TESTNAME="startssl valid, supports OCSP stapling"
>    echo "$SCRIPTNAME: $TESTNAME"
> +  echo "tstclnt -4 -V tls1.0: -T -v -F -M 1 -O -h kuix.de -p 5143 -d . < ${REQF}"

Please add a comment to explain why we need to pass -4 to
these tstclnt commands (as a temporary workaround for lack
of IPv6 connectivity on some build bot slaves).

@@ +95,5 @@
>  
>    TESTNAME="live valid, supports OCSP stapling"
>    echo "$SCRIPTNAME: $TESTNAME"
>    echo "tstclnt -V tls1.0: -T -v -F -M 1 -O -h login.live.com -p 443 -d . < ${REQF}"
>    ${BINDIR}/tstclnt -V tls1.0: -T -v -F -M 1 -O -h login.live.com -p 443 -d . < ${REQF}

Can you explain why you don't use -4 with these tstclnt
commands?

Comment 5

[Kai Engert (:KaiE:)]

(In reply to Wan-Teh Chang from comment #4)
> Can you explain why you don't use -4 with these tstclnt
> commands?

Because it's currently not necessary, as those hosts don't resolve to IPv6 addresses.

I don't think it's necessary to enforce these hosts to use IPv4, as it won't be a complete solution to all potential future IPv6 problems, because there are other tests that depend on external hostnames. For example, we have tests that will contact the OCSP responders embedded in some commercial certificates that we have checked in, and if those OCSP responder hostnames will ever resolve to IPv6 addresses... Let's postpone problem workarounds until we need them for specific tests.

Comment 6

[Kai Engert (:KaiE:)]

(In reply to Wan-Teh Chang from comment #4)
> @@ +990,2 @@
> >  	addrInfo = PR_GetAddrInfoByName(host, PR_AF_UNSPEC, 
> >  	                                PR_AI_ADDRCONFIG | PR_AI_NOCANONNAME);
> 
> The right way to restrict the address family is to change
> the PR_AF_UNSPEC argument to
> - PR_AF_INET if allowIPv6 is false, or
> - PR_AF_INET6 if allowIPv4 is false.

Your proposal doesn't work on my Linux computer.

When calling PR_GetAddrInfoByName with PR_AF_INET6 I get:
  tstclnt: error looking up host: PR_INVALID_ARGUMENT_ERROR: Invalid function argument

https://hg.mozilla.org/projects/nspr/file/287ac89292b5/pr/src/misc/prnetdb.c#l1978

Comment 7

[Kai Engert (:KaiE:)]

Attachment: Incremental patch v2 to address some review comments

Comment 8

[Wan-Teh Chang]

Comment on attachment 794818 [details] [diff] [review]
Incremental patch v2 to address some review comments

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

r=wtc. Thanks for testing my fixes. I will need to fix that NSPR
limitation first. (We can also remove the -6 option, which this
workaround doesn't need.)

::: cmd/tstclnt/tstclnt.c
@@ +236,5 @@
>      fprintf(stderr, "%-20s Restrict ciphers\n", "-c ciphers");
>      fprintf(stderr, "%-20s Print cipher values allowed for parameter -c and exit\n", "-Y");
>      fprintf(stderr, "%-20s Enforce using an IPv4 destination address\n", "-4");
>      fprintf(stderr, "%-20s Enforce using an IPv6 destination address\n", "-6");
> +    fprintf(stderr, "%-20s (Options -4 and -6 cannot be combined)\n", "");

Nit: add a period (.) after "combined".

::: tests/ocsp/ocsp.sh
@@ +52,5 @@
>  ocsp_stapling()
>  {
> +  # Parameter -4 is used as a temporary workaround for lack of IPv6 connectivity
> +  # on some build bot slaves.
> +  

Nit: remove the spaces on this line. (They're visible if you use the splint
review tool.)

Comment 9

[Kai Engert (:KaiE:)]

(In reply to Wan-Teh Chang from comment #8)
> We can also remove the -6 option, which this workaround doesn't need.

Or we can decide that an unnecessary loop iteration in a test tool isn't that much of a problem.

Comment 10

[Kai Engert (:KaiE:)]

Comment on attachment 794818 [details] [diff] [review]
Incremental patch v2 to address some review comments

Checked in with nits addressed.

https://hg.mozilla.org/projects/nss/rev/82e7c0ea4364