| Message ID | 20211122074727.25988-1-hare@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | nvme: In-band authentication support | expand |
On 11/22/21 9:47 AM, Hannes Reinecke 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. > > Changes to v5: > - Unify nvme_auth_generate_key() > - Unify nvme_auth_extract_key() You mean nvme_auth_extract_secret() ? > - Include reviews from Sagi What about the bug fix folded in?
On 11/22/21 9:13 AM, Sagi Grimberg wrote: > > > On 11/22/21 9:47 AM, Hannes Reinecke 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. >> >> Changes to v5: >> - Unify nvme_auth_generate_key() >> - Unify nvme_auth_extract_key() > > You mean nvme_auth_extract_secret() ? > Yes. >> - Include reviews from Sagi > > What about the bug fix folded in? Yeah, and that, to Forgot to mention it. Also note that I've already folded the nvme-cli patches into the git repository to ease testing; I gather that the interface won't change that much anymore, so I felt justified in doing so. And I got tired of explaining to interested parties how to build a non-standard nvme-cli :-) But that's why I didn't post separate patches for nvme-cli. Cheers, Hannes
>>> 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. >>> >>> Changes to v5: >>> - Unify nvme_auth_generate_key() >>> - Unify nvme_auth_extract_key() >> >> You mean nvme_auth_extract_secret() ? >> > Yes. > >>> - Include reviews from Sagi >> >> What about the bug fix folded in? > > Yeah, and that, to > Forgot to mention it. It is not the code that you shared in the other thread right? > > Also note that I've already folded the nvme-cli patches into the git > repository to ease testing; I gather that the interface won't change > that much anymore, so I felt justified in doing so. It's ok, we can still change if we want to.
On 11/22/21 12:32 PM, 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. >>>> >>>> Changes to v5: >>>> - Unify nvme_auth_generate_key() >>>> - Unify nvme_auth_extract_key() >>> >>> You mean nvme_auth_extract_secret() ? >>> >> Yes. >> >>>> - Include reviews from Sagi >>> >>> What about the bug fix folded in? >> >> Yeah, and that, to >> Forgot to mention it. > > It is not the code that you shared in the other thread right? > Yes, it is. It has been folded into v6. And test 043 has been updated to check for this issue. Cheers, Hannes
>>>> What about the bug fix folded in? >>> >>> Yeah, and that, to >>> Forgot to mention it. >> >> It is not the code that you shared in the other thread right? >> > Yes, it is. > It has been folded into v6. I don't see it in patch 07/12
On 11/22/21 12:45 PM, Sagi Grimberg wrote: > >>>>> What about the bug fix folded in? >>>> >>>> Yeah, and that, to >>>> Forgot to mention it. >>> >>> It is not the code that you shared in the other thread right? >>> >> Yes, it is. >> It has been folded into v6. > > I don't see it in patch 07/12 + ret = nvme_auth_process_dhchap_success1(ctrl, chap); + if (ret) { + /* Controller authentication failed */ + goto fail2; + } + v5 had 'if (ret < 0) [' here. Cheers, Hannes
> + ret = nvme_auth_process_dhchap_success1(ctrl, chap); > + if (ret) { > + /* Controller authentication failed */ > + goto fail2; > + } > + > > v5 had 'if (ret < 0) [' here. Right, thanks.