mbox series

[PATCHv12,00/11] nvme: In-band authentication support

Message ID 20220518112234.24264-1-hare@suse.de (mailing list archive)
Headers show
Series nvme: In-band authentication support | expand

Message

Hannes Reinecke May 18, 2022, 11:22 a.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.

Thanks to Nicolai Stange the crypto DH framework has been upgraded
to provide us with a FFDHE implementation; I've updated the patchset
to use the ephemeral key generation provided there.

Note that this is just for in-band authentication. Secure
concatenation (ie starting TLS with the negotiated parameters)
requires a TLS handshake, which the in-kernel TLS implementation
does not provide. This is being worked on with a different patchset
which is still WIP.

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.v12

It is being cut against the latest master branch from Linus.

As usual, comments and reviews are welcome.

Changes to v11:
- Fixup type for FAILURE2 message (Prashant Nayak)
- Do not sent SUCCESS2 if bi-directional authentication is not requested
  (Martin George)

Changes to v10:
- Fixup error return value when authentication failed

Changes to v9:
- Include review from Chaitanya
- Use sparse array for dhgroup and hash lookup
- Common function for auth_send and auth_receive

Changes to v8:
- Rebased to Nicolais crypto DH rework
- Fixed oops on non-fabrics devices

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 (11):
  crypto: add crypto_has_shash()
  crypto: add crypto_has_kpp()
  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/kpp.c                           |    6 +
 crypto/shash.c                         |    6 +
 drivers/nvme/host/Kconfig              |   12 +
 drivers/nvme/host/Makefile             |    1 +
 drivers/nvme/host/auth.c               | 1464 ++++++++++++++++++++++++
 drivers/nvme/host/auth.h               |   40 +
 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        |    4 +-
 drivers/nvme/target/auth.c             |  525 +++++++++
 drivers/nvme/target/configfs.c         |  138 ++-
 drivers/nvme/target/core.c             |   15 +
 drivers/nvme/target/fabrics-cmd-auth.c |  536 +++++++++
 drivers/nvme/target/fabrics-cmd.c      |   55 +-
 drivers/nvme/target/nvmet.h            |   75 +-
 include/crypto/hash.h                  |    2 +
 include/crypto/kpp.h                   |    2 +
 include/linux/base64.h                 |   16 +
 include/linux/nvme.h                   |  204 +++-
 lib/Makefile                           |    2 +-
 lib/base64.c                           |  103 ++
 28 files changed, 3501 insertions(+), 15 deletions(-)
 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/linux/base64.h
 create mode 100644 lib/base64.c

Comments

Hannes Reinecke May 25, 2022, 9:54 a.m. UTC | #1
On 5/18/22 13:22, 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.
> 
> Thanks to Nicolai Stange the crypto DH framework has been upgraded
> to provide us with a FFDHE implementation; I've updated the patchset
> to use the ephemeral key generation provided there.
> 
> Note that this is just for in-band authentication. Secure
> concatenation (ie starting TLS with the negotiated parameters)
> requires a TLS handshake, which the in-kernel TLS implementation
> does not provide. This is being worked on with a different patchset
> which is still WIP.
> 
> 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.v12
> 
> It is being cut against the latest master branch from Linus.
> 
> As usual, comments and reviews are welcome.
> 
How do we proceed here?
This has been lingering for quite some time now, without any real 
progress. Despite everyone agreeing that we would need to have it.
Anything which is missing from my side?
Any other obstacles?

Thanks.

Cheers,

Hannes
Sagi Grimberg May 25, 2022, 10:37 a.m. UTC | #2
>> 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.
>>
>> Thanks to Nicolai Stange the crypto DH framework has been upgraded
>> to provide us with a FFDHE implementation; I've updated the patchset
>> to use the ephemeral key generation provided there.
>>
>> Note that this is just for in-band authentication. Secure
>> concatenation (ie starting TLS with the negotiated parameters)
>> requires a TLS handshake, which the in-kernel TLS implementation
>> does not provide. This is being worked on with a different patchset
>> which is still WIP.
>>
>> 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.v12
>>
>> It is being cut against the latest master branch from Linus.
>>
>> As usual, comments and reviews are welcome.
>>
> How do we proceed here?
> This has been lingering for quite some time now, without any real 
> progress. Despite everyone agreeing that we would need to have it.
> Anything which is missing from my side?
> Any other obstacles?

I've been through it a number of times during the iterations, I feel
comfortable with it. I'd be more comfortable to get a second review
at least on this code.

But regardless, for the patches where it is missing:
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Christoph Hellwig May 26, 2022, 9 a.m. UTC | #3
On Wed, May 25, 2022 at 11:54:54AM +0200, Hannes Reinecke wrote:
> How do we proceed here?
> This has been lingering for quite some time now, without any real progress. 

As said it is a high priority for the upcoming merge window.  But we
also really need reviews from the crypto maintainers for the crypto
patches, without that I can't merge the series even if I'd like to.
Hannes Reinecke May 27, 2022, 5:50 a.m. UTC | #4
On 5/26/22 11:00, Christoph Hellwig wrote:
> On Wed, May 25, 2022 at 11:54:54AM +0200, Hannes Reinecke wrote:
>> How do we proceed here?
>> This has been lingering for quite some time now, without any real progress.
> 
> As said it is a high priority for the upcoming merge window.  But we
> also really need reviews from the crypto maintainers for the crypto
> patches, without that I can't merge the series even if I'd like to.

Hmm. Guess I can remove those helpers; after all,
both are just wrappers around existing exported helpers.
I'll resend.

Cheers,

Hannes
Hannes Reinecke May 27, 2022, 6:31 a.m. UTC | #5
On 5/27/22 07:50, Hannes Reinecke wrote:
> On 5/26/22 11:00, Christoph Hellwig wrote:
>> On Wed, May 25, 2022 at 11:54:54AM +0200, Hannes Reinecke wrote:
>>> How do we proceed herese h
>>> This has been lingering for quite some time now, without any real 
>>> progress.
>>
>> As said it is a high priority for the upcoming merge window.  But we
>> also really need reviews from the crypto maintainers for the crypto
>> patches, without that I can't merge the series even if I'd like to.
> 
> Hmm. Guess I can remove those helpers; after all,
> both are just wrappers around existing exported helpers.
> I'll resend.
> 
Bummer. There was a reason after all why I coded those helpers; they 
require an internal pointer :-(
But all of these are just checks if specific hash/kpp functions are 
supported; replacing them with appropriate CONFIG_ statements should 
work as well.
I'll check.

Cheers,

Hannes
Herbert Xu May 27, 2022, 10:06 a.m. UTC | #6
On Fri, May 27, 2022 at 07:50:42AM +0200, Hannes Reinecke wrote:
> On 5/26/22 11:00, Christoph Hellwig wrote:
> > On Wed, May 25, 2022 at 11:54:54AM +0200, Hannes Reinecke wrote:
> > > How do we proceed here?
> > > This has been lingering for quite some time now, without any real progress.
> > 
> > As said it is a high priority for the upcoming merge window.  But we
> > also really need reviews from the crypto maintainers for the crypto
> > patches, without that I can't merge the series even if I'd like to.
> 
> Hmm. Guess I can remove those helpers; after all,
> both are just wrappers around existing exported helpers.
> I'll resend.

I've just acked those two patches.

Cheers,
Hannes Reinecke May 27, 2022, 10:21 a.m. UTC | #7
On 5/27/22 12:06, Herbert Xu wrote:
> On Fri, May 27, 2022 at 07:50:42AM +0200, Hannes Reinecke wrote:
>> On 5/26/22 11:00, Christoph Hellwig wrote:
>>> On Wed, May 25, 2022 at 11:54:54AM +0200, Hannes Reinecke wrote:
>>>> How do we proceed here?
>>>> This has been lingering for quite some time now, without any real progress.
>>>
>>> As said it is a high priority for the upcoming merge window.  But we
>>> also really need reviews from the crypto maintainers for the crypto
>>> patches, without that I can't merge the series even if I'd like to.
>>
>> Hmm. Guess I can remove those helpers; after all,
>> both are just wrappers around existing exported helpers.
>> I'll resend.
> 
> I've just acked those two patches.
> 
Ah. Thanks for this.

Christoph, you can pick either v12 or v13; the difference is just the 
check for available hash and kpp functions. v12 has the dynamic version
using the crypto helpers, v13 has the static version checking 
compile-time configuration.
Either way would work.

Cheers,

Hannes
Christoph Hellwig June 7, 2022, 10:45 a.m. UTC | #8
On Fri, May 27, 2022 at 12:21:59PM +0200, Hannes Reinecke wrote:
> Christoph, you can pick either v12 or v13; the difference is just the check 
> for available hash and kpp functions. v12 has the dynamic version
> using the crypto helpers, v13 has the static version checking compile-time 
> configuration.
> Either way would work.

Please resend with these helpers included and any other fixups after rc2
is released.