feat: support OSCAL 1.1.2

[degenaro]

Types of changes

• Hot fix (emergency fix and release)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Documentation (change which affects the documentation site)
• Breaking change (fix or feature that would cause existing functionality to change)
• Release (develop -> main)

Quality assurance (all should be covered).

• My code follows the code style of this project.
• Documentation for my change is up to date?
• My PR meets testing requirements.
• All new and existing tests passed.
• All commits are signed-off.

Summary

Key links:

• Sonar coverage

Before you merge

• Ensure it is a 'squash commit' if not a release.
• Ensure CI is currently passing
• Check sonar. If you are working for a fork a maintainer will reach out, if required.

[degenaro]

Trestle upgrade to OSCAL v1.1.2 - STATUS.pdf

[jpower432]

Leaving general feedback/questions, nothing major. I see that the semantic-release issue is still a discussion point based on the attached document. Looks like a great improvement on the class naming under trestle/oscal.

[jpower432]

Is there an alternative plan for installation of semantic-release? I don't see any changes to the make release target in the Makefile.

[degenaro]

Ok, good catch!! Back to the drawing board here. The hope was to eliminate a set of warnings simply by getting rid of python-semantic-release. What wasn't considered was the fact that trestle make release won't work if this is removed, which wasn't noticed because make release is not part of the test harness! How dumb? Please don't answer :-)

And by the way, these warnings already occur in the current develop branch, so there is no regression if the warnings reappear. The warnings are for removal of support for this level of semantic-release in python 3.12. However, we upgraded support to 3.9, 3.10, 3.11 in this PR, but not yet 3.12, so that is not a show stopper.

[butler54]

Changing to the latest version of python-semantic-release requires a change in the CICD pipeline as it no longer contains the functionality to upload to pypi. Also moving to the latest release requires moving to pydantic>=2 which is a breaking change with the trestle code.

[degenaro]

In this PR pydantic[email], meaning the latest version of pydantic is pulled. But, in the trestle code where pydantic is used for example import pydantic.v1.networks. The thought here was that we use the latest pydnatic publications, but for now use the v1 interface. The thought being that support and CVE fixes for older publications may not be forthcoming.

[degenaro]

It is not clear how difficult it would be to upgrade the trestle CICD pipeline to employ a newer version of semantic-release. I can investigate, but if it becomes too much of a chore (say, not finished by end of this week) perhaps we should back off and live with the warnings in order to get support for OSCAL 1.1.2 out the door. Also, anyone with more expertise willing to help on this topic (e.g. making trestle CICD pipeline changes needed) is welcomed!

[butler54]

@degenaro let me know if you need a hand. You will probably want to do this on a fork

[jpower432]

This is just a nit, but expressing the imports this way can reduce some duplication here:

from trestle.oscal.common import (
     ControlSelection,
     ImplementedComponent,
     )

[AleJo2995]

Agree with Jenn

[degenaro]

(venv.fix-1.1.2) degenaro:trestle.fix-1.1.2$ make code-lint
pre-commit run flake8 --all-files
flake8...................................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

trestle/transforms/implementations/xccdf.py:29:121: E501 line too long (160 > 120 characters)

Because from trestle.oscal.common import all on one line failed the linter, I chose to make all the imports singletons for consistency. I can make those that are less than 120 into one line if you like. I'm not too fussy here.

[degenaro]

Fixed.

[AleJo2995]

Leaving just general feedback. Will add 1 more test for backward compatibility of trestle import test

[AleJo2995]

wasn't this intended to protect ourselves from some mal-versioned problem with the newer versions? I think that if tests are passing there are no worries, but just wanted to know

[degenaro]

The idea was to pull the latest version of pydantic, but use the v1 interface for now. Previously, we were pulling an older published release. At some point in the near future we should make the changes needed to use the v2 interface, but not now.

[AleJo2995]

So, just to understand, we're using certain models, functionalities from v1 but the current pydantic version we're pointing at is just 2.0.0 right?

[degenaro]

Yes, the claim is we are using the v1 interface of the v2 publication.

[AleJo2995]

Why are you passing orig_prof_import and catalog_api as parameters? I don´t see them used along the function. I think that you passed them just because the parent function is using those parameters but in other places, but not necessarily because this extracted function needs it. Besides that, this function contributes a lot to lower down cognitive function which is fantastic 👍

[degenaro]

fixed. unused parameters removed.

[AleJo2995]

These imports from common mean that initially they were being imported from each specific class and instead now they are being pulled from common classes? Or was this included as new as part of the upgrades in the schema coming from NIST?

[degenaro]

More did move to common, and therefore some imports were changed or were newly needed. With respect to the generators.py module, there were other reason for inclusions. I did make this comment:

Apr 17: 82 failed, 1121 passed, 3 skipped, 37 warnings
(LD) improve pre-process reordering & valid values
(LD) handle special case (e.g. Base64) code generator

Why the generator now had to deal with Base64 whereas before it did not, I do not precisely recall. I can spend some time re-investigating and explaining if wanted.

[AleJo2995]

I´m assuming that metadata has been deleted automatically due to moving it to the common class right?

[degenaro]

For sure, Metadata is in common now. But as far as I can tell it is in common in the develop and main branches too. Not understanding why it appears here!?

[AleJo2995]

I´m assuming that party has been deleted automatically due to moving it to the common class right?

[degenaro]

Yes.

[AleJo2995]

I´m assuming that location has been deleted automatically due to moving it to the common class right?

[degenaro]

Yes.

[AleJo2995]

I´m assuming that address has been deleted automatically due to moving it to the common class right?

[degenaro]

Yes.

[AleJo2995]

A really silly/minor thing, why are these imports separated instead of being a one liner? Any particular reason even though they are coming from the same class?

[degenaro]

fixed.

[AleJo2995]

Agree with Jenn

[degenaro]

re: semantic release 9.7.2

Sample workflow https://python-semantic-release.readthedocs.io/en/latest/automatic-releases/github-actions.html

[degenaro]

Specification of semantic-release 7.33.2 has been restored to the setup.cfg. This was an oversight (nicely caught by Jenn in her review) in an overzealous attempt to eliminate all warnings during make test. Upgrading to a newer version of semantic-release should be on our radar for a next release of trestle. For now, we can live with the 17 warnings that occur. Note that the prior trestle release has 54 warnings, so we are better off with respect to warnings.

[degenaro]

Thanks for the reviews! I think all comments made thus far have been addressed. Would appreciate a re-review toward getting the next release of trestle closer to reality.

[degenaro]

So here's what I did, right or wrong. I did not understand why trestle/oscal/catalog.py previous version did not match what I expected to see. Note also, that this PR and branch was derived not directly from develop. So, I decided to create another PR that would deliver directly to develop. I was allowed to do so, but perhaps I don't fully understand the consequences of doing so?

Follow-up: 2 things to report.

1. Creating a new PR using the same branch and delivering to a different base seems to be a bad idea. In this case delivering the present branch to oscal_nist_upgrade and to develop. (the PR title seems to be shared somehow?) So closed the PR that attempted to deliver to develop. However...

2. It seems that oscal_nist_upgrade has some changes already, above and beyond what is in develop, and that explains why the trestle/oscal/catalog.py what not as expected when comparing to previous version.

Conclusion: once this PR is approved and merged into oscal_nist_upgrade, another review will be needed to merge oscal_nist_upgrade into develop. My discovery was that potential delivery to develop yields 79 changed files, while potential delivery to oscal_nist_upgrade yields 67 changed files.

I created a PR for just oscal_nist_upgrade to understand what it alone changed and upon which the present PR has been built. For your consideration, here is my analysis of what will need to be reviewed once the present PR is merged into oscal_nist_upgrade before it can be merged into develop:

# files	path	description
8	release-1.1.2-schemas/*	the new schemas, which are not changed by child PR, so no review needed
1	scripts/gen_oscal.py	changed by child PR, so already reviewed
1	tests/trestle/core/profile_resolver_test.py	needs review
1	trestle/common/const.py	needs review
1	trestle/core/control_reader.py	changed by child PR, so already reviewed
1	trestle/core/resolver/merge/reader.py	changed by child PR, so already reviewed
8	trestle/oscal/*	changed by child PR, so already reviewed