Details

Reviewers

jhb
gallatin
glebius

Group Reviewers

manpages

Commits

rGb9cbc85d7272: nfs-over-tls: add user space daemons rpc.tlsclntd and rpc.tlsservd

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 requested review of this revision.Jan 30 2021, 5:31 PM

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.

rmacklem mentioned this in D28432: add rc.d scripts for nfs-over-tls daemons.Jan 31 2021, 12:22 AM

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.

jhb added inline comments.Feb 10 2021, 10:42 PM

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.

rmacklem added inline comments.Feb 17 2021, 12:39 AM

usr.sbin/rpc.tlsclntd/rpc.tlsclntd.c
83	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?

jhb added inline comments.Feb 17 2021, 11:36 PM

usr.sbin/rpc.tlsclntd/rpc.tlsclntd.c
83	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.

rmacklem marked an inline comment as done.Feb 18 2021, 1:16 AM

This revision was not accepted when it landed; it landed in state Needs Review.Feb 18 2021, 10:17 PM

Closed by commit rGb9cbc85d7272: nfs-over-tls: add user space daemons rpc.tlsclntd and rpc.tlsservd (authored by rmacklem). · Explain Why

This revision was automatically updated to reflect the committed changes.

rmacklem added a commit: rGb9cbc85d7272: nfs-over-tls: add user space daemons rpc.tlsclntd and rpc.tlsservd.

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 83196

usr.sbin/Makefile

usr.sbin/rpc.tlsclntd/Makefile

usr.sbin/rpc.tlsclntd/rpc.tlsclntd.8

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

usr.sbin/rpc.tlsservd/Makefile

usr.sbin/rpc.tlsservd/rpc.tlscommon.h

usr.sbin/rpc.tlsservd/rpc.tlscommon.c

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

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

add nfs-over-tls daemons to head/mainClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 83196

usr.sbin/Makefile

usr.sbin/rpc.tlsclntd/Makefile

usr.sbin/rpc.tlsclntd/rpc.tlsclntd.8

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

usr.sbin/rpc.tlsservd/Makefile

usr.sbin/rpc.tlsservd/rpc.tlscommon.h

usr.sbin/rpc.tlsservd/rpc.tlscommon.c

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

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

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

Revision Contents
Changeset List