| Message ID | 20211112125928.97318-1-hare@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | nvme: In-band authentication support | expand |
On 11/12/21 2:59 PM, 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 v4: > - Validate against blktest suite Nice! thanks hannes, this is going to be very useful moving forward. > - Fixup base64 decoding What was fixed up there? > - Transform secret with correct hmac algorithm Is that what I reported last time? Can you perhaps point me to the exact patch that fixes this?
On 11/14/21 11:40 AM, Sagi Grimberg wrote: > > > On 11/12/21 2:59 PM, 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 v4: >> - Validate against blktest suite > > Nice! thanks hannes, this is going to be very useful moving > forward. > Oh, definitely. The number of issue these tests found... >> - Fixup base64 decoding > > What was fixed up there? > The padding character '=' wasn't handled correctly on decoding (the character itself was skipped, by the 'bits' value wasn't increased, leading to a spurious error in decoding an any key longer than 32 bit not being accepted. >> - Transform secret with correct hmac algorithm > > Is that what I reported last time? Can you perhaps > point me to the exact patch that fixes this? Well, no, not really; the patch itself got squashed in the main patches. But problem here was that the key transformation from section 8.13.5.7 had been using the hash algorithm from the initial challenge, not the one specified in the key itself. This lead to decoding errors when using a key with a different length than the hash algorithm. Cheers, Hannes
>>> Changes to v4: >>> - Validate against blktest suite >> >> Nice! thanks hannes, this is going to be very useful moving >> forward. >> > Oh, definitely. The number of issue these tests found... Great, good that this was useful for you. >>> - Fixup base64 decoding >> >> What was fixed up there? >> > The padding character '=' wasn't handled correctly on decoding (the > character itself was skipped, by the 'bits' value wasn't increased, > leading to a spurious error in decoding an any key longer than 32 bit > not being accepted. I see. >>> - Transform secret with correct hmac algorithm >> >> Is that what I reported last time? Can you perhaps >> point me to the exact patch that fixes this? > > Well, no, not really; the patch itself got squashed in the main patches. > But problem here was that the key transformation from section 8.13.5.7 > had been using the hash algorithm from the initial challenge, not the > one specified in the key itself. > This lead to decoding errors when using a key with a different length > than the hash algorithm. That is exactly what I reported, changing the key length leads to authentication errors.
On 11/15/21 11:20 AM, Sagi Grimberg wrote: > >>>> Changes to v4: >>>> - Validate against blktest suite >>> >>> Nice! thanks hannes, this is going to be very useful moving >>> forward. >>> >> Oh, definitely. The number of issue these tests found... > > Great, good that this was useful for you. > >>>> - Fixup base64 decoding >>> >>> What was fixed up there? >>> >> The padding character '=' wasn't handled correctly on decoding (the >> character itself was skipped, by the 'bits' value wasn't increased, >> leading to a spurious error in decoding an any key longer than 32 bit >> not being accepted. > > I see. > >>>> - Transform secret with correct hmac algorithm >>> >>> Is that what I reported last time? Can you perhaps >>> point me to the exact patch that fixes this? >> >> Well, no, not really; the patch itself got squashed in the main patches. >> But problem here was that the key transformation from section 8.13.5.7 >> had been using the hash algorithm from the initial challenge, not the >> one specified in the key itself. >> This lead to decoding errors when using a key with a different length >> than the hash algorithm. > > That is exactly what I reported, changing the key length leads to > authentication errors. Right-o. So it should be sorted then. BTW, I've created a pull request for nvme-cli (https://github.com/linux-nvme/nvme-cli/pull/1237) to add a new command-line option 'dump-config'; can you check if that's what you had in mind or whether it needs to be improved further? Cheers, Hannes
> BTW, I've created a pull request for nvme-cli > (https://github.com/linux-nvme/nvme-cli/pull/1237) > to add a new command-line option 'dump-config'; can you check if that's > what you had in mind or whether it needs to be improved further? The dump is nice, thanks. I gave a couple of comments on the PR. I do suspect that people will want to populate this file outside of dump-config, hence we need a markdown documentation for it or in this case a json schema...
>>>>> - Transform secret with correct hmac algorithm >>>> >>>> Is that what I reported last time? Can you perhaps >>>> point me to the exact patch that fixes this? >>> >>> Well, no, not really; the patch itself got squashed in the main patches. >>> But problem here was that the key transformation from section 8.13.5.7 >>> had been using the hash algorithm from the initial challenge, not the >>> one specified in the key itself. >>> This lead to decoding errors when using a key with a different length >>> than the hash algorithm. >> >> That is exactly what I reported, changing the key length leads to >> authentication errors. > > Right-o. So it should be sorted then. Hannes, was the issue on the host side or the controller side? I'm a little lost into what was the actual fix...
On 11/16/21 11:18 AM, Sagi Grimberg wrote: > >>>>>> - Transform secret with correct hmac algorithm >>>>> >>>>> Is that what I reported last time? Can you perhaps >>>>> point me to the exact patch that fixes this? >>>> >>>> Well, no, not really; the patch itself got squashed in the main >>>> patches. >>>> But problem here was that the key transformation from section 8.13.5.7 >>>> had been using the hash algorithm from the initial challenge, not the >>>> one specified in the key itself. >>>> This lead to decoding errors when using a key with a different length >>>> than the hash algorithm. >>> >>> That is exactly what I reported, changing the key length leads to >>> authentication errors. >> >> Right-o. So it should be sorted then. > > Hannes, was the issue on the host side or the controller side? > The issue was actually on the host side. > I'm a little lost into what was the actual fix... The basic fix was this: @@ -927,13 +944,17 @@ static int nvme_auth_dhchap_host_response(struct nvme_ctrl *ctrl, if (!chap->host_response) { chap->host_response = nvme_auth_transform_key(ctrl->dhchap_key, - ctrl->dhchap_key_len, chap->hash_id, + ctrl->dhchap_key_len, + ctrl->dhchap_key_hash, ctrl->opts->host->nqn); if (IS_ERR(chap->host_response)) { ret = PTR_ERR(chap->host_response); chap->host_response = NULL; return ret; } (minus the linebreaks, of course). 'chap->hash_id' is the hash selected by the initial negotiation, which is wrong as we should have used the hash function selected by the key itself. Cheers, Hannes
>> Hannes, was the issue on the host side or the controller side? >> > The issue was actually on the host side. > >> I'm a little lost into what was the actual fix... > > The basic fix was this: > > @@ -927,13 +944,17 @@ static int nvme_auth_dhchap_host_response(struct > nvme_ctrl > *ctrl, > > if (!chap->host_response) { > chap->host_response = > nvme_auth_transform_key(ctrl->dhchap_key, > - ctrl->dhchap_key_len, chap->hash_id, > + ctrl->dhchap_key_len, > + ctrl->dhchap_key_hash, > ctrl->opts->host->nqn); > if (IS_ERR(chap->host_response)) { > ret = PTR_ERR(chap->host_response); > chap->host_response = NULL; > return ret; > } > > > (minus the linebreaks, of course). > 'chap->hash_id' is the hash selected by the initial negotiation, which > is wrong as we should have used the hash function selected by the key > itself. Makes sense. thanks.