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: Sunday, April 5, 2020 8:54 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?

> 
> >
> > v2:
> > * Updated doc and release notes
> >
> >  doc/guides/rel_notes/release_20_05.rst   |  7 ++++++
> >  doc/guides/sample_app_ug/ipsec_secgw.rst |  3 +++
> >  examples/ipsec-secgw/ipsec.h             |  3 ++-
> >  examples/ipsec-secgw/sa.c                | 38 ++++++++++++++++++++++++++------
> >  4 files changed, 43 insertions(+), 8 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/release_20_05.rst
> > b/doc/guides/rel_notes/release_20_05.rst
> > index 1dfcfcc..c0b0625 100644
> > --- a/doc/guides/rel_notes/release_20_05.rst
> > +++ b/doc/guides/rel_notes/release_20_05.rst
> > @@ -70,6 +70,13 @@ New Features
> >    by making use of the event device capabilities. The event mode
> > currently supports
> >    only inline IPsec protocol offload.
> >
> > +* **Added 192/256 AES key sizes in ipsec-secgw application.**
> > +
> > +  Updated ipsec-secgw application to support the following key sizes,
> > +    - AES-192-CBC
> > +    - AES-192-GCM
> > +    - AES-256-GCM
> > +
> >
> >  Removed Items
> >  -------------
> > diff --git a/doc/guides/sample_app_ug/ipsec_secgw.rst
> > b/doc/guides/sample_app_ug/ipsec_secgw.rst
> > index 038f593..f5e94bf 100644
> > --- a/doc/guides/sample_app_ug/ipsec_secgw.rst
> > +++ b/doc/guides/sample_app_ug/ipsec_secgw.rst
> > @@ -538,6 +538,7 @@ where each options means:
> >
> >     * *null*: NULL algorithm
> >     * *aes-128-cbc*: AES-CBC 128-bit algorithm
> > +   * *aes-192-cbc*: AES-CBC 192-bit algorithm
> >     * *aes-256-cbc*: AES-CBC 256-bit algorithm
> >     * *aes-128-ctr*: AES-CTR 128-bit algorithm
> >     * *3des-cbc*: 3DES-CBC 192-bit algorithm @@ -593,6 +594,8 @@ where
> > each options means:
> >   * Available options:
> >
> >     * *aes-128-gcm*: AES-GCM 128-bit algorithm
> > +   * *aes-192-gcm*: AES-GCM 192-bit algorithm
> > +   * *aes-256-gcm*: AES-GCM 256-bit algorithm
> >
> >   * Syntax: *cipher_algo <your algorithm>*
> >
> > diff --git a/examples/ipsec-secgw/ipsec.h
> > b/examples/ipsec-secgw/ipsec.h index f8f29f9..476a6d5 100644
> > --- a/examples/ipsec-secgw/ipsec.h
> > +++ b/examples/ipsec-secgw/ipsec.h
> > @@ -73,6 +73,7 @@ struct ip_addr {
> >  };
> >
> >  #define MAX_KEY_SIZE		32
> > +#define SALT_SIZE		4
> >
> >  /*
> >   * application wide SA parameters
> > @@ -133,7 +134,7 @@ struct ipsec_sa {
> >  #define IP6_TRANSPORT (1 << 4)
> >  	struct ip_addr src;
> >  	struct ip_addr dst;
> > -	uint8_t cipher_key[MAX_KEY_SIZE];
> > +	uint8_t cipher_key[MAX_KEY_SIZE + SALT_SIZE];
> >  	uint16_t cipher_key_len;
> >  	uint8_t auth_key[MAX_KEY_SIZE];
> >  	uint16_t auth_key_len;
> > diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
> > index 0eb52d1..fc6bc97 100644
> > --- a/examples/ipsec-secgw/sa.c
> > +++ b/examples/ipsec-secgw/sa.c
> > @@ -77,6 +77,13 @@ const struct supported_cipher_algo cipher_algos[] = {
> >  		.key_len = 16
> >  	},
> >  	{
> > +		.keyword = "aes-192-cbc",
> > +		.algo = RTE_CRYPTO_CIPHER_AES_CBC,
> > +		.iv_len = 16,
> > +		.block_size = 16,
> > +		.key_len = 24
> > +	},
> > +	{
> >  		.keyword = "aes-256-cbc",
> >  		.algo = RTE_CRYPTO_CIPHER_AES_CBC,
> >  		.iv_len = 16,
> > @@ -127,7 +134,25 @@ const struct supported_aead_algo aead_algos[] = {
> >  		.algo = RTE_CRYPTO_AEAD_AES_GCM,
> >  		.iv_len = 8,
> >  		.block_size = 4,
> > -		.key_len = 20,
> > +		.key_len = 16,
> > +		.digest_len = 16,
> > +		.aad_len = 8,
> > +	},
> > +	{
> > +		.keyword = "aes-192-gcm",
> > +		.algo = RTE_CRYPTO_AEAD_AES_GCM,
> > +		.iv_len = 8,
> > +		.block_size = 4,
> > +		.key_len = 24,
> > +		.digest_len = 16,
> > +		.aad_len = 8,
> > +	},
> > +	{
> > +		.keyword = "aes-256-gcm",
> > +		.algo = RTE_CRYPTO_AEAD_AES_GCM,
> > +		.iv_len = 8,
> > +		.block_size = 4,
> > +		.key_len = 32,
> >  		.digest_len = 16,
> >  		.aad_len = 8,
> >  	}
> > @@ -495,16 +520,14 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
> >  				return;
> >
> >  			key_len = parse_key_string(tokens[ti],
> > -				rule->cipher_key);
> > +				rule->cipher_key) - SALT_SIZE;
> >  			APP_CHECK(key_len == rule->cipher_key_len, status,
> >  				"unrecognized input \"%s\"", tokens[ti]);
> >  			if (status->status < 0)
> >  				return;
> >
> > -			key_len -= 4;
> > -			rule->cipher_key_len = key_len;
> > -			memcpy(&rule->salt,
> > -				&rule->cipher_key[key_len], 4);
> > +			memcpy(&rule->salt, &rule->cipher_key[key_len],
> > +				SALT_SIZE);
> >
> >  			aead_algo_p = 1;
> >  			continue;
> > @@ -751,7 +774,8 @@ print_one_sa_rule(const struct ipsec_sa *sa, int
> inbound)
> >  	}
> >
> >  	for (i = 0; i < RTE_DIM(aead_algos); i++) {
> > -		if (aead_algos[i].algo == sa->aead_algo) {
> > +		if (aead_algos[i].algo == sa->aead_algo &&
> > +				aead_algos[i].key_len == sa->cipher_key_len) {
> >  			printf("%s ", aead_algos[i].keyword);
> >  			break;
> >  		}
> > --
> > 2.7.4