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

[Akhil Goyal <akhil.goyal@nxp.com>]

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