-
Notifications
You must be signed in to change notification settings - Fork 17
MACsec/ MKA config and metric support in OTG model #408
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
base: master
Are you sure you want to change the base?
Conversation
…act on test. Also change some description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review.
device/macsec/cryptoengine.yaml
Outdated
Fixed Tx XPN. 8 bytes PN with which all packets will be sent out. | ||
type: integer | ||
minimum: 1 | ||
default: 0x0000000000000006 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this just be 6 . Is pn and xpn both always needed and set ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, numerically 6 is default - but it should be input in HEX format for XPN because it is easy to think 64 bytes in hex.
Let me try with:
type: string
format: hex
minLength: 1
maxLength: 8
minimum: 1
default: "0x0000000000000006"
More details:
Either PN or XPN is used at a time depending on the cipher dynamically selected by MKA protocol. So DUT is a factor here. However for static key, test port knows it.
In almost all cases, MKA will be used. Chip vendors use static key to test their hardware - there MKA may not be available and static key is used.
Some more details:
PN - 4 bytes packet number used to determine initialization vector (IV) for basic ciphers (A).
XPN - 8 bytes packet numbed used for the same for extended packet number (XPN) based ciphers (B).
Now for both the cipher types (either set from from static key or MKA protocol), 4 bytes go in MACsec header in packet. For XPN ciphers, the most significant 4 bytes are tracked encryption and decryption end points.
device/macsec/cryptoengine.yaml
Outdated
first_xpn: | ||
description: >- | ||
The first XPN. | ||
type: integer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you wish this to be uint64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uint64 is a format of type integer. But that won't let XPN be input as HEX string - this is the requirement.
device/macsec/cryptoengine.yaml
Outdated
x-field-uid: 2 | ||
Macsec.CryptoEngine.StatefulEncryptionDecryption.HardwareAcceleration.InlineCrypto: | ||
description: >- | ||
Inline cryto engine settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crypto ( or cryptographic ? )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost all documentations online write as "crypto engine" E.g. DPDK also uses the same name here: https://core.dpdk.org/supported/cryptos/
description: |- | ||
Key start time in HH:MM. | ||
type: string | ||
default: "00:00" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think much better to have a class with hours (0-23 ? ) and minutes ( 0-59) as specific fields . Dont want user to have to create correctly formatted strings in input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User can input any value e.g. 27:80 (I verified this from IxNetwork). This time is relative to test start time i.e. port assignment time as per clock time known from chassis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this requires user to form the string. This is not type checked as well by model. So not preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of now MKA in snappi-ixnetwork can be modified to validate "HH:MM" of this MKA field. However I do not know how to add a new format e.g. "time_hh_mm" in model.
format: ipv4 - is an example but I could not find where it is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not asking to add a ne format .. I am asking to allow user to provide as:
start_time.hour = 0 to 23
start_time.minute = 0 to 59
instead of having to create a type unchecked string like 01:59
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to review, however found inadequate information of the fields. Requested to look at any recent protocol say OSPFv2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have responded to review comments. Also pushed commits wherever rework was necessary.
Please take a look and resolve remaining open conversations/ comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some breaking changes must be fixed. Some need a discussion with hopefully few people present who can take a call on Macsec/MKA modelling to get a conclusion.
description: |- | ||
Key start time in HH:MM. | ||
type: string | ||
default: "00:00" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this requires user to form the string. This is not type checked as well by model. So not preferable.
@@ -123,8 +133,12 @@ components: | |||
x-field-uid: 13 | |||
ospfv2_metrics: | |||
x-field-uid: 14 | |||
convergence_metrics: | |||
macsec_metrics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking change. Need to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to keep convergence ( older field) as is and add your new stuff at the end with higher ids . Else the break is still there .
12 bytes salt used in case of XPN cipher suites. | ||
type: string | ||
format: hex | ||
minLength: 12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make it 6 bytes and 12 characters. Not what you want from description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. salt "000000000000000000000001" comprises of 12 bytes - each byte is a two digit hex number "0x00".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a preferred description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not about description . Since maxLength is 12, user can enter max 6 bytes = 6x2 = 12 characters . not 12 bytes = 12x2 = needed 24 as max.
And probably min if you want to enforce user needs to always enter the whole thing, though min probably can be 0 or 1 also if just '6' is okay as input , not sure why you want user to enter as 0000000000000...6 instead of just 6
x-constraint: | ||
- '/components/schemas/Device.Ethernet/properties/name' | ||
x-field-uid: 1 | ||
secy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to discuss this with some you and some other MacSec stakeholders if possible. Need to also discuss if external ( to Keysight ) is planned for this model or has been done. Ideally prefer validations to be built in into model where possible not allowing users to make mistakes at compile time itself. Not sure on this. Need to also see what are the number of common parameters you describe.
12 bytes salt used in case of XPN cipher suites. | ||
type: string | ||
format: hex | ||
minLength: 12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not about description . Since maxLength is 12, user can enter max 6 bytes = 6x2 = 12 characters . not 12 bytes = 12x2 = needed 24 as max.
And probably min if you want to enforce user needs to always enter the whole thing, though min probably can be 0 or 1 also if just '6' is okay as input , not sure why you want user to enter as 0000000000000...6 instead of just 6
description: |- | ||
Key start time in HH:MM. | ||
type: string | ||
default: "00:00" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not asking to add a ne format .. I am asking to allow user to provide as:
start_time.hour = 0 to 23
start_time.minute = 0 to 59
instead of having to create a type unchecked string like 01:59
@@ -123,8 +133,12 @@ components: | |||
x-field-uid: 13 | |||
ospfv2_metrics: | |||
x-field-uid: 14 | |||
convergence_metrics: | |||
macsec_metrics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to keep convergence ( older field) as is and add your new stuff at the end with higher ids . Else the break is still there .
…ments from PR #408 (on macsec branch) (#412) * MACsec OTG model reworked based on review of model from macsec branch * Update auto generated content * More rework based on review comments * Add missing file * Update auto generated content * Fix secure channels * Update auto generated content * Fix secure channels * Update auto generated content * Fix secure channels * Update auto generated content * Fix secure channels * Update auto generated content * Correct min and max length of hex fields * Update auto generated content * Update key time descriptions * Update auto generated content * Add MACsec and MKA metrics * Update auto generated content * More rework based on review * Update auto generated content * Split time offset and key chain start time into subfields * Update auto generated content * Fix time fields * Update auto generated content * Fix time fields * Update auto generated content * Add integer format to time subfields * Change class name from Macsec to SecureEntity to match field name secure_entity * Update auto generated content * Change description of psk_chain_start_time * Update auto generated content * Try to set psk chain start time description from the field description itself * Update auto generated content * Move re-shared key(PSK) chain start time description * Update auto generated content * Add lifetime validity information * Update auto generated content * add required fields * Update auto generated content * Minutes field max limit set to 59 * Remove encrypt_decrypt engine type from the model as of now as it is not implemented/ tested yet. * Update auto generated content * Some change in description to reflect previus change in redocly view * Update auto generated content --------- Co-authored-by: Github Actions Bot <[email protected]>
Issue #407
Redocly View:
config:
https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/open-traffic-generator/models/macsec/artifacts/openapi.yaml&nocors#tag/Configuration/operation/set_config
metric:
https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/open-traffic-generator/models/macsec/artifacts/openapi.yaml&nocors#tag/Monitor/operation/get_metrics
Objectives:
Add MACsec and MKA model in OTG.
New fields for config:
devices: macsec
devices: mka
New fields for metric:
get_metrics: macsec
get_metrics: mka
Sample Code Snippet (MACsec with MKA):
import pytest
import time
def test_stateless_encryption_with_mka(api, b2b_raw_config, utils):
"""
Test for the macsec configuration
"""
config = b2b_raw_config
api.set_config(api.config())
config.flows.clear()
#ixnetwork = api._ixnetwork
def results_macsec_ok(api):
#req = api.metrics_request()
#req.macsec.column_names = ["session_state"]
#results = api.get_metrics(req)
ok = []
#for r in results.macsec_metrics:
# ok.append(r.session_state == "up")
return all(ok)
def results_mka_ok(api):
req = api.metrics_request()
req.mka.column_names = ["session_state"]
results = api.get_metrics(req)
ok = []
for r in results.mka_metrics:
ok.append(r.session_state == "up")
return all(ok)
if name == "main":
pytest.main(["-vv", "-s", file])
Sample Code Snippet (MACsec with static key):
def test_stateless_encryption(api, b2b_raw_config, utils):
"""
Test for the macsec configuration
"""
config = b2b_raw_config
api.set_config(api.config())
config.flows.clear()
#ixnetwork = api._ixnetwork
def results_ok(api):
#req = api.metrics_request()
#req.macsec.column_names = ["session_state"]
#results = api.get_metrics(req)
ok = []
#for r in results.macsec_metrics:
# ok.append(r.session_state == "up")
return all(ok)
if name == "main":
pytest.main(["-vv", "-s", file])