Page MenuHomeFreeBSD

add nfs-over-tls daemons to head/main
ClosedPublic

Authored by rmacklem on Jan 30 2021, 5:31 PM.
Tags
None
Referenced Files
F105937531: D28430.id83255.diff
Sun, Dec 22, 7:54 PM
Unknown Object (File)
Sat, Dec 21, 4:10 PM
Unknown Object (File)
Sat, Dec 21, 4:07 PM
Unknown Object (File)
Sat, Dec 21, 3:56 PM
Unknown Object (File)
Sat, Dec 21, 3:56 PM
Unknown Object (File)
Sat, Dec 7, 3:06 AM
Unknown Object (File)
Wed, Dec 4, 3:37 PM
Unknown Object (File)
Mon, Dec 2, 3:59 PM
Subscribers

Details

Summary

The kernel changes required for nfs-over-tls have been
committed to head for some time.
However, the userspace daemons could not be committed,
since they require an OpenSSL with KTLS support.

Since openssl patches that add KTLS support have now
been committed to head/main (thanks jhb@), these
daemons can now be committed.

I realize this is a lot to review, but if someone could
at least look at the Makefile change for usr.sbin/Makefile,
that would be appreciated.

I would like to commit this soon in hopes that they
can be MFC'd to stable/13 and make it in FreeBSD13.

Test Plan

These daemons have been tested for nfs-over-tls for
some time and are also available via a port I created
for third party testers to use.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

rmacklem created this revision.

Are there any rc.d changes missing to use the new daemons?

Well, not exactly missing. I was planning on
the rc.d changes being a separate review,
in part because they will need a detailed
look, given my lack of expertise w.r.t. them.

I will put them up soon.

Just a small nit, otherwise LGTM from manpages

usr.sbin/rpc.tlsservd/rpc.tlsservd.8
203

This should be exports(5).

Fix export to exports as reported by gbe@.
Also, delete a couple of debug output lines
that got missed by a previous edit.

Mark inline comment as done.

The return value for SSL_get_verify_result() is
long and not int.
This update declares a long variable called verfret
to hold the value returned by SSL_get_verify_result()
instead of using "ret", which is an int.

This does not make semantic change, since the
returned value easily fits in an int.
Found by code inspection.

Patch the daemons so that they will build
for an OpenSSL that is not patched for KTLS.
Also added a check for them being built with
an OpenSSL with KTLS enabled and just errx()
if not.

This should not matter for main, etc. since
they are only built when KTLS is enabled, but
it allows the port to build the daemons.

usr.sbin/Makefile
108

I think the current style is to instead add this below as

SUBDIR.${MK_OPENSSL_KTLS}+= rpc.tlsclntd
SUBDIR.${MK_OPENSSL_KTLS}+= rpc.tlsservd

Fixed usr.sbin/Makefile as suggested by jhb@.

I saw lines like these, but didn't know what
they meant. Now I do.

Tested via a tinderbox build.

Marked inline comment as done.

I haven't really read over the daemon code in detail and probably won't get to it, but I think the Makefile glue is fine. Someone might want a dedicated src.conf knob for this that depends on MK_OPENSSL_KTLS, but given we don't have an existing src.conf knob for the other NFS daemons, I think what you've done here is probably fine for now.

usr.sbin/rpc.tlsservd/rpc.tlsservd.c
155

I suspect what would be better here is to just make this be a compile error instead by removing these #ifdef's here and elsewhere and assume the KTLS API is present.

Essentially revert the last changes that made the
code build without KTLS. Always building seems
to be a desirable property for a port, which is why
I made the change.

For head (I guess I should call it main now?),
jhb@ suggests that compile time errors are
preferred and, since I don't like reading #ifdef'd
code, revert the last changes.

Mark jhb@s comment as done.
W.r.t. the review of the rest of it, I understand
completely and will probably commit soon without
a full review.

Since KTLS is not enabled by default in 13.0,
I don't think there is any need to try and
get this in releng/13.0, so there is no rush.

Thanks for your comments.

usr.sbin/rpc.tlsservd/rpc.tlsservd.c
155

I did it this way because the ports guys wanted it
to always build for the port.

However I agree that for src, it seems better to
just have it fail to compile.
Maintaining a slightly different version for the
port should be ok.

usr.sbin/rpc.tlsclntd/rpc.tlsclntd.c
82

Does anyone know what the best setting is for
SSL_CTX_set_cipher_lisr() when using KTLS?

I found the above in some example code and
it seems to work, but I have no idea if there
is a better choice?

usr.sbin/rpc.tlsclntd/rpc.tlsclntd.c
82

Hmmm, I would say to let OpenSSL use its default settings, but you might want to restrict this a bit to match the ciphers supported by KTLS. You can use 'openssl ciphers -s <string>' to see what ciphers match the strings you use. I would use "AESGCM" for now which matches all of the cipher variants using AES-GCM. Once Chacha20-Poly1305 support for KTLS lands, you should change this to "AESGCM:CHACHA20". The older AES-CBC variants would be covered by "AES+SHA", but we don't have RX support for those in all cases and they really shouldn't be used unless someone needs them. You might consider adding a command line option to allow the user to override it? Keep in mind that TLS 1.3 uses a different API for this than TLS <= 1.2, and we don't currently support KTLS RX for TLS 1.3.

As suggested by jhb@, add a command line option
to rpc.tlsclntd to specify preferred ciphers.
If the option is not specified, the daemon does
not make a SSL_CTX_set_cipher_list() call.

This works for the OpenSSL in head (main).

Mark inline comment as done.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 18 2021, 10:17 PM
This revision was automatically updated to reflect the committed changes.