[PATCH 10/12] nvmet: Implement basic In-Band Authentication

[Sagi Grimberg]

On 9/27/21 10:17 AM, Hannes Reinecke wrote:
> On 9/27/21 8:40 AM, Hannes Reinecke wrote:
>> On 9/27/21 12:51 AM, Sagi Grimberg wrote:
>>>
>>>> +void nvmet_execute_auth_send(struct nvmet_req *req)
>>>> +{
>>>> +    struct nvmet_ctrl *ctrl = req->sq->ctrl;
>>>> +    struct nvmf_auth_dhchap_success2_data *data;
>>>> +    void *d;
>>>> +    u32 tl;
>>>> +    u16 status = 0;
>>>> +
>>>> +    if (req->cmd->auth_send.secp != 
>>>> NVME_AUTH_DHCHAP_PROTOCOL_IDENTIFIER) {
>>>> +        status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
>>>> +        req->error_loc =
>>>> +            offsetof(struct nvmf_auth_send_command, secp);
>>>> +        goto done;
>>>> +    }
>>>> +    if (req->cmd->auth_send.spsp0 != 0x01) {
>>>> +        status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
>>>> +        req->error_loc =
>>>> +            offsetof(struct nvmf_auth_send_command, spsp0);
>>>> +        goto done;
>>>> +    }
>>>> +    if (req->cmd->auth_send.spsp1 != 0x01) {
>>>> +        status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
>>>> +        req->error_loc =
>>>> +            offsetof(struct nvmf_auth_send_command, spsp1);
>>>> +        goto done;
>>>> +    }
>>>> +    tl = le32_to_cpu(req->cmd->auth_send.tl);
>>>> +    if (!tl) {
>>>> +        status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
>>>> +        req->error_loc =
>>>> +            offsetof(struct nvmf_auth_send_command, tl);
>>>> +        goto done;
>>>> +    }
>>>> +    if (!nvmet_check_transfer_len(req, tl)) {
>>>> +        pr_debug("%s: transfer length mismatch (%u)\n", __func__, tl);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    d = kmalloc(tl, GFP_KERNEL);
>>>> +    if (!d) {
>>>> +        status = NVME_SC_INTERNAL;
>>>> +        goto done;
>>>> +    }
>>>> +
>>>> +    status = nvmet_copy_from_sgl(req, 0, d, tl);
>>>> +    if (status) {
>>>> +        kfree(d);
>>>> +        goto done;
>>>> +    }
>>>> +
>>>> +    data = d;
>>>> +    pr_debug("%s: ctrl %d qid %d type %d id %d step %x\n", __func__,
>>>> +         ctrl->cntlid, req->sq->qid, data->auth_type, data->auth_id,
>>>> +         req->sq->dhchap_step);
>>>> +    if (data->auth_type != NVME_AUTH_COMMON_MESSAGES &&
>>>> +        data->auth_type != NVME_AUTH_DHCHAP_MESSAGES)
>>>> +        goto done_failure1;
>>>> +    if (data->auth_type == NVME_AUTH_COMMON_MESSAGES) {
>>>> +        if (data->auth_id == NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE) {
>>>> +            /* Restart negotiation */
>>>> +            pr_debug("%s: ctrl %d qid %d reset negotiation\n", 
>>>> __func__,
>>>> +                 ctrl->cntlid, req->sq->qid);
>>>
>>> This is the point where you need to reset also auth config as this may
>>> have changed and the host will not create a new controller but rather
>>> re-authenticate on the existing controller.
>>>
>>> i.e.
>>>
>>> +                       if (!req->sq->qid) {
>>> +                               nvmet_destroy_auth(ctrl);
>>> +                               if (nvmet_setup_auth(ctrl) < 0) {
>>> +                                       pr_err("Failed to setup 
>>> re-authentication\n");
>>> +                                       goto done_failure1;
>>> +                               }
>>> +                       }
>>>
>>>
>>>
>>
>> Not sure. We have two paths how re-authentication can be triggered.
>> The one is from the host, which sends a 'negotiate' command to the 
>> controller (ie this path).  Then nothing on the controller has 
>> changed, and we just need to ensure that we restart negotiation.
>> IE we should _not_ reset the authentication (as that would also remove 
>> the controller keys, which haven't changed). We should just ensure 
>> that all ephemeral data is regenerated. But that should be handled 
>> in-line, and I _think_ I have covered all of that.
>> The other path to trigger re-authentication is when changing values on 
>> the controller via configfs. Then sure we need to reset the controller 
>> data, and trigger reauthentication.
>> And there I do agree, that path isn't fully implemented / tested.
>> But should be started whenever the configfs values change.
>>
> Actually, having re-read the spec I'm not sure if the second path is 
> correct.
> As per spec only the _host_ can trigger re-authentication. There is no 
> provision for the controller to trigger re-authentication, and given 
> that re-auth is a soft-state anyway (ie the current authentication stays 
> valid until re-auth enters a final state) I _think_ we should be good 
> with the current implementation, where we can change the controller keys
> via configfs, but they will only become active once the host triggers
> re-authentication.

Agree, so the proposed addition is good with you?

> And indeed, that's the only way how it could work, otherwise it'll be 
> tricky to change keys in a running connection.
> If we were to force renegotiation when changing controller keys we would 
> immediately fail the connection, as we cannot guarantee that controller 
> _and_ host keys are changed at the same time.

Exactly, changing the hostkey in the controller must not trigger
re-auth, the host will remain connected and operational as it
authenticated before. As the host re-authenticates or reconnect
it needs to authenticate against the new key.