[PATCH 1/7] tlshd_handshake_parms: Dispense with unnecessary dynamic storage for peerids.

[PATCH 1/7] tlshd_handshake_parms: Dispense with unnecessary dynamic storage for peerids.

[Ken Milmore]

Signed-off-by: Ken Milmore <ken.milmore@gmail.com>
---
 src/tlshd/client.c    | 2 +-
 src/tlshd/handshake.c | 2 --
 src/tlshd/netlink.c   | 9 +--------
 src/tlshd/tlshd.h     | 2 +-
 4 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/src/tlshd/client.c b/src/tlshd/client.c
index 9c8f512..80107b2 100644
--- a/src/tlshd/client.c
+++ b/src/tlshd/client.c
@@ -434,7 +434,7 @@ static void tlshd_tls13_client_psk_handshake(struct tlshd_handshake_parms *parms
 {
 	unsigned int i;
 
-	if (!parms->peerids) {
+	if (!parms->num_peerids) {
 		tlshd_log_error("No key identities");
 		return;
 	}
diff --git a/src/tlshd/handshake.c b/src/tlshd/handshake.c
index b9de6b3..6d10eaf 100644
--- a/src/tlshd/handshake.c
+++ b/src/tlshd/handshake.c
@@ -185,8 +185,6 @@ out:
 	if (parms.keyring)
 		keyctl_unlink(parms.keyring, KEY_SPEC_SESSION_KEYRING);
 
-	free(parms.peerids);
-
 	if (parms.session_status) {
 		tlshd_log_failure(parms.peername, parms.peeraddr,
 				  parms.peeraddr_len);
diff --git a/src/tlshd/netlink.c b/src/tlshd/netlink.c
index ff9e35a..34a1f47 100644
--- a/src/tlshd/netlink.c
+++ b/src/tlshd/netlink.c
@@ -188,13 +188,6 @@ static void tlshd_parse_peer_identity(struct tlshd_handshake_parms *parms,
 	}
 
 	parms->num_peerids = 1;
-
-	parms->peerids = calloc(parms->num_peerids, sizeof(key_serial_t));
-	if (!parms->peerids) {
-		parms->num_peerids = 0;
-		return;
-	}
-
 	parms->peerids[0] = nla_get_s32(head);
 }
 
@@ -312,7 +305,7 @@ static const struct tlshd_handshake_parms tlshd_default_handshake_parms = {
 	.auth_mode		= HANDSHAKE_AUTH_UNSPEC,
 	.x509_cert		= TLS_NO_CERT,
 	.x509_privkey		= TLS_NO_PRIVKEY,
-	.peerids		= NULL,
+	.peerids		= { 0 },
 	.num_peerids		= 0,
 	.msg_status		= 0,
 	.session_status		= EIO,
diff --git a/src/tlshd/tlshd.h b/src/tlshd/tlshd.h
index 135e1e0..f058a1a 100644
--- a/src/tlshd/tlshd.h
+++ b/src/tlshd/tlshd.h
@@ -39,7 +39,7 @@ struct tlshd_handshake_parms {
 	key_serial_t	keyring;
 	key_serial_t	x509_cert;
 	key_serial_t	x509_privkey;
-	key_serial_t	*peerids;
+	key_serial_t	peerids[1];
 	unsigned int	num_peerids;
 	int		msg_status;
 
-- 
2.47.2

Re: [PATCH 1/7] tlshd_handshake_parms: Dispense with unnecessary dynamic storage for peerids.

[Chuck Lever]

On 6/8/25 1:42 PM, Ken Milmore wrote:

Needs a patch description. Why is this change necessary?


> Signed-off-by: Ken Milmore <ken.milmore@gmail.com>
> ---
>  src/tlshd/client.c    | 2 +-
>  src/tlshd/handshake.c | 2 --
>  src/tlshd/netlink.c   | 9 +--------
>  src/tlshd/tlshd.h     | 2 +-
>  4 files changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/src/tlshd/client.c b/src/tlshd/client.c
> index 9c8f512..80107b2 100644
> --- a/src/tlshd/client.c
> +++ b/src/tlshd/client.c
> @@ -434,7 +434,7 @@ static void tlshd_tls13_client_psk_handshake(struct tlshd_handshake_parms *parms
>  {
>  	unsigned int i;
>  
> -	if (!parms->peerids) {
> +	if (!parms->num_peerids) {
>  		tlshd_log_error("No key identities");
>  		return;
>  	}
> diff --git a/src/tlshd/handshake.c b/src/tlshd/handshake.c
> index b9de6b3..6d10eaf 100644
> --- a/src/tlshd/handshake.c
> +++ b/src/tlshd/handshake.c
> @@ -185,8 +185,6 @@ out:
>  	if (parms.keyring)
>  		keyctl_unlink(parms.keyring, KEY_SPEC_SESSION_KEYRING);
>  
> -	free(parms.peerids);
> -
>  	if (parms.session_status) {
>  		tlshd_log_failure(parms.peername, parms.peeraddr,
>  				  parms.peeraddr_len);
> diff --git a/src/tlshd/netlink.c b/src/tlshd/netlink.c
> index ff9e35a..34a1f47 100644
> --- a/src/tlshd/netlink.c
> +++ b/src/tlshd/netlink.c
> @@ -188,13 +188,6 @@ static void tlshd_parse_peer_identity(struct tlshd_handshake_parms *parms,
>  	}
>  
>  	parms->num_peerids = 1;
> -
> -	parms->peerids = calloc(parms->num_peerids, sizeof(key_serial_t));
> -	if (!parms->peerids) {
> -		parms->num_peerids = 0;
> -		return;
> -	}
> -
>  	parms->peerids[0] = nla_get_s32(head);
>  }
>  
> @@ -312,7 +305,7 @@ static const struct tlshd_handshake_parms tlshd_default_handshake_parms = {
>  	.auth_mode		= HANDSHAKE_AUTH_UNSPEC,
>  	.x509_cert		= TLS_NO_CERT,
>  	.x509_privkey		= TLS_NO_PRIVKEY,
> -	.peerids		= NULL,
> +	.peerids		= { 0 },
>  	.num_peerids		= 0,
>  	.msg_status		= 0,
>  	.session_status		= EIO,
> diff --git a/src/tlshd/tlshd.h b/src/tlshd/tlshd.h
> index 135e1e0..f058a1a 100644
> --- a/src/tlshd/tlshd.h
> +++ b/src/tlshd/tlshd.h
> @@ -39,7 +39,7 @@ struct tlshd_handshake_parms {
>  	key_serial_t	keyring;
>  	key_serial_t	x509_cert;
>  	key_serial_t	x509_privkey;
> -	key_serial_t	*peerids;
> +	key_serial_t	peerids[1];
>  	unsigned int	num_peerids;
>  	int		msg_status;
>  


-- 
Chuck Lever

Re: [PATCH 1/7] tlshd_handshake_parms: Dispense with unnecessary dynamic storage for peerids.

[Ken Milmore]

On 08/06/2025 19:03, Chuck Lever wrote:
> On 6/8/25 1:42 PM, Ken Milmore wrote:
> 
> Needs a patch description. Why is this change necessary?
> 

I find the varied allocation policies used for members of tlshd_handshake_parms a bit sloppy:

- Some of the members are pointers to static singleton buffers outside the struct (peername, peeraddr).
- Then we have remote_peerid, which is an in-struct array of of key_serial_t, size 10.
- Then we have peerids, which is a dynamically-allocated array of key_serial_t, maximum runtime length 1!

This inconsistency has no obvious rationale, is very confusing to newcomers (me!) and is a banana skin for anyone 
attempting to audit the code. This seemed like a bit of low-hanging fruit to tidy up.

Re: [PATCH 1/7] tlshd_handshake_parms: Dispense with unnecessary dynamic storage for peerids.

[Chuck Lever]

On 6/8/25 3:08 PM, Ken Milmore wrote:
> 
> 
> On 08/06/2025 19:03, Chuck Lever wrote:
>> On 6/8/25 1:42 PM, Ken Milmore wrote:
>>
>> Needs a patch description. Why is this change necessary?
>>
> 
> I find the varied allocation policies used for members of tlshd_handshake_parms a bit sloppy:
> 
> - Some of the members are pointers to static singleton buffers outside the struct (peername, peeraddr).
> - Then we have remote_peerid, which is an in-struct array of of key_serial_t, size 10.
> - Then we have peerids, which is a dynamically-allocated array of key_serial_t, maximum runtime length 1!
> 
> This inconsistency has no obvious rationale, is very confusing to newcomers (me!) and is a banana skin for anyone 
> attempting to audit the code. This seemed like a bit of low-hanging fruit to tidy up.

The kernel's handshake netlink code can insert up to five peer
identities into a handshake request. PSK handshakes only support
a single identity, currently; but I recall us discussing the need
to eventually handle more than one.

Hannes, do you see a need for tlshd to handle multiple identities
during a handshake?


-- 
Chuck Lever

Re: [PATCH 1/7] tlshd_handshake_parms: Dispense with unnecessary dynamic storage for peerids.

[Chuck Lever]

On 6/8/25 1:42 PM, Ken Milmore wrote:
> Signed-off-by: Ken Milmore <ken.milmore@gmail.com>
> ---
>  src/tlshd/client.c    | 2 +-
>  src/tlshd/handshake.c | 2 --
>  src/tlshd/netlink.c   | 9 +--------
>  src/tlshd/tlshd.h     | 2 +-
>  4 files changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/src/tlshd/client.c b/src/tlshd/client.c
> index 9c8f512..80107b2 100644
> --- a/src/tlshd/client.c
> +++ b/src/tlshd/client.c
> @@ -434,7 +434,7 @@ static void tlshd_tls13_client_psk_handshake(struct tlshd_handshake_parms *parms
>  {
>  	unsigned int i;
>  
> -	if (!parms->peerids) {
> +	if (!parms->num_peerids) {
>  		tlshd_log_error("No key identities");
>  		return;
>  	}
> diff --git a/src/tlshd/handshake.c b/src/tlshd/handshake.c
> index b9de6b3..6d10eaf 100644
> --- a/src/tlshd/handshake.c
> +++ b/src/tlshd/handshake.c
> @@ -185,8 +185,6 @@ out:
>  	if (parms.keyring)
>  		keyctl_unlink(parms.keyring, KEY_SPEC_SESSION_KEYRING);
>  
> -	free(parms.peerids);
> -
>  	if (parms.session_status) {
>  		tlshd_log_failure(parms.peername, parms.peeraddr,
>  				  parms.peeraddr_len);
> diff --git a/src/tlshd/netlink.c b/src/tlshd/netlink.c
> index ff9e35a..34a1f47 100644
> --- a/src/tlshd/netlink.c
> +++ b/src/tlshd/netlink.c
> @@ -188,13 +188,6 @@ static void tlshd_parse_peer_identity(struct tlshd_handshake_parms *parms,
>  	}
>  
>  	parms->num_peerids = 1;
> -
> -	parms->peerids = calloc(parms->num_peerids, sizeof(key_serial_t));
> -	if (!parms->peerids) {
> -		parms->num_peerids = 0;
> -		return;
> -	}
> -
>  	parms->peerids[0] = nla_get_s32(head);
>  }
>  
> @@ -312,7 +305,7 @@ static const struct tlshd_handshake_parms tlshd_default_handshake_parms = {
>  	.auth_mode		= HANDSHAKE_AUTH_UNSPEC,
>  	.x509_cert		= TLS_NO_CERT,
>  	.x509_privkey		= TLS_NO_PRIVKEY,
> -	.peerids		= NULL,
> +	.peerids		= { 0 },
>  	.num_peerids		= 0,
>  	.msg_status		= 0,
>  	.session_status		= EIO,
> diff --git a/src/tlshd/tlshd.h b/src/tlshd/tlshd.h
> index 135e1e0..f058a1a 100644
> --- a/src/tlshd/tlshd.h
> +++ b/src/tlshd/tlshd.h
> @@ -39,7 +39,7 @@ struct tlshd_handshake_parms {
>  	key_serial_t	keyring;
>  	key_serial_t	x509_cert;
>  	key_serial_t	x509_privkey;
> -	key_serial_t	*peerids;
> +	key_serial_t	peerids[1];
>  	unsigned int	num_peerids;
>  	int		msg_status;
>  

I've pulled all 7 patches in this series into a development branch. I'm
planning to take some of these ideas (with credit to Ken) but rework
them a bit.

I'm leaning towards dynamic allocation for all of the array-related
handshake parameters. That will replace 1/7.

I agree with cleaning up the way hostnames and socket addresses are
stored and how they are dealt with in error flows. Those will probably
also get dynamically allocated, going forward.

I think server-side tlshd should /always/ check a client's source IP
address when IP addresses are listed in the presented certificate's SAN
field. No new configuration option is needed, I will test to make sure
all of that is working as we expect.

The two changes I'm still equivocating on are:

1. [PATCH 5/7] server: Add boolean config option "verify_peername" under
   "[authenticate.server]"

   I'm not comfortable adding a DNS query. There's no way for tlshd to
   confirm that the query results are trustworthy, and it will add an
   indeterminate latency to every handshake.

   IIRC, tlshd used to perform a query like this, and we were asked to
   remove it by our security reviewers.

2. [PATCH 7/7] client: Add boolean config option "relax_sni" under
   "[authenticate.client]"

   I haven't looked closely at this one yet, but I suspect it's
   nearly the same as the deprecated "-n" option, which was too
   insecure to be allowed to remain alive.

I am working on implementing tagging for x.509 certificates. I found
other software products that do something similar, so we are on the
right track with this approach. But it's actually quite a complex
undertaking, due to the variety of information that can be carried in a
certificate. Stay tuned.


-- 
Chuck Lever

Re: [PATCH 1/7] tlshd_handshake_parms: Dispense with unnecessary dynamic storage for peerids.

[Ken Milmore]

On 13/06/2025 22:42, Chuck Lever wrote:
> I think server-side tlshd should /always/ check a client's source IP
> address when IP addresses are listed in the presented certificate's SAN
> field. No new configuration option is needed, I will test to make sure
> all of that is working as we expect.

See my comment below re patch 7 for a possible banana skin.

> The two changes I'm still equivocating on are:
> 
> 1. [PATCH 5/7] server: Add boolean config option "verify_peername" under
>    "[authenticate.server]"
> 
>    I'm not comfortable adding a DNS query. There's no way for tlshd to
>    confirm that the query results are trustworthy, and it will add an
>    indeterminate latency to every handshake.

But note that the existing code already performs a reverse DNS query in order
to obtain a host name from the peer address for logging purposes, so there is
already an indeterminate latency cost.

The forward DNS lookup is a standard practice for increasing the relibility
of a name obtained by reverse lookup, as here. I agree there are ways that
this can be subverted in general cases, but that is true of x.509 with DNS in
general. The present code doesn't perform any additional checks on the
certificate SAN at all.

>    IIRC, tlshd used to perform a query like this, and we were asked to
>    remove it by our security reviewers.

Fair enough, but I would be interested to know where the security risk comes
from. AFAICT, providing a (forward-confirmed) host name for verification against
the certificate can only reduce the scenarios in which the certificate gets accepted,
it cannot increase them. That is to say, if a certificate is rejected by the existing
code without any name check, it will continue to be rejected with the patch applied.
If I'm wrong on this point, I'd be grateful for a citation I can follow up.

The only downside I can think of is that enabling this check may create a false
sense of security (which was never there in the first place), if the admin does
not understand the implications.

That's not to say I don't think that tagging sounds like a better solution,
although given the widest use of x.509 certs is effectively to verify DNS host
names I'm slightly surprised that you don't want to support DNS at all.

Note that the client code *does* already pass the supplied host name to GnuTLS
for verification, so we have a situation where the server name matters on the client
but not vice versa. I was attempting to redress this asymmetry a little.

Checking client names on the server this way will be no good in general internet
situations with NAT, proxies etc (where the client certificate is always likely to
be rejected), but is potentially very useful for adding additional security in a
controlled LAN or VPN situation.

> 2. [PATCH 7/7] client: Add boolean config option "relax_sni" under
>    "[authenticate.client]"
> 
>    I haven't looked closely at this one yet, but I suspect it's
>    nearly the same as the deprecated "-n" option, which was too
>    insecure to be allowed to remain alive.

The documentation for gnutls_server_name_set() states that "IPv4 or IPv6
addresses are not permitted to be set by this function". So if the client
peer address is actually a numeric IP address, surely we should not be calling
this function regardless?

In particular, if you are using certificates with IP addresses in the SAN,
and you are validating those on the server, (as you mention above), I suspect the
certificate will always be rejected regardless of validity. I haven't tested such
cases extensively, so I'm open to correction on this. Once I read the
documentation for gnutls_server_name_set() I realised it should never be given a
hostname containing a numeric IP address regardless of the situation.

> I am working on implementing tagging for x.509 certificates. I found
> other software products that do something similar, so we are on the
> right track with this approach. But it's actually quite a complex
> undertaking, due to the variety of information that can be carried in a
> certificate. Stay tuned.

I really look forward to it!

I'm glad that someone has finally taken this up: A means of securing NFS properly
without Kerberos is *very* long overdue. Was it 15 years or more ago now that
SKPM/LIPKEY was about to be supported but then never made it to fruition? So I
really hope you achieve it this time.

Best wishes,

Ken.

Re: [PATCH 1/7] tlshd_handshake_parms: Dispense with unnecessary dynamic storage for peerids.

[Chuck Lever]

On 6/13/25 7:09 PM, Ken Milmore wrote:
> On 13/06/2025 22:42, Chuck Lever wrote:
>> 1. [PATCH 5/7] server: Add boolean config option "verify_peername" under
>>    "[authenticate.server]"
>>
>>    I'm not comfortable adding a DNS query. There's no way for tlshd to
>>    confirm that the query results are trustworthy, and it will add an
>>    indeterminate latency to every handshake.
> 
> But note that the existing code already performs a reverse DNS query in order
> to obtain a host name from the peer address for logging purposes, so there is
> already an indeterminate latency cost.

In the current 1.1.0 code base, there are three calls to getnameinfo(3).

There are two calls in src/tlshd/log.c. Both of those use only the
NI_NUMERICHOST flag. They do not perform a DNS query.

The third usage, in tlshd_genl_valid_handler(), is done only if the
kernel does not provide a DNS label for the peer. On a server peer, that
can add handshake latency that could be avoided. Adding more uses of the
peer DNS label makes it only more difficult to get rid of this query or
make it optional.


> The forward DNS lookup is a standard practice for increasing the relibility
> of a name obtained by reverse lookup, as here. I agree there are ways that
> this can be subverted in general cases, but that is true of x.509 with DNS in
> general. The present code doesn't perform any additional checks on the
> certificate SAN at all.

Yet. We have a lengthy to-do list.


>>    IIRC, tlshd used to perform a query like this, and we were asked to
>>    remove it by our security reviewers.
> 
> Fair enough, but I would be interested to know where the security risk comes
> from.

There are plenty of well-known DNS injection attacks. For example,
an attacker can pollute DNS caching servers that are not under the
control of the local security administrator.


> AFAICT, providing a (forward-confirmed) host name for verification against
> the certificate can only reduce the scenarios in which the certificate gets accepted,
> it cannot increase them. That is to say, if a certificate is rejected by the existing
> code without any name check, it will continue to be rejected with the patch applied.
> If I'm wrong on this point, I'd be grateful for a citation I can follow up.
> 
> The only downside I can think of is that enabling this check may create a false
> sense of security (which was never there in the first place), if the admin does
> not understand the implications.
> 
> That's not to say I don't think that tagging sounds like a better solution,
> although given the widest use of x.509 certs is effectively to verify DNS host
> names I'm slightly surprised that you don't want to support DNS at all.

A DNS label is trustworthy when it is carried in a cryptographically
signed envelope with a verifiable chain of trust. The DNS labels in
TLS peer certificates are therefore trustworthy. Those acquired by a
typical DNS query are not. I'm not sure what you learn if the two do
not match -- your DNS resolver is compromised?


> Note that the client code *does* already pass the supplied host name to GnuTLS
> for verification, so we have a situation where the server name matters on the client
> but not vice versa. I was attempting to redress this asymmetry a little.

When mounting an NFS server, the server's DNS label comes from the
client administrator, so we assume trust in that information. The server
does not have the same benefit with regard to client DNS labels.


-- 
Chuck Lever

Re: [PATCH 1/7] tlshd_handshake_parms: Dispense with unnecessary dynamic storage for peerids.

[Ken Milmore]

On 14/06/2025 01:40, Chuck Lever wrote:
> When mounting an NFS server, the server's DNS label comes from the
> client administrator, so we assume trust in that information. The server
> does not have the same benefit with regard to client DNS labels.

In case it helps, I'd like to provide a bit more detail on my use-case, as a
rationale for why I developed these patches, and to show what I would hope to
see covered in a future release of ktls-utils which supports certificate tags
or suchlike.

Suppose I have an NFS server with exports configured like so:

# /etc/exports:
/srv/foo foo.example.org(xprtsec=mtls,rw,sync,no_subtree_check)
/srv/bar bar.example.org(xprtsec=mtls,rw,sync,no_subtree_check)
/srv/baz baz.example.org(xprtsec=mtls,rw,sync,no_subtree_check)

Now I give each of the clients foo, bar and baz their own individual private
key/certificate pair and add all these certificates to the trust store on the
server.

I find that as expected, the exports are effectively secured against access
from random clients which have not been enrolled on the server.

Note that host foo is only meant to be able to access the /srv/foo share. But
suppose that the administrator of foo changes its IP address to match that of
host baz. The admin finds that they can in fact mount any of the shares simply
by adopting the IP address of the other clients. This is something that has, of
course, always been possible with unsecured NFS. But adding mtls actually has
not improved this aspect of security at all.

When I discovered this, I was quite surprised. There's nothing to warn about it
in the man pages relating to NFS and mtls. I expect I'm not the only person who
missed the fact that mtls simply doesn't protect against authorised clients
impersonating each other *even if they have different key/cert pairs and do not
divulge them*.

For my use case, this rendered mtls pretty much useless. I want each client to
be policed individually wrt the shares it gets access to.

Because the hostname(s), or address(es) of the permitted client(s) is/are
explicitly mentioned in /etc/exports, I realised that tying a client to its
certificate effectively allowed me to enforce these share "permissions".

With my patches and verify_peername=true, if host foo attempts to connect to
the baz share by spoofing its IP address, it will be rejected because it does
not have the baz certificate.

Now for this to be watertight, it does require the server admin to ensure one
of the following holds:

- All authorised clients are assigned static IP addresses and these are listed
in /etc/hosts or similar. Or,

- All authorised clients are assigned static IP addresses and these are used
explicitly in /etc/exports instead of DNS names. Or,

- The admin has control of local name resolution for the example.org domain.

What applies to host names above could also be applied to subnets/subdomains
with care, so all sorts of flexibility is possible.

Now I agree the above requirements will be unworkable in may scenarios. But
they happen to be fine for my use case. I've gained the ability to lock NFS
shares down to individual clients, or groups of clients, at the cost of having
to control client naming/addressing. I've found this fairly easy to achieve
using an IPv6 ULA subnet specifically for NFS mounts.


If certificate tagging gets implemented, then I'd hope that this can be used
to tie client certificates to exports in a similar way. But presumably that
would require another change to the NFS export syntax, and a change in the
kernel netlink interface?

Eventually I might hope to be able to do something like this to achieve the
same effect without any reliance on client addressing:

# /etc/exports:
/srv/foo foo.example.org(xprtsec=mtls,tag=FOOTAG,rw,sync,no_subtree_check)
/srv/bar bar.example.org(xprtsec=mtls,tag=BARTAG,rw,sync,no_subtree_check)
/srv/baz baz.example.org(xprtsec=mtls,tag=BAZTAG,rw,sync,no_subtree_check)

Where presumably FOOTAG is mentioned somewhere in foo's client certificate.

In the meantime, it might be helpful if the NFS documentation were to
explicitly mention that mtls does not discriminate between clients for the
purpose of granting access to mounts, and therefore does not protect against
cross-impersonation attacks between "trusted" clients.

Re: [PATCH 1/7] tlshd_handshake_parms: Dispense with unnecessary dynamic storage for peerids.

[Chuck Lever]

On 6/13/25 5:42 PM, Chuck Lever wrote:
> I've pulled all 7 patches in this series into a development branch. I'm
> planning to take some of these ideas (with credit to Ken) but rework
> them a bit.
> 
> I'm leaning towards dynamic allocation for all of the array-related
> handshake parameters. That will replace 1/7.
> 
> I agree with cleaning up the way hostnames and socket addresses are
> stored and how they are dealt with in error flows. Those will probably
> also get dynamically allocated, going forward.
> 
> I think server-side tlshd should /always/ check a client's source IP
> address when IP addresses are listed in the presented certificate's SAN
> field. No new configuration option is needed, I will test to make sure
> all of that is working as we expect.

I implemented this.

However, apparently, source IP addresses can be spoofed as easily as DNS
query results. I don't think we can make even the IP address check as
secure as it should be.

Add to that, such a check is not possible to do for clients with
dynamically-assigned IP addresses. That includes most IPv6-based
clients.


> The two changes I'm still equivocating on are:
> 
> 1. [PATCH 5/7] server: Add boolean config option "verify_peername" under
>    "[authenticate.server]"
> 
>    I'm not comfortable adding a DNS query. There's no way for tlshd to
>    confirm that the query results are trustworthy, and it will add an
>    indeterminate latency to every handshake.
> 
>    IIRC, tlshd used to perform a query like this, and we were asked to
>    remove it by our security reviewers.
> 
> 2. [PATCH 7/7] client: Add boolean config option "relax_sni" under
>    "[authenticate.client]"
> 
>    I haven't looked closely at this one yet, but I suspect it's
>    nearly the same as the deprecated "-n" option, which was too
>    insecure to be allowed to remain alive.

I might not fully understand the vulnerabilities you are trying to
defend against. IIUC, there are two distinct impersonation attacks:

- A client's certificate and private key are copied to a different
  system

- An attacker creates a certificate that duplicates an existing
  Subject

Seems like both of these can be mitigated using a narrow truststore
for the NFS server's clients and/or a CRL.

Do you feel up to opening an issue [1] and describing your threat
model?


-- 
Chuck Lever

[1]: https://github.com/oracle/ktls-utils/issues

Re: [PATCH 1/7] tlshd_handshake_parms: Dispense with unnecessary dynamic storage for peerids.

[Ken Milmore]

On 18/06/2025 14:42, Chuck Lever wrote:
> I implemented this.
> 
> However, apparently, source IP addresses can be spoofed as easily as DNS
> query results. I don't think we can make even the IP address check as
> secure as it should be.
> 
> Add to that, such a check is not possible to do for clients with
> dynamically-assigned IP addresses. That includes most IPv6-based
> clients.

I'm grateful you've given these patches such careful consideration.
You've demonstrated how this sort of client verification doesn't apply
in many situations (insecure DNS, dynamic IP addresses etc). I always
understood these limitations, which is why I made the functionality
completely optional.

Given that tlshd is a very general mechanism, proposed for purposes
other than just securing NFS mounts, I can understand that my approach
is not an ideal fit.

However I still don't feel you've grasped the reasons why this approach
is useful, and could be used to enhance NFS security in certain limited
(but IMHO very important) cases.

To spell it out again:

- The current position is that mtls will allow access to ANY client with
a certificate in the local trust store.

- This means a client is ether trusted to access ALL exported NFS mounts,
or NONE of them.

- For effective security, I wish to limit that further so that a client
must have a certificate in the local trust store AND that certificate must
match the network address or host name they are connecting from.

Why is that useful? Because each NFS mount is exported to a particular
client or group of clients named by network address or hostname in
/etc/exports. With insecure NFS, this is not enforced in any secure way
and therefore a client can easily impersonate any other client and gain
access to the mounts which are exported to it, simply by address spoofing.

These patches were to enforce that the same checking already performed
against the NFS mount point are also applied to the certificate, therefore
it attempts to PREVENT TRUSTED CLIENTS FROM IMPERSONATING EACH OTHER,
limiting them to only those mounts for which they have been specifically
addressed in /etc/exports. See my previous post for examples.

Without this additional checking, a client is either trusted to access
every mount, or untrusted to access any of them. This greatly reduces the
usefulness of mtls. An analogy with user accounts would be to say that
you can have as many users as you like, but they all get root access!

When looked at this way, you will hopefully realise that in scenarios
where it is possible to verify all client addresses, doing so can only
add to security because it ensures that a client must have the correct
certificate AND an address that matches it, a more restrictive criterion.

> I might not fully understand the vulnerabilities you are trying to
> defend against. IIUC, there are two distinct impersonation attacks:
> 
> - A client's certificate and private key are copied to a different
>   system
> 
> - An attacker creates a certificate that duplicates an existing
>   Subject
> 
> Seems like both of these can be mitigated using a narrow truststore
> for the NFS server's clients and/or a CRL.

The problem, it seems to me, is that having a global truststore is a poor
model for securing client access with mtls.

A global truststore works well for a client accessing a server, because
it wants to verify that the server is not being spoofed and gain a secure
channel to it. This is how CA certificates are most commonly used.

But with mtls we need to look at things from the server POV. The server is
going to grant access to some resource (e.g. an NFS share). It can check
if the client has a trusted certificate, but if that is all, then it implies
that EVERY trusted client gets access to EVERY resource or share. Not very
fine-grained.

In terms of trust stores, I guess the only way to mitigate this would be
to specify a different server trust store explicitly for every share in
/etc/exports. That seems rather unwieldy, but it would work.

> Do you feel up to opening an issue [1] and describing your threat
> model?

I can do so, but I'm not sure whether to frame this as merely a lack of
documentation or a security threat. It depends on the security expectations
people have of mtls. A big warning in the NFS man page saying "mtls does not
protect against trusted clients impersonating each other" might be sufficient.

Let me know your thoughts.

-Ken.





 

Re: [PATCH 1/7] tlshd_handshake_parms: Dispense with unnecessary dynamic storage for peerids.

[Chuck Lever]

On 6/18/25 11:03 AM, Ken Milmore wrote:
> 
> On 18/06/2025 14:42, Chuck Lever wrote:
>> I implemented this.
>>
>> However, apparently, source IP addresses can be spoofed as easily as DNS
>> query results. I don't think we can make even the IP address check as
>> secure as it should be.
>>
>> Add to that, such a check is not possible to do for clients with
>> dynamically-assigned IP addresses. That includes most IPv6-based
>> clients.
> 
> I'm grateful you've given these patches such careful consideration.
> You've demonstrated how this sort of client verification doesn't apply
> in many situations (insecure DNS, dynamic IP addresses etc). I always
> understood these limitations, which is why I made the functionality
> completely optional.
> 
> Given that tlshd is a very general mechanism, proposed for purposes
> other than just securing NFS mounts, I can understand that my approach
> is not an ideal fit.
> 
> However I still don't feel you've grasped the reasons why this approach
> is useful, and could be used to enhance NFS security in certain limited
> (but IMHO very important) cases.
> 
> To spell it out again:
> 
> - The current position is that mtls will allow access to ANY client with
> a certificate in the local trust store.
> 
> - This means a client is ether trusted to access ALL exported NFS mounts,
> or NONE of them.
> 
> - For effective security, I wish to limit that further so that a client
> must have a certificate in the local trust store AND that certificate must
> match the network address or host name they are connecting from.

I think you are conflating "trusting a certificate" with "using the
client identity for authorizing access to an NFS share".

tlshd does the former. NFSD is going to have to do the latter, and it
doesn't do that yet. That's what we want to do with handshake tagging.


> Why is that useful? Because each NFS mount is exported to a particular
> client or group of clients named by network address or hostname in
> /etc/exports. With insecure NFS, this is not enforced in any secure way
> and therefore a client can easily impersonate any other client and gain
> access to the mounts which are exported to it, simply by address spoofing.
> 
> These patches were to enforce that the same checking already performed
> against the NFS mount point are also applied to the certificate, therefore
> it attempts to PREVENT TRUSTED CLIENTS FROM IMPERSONATING EACH OTHER,
> limiting them to only those mounts for which they have been specifically
> addressed in /etc/exports. See my previous post for examples.
> 
> Without this additional checking, a client is either trusted to access
> every mount, or untrusted to access any of them. This greatly reduces the
> usefulness of mtls. An analogy with user accounts would be to say that
> you can have as many users as you like, but they all get root access!

Yes, that's the situation today. Our proposed solution is tagging the
client handshakes and letting NFSD gate access based on the tag list.


> When looked at this way, you will hopefully realise that in scenarios
> where it is possible to verify all client addresses, doing so can only
> add to security because it ensures that a client must have the correct
> certificate AND an address that matches it, a more restrictive criterion.

We are very aware that "xprtsec=mtls", as NFSD implements it, currently
does not perform fine-grained access control checking.


>> I might not fully understand the vulnerabilities you are trying to
>> defend against. IIUC, there are two distinct impersonation attacks:
>>
>> - A client's certificate and private key are copied to a different
>>   system
>>
>> - An attacker creates a certificate that duplicates an existing
>>   Subject
>>
>> Seems like both of these can be mitigated using a narrow truststore
>> for the NFS server's clients and/or a CRL.
> 
> The problem, it seems to me, is that having a global truststore is a poor
> model for securing client access with mtls.
> 
> A global truststore works well for a client accessing a server, because
> it wants to verify that the server is not being spoofed and gain a secure
> channel to it. This is how CA certificates are most commonly used.
> 
> But with mtls we need to look at things from the server POV. The server is
> going to grant access to some resource (e.g. an NFS share). It can check
> if the client has a trusted certificate, but if that is all, then it implies
> that EVERY trusted client gets access to EVERY resource or share. Not very
> fine-grained.

That is the current picture, and we agree that is not adequate.

The only task for tlshd is to determine if the certificate is trusted.
It is up to NFSD to authorize access based on the certificate, once it
is trusted.

The question of TLS client impersonation is strictly one of how the
client certificates can be misused to become trusted. The question of
authorizing access to NFS shares is entirely separate.


> In terms of trust stores, I guess the only way to mitigate this would be
> to specify a different server trust store explicitly for every share in
> /etc/exports. That seems rather unwieldy, but it would work.
> 
>> Do you feel up to opening an issue [1] and describing your threat
>> model?
> 
> I can do so, but I'm not sure whether to frame this as merely a lack of
> documentation or a security threat. It depends on the security expectations
> people have of mtls. A big warning in the NFS man page saying "mtls does not
> protect against trusted clients impersonating each other" might be sufficient.

TLS client impersonation has a particular meaning, so that is probably
not as accurate as it could be.

"The 'xprtsec=mtls' export option ensures the client is trusted. NFSD
continues to rely on the machine name (as described in this man page),
which is not secure, to authorize access to exported file systems."

That would be added to exports(5), not tlshd.conf.


-- 
Chuck Lever