| Message ID | 20210910064322.67705-1-hare@suse.de |
|---|---|
| Headers | show |
| Series | nvme: In-band authentication support | expand |
> 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. > > Tricky bit here is that the specification orients itself on TLS 1.3, > but supports only the FFDHE groups. Which of course the kernel doesn't > support. I've been able to come up with a patch for this, but as this > is my first attempt to fix anything in the crypto area I would invite > people more familiar with these matters to have a look. > > Also note that this is just for in-band authentication. Secure > concatenation (ie starting TLS with the negotiated parameters) is not > implemented; one would need to update the kernel TLS implementation > for this, which at this time is beyond scope. > > As usual, comments and reviews are welcome. Still no nvme-cli nor nvmetcli :(
On 9/13/21 11:16 AM, 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. >> >> Tricky bit here is that the specification orients itself on TLS 1.3, >> but supports only the FFDHE groups. Which of course the kernel doesn't >> support. I've been able to come up with a patch for this, but as this >> is my first attempt to fix anything in the crypto area I would invite >> people more familiar with these matters to have a look. >> >> Also note that this is just for in-band authentication. Secure >> concatenation (ie starting TLS with the negotiated parameters) is not >> implemented; one would need to update the kernel TLS implementation >> for this, which at this time is beyond scope. >> >> As usual, comments and reviews are welcome. > > Still no nvme-cli nor nvmetcli :( Just send it (for libnvme and nvme-cli). Patch for nvmetcli to follow. 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
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
On 9/9/21 11:43 PM, Hannes Reinecke wrote: > Add helper function to determine if a given key-agreement protocol primitive is supported. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > Looks good. Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
On 9/9/21 11:43 PM, Hannes Reinecke wrote: > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > include/linux/nvme.h | 186 ++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 185 insertions(+), 1 deletion(-) > Probably worth mentioning a TP name here so we can refer later, instead of empty commit message ?
On 9/9/21 11:43 PM, Hannes Reinecke wrote: > The 'connect' command might fail with NVME_SC_AUTH_REQUIRED, so we > should be decoding this error, too. > > Signed-off-by: Hannes Reinecke <hare@suse.de> Looks good. Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
On 9/16/21 7:04 PM, Chaitanya Kulkarni wrote: > On 9/9/21 11:43 PM, Hannes Reinecke wrote: >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> include/linux/nvme.h | 186 ++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 185 insertions(+), 1 deletion(-) >> > > Probably worth mentioning a TP name here so we can refer later, > instead of empty commit message ? > Had been thinking about it, but then decided against it. Once the TPAR is folded into the main spec it's getting really hard to figure out exactly what individual TPARs were referring to, so I prefer to stick with 'In-Band authentication' instead of the TPAR number. But I can add that to the commit message, sure. Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
>>> 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. >>> >>> Tricky bit here is that the specification orients itself on TLS 1.3, >>> but supports only the FFDHE groups. Which of course the kernel doesn't >>> support. I've been able to come up with a patch for this, but as this >>> is my first attempt to fix anything in the crypto area I would invite >>> people more familiar with these matters to have a look. >>> >>> Also note that this is just for in-band authentication. Secure >>> concatenation (ie starting TLS with the negotiated parameters) is not >>> implemented; one would need to update the kernel TLS implementation >>> for this, which at this time is beyond scope. >>> >>> As usual, comments and reviews are welcome. >> >> Still no nvme-cli nor nvmetcli :( > > Just send it (for libnvme and nvme-cli). Patch for nvmetcli to follow. Hey Hannes, I think that this series is getting into close-to-inclustion shape. Please in your next respin: 1. Make sure to send nvme-cli and nvmetcli with the series 2. Collect Review tags Thanks!
> +int nvmet_setup_auth(struct nvmet_ctrl *ctrl) > +{ > + int ret = 0; > + struct nvmet_host_link *p; > + struct nvmet_host *host = NULL; > + const char *hash_name; > + > + down_read(&nvmet_config_sem); > + if (ctrl->subsys->type == NVME_NQN_DISC) > + goto out_unlock; + if (ctrl->subsys->allow_any_host) + goto out_unlock;
> +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; + } + } > + req->sq->dhchap_step = NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE; > + } else if (data->auth_id != req->sq->dhchap_step) > + goto done_failure1; > + /* Validate negotiation parameters */ > + status = nvmet_auth_negotiate(req, d);/
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. Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
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. 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. Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
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.
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@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
>>> 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? Just need to update the new host dhchap_key from the host at this point as the host is doing a re-authentication. I agree we don't need a big hammer but we do need the re-authentication to not access old host dhchap_key.
On 9/29/21 12:36 AM, Sagi Grimberg wrote: > >>>> 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? > > Just need to update the new host dhchap_key from the host at this point > as the host is doing a re-authentication. I agree we don't need a big > hammer but we do need the re-authentication to not access old host > dhchap_key. Sure. And, upon reviewing, I guess you are right; will be including your snippet. For the next round :-) Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer