Skip to content
This repository has been archived by the owner on Aug 20, 2021. It is now read-only.

Add modify-add feature #52

Merged
merged 5 commits into from
Jan 24, 2019
Merged

Conversation

minhaj10p
Copy link
Contributor

@minhaj10p minhaj10p commented Jan 17, 2019

Description

Attempts to process manipulation attributes in the profile modify section.
Linking to #12.

Feature set

  1. Concurrent import chain traversal is enabled to fetch the root catalog for each import
  2. Recursively searches the import chain for the controls/subcontrols referenced by the root profile for add manipulation attribute
  3. Appends parts with the same class instead of adding a new part

generator/mapper.go Outdated Show resolved Hide resolved
@@ -123,3 +123,72 @@ func getMappedCatalogControlsFromImport(importedCatalog *catalog.Catalog, profil
}
return newCatalog, nil
}

func getCatalogForImport(ctx context.Context, i profile.Import, c chan *catalog.Catalog, e chan error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anweiss
This method traverses the entire import tree and finds the catalog concurrently. The first goroutine that finds it pushes it to the channel and all other goroutines get killed thanks to the context package.
Let me know if this approach works better in stead of the old fashioned LNR tree parsing

Copy link
Contributor

Choose a reason for hiding this comment

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

@anweiss anweiss added the enhancement New feature or request label Jan 17, 2019
@anweiss anweiss added this to the OSCAL 1.0 milestone Jan 17, 2019
@minhaj10p minhaj10p force-pushed the manipulation branch 4 times, most recently from 1ad414d to 56a14e9 Compare January 19, 2019 10:34
@minhaj10p
Copy link
Contributor Author

Currently this PR is shutting down unit tests as import chain traversal isn't possible without file system.
If you can approve, we can commit mock profiles to pass those tests.

@anweiss
Copy link
Contributor

anweiss commented Jan 19, 2019

@minhaj10p would it be possible to include the mocks in this PR?

@minhaj10p
Copy link
Contributor Author

@minhaj10p would it be possible to include the mocks in this PR?

Sure. I can create a folder and put some profiles in it. Another thing just occurred to me is that we always put the manipulation attributes in the same profile so it never has to go in the find the manipulation attribute in the import chain. I can try and see if that works.

@minhaj10p
Copy link
Contributor Author

@anweiss Tests updated. All are passing now with 73.0% code coverage

@minhaj10p minhaj10p force-pushed the manipulation branch 2 times, most recently from 1b25402 to 13ebd1e Compare January 20, 2019 07:19
@minhaj10p
Copy link
Contributor Author

@anweiss Let me know if theres anything you need changed in this pull request. Thanks.

Copy link
Contributor

@anweiss anweiss left a comment

Choose a reason for hiding this comment

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

@minhaj10p please add a space after // in all comments.

@@ -123,3 +123,72 @@ func getMappedCatalogControlsFromImport(importedCatalog *catalog.Catalog, profil
}
return newCatalog, nil
}

func getCatalogForImport(ctx context.Context, i profile.Import, c chan *catalog.Catalog, e chan error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

generator/manipulation.go Outdated Show resolved Hide resolved
generator/manipulation.go Outdated Show resolved Hide resolved
generator/manipulation.go Outdated Show resolved Hide resolved
generator/manipulation.go Outdated Show resolved Hide resolved
generator/mapper.go Outdated Show resolved Hide resolved
generator/mapper.go Outdated Show resolved Hide resolved
generator/mapper.go Outdated Show resolved Hide resolved
generator/mapper.go Outdated Show resolved Hide resolved
generator/manipulation.go Show resolved Hide resolved
@anweiss
Copy link
Contributor

anweiss commented Jan 21, 2019

@minhaj10p I tested this on my end with the FedRAMP High profile, and it looks like it's appending some erroneous structs. For example:

Parts: []catalog.Part{
	catalog.Part{
		Id:    "ir-9_smt",
		Class: "statement",
		Title: "",
	},

	catalog.Part{
		Id:    "ir-9_gdn",
		Class: "guidance",
		Title: "",
	},

	catalog.Part{
		Id:    "ir-9_obj",
		Class: "objective",
		Title: "",
	},

	catalog.Part{
		Id:    "",
		Class: "assessment",
		Title: "",
	},

	catalog.Part{
		Id:    "",
		Class: "assessment",
		Title: "",
	},

	catalog.Part{
		Id:    "",
		Class: "assessment",
		Title: "",
	},

	catalog.Part{
		Id:    "",
		Class: "justification",
		Title: "",
	},
},

Note the empty "assessment" and "justification" parts.

Also as far as the Prose, you should be able to just include it in your template.

@minhaj10p
Copy link
Contributor Author

minhaj10p commented Jan 22, 2019

@minhaj10p I tested this on my end with the FedRAMP High profile, and it looks like it's appending some erroneous structs. For example:

Parts: []catalog.Part{
	catalog.Part{
		Id:    "ir-9_smt",
		Class: "statement",
		Title: "",
	},

	catalog.Part{
		Id:    "ir-9_gdn",
		Class: "guidance",
		Title: "",
	},

	catalog.Part{
		Id:    "ir-9_obj",
		Class: "objective",
		Title: "",
	},

	catalog.Part{
		Id:    "",
		Class: "assessment",
		Title: "",
	},

	catalog.Part{
		Id:    "",
		Class: "assessment",
		Title: "",
	},

	catalog.Part{
		Id:    "",
		Class: "assessment",
		Title: "",
	},

	catalog.Part{
		Id:    "",
		Class: "justification",
		Title: "",
	},
},

Note the empty "assessment" and "justification" parts.

Also as far as the Prose, you should be able to just include it in your template.

@anweiss
These erroneous looking part's are coming from the NIST-80053 catalog where the parts have no ids and only classes

         <part class="assessment">
            <prop class="method">EXAMINE</prop>
            <part class="objects">
               <p>Incident response policy</p>
               <p>procedures addressing information spillage</p>
               <p>incident response plan</p>
               <p>records of information spillage alerts/notifications, list of personnel who should receive alerts of information spillage</p>
               <p>list of actions to be performed regarding information spillage</p>
               <p>other relevant documents or records</p>
            </part>
         </part>
         <part class="assessment">
            <prop class="method">INTERVIEW</prop>
            <part class="objects">
               <p>Organizational personnel with incident response responsibilities</p>
               <p>organizational personnel with information security responsibilities</p>
            </part>
         </part>
         <part class="assessment">
            <prop class="method">TEST</prop>
            <part class="objects">
               <p>Organizational processes for information spillage response</p>
               <p>automated mechanisms supporting and/or implementing information spillage response actions and related communications</p>
            </part>
         </part>

I haven't been able to implement nested part in our code generation.
The last part with justification class comes from the profile with no id in the profile.
They look erroneous probably because it doesn't have the Prose section populated.

It looks somewhat like this if just included in the template

	catalog.Part{
		Id:  "ir-9_obj",
		Class: "objective",
		Title: "",
		Prose: "{{ } [] [p] [{{http://csrc.nist.gov/ns/oscal/1.0 p} [] [] [] [] [] [] [] [] [] [] [] Determine if the organization:  }] [] [] []
        }",

@anweiss
Copy link
Contributor

anweiss commented Jan 23, 2019

@minhaj10p ah ok. That makes sense. I'll try to see what I can come up with re. nested parts and prose to help you out on this.

generator/profile.go Outdated Show resolved Hide resolved
@minhaj10p minhaj10p force-pushed the manipulation branch 2 times, most recently from 29d8b41 to aa73643 Compare January 23, 2019 19:43
@minhaj10p
Copy link
Contributor Author

@anweiss if you merge this, let me know so I can rebase the other dependent branches.

@anweiss
Copy link
Contributor

anweiss commented Jan 24, 2019

@minhaj10p after speaking internally with @justincormack, we can go ahead and merge this. Thanks again for your patience.

@anweiss anweiss removed the request for review from heavypackets January 24, 2019 17:32
Copy link
Contributor

@anweiss anweiss left a comment

Choose a reason for hiding this comment

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

LGTM

@anweiss anweiss merged commit 415eaf4 into docker-archive:master Jan 24, 2019
@minhaj10p minhaj10p deleted the manipulation branch January 24, 2019 18:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants