Page MenuHomeFreeBSD
Differential D28430

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

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
View All Files
Subscribers
gbe

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.
gbe added a subscriber: gbe.Jan 30 2021, 5:55 PM

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

rmacklem added a comment.Jan 30 2021, 6:12 PM

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
gbe accepted this revision as: manpages.Jan 31 2021, 1:17 PM

Just a small nit, otherwise LGTM from manpages

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

This should be exports(5).

rmacklem updated this revision to Diff 83196.Jan 31 2021, 9:49 PM

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

rmacklem marked an inline comment as done.Jan 31 2021, 9:50 PM

Mark inline comment as done.

rmacklem updated this revision to Diff 83255.Feb 2 2021, 1:08 AM

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.

rmacklem updated this revision to Diff 83598.Feb 9 2021, 5:39 PM

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
rmacklem updated this revision to Diff 83686.Feb 11 2021, 1:12 PM

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.

rmacklem marked an inline comment as done.Feb 11 2021, 1:16 PM

Marked inline comment as done.

jhb added a comment.Feb 16 2021, 7:09 PM

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.

rmacklem updated this revision to Diff 84056.Feb 17 2021, 12:18 AM

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.

rmacklem added a comment.Feb 17 2021, 12:27 AM

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.

rmacklem updated this revision to Diff 84175.Feb 18 2021, 1:13 AM

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).

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

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.

Revision Contents
Changeset List

PathSize
usr.sbin/
Makefile
Makefile.sav
5 lines
rpc.tlsclntd/
Makefile
Makefile.sav
29 lines
rpc.tlsclntd.8
rpc.tlsclntd.8.sav
190 lines
rpc.tlsclntd.c
rpc.tlsclntd.c.sav
723 lines
rpc.tlsservd/
Makefile
Makefile.sav
29 lines
rpc.tlscommon.h
rpc.tlscommon.h.sav
68 lines
rpc.tlscommon.c
rpc.tlscommon.c.sav
295 lines
rpc.tlsservd.8
rpc.tlsservd.8.sav
348 lines
rpc.tlsservd.c
rpc.tlsservd.c.sav
883 lines

Diff 83196

View Options

usr.sbin/Makefile

Loading...
View Options

usr.sbin/rpc.tlsclntd/Makefile

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

usr.sbin/rpc.tlsservd/Makefile

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...