Improve xccdf_results_mapper when converting XCCDF Results to HDF Results

[candrews]

• Support XCCDF generated using ComplianceAsCode: https://github.com/ComplianceAsCode/content/
• Support nested XCCDF groups
• Set code_desc to an appropriate value (not empty string)
• Refactor to remove global variables

Bug: mitre/saf#1163
Fixes: #4194

[aaronlippold]

Hi the team @Amndeep7 @em-c-rod @wdower @Hookwitz @georgedias are looking over this and seeing if some of our other PR work can be rolled into this as well.

A couple questions:

• Given some of the changes that were make, would we be able to make some smaller unit tests that cover actual function @em-c-rod @Amndeep7 @candrews
• Should we add test data for other XCCDF profiles -windows, Ubuntu, DB, IIS, etc or would a subset of XCCDF content element likely be sufficient
• Have we seen any corner cases - nesting, tags inside tags, etc - that we should make specific tests for?

[aaronlippold]

@candrews looks like SonarCloud is also unhappy about something - which we may want to poke at as well as doing a rebase from master/main

[aaronlippold]

Question: Is any of the work here also useful or overtaken by this PR? #2958

[candrews]

looks like SonarCloud is also unhappy about something - which we may want to poke at as well as doing a rebase from master/main

I fixed the SonarCloud findings. The only finding now is in the test for a comment (the same comment is in every other test, so I think it should be in this one as well) and I believe that finding should be suppressed.

[candrews]

Question: Is any of the work here also useful or overtaken by this PR? #2958

This PR completely supersedes #2958 - it does everything that PR does and more.

[candrews]

Can I do something to help this along?

I'd really like to have these issues fixed soon as it's blocking progress on a project which is import to my corporate overlords.

[Amndeep7]

Sorry for the delay @candrews - could you address the findings for the HDF-Converters tests? It seems like some of the other XML mappers such as the Veracode are now having issues.

Also now that I've enabled workflows in the repo, they should continue to be enabled for this PR, but shoot me a ping if they aren't after you push up some changes.

[Amndeep7]

Another point that I wanted to bring up was that we were intending on eventually rewriting this mapper to use jsonix to be able to match against the XCCDF schema properly instead of using the fast-xml-parser which has issues with (for example) determining the difference between singular nodes and arrays of length one. This isn't a request for you to rework this entire PR to use that other library, just a heads up about part of the delay in dealing with this mapper since we're still exploring how to use jsonix and write a mapper properly with it in our checklist mapper work (#3859).

[Amndeep7]

Still need to finish reading/reviewing the code, which I'll try to do next week.

Also @candrews - it would be really nice if we could arrange a meeting to discuss how you and your corporate overlords are using HDF-Converters / the SAF. We don't do any tracking, so we gotta go the old fashioned way and actually talk to our users in order to learn how/why they're using our stuff, and what we could do to deepen the partnership.

If you could email me at [email protected] with some dates/times that you/your team/your management chain are available, I'd appreciate it a lot.

[mergify]

This pull request has a conflict. Could you fix it @candrews?

[candrews]

This pull request has a conflict. Could you fix it @candrews?

I've addressed the conflict.

[candrews]

@Amndeep7 I believe that I have addressed all the feedback.

As you know, I'm not terribly familiar with the heimdall tool, but with that disclaimer, I think the output looks good and is definitely improvement over when we last looked at this together.

Can you please take another look and let me know what else needs to be done?

[Amndeep7]

Feeling like this should probably be able to be merged in after this final set of changes is made. Thanks for bearing with the process for so long.

[Amndeep7]

seems like your sample is not updated properly still? or this function is borked lol

xccdf-scc-rhel7-hdf.json

seems like the nist tag array is including the stig control ids, which it shouldn't do

[candrews]

is_control(parse_nist('SV-86515')) return true... so as far as inspectjs is concerned, 'SV-86515' is a nist control and therefore should be included.

Am I misunderstanding nist.js? How should I determine if an item should be included or excluded from this list?

[Amndeep7]

this feels like a failure on inspecjs' part in not removing things that are clearly wrong. parse_nist should throw errors/explicitly fail when they are not nist controls/control families. it not doing so confuses the type system by mistakenly typing that string. is_control more or less is only used to narrow down types and doesn't do much actual validation.

anyways as shown there and in 'raw_nist.ts', nist controls look like 'two_letter_family-small_number possibly followed by letters and numbers in parentheses' whereas stig ids are 'S?V-5__or_6_digit_number'.

the issue that arose here is that it thinks 'sv' is a control family and then processes the numbers after it as like 'control "big number" from within that control family' but doesn't take into account the reality of the situation where 'sv' is not a valid control family nor is there a control with an index as high as '86515' in existence.

looking briefly, it seems like the regexes that parse_nist uses are vastly broader than the actually allowed set of inputs. i will experiment to see if it's possible to restrict it to allowed control families.

[candrews]

Using stopNodes would be a great way to fix the parsing problem that occurs with nodes that contain HTML.

However... stopNodes doesn't work for XML documents that use namespaces: https://github.com/NaturalIntelligence/fast-xml-parser/blob/v4.2.4/src/xmlparser/OrderedObjParser.js#L308 Attempting to use it results in an "Unexpected end of " error. I reported this issue at NaturalIntelligence/fast-xml-parser#607

If this issue in the xml parser is fixed, then modifying xccdf-results-mapper.ts to say:

  constructor(scapXml: string, withRaw = false) {
    super(
      parseXml(scapXml, {
        stopNodes: [
          '*.fixtext',
          '*.fix',
          '*.rationale',
          '*.warning',
          '*.title',
          '*.description'
        ]
      })
    );

would solve the html parsing problem.

[candrews]

@Amndeep7 do you have a guess as to when we'll be able to progress this PR?

[Amndeep7]

Sorry I've been swamped this week. I'll try to take a look next week to fix that bug in Heimdall that I mentioned.

[mergify]

This pull request has a conflict. Could you fix it @candrews?

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[Amndeep7]

Thank you for your patience and hard work on this PR @candrews. You did some absolutely fantastic work here that'll be appreciated across many of the users of Heimdall.