Re: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: support 192/256 AES key sizes

[Anoob Joseph <anoobj@marvell.com>]

Hi Akhil,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Monday, April 6, 2020 12:12 PM
> To: Anoob Joseph <anoobj@marvell.com>; Radu Nicolau
> <radu.nicolau@intel.com>
> Cc: Narayana Prasad Raju Athreya <pathreya@marvell.com>; Tejasree Kondoj
> <ktejasree@marvell.com>; dev@dpdk.org
> Subject: [EXT] RE: [PATCH v3] examples/ipsec-secgw: support 192/256 AES key
> sizes
> 
> External Email
> 
> ----------------------------------------------------------------------
> > > Hi Anoob,
> > >
> > > >
> > > > Adding support for the following,
> > > > 1. AES-192-GCM
> > > > 2. AES-256-GCM
> > > > 3. AES-192-CBC
> > > >
> > > > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > > > Signed-off-by: Tejasree Kondoj <ktejasree@marvell.com>
> > > > ---
> > > > v3:
> > > > * Fixed incorrect AES-GCM key length being printed during app
> > > > startup
> > > > * Introduced new macro 'SALT_SIZE' to make the usage more obvious
> > > > (AES-
> > > GCM
> > > >   key has key following 4 byte salt)
> > > > * Minor cleanup for the existing code.
> > >
> > > I believe GCM keys are extended by 4 bytes to include the SALT value
> > > in many apps.
> > > We may add a comment that it is including the SALT value, but it
> > > makes more confusing now.
> > >
> > > The length which is being printed is 16Bytes but we expect the user
> > > to have 20Bytes In the ep0.cfg file. This will be confusing also to
> > > configure the packet capturing APPs Like wireshark which accepts 20Byte
> keys in case of GCM.
> >
> > [Anoob] The ones I've edited is just internal data structures. These
> > are not exposed and not directly printed anywhere.
> >
> > spi_in( 51):aes-128-gcm mode:IP4Tunnel 10.0.10.1 10.0.10.2
> > type:inline- protocol-offload spi_in( 52):aes-192-gcm mode:IP4Tunnel
> > 10.0.20.1 10.0.20.2 type:inline- protocol-offload spi_in(
> > 53):aes-256-gcm mode:IP4Tunnel 10.0.30.1 10.0.30.2 type:inline-
> > protocol-offload
> >
> > Also, my initial patch didn't try to address this aspect. In that
> > patch, I had the following addition, in which key length was clearly not
> matching the string.
> >
> > 	{
> > 		.keyword = "aes-192-gcm",
> > 		.algo = RTE_CRYPTO_AEAD_AES_GCM,
> > 		.iv_len = 8,
> > 		.block_size = 4,
> > 		.key_len = 28,
> > 		.digest_len = 16,
> > 		.aad_len = 8,
> > 	},
> >
> > In either case, the "misleading" part in config file would stay as the
> > string would be "aes-128-gcm"/"aes-192-gcm"/"aes-256-gcm", and the key
> > specified will have additional 4 bytes. Please do comment inline on
> > what you think is the right approach. You can check if you are fine
> > with v2 approach. I can resend that with a minor change required in the print.
> >
> > One more thing. I was just checking the ipsec-secgw documentation of
> > AEAD keys. I think we need to update that as well.
> >
> > Syntax: Hexadecimal bytes (0x0-0xFF) concatenate by colon symbol ':'.
> > The number of bytes should be as same as the specified AEAD algorithm key
> size.
> >
> > For example: aead_key A1:B2:C3:D4:A1:B2:C3:D4:A1:B2:C3:D4: A1:B2:C3:D4
> >
> > Can you advice on what should be the approach here?
> >
> I think it is better to have the key len include the 4 bytes of SALT and cfg file has
> those 4 bytes Inline with the key. We can add a print to specify that last 4 bytes
> are salt.
> And Yes for AEAD doc, we can add a statement that keylen should include the
> the 4bytes of SALT.
> And user should specify the extra 4 bytes.
> 
> So I believe your v2 was good enough with some additional documentations.

[Anoob] Will submit v2 with the changes discussed.