On 9/27/21 9:55 AM, Sagi Grimberg wrote:
>
>
> 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?
>
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?
>> 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@xxxxxxx +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
