pre-release of v7

pre-release of v7

[Chuck Lever III]

Hi-

I'm now trying to take a step or two towards a more typical development
process for the kernel patches and ktls-utils, as we replace the old
upcall mechanism with netlink. This is in preparation for a release of
ktls-utils-0.8, once netdev is close to happy with the kernel part.

I have pushed pre-release branches of v7 so you can comment early, test
it, or send me patches /before/ I post "Another crack at a handshake
upcall mechanism" v7 on netdev@. What a concept, eh?


Kernel part: The upcall-v7 branch in

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git


User space part: The netlink-v7 branch in

https://github.com/oracle/ktls-utils


The one thing I'm waiting on now is for Jakub to deal properly with
the license and copyright notices in the YAML specs and generated C
source code. As an old SunRPC hand, I feel that the code generator
needs to copy the spec's license and copyright notices verbatim to
the generated C code. Jakub is not certain this is required; his code
generator adds a license notice that is sometimes not compatible with
the spec's license.

We are planning to loop the Linux Foundation in to try to extract some
legal advice.

--
Chuck Lever

Re: pre-release of v7

[Hannes Reinecke]

On 3/9/23 15:41, Chuck Lever III wrote:
> Hi-
> 
> I'm now trying to take a step or two towards a more typical development
> process for the kernel patches and ktls-utils, as we replace the old
> upcall mechanism with netlink. This is in preparation for a release of
> ktls-utils-0.8, once netdev is close to happy with the kernel part.
> 
> I have pushed pre-release branches of v7 so you can comment early, test
> it, or send me patches /before/ I post "Another crack at a handshake
> upcall mechanism" v7 on netdev@. What a concept, eh?
> 
<applause>

> 
> Kernel part: The upcall-v7 branch in
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> 
Hmm. Guess I'll branch off that, and push my patches on top of
that. They probably have to go via a different maintainer (read: hch),
so keeping them separately might be useful.

Or having them on your tree, too?
Dunno.

> 
> User space part: The netlink-v7 branch in
> 
> https://github.com/oracle/ktls-utils
> 
Ah. There was the 'before' part of your mail...
I've pushed a patchset for the 'netlink' branch, containing fixes
to get PSK running.
Will be rebasing them to v7.

> 
> The one thing I'm waiting on now is for Jakub to deal properly with
> the license and copyright notices in the YAML specs and generated C
> source code. As an old SunRPC hand, I feel that the code generator
> needs to copy the spec's license and copyright notices verbatim to
> the generated C code. Jakub is not certain this is required; his code
> generator adds a license notice that is sometimes not compatible with
> the spec's license.
> 
Hmm. Not that concerned; my company has not a strong policy here :-)

But anyway, things look (mostly) good:

[89683.623479] nvme nvme0: connecting queue 0
[89683.623610] nvme nvme0: queue 0: start TLS with key 21712227
[89683.623622] tls_client_hello_psk: peerid 21712227
[89683.623687] nvmet_tcp: queue 0: TLS ServerHello
[89683.623695] tls_server_hello_psk: peerid 0
[89683.707089] nvmet_tcp: queue 0: TLS handshake done, key 21712227, 
status 0
[89683.707099] nvmet_tcp: queue 0: restarting queue after TLS handshake
[89683.707109] nvme nvme0: queue 0: TLS handshake done, key 21712227, 
status 0
[89683.708138] nvme nvme0: queue 0: TLS handshake complete, error 0
[89683.708781] nvmet_tcp: queue 0: failed to map data
[89745.034618] nvme nvme0: queue 0: timeout cid 0x0 type 4 opcode 0x7f 
(Connect)
[89745.036161] nvme nvme0: Connect command failed, error wo/DNR bit: 881
[89745.037437] nvme nvme0: failed to connect queue: 0 ret=881

So, TLS handshake works; I 'just' have to work on the actual data
transport. But still, a _big_ step forward.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: pre-release of v7

[Chuck Lever III]

> On Mar 9, 2023, at 1:29 PM, Hannes Reinecke <hare@suse.de> wrote:
> 
> On 3/9/23 15:41, Chuck Lever III wrote:
>> Hi-
>> I'm now trying to take a step or two towards a more typical development
>> process for the kernel patches and ktls-utils, as we replace the old
>> upcall mechanism with netlink. This is in preparation for a release of
>> ktls-utils-0.8, once netdev is close to happy with the kernel part.
>> I have pushed pre-release branches of v7 so you can comment early, test
>> it, or send me patches /before/ I post "Another crack at a handshake
>> upcall mechanism" v7 on netdev@. What a concept, eh?
> <applause>
> 
>> Kernel part: The upcall-v7 branch in
>> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> Hmm. Guess I'll branch off that, and push my patches on top of
> that. They probably have to go via a different maintainer (read: hch),
> so keeping them separately might be useful.
> 
> Or having them on your tree, too?
> Dunno.

The first two commits in topic-rpc-with-tls-upcall will go
through netdev. The last three commits I can take through
the nfsd tree. The remaining commits will go through the
nfs client tree.

IMO having the latest PSK work included will be good for
anyone wanting a central place to get all of this work, and
it will make it a little easier for me to test PSK.

I'm not planning to send it all through one tree unless the
other tree maintainers ask me to.


>> User space part: The netlink-v7 branch in
>> https://github.com/oracle/ktls-utils
> Ah. There was the 'before' part of your mail...
> I've pushed a patchset for the 'netlink' branch, containing fixes
> to get PSK running.
> Will be rebasing them to v7.

	• 8eaa23f netlink: de-constify nla_policy

I'm dropping this one. My libnl headers have a "const", so
the compiler complaint you see is clearly due to a locally
aged version of libnl. This change would break my builds.


	• 7d2a2ea ktls: add 'use_psk' argument to tlshd_make_priorites_string()

I'll rework this a bit and fold it into netlink-v7.


	• 363ad37 netlink: Reshuffle argument parsing in tlshd_genl_valid_handler()

I don't understand this. There is no longer an ACCEPT_STATUS
attribute -- the v7 kernel now returns an invalid message with
a non-zero errno.

The patch description does not help me understand why the
attribute parsing order matters.


	• 2e6b71c netlink: No key is always an error

It's not an error for anonymous handshakes. This patch
introduces a layering violation -- the tlshd_genl_done() call
shouldn't turn a "success" reply into an error reply -- it's
job is only to form the reply message, not to enforce policy.

If this condition needs to be an error, the handshake specific
code needs to decide that before genl_done is ever called.

But more preferably, the kernel consumer for PSK should work
like x.509 (as we discussed last week). The consumer needs to
get the DONE downcall and then decide that, since there's no
peer ID, this is actually not a usable TLS session. (That will
be the case for x.509 handshakes where the client did not offer
a certificate, but the server's security policy requires one).


	• f0e9956 server: set peer id for PSK
	• 6651a4a client: set peer ID for PSK

I think you misunderstand what the session_peerid is. It's the
peer ID of the /remote/ peer, not the local peer ID that the
remote chose. This attribute has been renamed in v7 to better
reflect its purpose.

If PSK consumers need to know the local peer ID chosen by the
remote, that IMO needs to be a separate netlink attribute.


--
Chuck Lever

Re: pre-release of v7

[Hannes Reinecke]

On 3/9/23 21:20, Chuck Lever III wrote:
> 
> 
>> On Mar 9, 2023, at 1:29 PM, Hannes Reinecke <hare@suse.de> wrote:
>>
>> On 3/9/23 15:41, Chuck Lever III wrote:
>>> Hi-
>>> I'm now trying to take a step or two towards a more typical development
>>> process for the kernel patches and ktls-utils, as we replace the old
>>> upcall mechanism with netlink. This is in preparation for a release of
>>> ktls-utils-0.8, once netdev is close to happy with the kernel part.
>>> I have pushed pre-release branches of v7 so you can comment early, test
>>> it, or send me patches /before/ I post "Another crack at a handshake
>>> upcall mechanism" v7 on netdev@. What a concept, eh?
>> <applause>
>>
>>> Kernel part: The upcall-v7 branch in
>>> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
>> Hmm. Guess I'll branch off that, and push my patches on top of
>> that. They probably have to go via a different maintainer (read: hch),
>> so keeping them separately might be useful.
>>
>> Or having them on your tree, too?
>> Dunno.
> 
> The first two commits in topic-rpc-with-tls-upcall will go
> through netdev. The last three commits I can take through
> the nfsd tree. The remaining commits will go through the
> nfs client tree.
> 
> IMO having the latest PSK work included will be good for
> anyone wanting a central place to get all of this work, and
> it will make it a little easier for me to test PSK.
> 
> I'm not planning to send it all through one tree unless the
> other tree maintainers ask me to.
> 
> 
>>> User space part: The netlink-v7 branch in
>>> https://github.com/oracle/ktls-utils
>> Ah. There was the 'before' part of your mail...
>> I've pushed a patchset for the 'netlink' branch, containing fixes
>> to get PSK running.
>> Will be rebasing them to v7.
> 
> 	• 8eaa23f netlink: de-constify nla_policy
> 
> I'm dropping this one. My libnl headers have a "const", so
> the compiler complaint you see is clearly due to a locally
> aged version of libnl. This change would break my builds.
> 
Yeah, possibly.
Have to figure out what I can/will do here.

> 
> 	• 7d2a2ea ktls: add 'use_psk' argument to tlshd_make_priorites_string()
> 
> I'll rework this a bit and fold it into netlink-v7.
> 
Ta.

> 
> 	• 363ad37 netlink: Reshuffle argument parsing in tlshd_genl_valid_handler()
> 
> I don't understand this. There is no longer an ACCEPT_STATUS
> attribute -- the v7 kernel now returns an invalid message with
> a non-zero errno.
> 
> The patch description does not help me understand why the
> attribute parsing order matters.
> 
Thing is, in v6 any error will _just_ have the error status, with no 
additional value (auth type? mode?), playing havoc when debugging.
But as I said, it'll probably needs to be reworked for v7.

> 
> 	• 2e6b71c netlink: No key is always an error
> 
> It's not an error for anonymous handshakes. This patch
> introduces a layering violation -- the tlshd_genl_done() call
> shouldn't turn a "success" reply into an error reply -- it's
> job is only to form the reply message, not to enforce policy.
> 
Right.

> If this condition needs to be an error, the handshake specific
> code needs to decide that before genl_done is ever called.
> 
Okay, will do so.

> But more preferably, the kernel consumer for PSK should work
> like x.509 (as we discussed last week). The consumer needs to
> get the DONE downcall and then decide that, since there's no
> peer ID, this is actually not a usable TLS session. (That will
> be the case for x.509 handshakes where the client did not offer
> a certificate, but the server's security policy requires one).
> 
Well, slightly different for PSK; the PSK identity is bound to the key 
material, so if there if the PSK identity is not found the connection
won't have key material and consequently the TLS session can't be
established.

(That's the 'S' in Pre-Shared Key ...)

> 
> 	• f0e9956 server: set peer id for PSK
> 	• 6651a4a client: set peer ID for PSK
> 
> I think you misunderstand what the session_peerid is. It's the
> peer ID of the /remote/ peer, not the local peer ID that the
> remote chose. This attribute has been renamed in v7 to better
> reflect its purpose.
> 
> If PSK consumers need to know the local peer ID chosen by the
> remote, that IMO needs to be a separate netlink attribute.
> 

That is how PSK works.
The client sends a list of possible ids, where each id is bound
to a key. The server compares those ids against the ids present
on the the server, and picks one of those. This selected id
is then sent back to the client to inform it which id from the
list it should choose for the transaction.
Then both sides have agreed on a key, and that will be used for
the TLS session.

So for PSK we don't have a 'server' or 'client' ID; there is just
a single PSK identity on both sides.

For me, the 'peerid' attribute works just fine, and I think an
additional attribute is not necessary.
But if you decide otherwise, fine. Just let me know.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman