Fix SSCI parameter when creating MACsec tunnels on Bookworm (#1372)

* Fix SSCI parameter when creating MACsec tunnels on Bookworm While Bookworm's iproute2 has built-in support for the MACsec XPN protocols, the SSCI argument is passed in in a different way compared to the patch that we had. Instead of taking in a number, a hex string is taken in instead. Update the code in sairedis to pass in a hex string for SSCI instead of a number. Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Don't create a variable, since it won't be needed. Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Add iproute to aspell Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Add more tests for loadMACsecAttrFromMACsecSA The tests are not that great, but they at least make sure it goes through most of the function. Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> --------- Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>
sonic-net · Apr 17, 2024 · 73ada8d · 73ada8d
1 parent c41a0cb
commit 73ada8d
Show file tree

Hide file tree

Showing 5 changed files with 110 additions and 14 deletions.
diff --git a/tests/aspell.en.pws b/tests/aspell.en.pws
@@ -249,6 +249,7 @@ IPG
 ipgs
 IPGs
 ipmc
+iproute
 IPv
 isobjectid
 isoidattribute

diff --git a/unittest/vslib/TestSwitchStateBaseMACsec.cpp b/unittest/vslib/TestSwitchStateBaseMACsec.cpp
@@ -20,8 +20,11 @@ TEST(SwitchStateBase, loadMACsecAttrFromMACsecSA)
     sai_attribute_t attr;
     std::vector<sai_attribute_t> attrs;
     MACsecAttr macsecAttr;
+    sai_attribute_t attrList[6];
+    sai_attribute_t attrListXpn[8];
 
     attr.id = SAI_MACSEC_SC_ATTR_FLOW_ID;
+    attr.value.oid = 0;
     attrs.push_back(attr);
     attr.id = SAI_MACSEC_SC_ATTR_MACSEC_SCI;
     attrs.push_back(attr);
@@ -41,11 +44,101 @@ TEST(SwitchStateBase, loadMACsecAttrFromMACsecSA)
             static_cast<uint32_t>(attrs.size()),
             attrs.data()));
 
-    attr.id = SAI_MACSEC_SA_ATTR_SC_ID;
+    attr.id = SAI_MACSEC_SC_ATTR_FLOW_ID;
     attr.value.oid = 0;
-    ss.loadMACsecAttrFromMACsecSA(0, 1 , &attr, macsecAttr);
+    attrs.push_back(attr);
+    attr.id = SAI_MACSEC_SC_ATTR_MACSEC_SCI;
+    attrs.push_back(attr);
+    attr.id = SAI_MACSEC_SC_ATTR_ENCRYPTION_ENABLE;
+    attrs.push_back(attr);
+    attr.id = SAI_MACSEC_SC_ATTR_MACSEC_CIPHER_SUITE;
+    attr.value.s32 = sai_macsec_cipher_suite_t::SAI_MACSEC_CIPHER_SUITE_GCM_AES_XPN_256;
+    attrs.push_back(attr);
+    attr.id = SAI_MACSEC_SC_ATTR_MACSEC_EXPLICIT_SCI_ENABLE;
+    attrs.push_back(attr);
+    EXPECT_EQ(
+        SAI_STATUS_SUCCESS,
+        ss.create_internal(
+            SAI_OBJECT_TYPE_MACSEC_SC,
+            "oid:0x1",
+            0,
+            static_cast<uint32_t>(attrs.size()),
+            attrs.data()));
+
+    ss.m_macsecFlowPortMap[0] = 0;
+    auto eq = std::make_shared<EventQueue>(std::make_shared<Signal>());
+    int s = socket(AF_INET, SOCK_DGRAM, 0);
+    int fd = socket(AF_INET, SOCK_DGRAM, 0);
+    auto hii = std::make_shared<HostInterfaceInfo>(0, s, fd, "tap", 0, eq);
+    ss.m_hostif_info_map["tap"] = hii;
+
+    memset(attrList, 0, sizeof(attrList));
+    attrList[0].id = SAI_MACSEC_SA_ATTR_SC_ID;
+    attrList[0].value.oid = 0;
+    attrList[1].id = SAI_MACSEC_SA_ATTR_MACSEC_DIRECTION;
+    attrList[1].value.s32 = sai_macsec_direction_t::SAI_MACSEC_DIRECTION_INGRESS;
+    attrList[2].id = SAI_MACSEC_SA_ATTR_AN;
+    attrList[2].value.s32 = 0;
+    attrList[3].id = SAI_MACSEC_SA_ATTR_SAK;
+    attrList[4].id = SAI_MACSEC_SA_ATTR_AUTH_KEY;
+    attrList[5].id = SAI_MACSEC_SA_ATTR_MINIMUM_INGRESS_XPN;
+    EXPECT_EQ(SAI_STATUS_SUCCESS, ss.loadMACsecAttrFromMACsecSA(0, sizeof(attrList)/sizeof(attrList[0]), attrList, macsecAttr));
+
+    EXPECT_EQ(macsecAttr.m_cipher, MACsecAttr::CIPHER_NAME_GCM_AES_128);
+    EXPECT_EQ(macsecAttr.m_direction, sai_macsec_direction_t::SAI_MACSEC_DIRECTION_INGRESS);
+
+    memset(attrList, 0, sizeof(attrList));
+    attrList[0].id = SAI_MACSEC_SA_ATTR_SC_ID;
+    attrList[0].value.oid = 0;
+    attrList[1].id = SAI_MACSEC_SA_ATTR_MACSEC_DIRECTION;
+    attrList[1].value.s32 = sai_macsec_direction_t::SAI_MACSEC_DIRECTION_EGRESS;
+    attrList[2].id = SAI_MACSEC_SA_ATTR_AN;
+    attrList[2].value.s32 = 0;
+    attrList[3].id = SAI_MACSEC_SA_ATTR_SAK;
+    attrList[4].id = SAI_MACSEC_SA_ATTR_AUTH_KEY;
+    attrList[5].id = SAI_MACSEC_SA_ATTR_CONFIGURED_EGRESS_XPN;
+    EXPECT_EQ(SAI_STATUS_SUCCESS, ss.loadMACsecAttrFromMACsecSA(0, sizeof(attrList)/sizeof(attrList[0]), attrList, macsecAttr));
 
     EXPECT_EQ(macsecAttr.m_cipher, MACsecAttr::CIPHER_NAME_GCM_AES_128);
+    EXPECT_EQ(macsecAttr.m_direction, sai_macsec_direction_t::SAI_MACSEC_DIRECTION_EGRESS);
+
+    memset(attrListXpn, 0, sizeof(attrListXpn));
+    attrListXpn[0].id = SAI_MACSEC_SA_ATTR_SC_ID;
+    attrListXpn[0].value.oid = 1;
+    attrListXpn[1].id = SAI_MACSEC_SA_ATTR_MACSEC_DIRECTION;
+    attrListXpn[1].value.s32 = sai_macsec_direction_t::SAI_MACSEC_DIRECTION_INGRESS;
+    attrListXpn[2].id = SAI_MACSEC_SA_ATTR_AN;
+    attrListXpn[2].value.s32 = 0;
+    attrListXpn[3].id = SAI_MACSEC_SA_ATTR_SAK;
+    attrListXpn[4].id = SAI_MACSEC_SA_ATTR_AUTH_KEY;
+    attrListXpn[5].id = SAI_MACSEC_SA_ATTR_MINIMUM_INGRESS_XPN;
+    attrListXpn[6].id = SAI_MACSEC_SA_ATTR_MACSEC_SSCI;
+    attrListXpn[6].value.u32 = 0x23456789;
+    attrListXpn[7].id = SAI_MACSEC_SA_ATTR_SALT;
+    EXPECT_EQ(SAI_STATUS_SUCCESS, ss.loadMACsecAttrFromMACsecSA(0, sizeof(attrListXpn)/sizeof(attrListXpn[0]), attrListXpn, macsecAttr));
+
+    EXPECT_EQ(macsecAttr.m_cipher, MACsecAttr::CIPHER_NAME_GCM_AES_XPN_256);
+    EXPECT_EQ(macsecAttr.m_direction, sai_macsec_direction_t::SAI_MACSEC_DIRECTION_INGRESS);
+    EXPECT_EQ(macsecAttr.m_ssci, "23456789");
+
+    memset(attrListXpn, 0, sizeof(attrListXpn));
+    attrListXpn[0].id = SAI_MACSEC_SA_ATTR_SC_ID;
+    attrListXpn[0].value.oid = 1;
+    attrListXpn[1].id = SAI_MACSEC_SA_ATTR_MACSEC_DIRECTION;
+    attrListXpn[1].value.s32 = sai_macsec_direction_t::SAI_MACSEC_DIRECTION_EGRESS;
+    attrListXpn[2].id = SAI_MACSEC_SA_ATTR_AN;
+    attrListXpn[2].value.s32 = 0;
+    attrListXpn[3].id = SAI_MACSEC_SA_ATTR_SAK;
+    attrListXpn[4].id = SAI_MACSEC_SA_ATTR_AUTH_KEY;
+    attrListXpn[5].id = SAI_MACSEC_SA_ATTR_CONFIGURED_EGRESS_XPN;
+    attrListXpn[6].id = SAI_MACSEC_SA_ATTR_MACSEC_SSCI;
+    attrListXpn[6].value.u32 = 0x23456789;
+    attrListXpn[7].id = SAI_MACSEC_SA_ATTR_SALT;
+    EXPECT_EQ(SAI_STATUS_SUCCESS, ss.loadMACsecAttrFromMACsecSA(0, sizeof(attrListXpn)/sizeof(attrListXpn[0]), attrListXpn, macsecAttr));
+
+    EXPECT_EQ(macsecAttr.m_cipher, MACsecAttr::CIPHER_NAME_GCM_AES_XPN_256);
+    EXPECT_EQ(macsecAttr.m_direction, sai_macsec_direction_t::SAI_MACSEC_DIRECTION_EGRESS);
+    EXPECT_EQ(macsecAttr.m_ssci, "23456789");
 }
 
 TEST(SwitchStateBase, retryCreateIngressMaCsecSAs)

diff --git a/vslib/MACsecAttr.h b/vslib/MACsecAttr.h
@@ -7,10 +7,8 @@
 
 namespace saivs
 {
-    using macsec_sci_t = std::string;
     using macsec_an_t = std::uint16_t;
     using macsec_pn_t = std::uint64_t;
-    using macsec_ssci_t = std::uint32_t;
 
     struct MACsecAttr
     {
@@ -52,11 +50,11 @@ namespace saivs
         std::string m_authKey;
         std::string m_sak;
         std::string m_sci;
+        std::string m_ssci;
         std::string m_salt;
 
         macsec_an_t m_an;
         macsec_pn_t m_pn;
-        macsec_ssci_t m_ssci;
 
         bool m_sendSci;
         bool m_encryptionEnable;

diff --git a/vslib/MACsecManager.cpp b/vslib/MACsecManager.cpp
@@ -450,7 +450,7 @@ bool MACsecManager::create_macsec_egress_sa(
         << ( attr.is_xpn() ? " xpn " : " pn ")
         << attr.m_pn
         << ( attr.is_xpn() ? " ssci " : "" )
-        << ( attr.is_xpn() ? std::to_string(attr.m_ssci) : "" )
+        << ( attr.is_xpn() ? attr.m_ssci : "" )
         << ( attr.is_xpn() ? " salt " : "" )
         << ( attr.is_xpn() ? attr.m_salt : "" )
         << " on key "
@@ -487,7 +487,7 @@ bool MACsecManager::create_macsec_ingress_sa(
         << ( attr.is_xpn() ? " xpn " : " pn " )
         << attr.m_pn
         << ( attr.is_xpn() ? " ssci " : "" )
-        << ( attr.is_xpn() ? std::to_string(attr.m_ssci) : "" )
+        << ( attr.is_xpn() ? attr.m_ssci : "" )
         << ( attr.is_xpn() ? " salt " : "" )
         << ( attr.is_xpn() ? attr.m_salt : "" )
         << " on key "

diff --git a/vslib/SwitchStateBaseMACsec.cpp b/vslib/SwitchStateBaseMACsec.cpp
@@ -13,6 +13,7 @@
 #include <net/if.h>
 #include <arpa/inet.h>
 #include <byteswap.h>
+#include <endian.h>
 
 using namespace saivs;
 
@@ -554,12 +555,7 @@ sai_status_t SwitchStateBase::loadMACsecAttrFromMACsecSC(
     std::stringstream sciHexStr;
 
     sciHexStr << std::setw(MACSEC_SCI_LENGTH) << std::setfill('0');
-
-#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
-    sciHexStr << std::hex << bswap_64(sci);
-#else
-    sciHexStr << std::hex << sci;
-#endif
+    sciHexStr << std::hex << htobe64(sci);
 
     macsecAttr.m_sci = sciHexStr.str();
 
@@ -708,7 +704,15 @@ sai_status_t SwitchStateBase::loadMACsecAttrFromMACsecSA(
 
         // The Linux kernel directly uses ssci to XOR with the salt that is network order,
         // So, this conversion is useful to convert SSCI from the host order to network order.
-        macsecAttr.m_ssci = htonl(attr->value.u32);
+        //
+        // Starting with Debian Bookworm (iproute2 6.1), ssci is interpreted as a hex string,
+        // so this needs to convert the ssci integer to a hex string, and doesn't need to change
+        // the encoding at this point.
+        std::stringstream ssciHexStr;
+
+        ssciHexStr << std::hex << attr->value.u32;
+
+        macsecAttr.m_ssci = ssciHexStr.str();
 
         SAI_METADATA_GET_ATTR_BY_ID(attr, SAI_MACSEC_SA_ATTR_SALT, attrCount, attrList);
-Original file line number
+Diff line change
@@ Expand Up / @@ -249,6 +249,7 @@ IPG @@
     ipgs
     IPGs
     ipmc
+    iproute
     IPv
     isobjectid
     isoidattribute
@@ Expand Down @@