Skip to content

MACSec and IPSec FIPS Compliance #2162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from
Closed

Conversation

JaiOCP
Copy link
Contributor

@JaiOCP JaiOCP commented Apr 7, 2025

This PR brings support for running POST on an IPSec and MACSec engine mainly for FIPS compliance

inc/saimacsec.h Outdated
Comment on lines 75 to 85
/** Unknown */
SAI_MACSEC_POST_STATUS_UNKNOWN,

/** Pass */
SAI_MACSEC_POST_STATUS_PASS,

/** In Progress */
SAI_MACSEC_POST_STATUS_IN_PROGRESS,

/** Fail */
SAI_MACSEC_POST_STATUS_FAIL,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary comments, not introducing any meaningful information, please remove

FIPS-103 compliance requires that POST is executed on each port and only if POST completes with ‘success’, port must be enabled for admitting traffic, else port will remain in down state.

There are two variations of MACSec/IPSec engine and ports
1. Each MACSec/IPSec emgine serving ‘n’ number of ports (1::n)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo "engine"

typedef enum _sai_switch_attr_t
{
/**
* @brief Callback for completion status of all the MACSEC ports serviced by this MACSEC engine

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the notification callback gets registered at the sai switch level or at the macsec engine level

@skeesara-nokia
Copy link

Can we consider support for running POST - where it can be requested at the switch level - preferably also be able to request it as part of switch_create.

The most common use case for systems with FIPS is - they are running in a mode where FIPS is enabled or not enabled. It isn't typical for FIPS compliance to be required on some MACSEC ports and not on others. For this, the application implementation is simplified significantly, if it could request POST at initialization time and proceed on the knowledge of whether that was successful for all MACSEC engines in hardware or not.

If an implementation does not support requesting POST at this global/swich level - it would indicate that the attribute is not supported. In that case the application will need to implement code to request it whenever it creates individual MACSEC ports as described in this spec. That can be a more sophisticated application implementation that does not have be done right away.

@kcudnik
Copy link
Collaborator

kcudnik commented Apr 11, 2025

please resolve conflicts

Signed-off-by: JaiOCP <[email protected]>
#### 2.1.1 MACSec Engine
Each MACSec engine is represented by a MACSec SAI object id. Single MACSec object id can be serving one or more MACSec ports.

New attribute is introduced to trigger POST at the MACSec engine granularity.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically the entire system is FIPS certified and it requires POST before MACsec or IPsec is enabled. AFAIK, existing systems often use a simple implementation which invokes POST for all security engines in the system at the time of system initialization. For SAI, that would requires only static boolean controls (CREATE_ONLY attributes) SAI_SWITCH_ATTR_MACSEC_POST_ENABLED and SAI_SWITCH_ATTR_IPSEC_POST_ENABLED. With that approach, each ASIC vendor decides when to run POST within their system initialization sequence and removes the responsibility of correctly triggering POST from the NOS. Can we take this approach and remove all SAI attributes that trigger POST (SAI_IPSEC_ATTR_ENABLE_POST, SAI_MACSEC_ATTR_ENABLE_POST, SAI_SWITCH_ATTR_IPSEC_ENABLE_POST, SAI_SWITCH_ATTR_MACSEC_ENABLE_POST)?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can work as long as there is no assumption that POST completes synchronously. We cannot guarantee that every implementation is able to implement a synchronous POST. We need to retain the notification API that the NOS can hook to, so it can act when POST completes.

Can you please confirm you agree with this @dipankar-nexthop ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree POST is likely not going to complete synchronously and the notification API would be useful.

inc/saiswitch.h Outdated
* @type bool
* @flags READ_ONLY
*/
SAI_SWITCH_ATTR_MACSEC_FIPS_COMPLIANT,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove FIPS_COMPLIANT attribute since query can be done ENABLE_POST to find out if switch support MACSEC and/or IPSEC POST

* @flags CREATE_AND_SET
* @default false
*/
SAI_SWITCH_ATTR_IPSEC_ENABLE_POST,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the flags CREATE_ONLY so as to suppress runtime change of this attribute

@tjchadaga tjchadaga added the reviewed PR is discussed in SAI Meeting label Apr 17, 2025

FIPS-103 compliance requires that POST is executed on each MACSec/IPSec port and only if POST completes with ‘success’, the MACSec/IPSec port must be enabled for admitting traffic.

There are two variations of MACSec/IPSec engine and ports

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After 4/17/25 SAI meeting discussion, I went back and reexamined the definition of SAI_MACsec and SAI_IPsec objects. As per https://github.com/opencomputeproject/SAI/blob/master/doc/macsec-gearbox/SAI_MACsec_API_Proposal-v1.4.docx, each SAI ASIC is is expected to have only 2 instances of SAI_MACSEC - one for Rx (decryption) and another for Tx (encryption). The SAI IPsec documentation is less prescriptive, but AFAIR was based on the same principle. All MACsec/IPsec enabled ports of a SAI_SWITCH are associated to one instance of such objects. That is why there is no attribute for object count or associativity with ports.

Unless there is some important POST feature loss, the simplest would be to adopt this definiton and not increase the number of SAI_MACsec and SAI_IPsec objects. Otherwise we have to make signficant changes to ensure:

  1. Compatibility with existing SAI drivers
  2. Introduce the concept of multiple Security Engine per SAI_SWITCH and allow SAI_MACSEC and SAI_IPSEC instances per Security Engine
  3. Explain additional POST features enabled by allowing multiple Security Engines.
  4. Expose Security Engine count and port binding (typically each port is bound to one Security Engine)

@kcudnik
Copy link
Collaborator

kcudnik commented Apr 22, 2025

please resolve conflicts

sai_status_t status = SAI_STATUS_SUCCESS;
sai_attr_capability_t post_capability={0};

// Query if MACSec POST is supported at Switch level

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some concern here about how to trigger POST. As my understanding I need to take SAI_SWITCH_ATTR_MACSEC_ENABLE_POST as one of the attribute and then finally trigger sai_switch_api->create_switch() to create switch. the gSwitchId is the output of create_switch(). Before I pass in the SAI_SWITCH_ATTR_MACSEC_ENABLE_POST I need to check if the switch support POST or not. Which the query needs gSwitchId.
SAI_SWITCH_ATTR_MACSEC_ENABLE_POST will be CREATE_ONLY based on the comments below. Wondering how would I do the query before switch_create().

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could unconditionally allow setting SAI_SWITCH_ATTR_MACSEC_ENABLE_POST and the switch could return a POST_NOT_SUPPORTED status?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work for us. In this case, also need to support SAI_SWITCH_ATTR_MACSEC(IPSEC)_POST_STATUS_NOTIFY etc as we will pass those attributes in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to handle the case where FIPS module is present and not present. Running POST always will be not honoring the configuration. POST should run based on the NOS configuration.

* @default false
*/
SAI_MACSEC_ATTR_ENABLE_POST,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MACSec and IPSec object level POST enable is mainly for diagnostics purposes.
Add comments in documentation and attribute clarifying this.

@kcudnik
Copy link
Collaborator

kcudnik commented Apr 25, 2025

please resolve conflicts

FIPS-103 compliance requires that POST is executed on each MACSec/IPSec port and only if POST completes with ‘success’, the MACSec/IPSec port must be enabled for admitting traffic.
Note that this specification will provide POST at the MACSec/IPSec single logical instance level or at the switch level.

As shown in Fig-1, there are two physical security engines and are represented by single logical instance in SAI. Enabling of POST happens at the logical intance level and in turn will trigger POST for all the physical instances present in a given hardware.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this confusing. There are 2 SAI_MACSEC objects (for ingress & egress) for all security engines. And there can be up to 2 SAI_IPSEC objects (for ingress & egress) for each security engine. Perhaps deleting this paragraph is best.

Following sections describes the attributes for MACSec engine

#### 2.1.1 MACSec Engine
All physical MACSec engines are represented by a single MACSec SAI object id. Single SAI MACSec object id can be serving one or more physical MACSec engines and ports.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, all physical MACsec engines, together, are represented by 2 SAI_MACSEC objects(for ingress & egress).

SAI_OBJECT_TYPE_MACSEC and SAI_OBJECT_TYPE_ IPSEC already exists
SAI_MACSEC_ATTR_SUPPORTED_PORT_LIST and SAI_IPSEC_ATTR_SUPPORTED_PORT_LIST provides the list of MACSec/IPSec ports being service by an instance of MACSEC and IPSEC objects respectively. Each MACSec/IPSec port has a correspoding binding to the physical port SAI_MACSEC_PORT_ATTR_PORT_ID.
```
SAI_OBJECT_TYPE_SWITCH -> SAI_SWITCH_ATTR_MACSEC_OBJECT_LIST[x] -> SAI_MACSEC_ATTR_SUPPORTED_PORT_LIST[] -> SAI_MACSEC_PORT_ATTR_PORT_ID

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the object relationship has not been changed, perhaps there is no need to mention this as enhancement.
While this description is correct, the reader misses one complexity - each of the 2 elements (for ingress & egress) of the MACSEC_OBJECT_LIST have SUPPORTED_PORT_LIST with identical membership.

```
SAI_OBJECT_TYPE_SWITCH -> SAI_SWITCH_ATTR_MACSEC_OBJECT_LIST[x] -> SAI_MACSEC_ATTR_SUPPORTED_PORT_LIST[] -> SAI_MACSEC_PORT_ATTR_PORT_ID

SAI_OBJECT_TYPE_SWITCH -> SAI_SWITCH_ATTR_IPSEC_OBJECT_LIST[x] -> SAI_IPSEC_ATTR_SUPPORTED_PORT_LIST[] -> SAI_IPSEC_PORT_ATTR_PORT_ID

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment as for MACSEC. However, IPsec can be a little more complex since it allows N security engines to serve N distinct subsets of ports. In that case, there would be be 2*N elements (for ingress and egress) of IPSEC_OBJECT_LIST.


#### 2.1.2 MACSec Port
New READ only attribute SAI_MACSEC_PORT_ATTR_POST_STATUS is introduced to read the status of POST for a given MACSec port.
This attribute can be read after the POST is completed for the MACSec object hosting this port.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should point out that this provides POST status separately for ingress and egress and both have to pass POST for a port to be usable for MACsec.

read(macsec_port_obj->SAI_MACSEC_PORT_ATTR_POST_STATUS)
```
**Step 5: **
Set the POST enable flag in the macsec object to false. This is mainly for the hw where POST can be triggered runtime after the initialization.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot be done for a CREATE_ONLY attribute.

@@ -3316,6 +3344,86 @@ typedef enum _sai_switch_attr_t
*/
SAI_SWITCH_ATTR_PACKET_TRIM_DSCP_RESOLUTION_MODE,

/**
* @brief Callback for completion status of all the MACSEC ports serviced by this MACSEC engine

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be "all MACsec capble ports of the switch", not related to a specific MACsec engine.

SAI_SWITCH_ATTR_MACSEC_POST_STATUS_NOTIFY,

/**
* @brief Callback for completion status of all the IPSEC ports serviced by this IPSEC engine

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be "all IPsec capble ports of the switch", not related to a specific IPsec engine.


If the engine is servicing a single port then this callback essentially becomes a per port callback.

If the engine is servicing multiple ports then each port needs to be queried for its POST status using the READ only attribute of the port.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this unconditional. A single instance of SAI_MACSEC object is expected to support all MACsec capable ports of a an SAI_SWITCH.

#### 2.1.3 POST Completion Callback
Single aggregate callback function is provided to return the status of POST status for the entire MACSec engine.

If the engine is servicing a single port then this callback essentially becomes a per port callback.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. See below.

@JaiOCP
Copy link
Contributor Author

JaiOCP commented May 7, 2025

#2167 new PR

@JaiOCP JaiOCP closed this May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewed PR is discussed in SAI Meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.