[PATCHv16 00/11] nvme: In-band authentication support

[Hannes Reinecke]

On 6/21/22 23:22, Sagi Grimberg 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.
>>
>> Thanks to Nicolai Stange the crypto DH framework has been upgraded
>> to provide us with a FFDHE implementation; I've updated the patchset
>> to use the ephemeral key generation provided there.
>>
>> Note that this is just for in-band authentication. Secure
>> concatenation (ie starting TLS with the negotiated parameters)
>> requires a TLS handshake, which the in-kernel TLS implementation
>> does not provide. This is being worked on with a different patchset
>> which is still WIP.
>>
>> The nvme-cli support has already been merged; please use the latest
>> nvme-cli git repository to build the most recent version.
>>
>> A copy of this patchset can be found at
>> git://git.kernel.org/pub/scm/linux/kernel/git/hare/scsi-devel
>> branch auth.v15
>>
>> The patchset is being cut against nvme-5.20.
>>
>> As usual, comments and reviews are welcome.
> 
> Hannes, did you see my panic report on a malformed dhchap_ctrl_key?
> 
> Also, why does the dhchap_ctrl_key not passed when connecting
> via discovery?
> 
> I have in the target:
> -- 
> # grep -r '' /sys/kernel/config/nvmet/hosts/
> /sys/kernel/config/nvmet/hosts/nqn.2014-08.org.nvmexpress:uuid:302ae323-4acd-465d-ace4-3d4102e9d11f/dhchap_dhgroup:null 
> 
> /sys/kernel/config/nvmet/hosts/nqn.2014-08.org.nvmexpress:uuid:302ae323-4acd-465d-ace4-3d4102e9d11f/dhchap_hash:hmac(sha256) 
> 
> /sys/kernel/config/nvmet/hosts/nqn.2014-08.org.nvmexpress:uuid:302ae323-4acd-465d-ace4-3d4102e9d11f/dhchap_ctrl_key:DHHC-1:00:Jc/My1o0qtLCWRp+sHhAVN6mFaS7YQOMYhk9zSmlatobqB8C: 
> 
> /sys/kernel/config/nvmet/hosts/nqn.2014-08.org.nvmexpress:uuid:302ae323-4acd-465d-ace4-3d4102e9d11f/dhchap_key:DHHC-1:00:QpxVGpctx5J+4SeW2MClUI8rfZO3WdP1llImvsPsx7e3TK+I: 
> 
> -- 
> 
> Then on the host I have:
> -- 
> # cat /etc/nvme/config.json
> [
>    {
>      "hostnqn": 
> "nqn.2014-08.org.nvmexpress:uuid:302ae323-4acd-465d-ace4-3d4102e9d11f",
>      "hostid": "14f15c4e-f6cb-434b-90cd-7c1f84f0c194",
>      "dhchap_key": 
> "DHHC-1:00:QpxVGpctx5J+4SeW2MClUI8rfZO3WdP1llImvsPsx7e3TK+I:",
>      "subsystems": [
>        {
>          "nqn": "testnqn1",
>          "ports": [
>            {
>              "transport": "tcp",
>              "traddr": "192.168.123.1",
>              "trsvcid": "8009",
>              "dhchap_key": 
> "DHHC-1:00:Jc/My1o0qtLCWRp+sHhAVN6mFaS7YQOMYhk9zSmlatobqB8C:"
>            }
>          ]
>        }
>      ]
>    }
> ]
> -- 
> 
> And when I do connect-all (i.e. connect via the discovery log page:
> -- 
> # grep -r '' /sys/class/nvme/nvme1/dhchap*
> /sys/class/nvme/nvme1/dhchap_ctrl_secret:none
> /sys/class/nvme/nvme1/dhchap_secret:DHHC-1:00:QpxVGpctx5J+4SeW2MClUI8rfZO3WdP1llImvsPsx7e3TK+I: 
> 
> -- 
> 
> This means that I can corrupt the dhchap_ctrl_key entry in the config
> and no one will care (because it is not authenticating the ctrl if
> dhchap_ctrl_key is not passed)
> 
Unfortunate design on both ends.
It's the host who requests controller authentication.
So if the host doesn't request it nothing will happen.
But that also means that controller authentication is optional, and so I 
didn't feel comfortable to refuse connections for an optional feature.

But we can make that a real error, and refuse the 'connect' call if the 
controller key is invalid. Might be a better choice after all.

Something like this would help:

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 53184ac76240..a03f41fa146e 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -842,6 +842,10 @@ int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int 
qid)
                 dev_warn(ctrl->device, "qid %d: no key\n", qid);
                 return -ENOKEY;
         }
+       if (ctrl->opts->dhchap_ctrl_secret && !ctrl->ctrl_key) {
+               dev_warn(ctrl->device, "qid %d: invalid ctrl key\n", qid);
+               return -ENOKEY;
+       }

         mutex_lock(&ctrl->dhchap_auth_mutex);
         /* Check if the context is already queued */


Is this what you had in mind?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare at 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