Skip to content
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

feat: add cyclonedx.model.crypto.ProtocolProperties.crypto_refs #767

Merged
merged 5 commits into from
Feb 12, 2025

Conversation

indiVar0508
Copy link
Contributor

@indiVar0508 indiVar0508 commented Jan 30, 2025

applied the fix recommended in the thread.
added testcase and BOM json from the mentioned issue.

fixes #692

@indiVar0508 indiVar0508 requested a review from a team as a code owner January 30, 2025 17:41
@jkowalleck jkowalleck changed the title fix(#692): add crypto-ref-array to ProtocolProperties feat: crypto-ref-array to ProtocolProperties Jan 30, 2025
@jkowalleck jkowalleck changed the title feat: crypto-ref-array to ProtocolProperties feat: add cyclonedx.model.crypto.ProtocolProperties.crypto_ref_array Jan 30, 2025
@jkowalleck jkowalleck added enhancement New feature or request schema 1.6 labels Jan 30, 2025
@jkowalleck
Copy link
Member

jkowalleck commented Jan 30, 2025

Thanks for donating this feature.

The provided tests - they need rework. please remove the ones you've added.

Instead, add a new data model to https://github.com/CycloneDX/cyclonedx-python-lib/blob/main/tests/_data/models.py

  • craft a new function that starts with get_bom_. it must return a Bom
  • in case your Bom has incomplete dep tree, add the function to all_get_bom_funct_with_incomplete_deps

after that, the tests should pick up the new method automatically, and run a serialization-validation-deserialization roundtrip with it and create snapshots.
see https://github.com/CycloneDX/cyclonedx-python-lib/blob/main/tests/_data/snapshots/README.md in addition to https://github.com/CycloneDX/cyclonedx-python-lib/blob/main/CONTRIBUTING.md

@indiVar0508
Copy link
Contributor Author

indiVar0508 commented Jan 31, 2025

I'll clean up the commit message and squash as well with signoff

I needed some direction/feedback on how to resolve issues i am facing in tests (if i added them correctly) in 2be5db0, commit message has been documented with issues i faced

@jkowalleck
Copy link
Member

I'll clean up the commit message and squash as well with signoff

That's fantastic. Somehow, the sign-off was not quite right.
The DCO checker has something to complain about: https://github.com/CycloneDX/cyclonedx-python-lib/pull/767/checks?check_run_id=36476165496
It also has some instructions how to fix this.

@jkowalleck
Copy link
Member

jkowalleck commented Jan 31, 2025

I needed some direction/feedback on how to resolve issues i am facing in tests (if i added them correctly) in 2be5db0, commit message has been documented with issues i faced

It looks like you are facing the situation, that your data model's properties are not properly serializing to a certain version of CycloneDX.
You might want to look at these examples:

  • @property
    @serializable.view(SchemaVersion1Dot2)
    @serializable.view(SchemaVersion1Dot3)
    @serializable.view(SchemaVersion1Dot4)
    @serializable.view(SchemaVersion1Dot5)
    @serializable.view(SchemaVersion1Dot6)
    @serializable.xml_sequence(6)
    def manufacture(self) -> Optional[OrganizationalEntity]:

in case you still run into issue, for example a enum value that is available in one vetrsion, but not in the pother, let me know. it basically comes down to something like this

class _ComponentTypeSerializationHelper(serializable.helpers.BaseHelper):
""" THIS CLASS IS NON-PUBLIC API """
__CASES: Dict[Type[serializable.ViewType], FrozenSet[ComponentType]] = dict()
__CASES[SchemaVersion1Dot0] = frozenset({
ComponentType.APPLICATION,
ComponentType.DEVICE,
ComponentType.FRAMEWORK,
ComponentType.LIBRARY,
ComponentType.OPERATING_SYSTEM,
})
__CASES[SchemaVersion1Dot1] = __CASES[SchemaVersion1Dot0] | {
ComponentType.FILE,
}
__CASES[SchemaVersion1Dot2] = __CASES[SchemaVersion1Dot1] | {
ComponentType.CONTAINER,
ComponentType.FIRMWARE,
}
__CASES[SchemaVersion1Dot3] = __CASES[SchemaVersion1Dot2]
__CASES[SchemaVersion1Dot4] = __CASES[SchemaVersion1Dot3]
__CASES[SchemaVersion1Dot5] = __CASES[SchemaVersion1Dot4] | {
ComponentType.DATA,
ComponentType.DEVICE_DRIVER,
ComponentType.MACHINE_LEARNING_MODEL,
ComponentType.PLATFORM,
}
__CASES[SchemaVersion1Dot6] = __CASES[SchemaVersion1Dot5] | {
ComponentType.CRYPTOGRAPHIC_ASSET,
}
@classmethod
def __normalize(cls, ct: ComponentType, view: Type[serializable.ViewType]) -> Optional[str]:
if ct in cls.__CASES.get(view, ()):
return ct.value
raise SerializationOfUnsupportedComponentTypeException(f'unsupported {ct!r} for view {view!r}')
@classmethod
def json_normalize(cls, o: Any, *,
view: Optional[Type[serializable.ViewType]],
**__: Any) -> Optional[str]:
assert view is not None
return cls.__normalize(o, view)
@classmethod
def xml_normalize(cls, o: Any, *,
view: Optional[Type[serializable.ViewType]],
**__: Any) -> Optional[str]:
assert view is not None
return cls.__normalize(o, view)
@classmethod
def deserialize(cls, o: Any) -> ComponentType:
return ComponentType(o)

@indiVar0508
Copy link
Contributor Author

I saw thos but couldn't figure out the fix :')
for my first issue
FileNotFoundError: [Errno 2] No such file or directory: '~/cyclonedx-python-lib/tests/_data/snapshots/get_bom_for_issue_692_components-1.6.json.bin'
I am not sure how to fix these

  • ERROR: test_prepared_06_get_bom_for_issue_692_components (tests.test_deserialize_json.TestDeserializeJson)
  • ERROR: test_prepared_06_get_bom_for_issue_692_components (tests.test_deserialize_xml.TestDeserializeXml)

for second issue the test failure seems to valid given the dictionary mapping currently

     __CASES: Dict[Type[serializable.ViewType], FrozenSet[ComponentType]] = dict() 
     __CASES[SchemaVersion1Dot0] = frozenset({ 
         ComponentType.APPLICATION, 
         ComponentType.DEVICE, 
         ComponentType.FRAMEWORK, 
         ComponentType.LIBRARY, 
         ComponentType.OPERATING_SYSTEM, 
     }) 
     __CASES[SchemaVersion1Dot1] = __CASES[SchemaVersion1Dot0] | { 
         ComponentType.FILE, 
     } 
     __CASES[SchemaVersion1Dot2] = __CASES[SchemaVersion1Dot1] | { 
         ComponentType.CONTAINER, 
         ComponentType.FIRMWARE, 
     } 
     __CASES[SchemaVersion1Dot3] = __CASES[SchemaVersion1Dot2] 
     __CASES[SchemaVersion1Dot4] = __CASES[SchemaVersion1Dot3] 
     __CASES[SchemaVersion1Dot5] = __CASES[SchemaVersion1Dot4] | { 
         ComponentType.DATA, 
         ComponentType.DEVICE_DRIVER, 
         ComponentType.MACHINE_LEARNING_MODEL, 
         ComponentType.PLATFORM, 
     } 
     __CASES[SchemaVersion1Dot6] = __CASES[SchemaVersion1Dot5] | { 
         ComponentType.CRYPTOGRAPHIC_ASSET, 
     } 

so cryptographi-asset is not supported for any version < v1.6, so how to do i restrict tests not be generated from all v1.6?

e.g. error
ERROR: test_valid_028_get_bom_for_issue_692_components_1_4 (tests.test_output_json.TestOutputJson)
ERROR: test_valid_038_get_bom_for_issue_692_components_1_4 (tests.test_output_xml.TestOutputXml
..
..
ERROR: test_valid_042_get_bom_for_issue_692_components_1_0 (tests.test_output_xml.TestOutputXml)

err log : cyclonedx.exception.serialization.SerializationOfUnsupportedComponentTypeException: unsupported <ComponentType.CRYPTOGRAPHIC_ASSET: 'cryptographic-asset'> for view <class 'cyclonedx.schema.schema.SchemaVersion1Dot5'>

@jkowalleck
Copy link
Member

Will take a look at it sometime next week.
And if i don't come back in time, please ping me :-)

@indiVar0508 indiVar0508 force-pushed the fix/crypto-ref-array branch 2 times, most recently from b85dd98 to 3583283 Compare February 1, 2025 07:02
@indiVar0508
Copy link
Contributor Author

I have squashed and rebased the PR with latest upstream main

@indiVar0508 indiVar0508 marked this pull request as draft February 1, 2025 11:57
@jkowalleck jkowalleck marked this pull request as ready for review February 6, 2025 07:45
@jkowalleck
Copy link
Member

jkowalleck commented Feb 6, 2025

@indiVar0508 , could you add the test snapshots?

see here for instructions: https://github.com/CycloneDX/cyclonedx-python-lib/blob/main/tests/_data/snapshots/README.md

applied the fix recommended in the thread, to add crypto_ref_array
as an argument to ProtocolProperties class
Also updated bom1.6.SNAPSHOT.xsd from cryptoRef -> cryptoRefArray
added testcase and BOM json from the mentioned issue.

Signed-off-by: Indivar Mishra <indimishra@gmail.com>
resolve review comments

Signed-off-by: Indivar Mishra <indimishra@gmail.com>
so use applciation instead that is supported from v1.0 - v1.6

Signed-off-by: Indivar Mishra <indimishra@gmail.com>
@indiVar0508
Copy link
Contributor Author

@indiVar0508 , could you add the test snapshots?

see here for instructions: https://github.com/CycloneDX/cyclonedx-python-lib/blob/main/tests/_data/snapshots/README.md

Hi @jkowalleck ,

image

v1.6 one seems to be failing saying file doesn't exist but file is created as seen in file diff, not sure why it is failing

@jkowalleck
Copy link
Member

@indiVar0508 , could you add the test snapshots?
see here for instructions: https://github.com/CycloneDX/cyclonedx-python-lib/blob/main/tests/_data/snapshots/README.md

Hi @jkowalleck ,

image

v1.6 one seems to be failing saying file doesn't exist but file is created as seen in file diff, not sure why it is failing

of cause new snapshots do not exist for comparison when they are not provided, so some tests might fail when run the first time.
then run the second time, they are provided and comparrison should work.
please provide the needed snapshots.

@indiVar0508
Copy link
Contributor Author

indiVar0508 commented Feb 10, 2025

image

image

----------------------------------------------------------------------
Ran 5257 tests in 7.352s

FAILED (failures=2)
py: exit 1 (8.33 seconds) /home/indivar/github/cyclonedx-python-lib> poetry run coverage run --source=cyclonedx -m unittest discover -t . -s tests -v pid=2814
  py: FAIL code 1 (9.67=setup[0.03]+cmd[0.00,0.73,0.58,8.33] seconds)
  evaluation failed :( (9.72 seconds)

one of the value seems to be come None, for same two tests

test failed names

  • FAIL: test_prepared_06_get_bom_for_issue_692_components (tests.test_deserialize_xml.TestDeserializeXml)
  • FAIL: test_prepared_06_get_bom_for_issue_692_components (tests.test_deserialize_json.TestDeserializeJson)

@jkowalleck
Copy link
Member

jkowalleck commented Feb 11, 2025

one of the value seems to be come None, for same two tests

try to set a bom-ref for comp_root.

Signed-off-by: Indivar Mishra <indimishra@gmail.com>
@indiVar0508
Copy link
Contributor Author

one of the value seems to be come None, for same two tests

try to set a bom-ref for comp_root.

Thanks for guiding me, yeah this worked
image
image

Although want to mention two things

  1. running first time without pre-created snapshot files 1.6 one fails but re-running it passes
  2. 41d807b commit changes component_type from cryptographic-asset (as mentioned in MRC in issue) to application as it is valid from all v1.0 - v1.6, because of _ComponentTypeSerializationHelper mapping i think you also mentioned it in above thread here

Please let me know if any other change is required ✨

@jkowalleck
Copy link
Member

running first time without pre-created snapshot files 1.6 one fails but re-running it passes

could you commit the new test snapshot files?

Signed-off-by: Indivar Mishra <indimishra@gmail.com>
@jkowalleck jkowalleck changed the title feat: add cyclonedx.model.crypto.ProtocolProperties.crypto_ref_array feat: add cyclonedx.model.crypto.ProtocolProperties.crypto_refs Feb 12, 2025
@jkowalleck jkowalleck merged commit beb35f5 into CycloneDX:main Feb 12, 2025
47 checks passed
jkowalleck added a commit that referenced this pull request Feb 12, 2025
followup of #767

Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
@jkowalleck
Copy link
Member

was released via https://github.com/CycloneDX/cyclonedx-python-lib/releases/tag/v8.8.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request schema 1.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add cyclonedx.model.crypto.ProtocolProperties.crypto_ref_array
2 participants