Adding IPsec support #1206

dipankar-ba · 2021-03-19T21:10:35Z

Signed-off-by: dipankar-ba dipankar@arista.com

lguohan · 2021-04-16T16:48:13Z

can you remove the file .DS_Store?

dipankar-ba · 2021-06-02T01:29:22Z

Please see in-line.

We recommend to add an attribute for supporting both per SA and GLOBAL counters. Globals are aggregates for ingress and egress direction across all SAs. Per SA stats are directional per SA.

SAI_IPSEC_ATTR_STATS_TYPE_SA , SAI_IPSEC_ATTR_STATS_TYPE_GLOBAL

GLOBAL counters support needs an extra attribute in addition to those defined for the per SA stats in sai_ipsec_sa_stat_t

AFAIK, SAI counters are always associated with an SAI object. Any top level "foo" counter would be associated with SAI_SWITCH object and would have an enum named SAI_SWITCH_STAT_foo. From your description, I do not understand which object you want these "global" counters for and what are counter semantics you are looking for.

Also note the hardware is not capable of having a counter at SAI_SWITCH level, since the counters are per IPsec engine and each engine may serve a subset of ports. Normally the highest level the IPsec engine hardware counts is at port level. If you do not expose that details in the counter API, you lose information about which port is contributing to the "global" counters.

SAI_IPSEC_SA_STAT_SA_MISS (to count for STATS for invalid SA or not found SA)

From this comment, as well as from the request for notify criteria, it seems you consider packets that cannot be decrypted by hardware as exception packets. However, our model was there can be 3 types of IPsec packets that can match the host/switch IP address:

Packets decrypted by hardware
Packets decrypted by software
Exception packets that cannot be decrypted by either

To allow software decryption, the hardware has the capability of forwarding encrypted packets to the switch software. If there is no decryption capability in software, the switch can install a "system ACL" to detect all such packets (with ESP header) and to drop and count those. This seems a more flexible way of implementing the IPsec decryption.

dipankar-ba · 2021-06-02T01:53:35Z

For notifications related to SN counter, don't we need the SA_INDEX? And in that case, do we want to send a separate notification for each event on each SA?

Please see 3 more comments in-line.

We recommend adding notifications for soft/hard limit, seq rollover etc...
/**

@brief Defines the conditional status for packet anomalies

typedef struct _sai_ipsec_sa_condition_notification_t
/** Soft limit crossed */
SAI_IPSEC_CONDITION_TYPE_SOFT_LIMIT = 0,

/** Hard limit crossed */
SAI_IPSEC_CONDITION_TYPE_HARD_LIMIT,

Do the soft- and hard-limits refer to SN in the ESP header or something else? Do we need any per-SA attributes to define these thresholds?

/** Invalid SA */
SAI_IPSEC_CONDITION_TYPE_SA_INVALID,
Refer to comment about invalid SA counter. I think it is better to allow flexibility for IPsec decryption in software.

/** SN rollover */
SAI_IPSEC_CONDITION_TYPE_SN_ROLLOVER,

This is for tx (encryption) SAs only - right? Would it better to call this something like SAI_IPSEC_CONDITION_TYPE_SN_MAX_REACHED? After all packets are not transmitted with rolled over SN.


} sai_ipsec_sa_condition_notification_t;

typedef void (*sai_ipsec_sa_condition_notification_fn)(
_In_ sai_object_id_t ipsec_sa_id,
_Out_ sai_ipsec_sa_condition_notification_t *data);

nupurjai · 2021-06-18T15:59:02Z

For notifications related to SN counter, don't we need the SA_INDEX? And in that case, do we want to send a separate notification for each event on each SA?

Please see 3 more comments in-line.
We recommend adding notifications for soft/hard limit, seq rollover etc...
/**

@brief Defines the conditional status for packet anomalies

typedef struct _sai_ipsec_sa_condition_notification_t
/** Soft limit crossed */
SAI_IPSEC_CONDITION_TYPE_SOFT_LIMIT = 0,

/** Hard limit crossed */
SAI_IPSEC_CONDITION_TYPE_HARD_LIMIT,
Do the soft- and hard-limits refer to SN in the ESP header or something else? Do we need any per-SA attributes to define these thresholds?

/** Invalid SA */
SAI_IPSEC_CONDITION_TYPE_SA_INVALID,
Refer to comment about invalid SA counter. I think it is better to allow flexibility for IPsec decryption in software.

/** SN rollover */
SAI_IPSEC_CONDITION_TYPE_SN_ROLLOVER,

This is for tx (encryption) SAs only - right? Would it better to call this something like SAI_IPSEC_CONDITION_TYPE_SN_MAX_REACHED? After all packets are not transmitted with rolled over SN.
} sai_ipsec_sa_condition_notification_t;

typedef void (*sai_ipsec_sa_condition_notification_fn)(
_In_ sai_object_id_t ipsec_sa_id,
_Out_ sai_ipsec_sa_condition_notification_t *data);

RE: This is for tx (encryption) SAs only - right? Would it better to call this something like SAI_IPSEC_CONDITION_TYPE_SN_MAX_REACHED
Yes, That should be fine. These would be per SA and would need SA_INDEX.
Can global counters indicate the INVALID_SA per port or port group? This is to indicate what hardware failed to decrypt. Software stack can maintain it's own counters. For packets that are dropped, we already have counters today.

dipankar-ba · 2021-06-18T16:34:04Z

As agreed in IPSEC review meetings, there is no need to use first nibble to indicate the Next Header field.
Instead it was proposed to use separate SAs for encrypted payloads that start with UDP or IP header (IPv4/IPV6).
Assuming this description will be updated to capture the same.

This is resolved

inc/saiipsec.h

dipankar-ba · 2021-06-18T16:52:13Z

For notifications related to SN counter, don't we need the SA_INDEX? And in that case, do we want to send a separate notification for each event on each SA?
Please see 3 more comments in-line.
We recommend adding notifications for soft/hard limit, seq rollover etc...
/**

@brief Defines the conditional status for packet anomalies

typedef struct _sai_ipsec_sa_condition_notification_t
/** Soft limit crossed */
SAI_IPSEC_CONDITION_TYPE_SOFT_LIMIT = 0,

/** Hard limit crossed */
SAI_IPSEC_CONDITION_TYPE_HARD_LIMIT,
Do the soft- and hard-limits refer to SN in the ESP header or something else? Do we need any per-SA attributes to define these thresholds?
/** Invalid SA */
SAI_IPSEC_CONDITION_TYPE_SA_INVALID,
Refer to comment about invalid SA counter. I think it is better to allow flexibility for IPsec decryption in software.

/** SN rollover */
SAI_IPSEC_CONDITION_TYPE_SN_ROLLOVER,

This is for tx (encryption) SAs only - right? Would it better to call this something like SAI_IPSEC_CONDITION_TYPE_SN_MAX_REACHED? After all packets are not transmitted with rolled over SN.
} sai_ipsec_sa_condition_notification_t;

typedef void (*sai_ipsec_sa_condition_notification_fn)(
_In_ sai_object_id_t ipsec_sa_id,
_Out_ sai_ipsec_sa_condition_notification_t *data);
RE: This is for tx (encryption) SAs only - right? Would it better to call this something like SAI_IPSEC_CONDITION_TYPE_SN_MAX_REACHED
Yes, That should be fine. These would be per SA and would need SA_INDEX.
Can global counters indicate the INVALID_SA per port or port group? This is to indicate what hardware failed to decrypt. Software stack can maintain it's own counters. For packets that are dropped, we already have counters today.

I am not exactly sure what you mean by invalid SA/decryption failure. Perhaps you mean IPsec packets that match one of the Security Engine's tunnel termination criteria, but do not terminate the IPsec in the Security Engine? This is not supported in the other IPsec hardware and sounds like an implementation specific debug counter. AFAIK, the NOS can allow such implementation-specific debug features, but outside the scope of the standard SAI API.

inc/saiipsec.h

inc/saiswitch.h

kcudnik · 2021-07-07T07:01:38Z

please resolve conflicts and correct possible errors

meta/saiserialize.c

inc/saiport.h

kcudnik · 2021-07-16T13:04:10Z

meta/saiserialize.c

+        _In_ const char *buffer,
+        _Out_ sai_macsec_sak_t sak)
+{
+   return sai_deserialize_encrypt_key( buffer, sak );


if _macsec and encrypt_key are the same why 2 differnt types introduced ?

In the sai-ipsec subgroup meeting on July 14, it was decided to duplicate the data type for IPsec because of the unfortunate naming of the MACsec data type.
The existing sai_macsec_sak_t and the corresponding enum SAI_MACSEC_SA_ATTR_SAK should not be changed because those are used in existing MACsec drivers and NOS code.
Using sai_macsec_sak_t for IPsec could have worked with an enum SAI_IPSEC_SA_ATTR_MACSEC_SAK, but those names have no meaning in the IPsec context. Hence duplication was preferred.

kcudnik · 2021-07-16T13:04:49Z

meta/saiserialize.c

+        _In_ const char *buffer,
+        _Out_ sai_macsec_auth_key_t auth)
+{
+   return sai_deserialize_auth_key( buffer, auth );


if auth_key and mansec_auth_key is the same, why 2 different types intdoduced ?

In the sai-ipsec subgroup meeting on July 14, it was decided to duplicate the data type for IPsec because of the unfortunate naming of the MACsec data type.
The existing sai_macsec_auth_key_t and the corresponding enum SAI_MACSEC_SA_ATTR_AUTH_KEY should not be changed because those are used in existing MACsec drivers and NOS code.
Using sai_macsec_auth_key_t for IPsec could have worked with an enum SAI_IPSEC_SA_ATTR_MACSEC_AUTH_KEY, but the MACsec reference in IPsec context is weird and confusing. Hence duplication was preferred.

instead of having 2 different types, can we have 1 type (new one) and add a "define OLD_type NEW_type" ??

In the sai-ipsec subgroup meeting on July 14, it was decided to duplicate the data type for IPsec because of the unfortunate naming of the MACsec data type.

Why SAI_MACSEC_ATTR_MACSEC_AUTH_KEY is needed when MACsec Secure Association created?
MACsec is based on 802.1AE cipher suite.
Data plane encryption is based on GCM-AES-128/256 or GCM-XPN-AES-128/256.
Data plane authentication is based on GHASH function as GCM Spec.
What can the MACsec crypto hardware do with an SAI_MACSEC_ATTR_MACSEC_AUTH_KEY ?

kcudnik · 2021-08-16T15:09:40Z

I made mistake on resolving conflicts:

     * @condition SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_IPINIP or SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_IPINIP_GRE or SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_VXLAN or SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_SRV6 or SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_NVGRE
     * @condition SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_IPINIP or SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_IPINIP_GRE or SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_VXLAN or SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_SRV6 or SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_IPINIP_ESP or SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_IPINIP_UDP_ESP or SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_VXLAN_UDP_ESP
     */
    SAI_TUNNEL_ATTR_UNDERLAY_INTERFACE,

if accidently remove "SAI_TUNNEL_ATTR_TYPE == SAI_TUNNEL_TYPE_NVGR" @dipankar-ba can you update?

rlhui · 2021-08-26T16:23:49Z

@dipankar-ba - please resolve conflicts? Thanks.

dipankar-ba · 2021-08-27T00:09:52Z

Sorry, I had thought it had merged. I will take a look. - Dipankar

…

On Thu, Aug 26, 2021 at 9:24 AM rlhui ***@***.***> wrote: @dipankar-ba <https://github.com/dipankar-ba> - please resolve conflicts? Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1206 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN3GZXMVEBU6PZHRF3QZGBTT6ZTCBANCNFSM4ZPQNHIA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

dipankar-ba · 2021-09-02T23:27:44Z

@kcudnik
After resolving the merge conflicts, I end up with a huge number of failure of:
Checking for possible enum values shift (current branch vs origin/master)
Most of these are for enums in files I have not modified e.g. SAI_ACL_ACTION_TYPE_ACL_DTEL_FLOW_OP fails. It is in saiacl.h, which I have not touched.
Can you please help?

kcudnik · 2021-09-03T11:18:05Z

your current fail is here:

WARNING: line ends in whitespace saiport.h 2034:  
WARNING: Header doesn't meet style requirements (most likely ident is not 4 or 8 spaces) saiport.h 2034:

as for ACL enum shifts, acl enums changed beginning from your PR, since march 19, and to actually fix that you would need to rebase this PR on top of master instead of having branches

Removed parsing hint from decrypted packet 1st nibble. Added notification support. (cherry-pick f30f96d 7a7f806 c984217) Fixed notification mechanism. Changed IPSEC_SA_TAG_TPID default value. (cherry-pick 2227b36 ce6d675 8869a2) Added sai_encrypt_key_t and sai_auth_key_t (cherry-pick 61a010b 67d01dd) Moving new attributes to the end of enums Removed SAI_TUNNEL_TYPE_NVGRE (cherry-pick de6a638 67b6caf) Signed-off-by: dipankar-ba <dipankar@arista.com>

dipankar-ba · 2022-11-07T19:45:24Z

The crypto hardware uses SAI_MACSEC_ATTR_MACSEC_AUTH_KEY for the GHASH function. My initial draft of SAI_MACSEC did not have this, because theoretically the hardware or the driver could compute it from the encryption key. But during the in-person review, someone wanted it on the interface, because traditionally it has been done that way in existing driver interfaces. And nobody objected to that request. - Dipankar

…

On Sat, Nov 5, 2022 at 1:58 AM Haidong Gong ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In meta/saiserialize.c <#1206 (comment)> : > +{ + return sai_deserialize_encrypt_key( buffer, sak ); +} + +int sai_serialize_macsec_auth_key( + _Out_ char *buffer, + _In_ const sai_macsec_auth_key_t auth) +{ + return sai_serialize_auth_key( buffer, auth ); +} + +int sai_deserialize_macsec_auth_key( + _In_ const char *buffer, + _Out_ sai_macsec_auth_key_t auth) +{ + return sai_deserialize_auth_key( buffer, auth ); In the sai-ipsec subgroup meeting on July 14, it was decided to duplicate the data type for IPsec because of the unfortunate naming of the MACsec data type. Why SAI_MACSEC_ATTR_MACSEC_AUTH_KEY is needed when MACsec Secure Association created? MACsec is based on 802.1AE cipher suite. Data plane encryption is based on GCM-AES-128/256 or GCM-XPN-AES-128/256. Data plane authentication is based on GHASH function as GCM Spec. What can the MACsec crypto hardware do with an SAI_MACSEC_ATTR_MACSEC_AUTH_KEY ? — Reply to this email directly, view it on GitHub <#1206 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN3GZXOK6SRNIBBVW5QN53LWGYOS5ANCNFSM4ZPQNHIA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Haidong988 · 2022-11-08T11:58:00Z

The crypto hardware uses SAI_MACSEC_ATTR_MACSEC_AUTH_KEY for the GHASH function. My initial draft of SAI_MACSEC did not have this, because theoretically the hardware or the driver could compute it from the encryption key. But during the in-person review, someone wanted it on the interface, because traditionally it has been done that way in existing driver interfaces. And nobody objected to that request. - Dipankar
…
On Sat, Nov 5, 2022 at 1:58 AM Haidong Gong @.> wrote: @.* commented on this pull request. ------------------------------ In meta/saiserialize.c <#1206 (comment)> : > +{ + return sai_deserialize_encrypt_key( buffer, sak ); +} + +int sai_serialize_macsec_auth_key( + Out char buffer, + In const sai_macsec_auth_key_t auth) +{ + return sai_serialize_auth_key( buffer, auth ); +} + +int sai_deserialize_macsec_auth_key( + In const char buffer, + Out sai_macsec_auth_key_t auth) +{ + return sai_deserialize_auth_key( buffer, auth ); In the sai-ipsec subgroup meeting on July 14, it was decided to duplicate the data type for IPsec because of the unfortunate naming of the MACsec data type. Why SAI_MACSEC_ATTR_MACSEC_AUTH_KEY is needed when MACsec Secure Association created? MACsec is based on 802.1AE cipher suite. Data plane encryption is based on GCM-AES-128/256 or GCM-XPN-AES-128/256. Data plane authentication is based on GHASH function as GCM Spec. What can the MACsec crypto hardware do with an SAI_MACSEC_ATTR_MACSEC_AUTH_KEY ? — Reply to this email directly, view it on GitHub <#1206 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AN3GZXOK6SRNIBBVW5QN53LWGYOS5ANCNFSM4ZPQNHIA . You are receiving this because you were mentioned.Message ID: @.*>

So SAI_MACSEC_ATTR_MACSEC_AUTH_KEY is H for GHASH(H,A,C) which is calculated by E(K,0).
SAI_MACSEC_ATTR_MACSEC_AUTH_KEY is not necessary to be MANDATORY_ON_CREATE.

dipankar-ba · 2022-11-08T18:30:30Z

It seemed logical to give the same flags to SAI_MACSEC_SA_ATTR_SAK and SAI_MACSEC_ATTR_MACSEC_AUTH_KEY, since the latter can be computed once at the same time SAK is available. What would be the advantage of delaying the AUTH_KEY computation? - Dipankar On Tue, Nov 8, 2022 at 3:58 AM Haidong Gong ***@***.***> wrote:

…

The crypto hardware uses SAI_MACSEC_ATTR_MACSEC_AUTH_KEY for the GHASH function. My initial draft of SAI_MACSEC did not have this, because theoretically the hardware or the driver could compute it from the encryption key. But during the in-person review, someone wanted it on the interface, because traditionally it has been done that way in existing driver interfaces. And nobody objected to that request. - Dipankar … <#m_4630814634200061671_> On Sat, Nov 5, 2022 at 1:58 AM Haidong Gong *@*.*> wrote: @.** commented on this pull request. ------------------------------ In meta/saiserialize.c <#1206 (comment) <#1206 (comment)>> : > +{ + return sai_deserialize_encrypt_key( buffer, sak ); +} + +int sai_serialize_macsec_auth_key( + *Out* char *buffer, + In const sai_macsec_auth_key_t auth) +{ + return sai_serialize_auth_key( buffer, auth ); +} + +int sai_deserialize_macsec_auth_key( + In const char buffer, + Out sai_macsec_auth_key_t auth) +{ + return sai_deserialize_auth_key( buffer, auth ); In the sai-ipsec subgroup meeting on July 14, it was decided to duplicate the data type for IPsec because of the unfortunate naming of the MACsec data type. Why SAI_MACSEC_ATTR_MACSEC_AUTH_KEY is needed when MACsec Secure Association created? MACsec is based on 802.1AE cipher suite. Data plane encryption is based on GCM-AES-128/256 or GCM-XPN-AES-128/256. Data plane authentication is based on GHASH function as GCM Spec. What can the MACsec crypto hardware do with an SAI_MACSEC_ATTR_MACSEC_AUTH_KEY ? — Reply to this email directly, view it on GitHub <#1206 (comment) <#1206 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AN3GZXOK6SRNIBBVW5QN53LWGYOS5ANCNFSM4ZPQNHIA <https://github.com/notifications/unsubscribe-auth/AN3GZXOK6SRNIBBVW5QN53LWGYOS5ANCNFSM4ZPQNHIA> . You are receiving this because you were mentioned.Message ID: @.**> So SAI_MACSEC_ATTR_MACSEC_AUTH_KEY is H for GHASH(H,A,C) which is calculated by E(K,0). SAI_MACSEC_ATTR_MACSEC_AUTH_KEY is not necessary to be MANDATORY_ON_CREATE. — Reply to this email directly, view it on GitHub <#1206 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN3GZXMFNFZUXY26OMWNHYTWHI55JANCNFSM4ZPQNHIA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Haidong988 · 2022-11-09T05:01:17Z

It seemed logical to give the same flags to SAI_MACSEC_SA_ATTR_SAK and SAI_MACSEC_ATTR_MACSEC_AUTH_KEY, since the latter can be computed once at the same time SAK is available. What would be the advantage of delaying the AUTH_KEY computation? - Dipankar On Tue, Nov 8, 2022 at 3:58 AM Haidong Gong @.> wrote:
…
The crypto hardware uses SAI_MACSEC_ATTR_MACSEC_AUTH_KEY for the GHASH function. My initial draft of SAI_MACSEC did not have this, because theoretically the hardware or the driver could compute it from the encryption key. But during the in-person review, someone wanted it on the interface, because traditionally it has been done that way in existing driver interfaces. And nobody objected to that request. - Dipankar … <#m_4630814634200061671_> On Sat, Nov 5, 2022 at 1:58 AM Haidong Gong @.> wrote: @. commented on this pull request. ------------------------------ In meta/saiserialize.c <#1206 (comment) <#1206 (comment)>> : > +{ + return sai_deserialize_encrypt_key( buffer, sak ); +} + +int sai_serialize_macsec_auth_key( + Out char buffer, + In const sai_macsec_auth_key_t auth) +{ + return sai_serialize_auth_key( buffer, auth ); +} + +int sai_deserialize_macsec_auth_key( + In const char buffer, + Out sai_macsec_auth_key_t auth) +{ + return sai_deserialize_auth_key( buffer, auth ); In the sai-ipsec subgroup meeting on July 14, it was decided to duplicate the data type for IPsec because of the unfortunate naming of the MACsec data type. Why SAI_MACSEC_ATTR_MACSEC_AUTH_KEY is needed when MACsec Secure Association created? MACsec is based on 802.1AE cipher suite. Data plane encryption is based on GCM-AES-128/256 or GCM-XPN-AES-128/256. Data plane authentication is based on GHASH function as GCM Spec. What can the MACsec crypto hardware do with an SAI_MACSEC_ATTR_MACSEC_AUTH_KEY ? — Reply to this email directly, view it on GitHub <#1206 (comment) <#1206 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AN3GZXOK6SRNIBBVW5QN53LWGYOS5ANCNFSM4ZPQNHIA https://github.com/notifications/unsubscribe-auth/AN3GZXOK6SRNIBBVW5QN53LWGYOS5ANCNFSM4ZPQNHIA . You are receiving this because you were mentioned.Message ID: @.> So SAI_MACSEC_ATTR_MACSEC_AUTH_KEY is H for GHASH(H,A,C) which is calculated by E(K,0). SAI_MACSEC_ATTR_MACSEC_AUTH_KEY is not necessary to be MANDATORY_ON_CREATE. — Reply to this email directly, view it on GitHub <#1206 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AN3GZXMFNFZUXY26OMWNHYTWHI55JANCNFSM4ZPQNHIA . You are receiving this because you were mentioned.Message ID: @.>

If hardware support calc H from SAI_MACSEC_SA_ATTR_SAK, H is not needed when create MACSEC SA.
MANDATORY_ON_CREATE means even hardware do not need that parameter, software still need to calculate one.

Haidong988 · 2022-11-09T05:19:28Z

What would be the advantage of delaying the AUTH_KEY computation?

Hardware crypto implement could have different architecture, it might be cipher-text based or packet flow based.
In packet flow based implement the crypto uint in hardware support pipeline process per cycle, support H calculation in "Start of packet" cycle causes no "delaying" or extra hardware cost.

dipankar-ba · 2022-11-09T17:00:55Z

Hi Haidong, Sorry, I do not understand. Are we still talking of 2 different implementation of the same AES-GCM cypher-suite? Do you have any references that explains the 2 implementations? Also, if we are still talking about AES-GCM, the NOS could provide the AUTH_KEY during the SA creation and the hardware could choose to not use it - right? - Dipankar

…

On Tue, Nov 8, 2022 at 9:19 PM Haidong Gong ***@***.***> wrote: What would be the advantage of delaying the AUTH_KEY computation? Hardware crypto implement could have different architecture, it might be cipher-text based or packet flow based. In packet flow based implement the crypto uint in hardware support pipeline process per cycle, support H calculation in "Start of packet" cycle causes no "delaying" or extra hardware cost. — Reply to this email directly, view it on GitHub <#1206 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN3GZXIK2HWSIYW4FV4SZZTWHMX6VANCNFSM4ZPQNHIA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Haidong988 · 2022-11-10T02:49:11Z

Also, if we are still talking about AES-GCM, the NOS could provide the

There are reasons for hardware crypto implement making choice to calculate H or put it in storage like SRAM:

session number
speed
architecture
I could understand some PHY implement put it in storage like SRAM as input parameter for only several session there.
And maybe there are timing or latency issue in their hardware implement to calc H especially in high frequency Clock like 800G PHY.
However, if session number is up to K level and frequency is not high enough，that's another story.

As you said "hardware could choose to not use it".
That's the point, if hardware could choose to not use it, why it must be MANDATORY_ON_CREATE ?
Or software need it for any other reason ?
It seems to me the only reason whole system need it, is some hardware could not calculate it per packet from SAK in line rate.
I don't think NOS always need to calucate it every time it need a new SAK.
For system like Sonic with SAI, it should be considered whether the driver layer is the better palce to put this parameter.

If you want to keep, that's ok to me.
Just change it from "MANDATORY_ON_CREATE | CREATE_ONLY" to CREATE_ONLY.

dipankar-ba · 2022-11-11T03:11:08Z

Hi Haidong, I do not want to do this because: 1. CREATE_ONLY without MANDATORY_ON_CREATE does not make sense. It does not define if the NOS should provide this attribute or not. 1. It is disruptive. If NOS does not provide this attribute, existing (hardware+driver) implementations will break - requiring at least a driver modification 2. It is unnecessary - it costs little for NOS to compute the AUTH_KEY once and provide it to the driver 3. The current AUTH_KEY attribute+flags work even with a (driver+hardware) implementation that chooses to compute the AUTH_KEY, instead of using one from the NOS. If you still feel strongly about this, you can submit a PR and have it reviewed by the community. BTW, there is no need to compute AUTH_KEY per packet - computing once for all packets in the SA is sufficient. So, hardware speed has nothing to do with the implementation choice. And I cannot see why anyone will choose to compute this in hardware. Regards. - Dipankar

…

On Wed, Nov 9, 2022 at 6:49 PM Haidong Gong ***@***.***> wrote: Also, if we are still talking about AES-GCM, the NOS could provide the There are reasons for hardware crypto implement making choice to calculate H or put it in storage like SRAM: 1. session number 2. speed 3. architecture I could understand some PHY implement put it in storage like SRAM as input parameter for only several session there. And maybe there are timing or latency issue in their hardware implement to calc H especially in high frequency Clock like 800G PHY. However, if session number is up to K level and frequency is not high enough，that's another story. As you said "hardware could choose to not use it". That's the point, if hardware could choose to not use it, why it must be MANDATORY_ON_CREATE ? Or software need it for any other reason ? It seems to me the only reason whole system need it, is some hardware could not calculate it per packet from SAK in line rate. I don't think NOS always need to calucate it every time it need a new SAK. For system like Sonic with SAI, it should be considered whether the driver layer is the better palce to put this parameter. If you want to keep, that's ok to me. Just change it from "MANDATORY_ON_CREATE | CREATE_ONLY" to CREATE_ONLY. — Reply to this email directly, view it on GitHub <#1206 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN3GZXNCZCWOZPKFZGDIZTLWHRPDFANCNFSM4ZPQNHIA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Haidong988 · 2022-11-15T05:53:44Z

I do not want to do this because: 1. CREATE_ONLY without MANDATORY_ON_CREATE does not make sense. It does not define if the NOS should provide this attribute or not. 1. It is disruptive. If NOS does not provide this attribute, existing (hardware+driver) implementations will break - requiring at least a driver modification 2. It is unnecessary - it costs little for NOS to compute the AUTH_KEY once and provide it to the driver 3. The current AUTH_KEY attribute+flags work even with a (driver+hardware) implementation that chooses to compute the AUTH_KEY, instead of using one from the NOS. If you still feel strongly about this, you can submit a PR and have it reviewed by the community. BTW, there is no need to compute AUTH_KEY per packet - computing once for all packets in the SA is sufficient. So, hardware speed has nothing to do with the implementation choice. And I cannot see why anyone will choose to compute this in hardware.

To 1. ”provide this attribute or not“ should not be deciced by NOS，it should be decided by hardware if this parameter is needed. For hardware need H, it should be calculated from SAK by driver. For hardware does not need it, SAK is enough. NOS has nothing to do with H.

To 2. As I said "session number" could lead to problems for both software and hardware. Hardware does not want to take the cost of SRAM for 10K session's extra parameter storage which could be calculated without any cost because the calc unit is always there. Software does not want to calculate 10K session's H from SAK when the controller or administrator switches 10K session's SAK periodically. If this must be done, only the driver of serveal PHY which need H calc it.

To 3. NOS has nothing to do with H and it must calculate it just because it may not known if the hardware could calc it. This may work with Sonic now because Sonic makes the assumption that H is always needed.
I don't know how could SAI work with NOS which feels nothing about H if it is MANDATORY_ON_CREATE.
It's weird if NOS need another driver layer for SAI.

To BTW. The need is there for the cost of SRAM of 10K session's extra parameter storage in ASIC. And the speed I said means the Clock Frequency of ASIC which could be 800MHz or even GHz, in this case session number is definitely small and the lantecy/timig is critical, the trade-off is naturely for ASIC designer to choice H to be an input parameter from SRAM storage.
But that does not means "hardware speed has nothing to do with the implementation choice", actually they did the choice to use storage which is not must be done in ASIC implementation with frequencey below 300MHz and session number large.

dipankar-ba closed this Jun 18, 2021

dipankar-ba reopened this Jun 18, 2021

ghost suggested changes Jun 18, 2021

View reviewed changes

inc/saiipsec.h Outdated Show resolved Hide resolved

inc/saiipsec.h Outdated Show resolved Hide resolved

inc/saiipsec.h Outdated Show resolved Hide resolved

ghost reviewed Jun 22, 2021

View reviewed changes

inc/saiipsec.h Outdated Show resolved Hide resolved

reshmaintel approved these changes Jul 6, 2021

View reviewed changes

nupurjai approved these changes Jul 6, 2021

View reviewed changes

kcudnik reviewed Jul 7, 2021

View reviewed changes

inc/saiswitch.h Outdated Show resolved Hide resolved

dipankar-ba force-pushed the Add_IPsec_support branch 2 times, most recently from bd820a8 to 8869a27 Compare July 9, 2021 21:24

dipankar-ba force-pushed the Add_IPsec_support branch from db87624 to 61a010b Compare July 15, 2021 22:47

kcudnik reviewed Jul 16, 2021

View reviewed changes

meta/saiserialize.c Outdated Show resolved Hide resolved

kcudnik reviewed Jul 16, 2021

View reviewed changes

inc/saiport.h Outdated Show resolved Hide resolved

lguohan approved these changes Jul 29, 2021

View reviewed changes

kcudnik reviewed Jul 29, 2021

View reviewed changes

reshmaintel approved these changes Aug 5, 2021

View reviewed changes

nupurjai approved these changes Aug 5, 2021

View reviewed changes

reshmaintel approved these changes Aug 27, 2021

View reviewed changes

dipankar-ba force-pushed the Add_IPsec_support branch from 0c355bd to 708f4a5 Compare September 13, 2021 18:46

dipankar-ba force-pushed the Add_IPsec_support branch 2 times, most recently from 9135d15 to 1469eca Compare September 14, 2021 16:43

dipankar-ba force-pushed the Add_IPsec_support branch from 1469eca to 215ece8 Compare September 14, 2021 17:46

kcudnik approved these changes Sep 14, 2021

View reviewed changes

lguohan merged commit 231e67e into opencomputeproject:master Sep 15, 2021

dipankar-ba deleted the Add_IPsec_support branch September 15, 2021 02:35

richardyu-ms mentioned this pull request Apr 10, 2022

[SAI]Fix build in v1.7 and remove unnecessary change #1468

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding IPsec support #1206

Adding IPsec support #1206

dipankar-ba commented Mar 19, 2021

lguohan commented Apr 16, 2021

dipankar-ba commented Jun 2, 2021

dipankar-ba commented Jun 2, 2021 •

edited

nupurjai commented Jun 18, 2021

dipankar-ba commented Jun 18, 2021

dipankar-ba commented Jun 18, 2021

kcudnik commented Jul 7, 2021

kcudnik Jul 16, 2021

dipankar-ba Jul 29, 2021 •

edited

kcudnik Jul 16, 2021

dipankar-ba Jul 29, 2021

kcudnik Jul 29, 2021

Haidong988 Nov 5, 2022

kcudnik commented Aug 16, 2021

rlhui commented Aug 26, 2021

dipankar-ba commented Aug 27, 2021 via email

dipankar-ba commented Sep 2, 2021

kcudnik commented Sep 3, 2021

dipankar-ba commented Nov 7, 2022 via email

Haidong988 commented Nov 8, 2022

dipankar-ba commented Nov 8, 2022 via email

Haidong988 commented Nov 9, 2022

Haidong988 commented Nov 9, 2022

dipankar-ba commented Nov 9, 2022 via email

Haidong988 commented Nov 10, 2022

dipankar-ba commented Nov 11, 2022 via email

Haidong988 commented Nov 15, 2022

Adding IPsec support #1206

Adding IPsec support #1206

Conversation

dipankar-ba commented Mar 19, 2021

lguohan commented Apr 16, 2021

dipankar-ba commented Jun 2, 2021

dipankar-ba commented Jun 2, 2021 • edited

nupurjai commented Jun 18, 2021

dipankar-ba commented Jun 18, 2021

dipankar-ba commented Jun 18, 2021

kcudnik commented Jul 7, 2021

kcudnik Jul 16, 2021

Choose a reason for hiding this comment

dipankar-ba Jul 29, 2021 • edited

Choose a reason for hiding this comment

kcudnik Jul 16, 2021

Choose a reason for hiding this comment

dipankar-ba Jul 29, 2021

Choose a reason for hiding this comment

kcudnik Jul 29, 2021

Choose a reason for hiding this comment

Haidong988 Nov 5, 2022

Choose a reason for hiding this comment

kcudnik commented Aug 16, 2021

rlhui commented Aug 26, 2021

dipankar-ba commented Aug 27, 2021 via email

dipankar-ba commented Sep 2, 2021

kcudnik commented Sep 3, 2021

dipankar-ba commented Nov 7, 2022 via email

Haidong988 commented Nov 8, 2022

dipankar-ba commented Nov 8, 2022 via email

Haidong988 commented Nov 9, 2022

Haidong988 commented Nov 9, 2022

dipankar-ba commented Nov 9, 2022 via email

Haidong988 commented Nov 10, 2022

dipankar-ba commented Nov 11, 2022 via email

Haidong988 commented Nov 15, 2022

dipankar-ba commented Jun 2, 2021 •

edited

dipankar-ba Jul 29, 2021 •

edited