On 7/20/21 10:26 PM, Vladislav Bolkhovitin wrote:

Hi,

Great to see those patches coming! After some review, they look to be
very well done. Some comments/suggestions below.

1. I strongly recommend to implement DH exponentials reuse (g x mod p /
g y mod p as well as g xy mod p) as specified in section 8.13.5.7
"DH-HMAC-CHAP Security Requirements". When I was working on TP 8006 I
had a prototype that demonstrated that DH math has quite significant
latency, something like (as far as I remember) 30ms for 4K group and few
hundreds of ms for 8K group. For single connection it is not a big deal,
but imagine AMD EPYC with 128 cores. Since all connections are created
sequentially, even with 30 ms per connection time to complete full
remote device connection would become 128*30 => almost 4 seconds. With
8K group it might be more than 10 seconds. Users are unlikely going to
be happy with this, especially in cases, when connecting multiple of
NVMe-oF devices is a part of a server or VM boot sequence.

Oh, indeed, I can confirm that. FFDHE calculations are quite time-consuming.
But incidentally, ECDH and curve25519 are reasonably fast, so maybe
there _is_ a value in having a TPAR asking for them to be specified, too ...

If DH exponential reuse implemented, for all subsequent connections the
DH math is excluded, so authentication overhead becomes pretty much
negligible.

In my prototype I implemented DH exponential reuse as a simple
per-host/target cache that keeps DH exponentials (including g xy mod p)
for up to 10 seconds. Simple and sufficient.

Frankly, I hadn't looked at exponential reuse; this implementation
really is just a first step to get feedback from people if this is a
direction they want to go.

Another, might be ever more significant reason why DH exponential reuse
is important is that without it x (or y on the host side) must always be
randomly generated each time a new connection is established. Which
means, for instance, for 8K groups for each connection 1KB of random
bytes must be taken from the random pool. With 128 connections it is now
128KB. Quite a big pressure on the random pool that DH exponential reuse
mostly avoids.

Those are the 2 reasons why we added this DH exponential reuse sentence
in the spec. In the original TP 8006 there was a small informative piece
explaining reasonings behind that, but for some reasons it was removed
from the final version.

Thanks for the hint. I'll be adding exponential reuse to the code.

2. What is the status of this code from perspective of stability in face
of malicious host behavior? Seems implementation is carefully done, but,
for instance, at the first look I was not able to find a code to clean
up if host in not acting for too long in the middle of exchange. Other
observation is that in nvmet_execute_auth_send()
nvmet_check_transfer_len() does not check if tl size is reasonable,
i.e., for instance, not 1GB.

That is true; exchange timeouts are missing. Will be adding them, of
course. And haven't thought of checking for tl size overflows; will be
adding them, too.

For sure, we don't want to allow remote hosts to hang or crash target.
For instance, because of OOM conditions that happened, because malicious
host asked target to allocate too much memory or open to many being
authenticated connections in which the host is not going to reply in the
middle of exchange.

This is something I'll need to look at, anyway. What we do not want is a
userspace application chipping in and send a 'negotiate' command without
any subsequent steps, thereby invalidating the existing authentication.

Asking, because don't want to go in my review too far ahead from the
author ;)

In this regard, it would be great if you add in your test application
ability to perform authentication with random parameters and randomly
stop responding. Overnight running of such test would give us good
degree of confidence that it will always work as expected.

That indeed would be good; let me think on how something like that can
be implemented.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer