Subject: Re: [PATCHv5 00/12] nvme: In-band authentication support
To:     Sagi Grimberg <sagi@grimberg.me>
Cc:     Christoph Hellwig <hch@lst.de>, Keith Busch <keith.busch@wdc.com>,
        linux-nvme@lists.infradead.org,
        Herbert Xu <herbert@gondor.apana.org.au>,
        David Miller <davem@davemloft.net>,
        linux-crypto@vger.kernel.org
References: <20211112125928.97318-1-hare@suse.de>
 <74db7c77-7cbf-4bc9-1c80-e7c42acaea64@grimberg.me>
From:   Hannes Reinecke <hare@suse.de>
Message-ID: <f67ca46e-f421-33f7-da8b-ff6e47acf8c2@suse.de>
Date:   Sun, 14 Nov 2021 14:44:12 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <74db7c77-7cbf-4bc9-1c80-e7c42acaea64@grimberg.me>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 11/14/21 11:40 AM, Sagi Grimberg wrote:
> 
> 
> On 11/12/21 2:59 PM, Hannes Reinecke wrote:
>> Hi all,
>>
>> recent updates to the NVMe spec have added definitions for in-band
>> authentication, and seeing that it provides some real benefit
>> especially for NVMe-TCP here's an attempt to implement it.
>>
>> Tricky bit here is that the specification orients itself on TLS 1.3,
>> but supports only the FFDHE groups. Which of course the kernel doesn't
>> support. I've been able to come up with a patch for this, but as this
>> is my first attempt to fix anything in the crypto area I would invite
>> people more familiar with these matters to have a look.
>>
>> Also note that this is just for in-band authentication. Secure
>> concatenation (ie starting TLS with the negotiated parameters) is not
>> implemented; one would need to update the kernel TLS implementation
>> for this, which at this time is beyond scope.
>>
>> As usual, comments and reviews are welcome.
>>
>> Changes to v4:
>> - Validate against blktest suite
> 
> Nice! thanks hannes, this is going to be very useful moving
> forward.
> 
Oh, definitely. The number of issue these tests found...

>> - Fixup base64 decoding
> 
> What was fixed up there?
> 
The padding character '=' wasn't handled correctly on decoding (the 
character itself was skipped, by the 'bits' value wasn't increased, 
leading to a spurious error in decoding an any key longer than 32 bit 
not being accepted.

>> - Transform secret with correct hmac algorithm
> 
> Is that what I reported last time? Can you perhaps
> point me to the exact patch that fixes this?

Well, no, not really; the patch itself got squashed in the main patches.
But problem here was that the key transformation from section 8.13.5.7 
had been using the hash algorithm from the initial challenge, not the 
one specified in the key itself.
This lead to decoding errors when using a key with a different length 
than the hash algorithm.

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: Felix Imendörffer