Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 06/11] nvme: Implement In-Band authentication
To:     Hannes Reinecke <hare@suse.de>, Sagi Grimberg <sagi@grimberg.me>,
        Christoph Hellwig <hch@lst.de>
Cc:     Keith Busch <keith.busch@wdc.com>, linux-nvme@lists.infradead.org,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S . Miller" <davem@davemloft.net>,
        linux-crypto@vger.kernel.org
References: <20210716110428.9727-1-hare@suse.de>
 <20210716110428.9727-7-hare@suse.de>
 <bd588839-8acc-91cf-5946-f702007b0c7d@grimberg.me>
 <d74c29b8-1c64-e439-9015-6c424baad3d3@suse.de>
From:   Vladislav Bolkhovitin <vst@vlnb.net>
Message-ID: <a3098fb2-2127-6f81-97e9-ab5de503508e@vlnb.net>
Date:   Tue, 20 Jul 2021 23:28:21 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <d74c29b8-1c64-e439-9015-6c424baad3d3@suse.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 7/18/21 3:21 PM, Hannes Reinecke wrote:
> On 7/17/21 9:22 AM, Sagi Grimberg wrote:
>>> Implement NVMe-oF In-Band authentication. This patch adds two new
>>> fabric options 'dhchap_key' to specify the PSK
>>
>> pre-shared-key.
>>
>> Also, we need a sysfs knob to rotate the key that will trigger
>> re-authentication or even a simple controller(s-plural) reset, so this
>> should go beyond just the connection string.
>>
> 
> Yeah, re-authentication currently is not implemented. I first wanted to
> get this patchset out such that we can settle on the userspace interface
> (both from host and target).
> I'll have to think on how we should handle authentication; one of the
> really interesting cases would be when one malicious admin will _just_
> send a 'negotiate' command to the controller. As per spec the controller
> will be waiting for an 'authentication receive' command to send a
> 'challenge' payload back to the host. But that will never come, so as it
> stands currently the controller is required to abort the connection.
> Not very nice.

Yes, in this case after some reasonable timeout (I would suggest 10-15
seconds) the controller expected to abort connection and clean up all
allocated resources.

To handle DoS possibility to make too many such "orphan" negotiations,
hence consume all controller memory, some additional handling is needed.
For simplicity as a first step I would suggest to have a global limit on
number of currently being authenticated connections.

[...]

>>> +    chap->key = nvme_auth_extract_secret(ctrl->opts->dhchap_secret,
>>> +                         &chap->key_len);
>>> +    if (IS_ERR(chap->key)) {
>>> +        ret = PTR_ERR(chap->key);
>>> +        chap->key = NULL;
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (key_hash == 0)
>>> +        return 0;
>>> +
>>> +    hmac_name = nvme_auth_hmac_name(key_hash);
>>> +    if (!hmac_name) {
>>> +        pr_debug("Invalid key hash id %d\n", key_hash);
>>> +        return -EKEYREJECTED;
>>> +    }
>>
>> Why does the user influence the hmac used? isn't that is driven
>> by the susbsystem?
>>
>> I don't think that the user should choose in this level.
>>
> 
> That is another weirdness of the spec.
> The _secret_ will be hashed with a specific function, and that function
> is stated in the transport representation.
> (Cf section "DH-HMAC-CHAP Security Requirements").
> This is _not_ the hash function used by the authentication itself, which
> will be selected by the protocol.
> So it's not the user here, but rather the transport specification of the
> key which selects the hash algorithm.

Yes, good catch. It looks as a minor errata material to specify that
hash function here is implementation specific.

I would suggest to just hardcode SHA512 here. Users don't have to be
confused by this.

Vlad