On 9/27/21 9:55 AM, Sagi Grimberg wrote:

On 9/27/21 10:17 AM, Hannes Reinecke wrote:

quoted

On 9/27/21 8:40 AM, Hannes Reinecke wrote:

quoted

On 9/27/21 12:51 AM, Sagi Grimberg wrote:

quoted

quoted

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

Why would we need it?
I do agree there's a bit missing for removing the old shash_tfm if there
is a hash-id mismatch, but why would we need to reset the entire
authentication?
The important (ie cryptographically relevant) bits are cleared in
nvmet_auth_sq_free(), and they are cleared after authentication is
completed.
So why would we need to reset keys and TFMs?

quoted

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.

Right. I'll be adding a comment to the configfs functions to the effect.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer