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.

Sign up for GitHub

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

Jump to bottom

Add FSCTv3 Common SBOM Baseline Attributes checker #224

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Add FSCTv3 Common SBOM Baseline Attributes checker #224

wants to merge 26 commits into from

Conversation

bact
Copy link
Contributor

@bact bact commented Nov 25, 2024 •
edited
Loading

Objective

To add the compliance check of the Common SBOM Baseline Attributes, as outlined in "Framing Software Component Transparency, Third Edition" https://www.cisa.gov/resources-tools/resources/framing-software-component-transparency-2024

To resolve #214

What this PR do?

  • Modify SbomChecker class:
    • Move all NTIA-specific functionalities to NTIAChecker
    • Move all common functionalities that can be shared among checkers of different compliance standards to BaseChecker
    • SbomChecker now acts as a "dispatcher" or a factory that returns a subclass of BaseChecker based on the given "compliance" argument during instantiation.
      • If no compliance argument is given, return an instance of NTIAChecker to maintain backward compatibility
    • To the eyes of an existing code that use ntia-compliance-checker package, SbomChecker is expected to behave the same.
  • Add 3 new classes:
    • BaseChecker contains common methods like parse_file
      • It also contains abstract methods like print_components_missing_info and output_json. These methods' details are specific to compliance standards and have to be implemented in subclasses.
      • Another two new methods are added to check components without concluded licenses and without copyright texts (to be used by FSCT3Checker, for example)
    • NTIAChecker, a subclass of BaseChecker
    • FSCT3Checker, a subclass of BaseChecker, contains functionalities specific to Framing Software Component Transparency, Third Edition check
  • Add --comply parameter:
    • Modify main.py to add a new parameter for choosing a compliance standard to check. It has two choices "fsct3-min" and "ntia". Default value is "ntia" to maintain backward compatibility.
    • Need feedback on what is an appropriate and concise name for this parameter.
  • Add sbomcheck command for CLI:
    • Add sbomcheck to the project.scripts section in pyproject.toml
    • This will add sbomcheck to the machine bin/ directory and allows the user to call sbomcheck from command line.
    • Both "ntia-checker" and "sbomcheck" commands are identical.
      • "ntia-checker" is kept for backward compatibility. While "sbomcheck" is introduced for a more generic name, to accommodate other compliance standards.
    • To allow both "ntia-checker" and "sbomcheck" to print an appropriate program name, that matched the name that it got called from the command line, Line 15: prog="ntia-checker", is removed from main.py. This allows the ArgumentParser to determine the actual program name.
  • To the eyes of an existing script that use "ntia-checker" command, "ntia-checker" is expected to behave the same.

Please see the design document https://docs.google.com/document/d/1pueRxlxoM9n1eG9g6AihjLvybEBTd77m22mRYBQltpg/edit?usp=sharing for reference.

Test

  • The new code passed all the existing 42 tests for NTIA minimum elements across Python 3.8 to 3.13.
    • These tests are largely untouched, apart from adding one test of a new property name ".compliant" on top of the existing test sets.
  • The new code also passed all new 20 tests for the new classes: NTIAChecker and FSCT3Checker

bact added 6 commits November 24, 2024 10:01
@bact
Move NTIA check to ntia_checker.py
185fe60 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Add FSCT3Checker class
1d7cc85 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Add sbomcheck script to pyproject.toml
fba1e1e 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Add a generic name self.compliant property
bf412d8 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Put common methods to BaseChecker
26ff58e 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Customise FSCTv3 printout messages
888b3cd 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact bact marked this pull request as draft November 25, 2024 15:54
@bact bact marked this pull request as ready for review November 25, 2024 15:55
@bact
Copy link
Contributor Author

bact commented Nov 25, 2024 •
edited
Loading

@jspeed-meyers @goneall This is still a work in progress, the actual functionality of checking the compliance of CISA Framing Software Component Transparency, Third Edition is not ready but I would like to have your comments about the refactoring, particularly about class/subclassing, naming of parameters and things, and any potential break of backward compatibility.

May be it would be nice as well if I can try if current code pass the CI checks on GitHub. Will put this to draft afterwards.

Thank you.

@bact bact mentioned this pull request Nov 25, 2024
Open
@goneall
Copy link
Member

goneall commented Nov 25, 2024

Thanks @bact

Overall structure LGTM - but my expertise is not Python, so take my feedback with a grain of salt.

I just approved the workflows - so they are running now.

I haven't had a chance to review in detail - I'll try to look in more detail over the next week or so

@bact
Copy link
Contributor Author

bact commented Nov 25, 2024

Thank you Gary. Not need to rush, I still working on this. I will turn this to draft.

@bact bact marked this pull request as draft November 25, 2024 19:40
bact added 2 commits November 25, 2024 19:54
@bact
Fix black and pylint warnings
e2a3639 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
raises a ValueError if compliance is not recognized
88f1d06 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact bact marked this pull request as ready for review November 25, 2024 20:22
@bact bact marked this pull request as draft November 25, 2024 20:22
@bact
Populate members of BaseChecker
ae59cdc 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@jspeed-meyers jspeed-meyers marked this pull request as ready for review November 26, 2024 01:41
@jspeed-meyers jspeed-meyers added the enhancement New feature or request label Nov 26, 2024
@jspeed-meyers
Copy link
Collaborator

This looks great to me. Wow! I like it. I don't have anything to add at the moment. If the current tests are passing, then I'm a happy reviewer.

bact added 6 commits November 26, 2024 13:47
@bact
Add type hintings
7020a0f 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Add get_components_without_concluded_licenses()
080090d 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Merge branch 'spdx:main' into add-fsct3-min-check
47f3dc7
@bact
Add tests for NTIAChecker and FSCT3Checker
12e773e 
Also add a test data for missing concluded license

Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Add get_components_without_copyright_texts
a24aca5 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Fix pylint: unnecessary else
d3e7357 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
bact added 7 commits November 27, 2024 09:18
@bact
Fix black format
8f97c53 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Install spdx-tools==0.8.3 in pylint workflow
c192cb3 
To fix import-error

Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Add pylint: disable=too-many-instance-attributes
e6596c3 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Add pylint: disable=too-many-instance-attributes
cec712d 
We have to keep these for backward compatibility.

Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Add more type hints and checks
a1d5bae 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Fix validation_messages type hint
15fe6c9 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Fix pylint warning too-many-instance-attributes
82fa41d 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@jspeed-meyers
Copy link
Collaborator

@bact: I begin paternity leave later today. If you want a review sometime today, please say something. I notice the build isn't passing, at the time of me writing this message, so I thought it might be best for me to hold off on a review.

@bact
Copy link
Contributor Author

bact commented Nov 27, 2024 •
edited
Loading

@jspeed-meyers I found a bug in my TagValue test data and I think it may take some time for me to figure that out. Trying to work on this as much as possible this afternoon but please don't worry about this. This can be waited.

Please enjoy your leave :)

bact added 3 commits November 27, 2024 14:23
@bact
Fix black formatting
7634a5f 
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
FilesAnalyzed: True -> FilesAnalyzed: true
c82775d 
TagValue is specific about casing of true/false

Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Use Use lazy % formatting in logging functions
575a8ad 
To fix pylint warnings

Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Copy link
Contributor Author

bact commented Nov 27, 2024

@jspeed-meyers I think I have found the bug in my test data. The boolean values in Tag/Value format are case sensitive and have to be exactly "true" or "false" (and not "True", "False", etc). So that make the test of concluded licenses failed at an irrelevant point. Fixed now. All tests are passed.

If you have time today and feel like to review please kindly go ahead. If not, we still have a lot of time after you come back.

Happy new year.

jspeed-meyers
jspeed-meyers approved these changes Nov 27, 2024
View reviewed changes
Copy link
Collaborator

@jspeed-meyers jspeed-meyers left a comment

Choose a reason for hiding this comment

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

I admit to a hasty review. This looks good to me. @goneall: Would you please review too? It might be worthwhile to get one more reviewer to take a look too.

This is a large code and I admit to being overwhelmed while reviewing it, partly because I have too little time to review it properly.

Nice, @bact!

ntia_conformance_checker/__init__.py
@@ -1,3 +1,12 @@
# SPDX-FileCopyrightText: 2024 SPDX contributors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for adding these SPDX file headers!

ntia_conformance_checker/main.py
Comment on lines +23 to +28
parser.add_argument(
"--comply",
choices=["fsct3-min", "ntia"],
default="ntia",
help="Specify which compliance standard to check against",
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
tests/test_checker.py Show resolved Hide resolved
@jspeed-meyers jspeed-meyers added the help wanted Extra attention is needed label Nov 27, 2024
@bact
Copy link
Contributor Author

bact commented Nov 27, 2024 •
edited
Loading

Thank you. Yes, it is a very large change and addition. Maybe it's better to take more time to review.

I guess we can advertise that the addition of FSCTv3 is experimental, not guaranteed its completeness yet, and open for wider review.

So this PR main contribution should be about restructuring the codebase to allow more compliance checkers in the future. And the priority is to keep the behavior of the NTIA minimum checker.

Improvements can be made further to refine the completeness of FSCTv3 checker. And at some point we can advertise that it is supported.

goneall
goneall approved these changes Nov 27, 2024
View reviewed changes
Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

Went through a bit more carefully and did not notice any issues. Overall, LGTM.

@bact bact mentioned this pull request Nov 28, 2024
Open
@bact
Copy link
Contributor Author

bact commented Nov 28, 2024

I'm not particular sure about the use of the word "comply" (as used in the option "--comply"), "compliance", "conformance", "standard", "document", "specification", etc. that I may put inside the printing message/code comment, and also the README (#226), whether they have a specific/different meaning or preference within this project.

Please flag any misuse or inconsistency. Thank you.

@goneall
Copy link
Member

goneall commented Nov 28, 2024

I'm not particular sure about the use of the word "comply" (as used in the option "--comply"), "compliance", "conformance", "standard", "document", "specification", etc. that I may put inside the printing message/code comment, and also the README (#226), whether they have a specific/different meaning or preference within this project.

Please flag any misuse or inconsistency. Thank you.

I'm not the best to comment on the best term - I've been in meetings where participants expressed strong opinions, but I'm personally OK comply and compliance. In SPDX, we settled on "conformance" for the profile compliance term. Based on this one datapoint, conformance may be a less controversial choice. I think that is an OMG term if I recall correctly. I don't know if @kestewart has any feedback from the framing document work in terms of a preferred term.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jspeed-meyers jspeed-meyers jspeed-meyers approved these changes

@goneall goneall goneall approved these changes

Assignees

@bact bact

Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update to new edition of minimum elements (2024)
3 participants
@bact @goneall @jspeed-meyers