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

[Sagi Grimberg <sagi@grimberg.me>]

>>> 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.

Yes.

> So if the host doesn't request it nothing will happen.

Correct.

> But that also means that controller authentication is optional, and so I 
> didn't feel comfortable to refuse connections for an optional feature.

If the ctrl authentication is unsuccessful then of course we need to
fail it. If the host requested the ctrl to authenticate, it needs to be
successful.

But I don't think that it explains what I'm seeing.

> 
> 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.

In my mind it is mandatory, its not really an interpretation thing...
But I think that if I pass a wrong dhchap_ctrl_key to connect I get
a failure.

> 
> 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?

So you are saying that nvme-cli does pass the ctrl key in the above
case? And even so when I read /sys/class/nvme/nvme1/dhchap_ctrl_secret
I see "none"? I'll clarify, in my test the ctrl dhchap_key is _valid_
and still it behaves like the ctrl dhchap key is ignored...