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

Add convert CCI list workflow #6336

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open

Conversation

jtquach1
Copy link
Contributor

No description provided.

@jtquach1 jtquach1 force-pushed the add-convert-cci-list-workflow branch from 3e8e7a0 to aa9fb80 Compare October 29, 2024 14:26
@jtquach1 jtquach1 marked this pull request as ready for review October 29, 2024 14:29
@jtquach1 jtquach1 force-pushed the add-convert-cci-list-workflow branch from 1d5d4ff to f892789 Compare October 29, 2024 17:23
@jtquach1 jtquach1 changed the title Add convert cci list workflow Add convert CCI list workflow Oct 29, 2024
.github/workflows/convert-cci-list.yml Outdated Show resolved Hide resolved
.github/workflows/convert-cci-list.yml Outdated Show resolved Hide resolved
.github/workflows/convert-cci-list.yml Outdated Show resolved Hide resolved
.github/workflows/convert-cci-list.yml Outdated Show resolved Hide resolved
.github/workflows/convert-cci-list.yml Outdated Show resolved Hide resolved
@@ -2,28 +2,31 @@ import fs from 'fs';
import * as _ from 'lodash';
import xml2js from 'xml2js';

// Documentation is located at https://github.com/mitre/heimdall2/wiki/Control-Correlation-Identifier-(CCI)-Converter.
const parser = new xml2js.Parser();
const pathToInfile = process.argv[2];
Copy link
Contributor

Choose a reason for hiding this comment

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

use an actual argparse library

https://nodejs.org/docs/latest-v18.x/api/util.html#utilparseargsconfig
http://yargs.js.org/
https://github.com/tj/commander.js

others are out there - I would try the built-in first

process.argv is enough to get off the ground with a one-off script but now that we're giving this a polish, might as well put in the polish

libs/hdf-converters/data/converters/cciListXml2json.ts Outdated Show resolved Hide resolved
libs/hdf-converters/data/converters/cciListXml2json.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

why did just this sample file for this mapper get changed and not any of the other samples for this mapper or any other mapper at all?

Copy link
Contributor Author

@jtquach1 jtquach1 Oct 30, 2024

Choose a reason for hiding this comment

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

In the HDF Converters tests GitHub Action, heimdall2/libs/hdf-converters/test/mappers/forward/xccdf_mapper.spec.ts was purely failing for the SCAP ubuntu 1804 test, but none of the others. The failed test comprised of a bunch of key/value pairs like "ident": undefined and the like, and upon me looking into where those values come from, "ident" those key/value pairs don't show up in the resulting JSON since JSON.stringify removes pairs with undefined values. Anyhow, that was the diff I saw on GitHub Actions.

Upon locally running the same test file, I saw some other test files (for different mappers) fail due to async file loading, even though those respective tests didn't seem to fail on GitHub Actions. The XCCDF test file strangely didn't have the same failing error as the GitHub Actions one. But upon regenerating the relevant "expected" HDF of that particular ubuntu 1804 test, I did a git diff and saw that some of the NIST tags changed for existing CCIs. Just as a shot in the dark, I reckoned to commit that, and it looked like this particular test finally turned green. (Perhaps that is not "the" solution though.)

TLDR: Local HDF Converters tests didn't seem to have consistent results with the GitHub Actions' ones. Maybe witchcraft?

@jtquach1 jtquach1 force-pushed the add-convert-cci-list-workflow branch from 229d078 to e350181 Compare October 30, 2024 17:56
@Amndeep7
Copy link
Contributor

libs/hdf-converters/src/mappings/NistCciMappingData.ts

Current state:

Defines some default CCI values for a select set of NIST tags.
Used in libs/hdf-converters/src/utils/global.ts in two places:

  1. to define a constant based off of another constant.
  2. to create a function (getCCIsForNISTTags) that will be used as a transformer in all the mappers to convert NIST tags if they're provided into CCIs.

Desired state:

Eugene is ideally working on doing a refresh of this data.
I want this file turned into just a raw json blob similar to what the ccilistxml2json script does.
Then I want it to be imported into and exposed as a constant from CciNistMapping.ts similar to how I described I wanted you to expose the data from the script.

libs/hdf-converters/src/utils/global.ts

Current state:

Amongst other things, it defines some constants related to NIST/CCIs and the getCCIsForNISTTags function.

Desired state:

Relevant constants and that function are moved over to libs/hdf-converters/src/mappings/CciNistMapping.ts.
All mappers are updated to point to the new place.
If Eugene gets the update in time, then you might need to regen some more samples.

libs/hdf-converters/src/mappings/CciNistMappingData.ts

Current state:

Currently exposes an object called 'data' that contains the CCI/Nist mapping.

Desired state:

As already described in the peer review, I want you to turn this into two separate files that each contain a raw json blob (i.e. no 'export const data = {' stuff necessary). The first file contains the object mapping CCI to latest NIST rev. The second file contains CCI to its description.
These files are then exposed from CCiNistMapping.ts.

libs/hdf-converters/src/utils/CCI_List.ts

Current state:

It is used in CciNistMapping.ts to help define the two way nist/cci mapper.

Desired state:

Deleted

libs/hdf-converters/src/mappings/CciNistMappingItem.ts

Current state:

Used to define a cci/nist mapping for use in the array form of the data which imo is pretty dumb.

Desired state:

Deleted

libs/hdf-converters/src/mappings/CciNistMapping.ts

Current state:

Defines several types that define the JSON object generated by the xml parser run against CCI_LIST.
Two classes:

  1. CciNistMapping
    Converts the CciNistMappingData object into an array in the constructor.
    Has a function called 'nistFilter' that maps from the provided array of CCIs and converts them into an array of NIST tags with the two interesting things being that it a) returns some default value if no nist tags are found and b) it has an option (set to true by default) to basically run the equivant of a unique pass across the returned data.
  2. CciNistTwoWayMapper
    Converts CCI_LIST into a json object which will have the types defined above.
    Has a function called 'nistFilter' that does a bunch of complicated shit and uses like private functions and stuff but essnetially it boils down to the exact same thing that the other class was doing in a manner that is still inefficient but not as completely dumb as the other implementation.
    Has a function called 'cciFilter' that tries to find all CCIs that have the same NIST tag.

Desired state:

Those constants defined in global are now moved here, and we've defined more constants here that expose the raw json blobs.
Since CCI_LIST is being deleted, those types are no longer required and can be deleted as well.
When getCCIsForNISTTags is moved here, rename it something like defaultNIST2CCI or limitedNIST2CCI, use inspecjs (libs/inspecjs/src/nist.ts) to extract out the tag instead of the handrolled regex that was added there, and then do it as map/reduce (map incoming nist tag strings to properly formatted nist tag top level control (AC-1, not AC-1 (b) blah), map that value to the CCI array using whatever you name the constant that contains the data for our handcrafted mapping, flatten the array of arrays, unique it, return).
I do not think we need those classes any more. They do not take any parameters for their constructors, which in fact are used purely to do some data pre-processing before exposing some functions. If instead we just define constants based off of processing other constants (and maybe a private function or two), then we can expose some raw functions instead of having to have a meaningless wrapper class around them.
CciNistMapping can be deleted.
CciNistTwoWayMapper can be simplified into two exposed functions (might need some private funcs but probably not):

  1. "CCI2NIST"
    Basically it'll look exactly like the nistFilter func but simpler cause instead of doing that forloop and manually having to assess which is the latest rev nist tag, we can just do a map against our prior work and updates to that mapping. I think that the return statement where we potentially return the default nist tags is buggy because ?? only returns the righthand side on null/undefined, but we want it to return that side when matches is the falsy value of an empty array so the operator we actually need there is ||. Sonarqube complains about this everytime and it's wrong about the suggestion to use ?? like 75% of the time.
  2. "NIST2CCI"
    Add a new constant that'll be an object that will be a 'trie'-esque data structure built off of inversing the ccinistmpapingdata data. ccinistmappingdata is { cci-1234: 'ac-1', cci-4321: 'ac-1', cci-2314: 'ac-1 a' }. strict inverse (that won't actually do what we want properly but for demonstration): { ac-1: cci-1234, ac-1: cci-4321, 'ac-1 a': cci-2314 }. We have problems where a) this isn't actually possible to do in js and b) we want adding the specifiers to restrict the ccis that are returned. Consequently, we can do something like this: { ac-1: { a: { ccis: [cci-2314] }, ccis: [cci-1234, cci-4321, cci-2314] } }.
    Use inspecjs to convert the incoming nist tags into their normalized forms, and then use that to go into the trie-esque data structure by going into it as far as the amount of specifiers that we have (so ac-1 a (2) is 3 levels), if there's nothing there then keep backing up a specifier (ac-1 a (2) has nothing, but ac-1 a has cci-2314) until you actually get results or you've backed yourself out of the structure entirely in which case just return the default ccis.

You'll then need to update the mappers and other locations as appropriate.


Future work

Update libs/inspecjs/src/raw_nist.ts to ensure that our NIST tags are all up to date. Maybe find out a way to automate this process.

Review the rest of what's going on in this mappings directory to see if we can simplify implementations / reduce redundancies like we're doing now with the nist/cci stuff.

@jtquach1 jtquach1 force-pushed the add-convert-cci-list-workflow branch from e8b69cd to 20b237b Compare November 1, 2024 16:29
Signed-off-by: Joyce Quach <[email protected]>
…cci_util.ts, and add NIST_DESCRIPTIONS array produced from cciListXml2json

Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
…SON file and check in that file

Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
Signed-off-by: Joyce Quach <[email protected]>
@jtquach1 jtquach1 force-pushed the add-convert-cci-list-workflow branch from 11d47a7 to e420048 Compare November 5, 2024 18:49
Copy link

sonarcloud bot commented Nov 5, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
2 New Code Smells (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants