mbox series

[PATCHv8,00/12] nvme: In-band authentication support

Message ID 20211202152358.60116-1-hare@suse.de
Headers show
Series nvme: In-band authentication support | expand

Message

Hannes Reinecke Dec. 2, 2021, 3:23 p.m. UTC
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.

The ffdhe implementation given here is preliminary; there is a
patchset from Nicolai Stange to implement FFDHE as a 'real'
crypto algorithm, and also implements ephemeral keys for in-kernel DH.
Once that is merged I'll be updating this patchset.

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.

The nvme-cli support has already been merged; please use the latest
nvme-cli git repository to build the most recent version.

A copy of this patchset can be found at
git://git.kernel.org/pub/scm/linux/kernel/git/hare/scsi-devel
branch auth.v8

As usual, comments and reviews are welcome.

Changes to v7:
- Space out hash list and dhgroup list in nvme negotiate data
  to be conformant with the spec
- Update sequence number handling to start with a random value and
  ignore '0' as mandated by the spec
- Update nvme_auth_generate_key to return the key as suggested by Sagi
- Add nvmet_parse_fabrics_io_cmd() as suggested by hch

Changes to v6:
- Use 'u8' for DH group id and hash id
- Use 'struct nvme_dhchap_key'
- Rename variables to drop 'DHCHAP'
- Include reviews from Chaitanya

Changes to v5:
- Unify nvme_auth_generate_key()
- Unify nvme_auth_extract_key()
- Fixed bug where re-authentication with wrong controller key would not fail
- Include reviews from Sagi

Changes to v4:
- Validate against blktest suite
- Fixup base64 decoding
- Transform secret with correct hmac algorithm

Changes to v3:
- Renamed parameter to 'dhchap_ctrl_key'
- Fixed bi-directional authentication
- Included reviews from Sagi
- Fixed base64 algorithm for transport encoding

Changes to v2:
- Dropped non-standard algorithms
- Reworked base64 based on fs/crypto/fname.c
- Fixup crash with no keys

Changes to the original submission:
- Included reviews from Vladislav
- Included reviews from Sagi
- Implemented re-authentication support
- Fixed up key handling

Hannes Reinecke (12):
  crypto: add crypto_has_shash()
  crypto: add crypto_has_kpp()
  crypto/ffdhe: Finite Field DH Ephemeral Parameters
  lib/base64: RFC4648-compliant base64 encoding
  nvme: add definitions for NVMe In-Band authentication
  nvme-fabrics: decode 'authentication required' connect error
  nvme: Implement In-Band authentication
  nvme-auth: Diffie-Hellman key exchange support
  nvmet: parse fabrics commands on io queues
  nvmet: Implement basic In-Band Authentication
  nvmet-auth: Diffie-Hellman key exchange support
  nvmet-auth: expire authentication sessions

 crypto/Kconfig                         |    8 +
 crypto/Makefile                        |    1 +
 crypto/ffdhe_helper.c                  |  880 +++++++++++++
 crypto/kpp.c                           |    6 +
 crypto/shash.c                         |    6 +
 drivers/nvme/host/Kconfig              |   12 +
 drivers/nvme/host/Makefile             |    1 +
 drivers/nvme/host/auth.c               | 1569 ++++++++++++++++++++++++
 drivers/nvme/host/auth.h               |   42 +
 drivers/nvme/host/core.c               |  141 ++-
 drivers/nvme/host/fabrics.c            |   83 +-
 drivers/nvme/host/fabrics.h            |    7 +
 drivers/nvme/host/nvme.h               |   31 +
 drivers/nvme/host/rdma.c               |    1 +
 drivers/nvme/host/tcp.c                |    1 +
 drivers/nvme/host/trace.c              |   32 +
 drivers/nvme/target/Kconfig            |   13 +
 drivers/nvme/target/Makefile           |    1 +
 drivers/nvme/target/admin-cmd.c        |    6 +-
 drivers/nvme/target/auth.c             |  523 ++++++++
 drivers/nvme/target/configfs.c         |  138 ++-
 drivers/nvme/target/core.c             |   15 +
 drivers/nvme/target/fabrics-cmd-auth.c |  534 ++++++++
 drivers/nvme/target/fabrics-cmd.c      |   55 +-
 drivers/nvme/target/nvmet.h            |   75 +-
 include/crypto/ffdhe.h                 |   24 +
 include/crypto/hash.h                  |    2 +
 include/crypto/kpp.h                   |    2 +
 include/linux/base64.h                 |   16 +
 include/linux/nvme.h                   |  188 ++-
 lib/Makefile                           |    2 +-
 lib/base64.c                           |  103 ++
 32 files changed, 4503 insertions(+), 15 deletions(-)
 create mode 100644 crypto/ffdhe_helper.c
 create mode 100644 drivers/nvme/host/auth.c
 create mode 100644 drivers/nvme/host/auth.h
 create mode 100644 drivers/nvme/target/auth.c
 create mode 100644 drivers/nvme/target/fabrics-cmd-auth.c
 create mode 100644 include/crypto/ffdhe.h
 create mode 100644 include/linux/base64.h
 create mode 100644 lib/base64.c

Comments

Christoph Hellwig Dec. 13, 2021, 8:08 a.m. UTC | #1
So if we want to make progress on this we need the first 3 patches
rewviewed by the crypto maintainers.  In fact I'd prefer to get them
merged through the crypto tree as well, and would make sure we have
a branch that pulls them in for the nvme changes.  I'll try to find
some time to review the nvme bits as well.
Hannes Reinecke Dec. 13, 2021, 8:46 a.m. UTC | #2
On 12/13/21 9:08 AM, Christoph Hellwig wrote:
> So if we want to make progress on this we need the first 3 patches
> rewviewed by the crypto maintainers.  In fact I'd prefer to get them
> merged through the crypto tree as well, and would make sure we have
> a branch that pulls them in for the nvme changes.  I'll try to find
> some time to review the nvme bits as well.
> 
That is _actually_ being addressed already.
Nicolai Stange send a patchset for ephemeral keys, FFDHE support, and 
FIPS-related thingies for the in-kernel DH crypto implementation 
(https://lore.kernel.org/linux-crypto/20211209090358.28231-1-nstange@suse.de/).
This obsoletes my preliminary patches, and I have ported my patchset to 
run on top of those.

Question is how to continue from here; I can easily rebase my patchset 
and send it relative to Nicolais patches. But then we'll be bound to the 
acceptance of those patches, so I'm not quite sure if that's the best 
way to proceed.

Cheers,

Hannes
Sagi Grimberg Dec. 13, 2021, 9:31 a.m. UTC | #3
>> So if we want to make progress on this we need the first 3 patches
>> rewviewed by the crypto maintainers.  In fact I'd prefer to get them
>> merged through the crypto tree as well, and would make sure we have
>> a branch that pulls them in for the nvme changes.  I'll try to find
>> some time to review the nvme bits as well.
>>
> That is _actually_ being addressed already.
> Nicolai Stange send a patchset for ephemeral keys, FFDHE support, and 
> FIPS-related thingies for the in-kernel DH crypto implementation 
> (https://lore.kernel.org/linux-crypto/20211209090358.28231-1-nstange@suse.de/). 
> 
> This obsoletes my preliminary patches, and I have ported my patchset to 
> run on top of those.
> 
> Question is how to continue from here; I can easily rebase my patchset 
> and send it relative to Nicolais patches. But then we'll be bound to the 
> acceptance of those patches, so I'm not quite sure if that's the best 
> way to proceed.

Don't know if we have a choice here... What is the alternative you are
proposing?
Hannes Reinecke Dec. 13, 2021, 10 a.m. UTC | #4
On 12/13/21 10:31 AM, Sagi Grimberg wrote:
> 
>>> So if we want to make progress on this we need the first 3 patches
>>> rewviewed by the crypto maintainers.  In fact I'd prefer to get them
>>> merged through the crypto tree as well, and would make sure we have
>>> a branch that pulls them in for the nvme changes.  I'll try to find
>>> some time to review the nvme bits as well.
>>>
>> That is _actually_ being addressed already.
>> Nicolai Stange send a patchset for ephemeral keys, FFDHE support, and
>> FIPS-related thingies for the in-kernel DH crypto implementation
>> (https://lore.kernel.org/linux-crypto/20211209090358.28231-1-nstange@suse.de/).
>>
>> This obsoletes my preliminary patches, and I have ported my patchset
>> to run on top of those.
>>
>> Question is how to continue from here; I can easily rebase my patchset
>> and send it relative to Nicolais patches. But then we'll be bound to
>> the acceptance of those patches, so I'm not quite sure if that's the
>> best way to proceed.
> 
> Don't know if we have a choice here... What is the alternative you are
> proposing?

That's the thing, I don't really have a good alternative, either.
It's just that I have so no idea about the crypto subsystem, and
consequently wouldn't know how long we need to wait...

But yeah, Nicolais patchset is far superior to my attempts, so I'd be
happy to ditch my preliminary attempts there.

Cheers,

Hannes
Sagi Grimberg Dec. 13, 2021, 1:53 p.m. UTC | #5
>>>> So if we want to make progress on this we need the first 3 patches
>>>> rewviewed by the crypto maintainers.  In fact I'd prefer to get them
>>>> merged through the crypto tree as well, and would make sure we have
>>>> a branch that pulls them in for the nvme changes.  I'll try to find
>>>> some time to review the nvme bits as well.
>>>>
>>> That is _actually_ being addressed already.
>>> Nicolai Stange send a patchset for ephemeral keys, FFDHE support, and
>>> FIPS-related thingies for the in-kernel DH crypto implementation
>>> (https://lore.kernel.org/linux-crypto/20211209090358.28231-1-nstange@suse.de/).
>>>
>>> This obsoletes my preliminary patches, and I have ported my patchset
>>> to run on top of those.
>>>
>>> Question is how to continue from here; I can easily rebase my patchset
>>> and send it relative to Nicolais patches. But then we'll be bound to
>>> the acceptance of those patches, so I'm not quite sure if that's the
>>> best way to proceed.
>>
>> Don't know if we have a choice here... What is the alternative you are
>> proposing?
> 
> That's the thing, I don't really have a good alternative, either.
> It's just that I have so no idea about the crypto subsystem, and
> consequently wouldn't know how long we need to wait...
> 
> But yeah, Nicolais patchset is far superior to my attempts, so I'd be
> happy to ditch my preliminary attempts there.

Can we get a sense from the crypto folks to the state of Nicolais
patchset?
Hannes Reinecke Dec. 13, 2021, 2:15 p.m. UTC | #6
On 12/13/21 2:53 PM, Sagi Grimberg wrote:
> 
>>>>> So if we want to make progress on this we need the first 3 patches
>>>>> rewviewed by the crypto maintainers.  In fact I'd prefer to get them
>>>>> merged through the crypto tree as well, and would make sure we have
>>>>> a branch that pulls them in for the nvme changes.  I'll try to find
>>>>> some time to review the nvme bits as well.
>>>>>
>>>> That is _actually_ being addressed already.
>>>> Nicolai Stange send a patchset for ephemeral keys, FFDHE support, and
>>>> FIPS-related thingies for the in-kernel DH crypto implementation
>>>> (https://lore.kernel.org/linux-crypto/20211209090358.28231-1-nstange@suse.de/).
>>>>
>>>>
>>>> This obsoletes my preliminary patches, and I have ported my patchset
>>>> to run on top of those.
>>>>
>>>> Question is how to continue from here; I can easily rebase my patchset
>>>> and send it relative to Nicolais patches. But then we'll be bound to
>>>> the acceptance of those patches, so I'm not quite sure if that's the
>>>> best way to proceed.
>>>
>>> Don't know if we have a choice here... What is the alternative you are
>>> proposing?
>>
>> That's the thing, I don't really have a good alternative, either.
>> It's just that I have so no idea about the crypto subsystem, and
>> consequently wouldn't know how long we need to wait...
>>
>> But yeah, Nicolais patchset is far superior to my attempts, so I'd be
>> happy to ditch my preliminary attempts there.
> 
> Can we get a sense from the crypto folks to the state of Nicolais
> patchset?

According to Nicolai things look good, rules seem to be that it'll be
accepted if it has positive reviews (which it has) and no-one objected
(which no-one did).
Other than that one would have to ask the maintainer.
Herbert?

Cheers,

Hannes
Sagi Grimberg Dec. 21, 2021, 8:55 p.m. UTC | #7
>>>>> Question is how to continue from here; I can easily rebase my patchset
>>>>> and send it relative to Nicolais patches. But then we'll be bound to
>>>>> the acceptance of those patches, so I'm not quite sure if that's the
>>>>> best way to proceed.
>>>>
>>>> Don't know if we have a choice here... What is the alternative you are
>>>> proposing?
>>>
>>> That's the thing, I don't really have a good alternative, either.
>>> It's just that I have so no idea about the crypto subsystem, and
>>> consequently wouldn't know how long we need to wait...
>>>
>>> But yeah, Nicolais patchset is far superior to my attempts, so I'd be
>>> happy to ditch my preliminary attempts there.
>>
>> Can we get a sense from the crypto folks to the state of Nicolais
>> patchset?
> 
> According to Nicolai things look good, rules seem to be that it'll be
> accepted if it has positive reviews (which it has) and no-one objected
> (which no-one did).
> Other than that one would have to ask the maintainer.
> Herbert?

Any updates on this?
Hannes Reinecke Dec. 22, 2021, 6:53 a.m. UTC | #8
On 12/21/21 9:55 PM, Sagi Grimberg wrote:
> 
>>>>>> Question is how to continue from here; I can easily rebase my 
>>>>>> patchset
>>>>>> and send it relative to Nicolais patches. But then we'll be bound to
>>>>>> the acceptance of those patches, so I'm not quite sure if that's the
>>>>>> best way to proceed.
>>>>>
>>>>> Don't know if we have a choice here... What is the alternative you are
>>>>> proposing?
>>>>
>>>> That's the thing, I don't really have a good alternative, either.
>>>> It's just that I have so no idea about the crypto subsystem, and
>>>> consequently wouldn't know how long we need to wait...
>>>>
>>>> But yeah, Nicolais patchset is far superior to my attempts, so I'd be
>>>> happy to ditch my preliminary attempts there.
>>>
>>> Can we get a sense from the crypto folks to the state of Nicolais
>>> patchset?
>>
>> According to Nicolai things look good, rules seem to be that it'll be
>> accepted if it has positive reviews (which it has) and no-one objected
>> (which no-one did).
>> Other than that one would have to ask the maintainer.
>> Herbert?
> 
> Any updates on this?

Sigh.

Herbert suggested reworking the patch, and make ffdhe a separate 
algorithm (instead of having enums to specify the values for the 
existing DH algorithm).
Discussion is ongoing :-(

Cheers,

Hannes
Sagi Grimberg March 13, 2022, 9:33 p.m. UTC | #9
> Hannes and co,
> 
> Do you know what is the state of this dependency? Or when should
> we expect to revisit this patch set?

Ping?
Christoph Hellwig March 14, 2022, 7:05 a.m. UTC | #10
On Mon, Mar 14, 2022 at 07:54:28AM +0100, Hannes Reinecke wrote:
> Pondering what the next steps should be. Herbert Xu has merged Nicolais 
> patchset, so I _could_ submit the patches, but then I would need to base it 
> on Herberts cryptodev-2.6 branch.
> And without these patches my patchset won't compile.
> So no sure how to proceed; sending the patches relative to Herberts tree? 
> Waiting for the patches to appear upstream? Not sure what'll be best...

We're at 5.17-rc8, so hopefully they will in mainline for 5.18-rc1 in
a few weeks. Let's wait until then to avoid complex tree dependencies.