Phase 2 #10
Phase 2 #10
Conversation
Signed-off-by: Mark Rivera <[email protected]>
Signed-off-by: Mark Rivera <[email protected]>
Signed-off-by: Mark Rivera <[email protected]>
Signed-off-by: Mark Rivera <[email protected]>
Signed-off-by: Mark Rivera <[email protected]>
Signed-off-by: Mark Rivera <[email protected]>
Will add with rest of deployment changes at a later date.
| ```ini | ||
| SQLITE=true | ||
| JWT_KEY= //Required: Key that TIR will use for JWT | ||
| SECRET_KEY= //Required |
There was a problem hiding this comment.
Add an explanation for what this field should be.
We should probably add a .env-example file or something similar.
| await upMigration.commit(); | ||
| }; | ||
|
|
||
| export const down: MigrationFn = async () => { |
There was a problem hiding this comment.
Add a comment explaining the sqlite behavior which is why we gotta do this split in the execution path
| if (!process.env.INIT_PASSWORD) { | ||
| throw new Error("INIT_PASSWORD environment variable not set."); | ||
| } | ||
| if (!process.env.SECRET_KEY) { |
There was a problem hiding this comment.
probably want to change this to have a better name like 'HMAC_SECRET'
| UserRoleId: 1, | ||
| creationDate: now, | ||
| lastUpdate: now, | ||
| salt: adminSalt, | ||
| }, | ||
| { |
There was a problem hiding this comment.
The seeder should only have the admin user provided with the expectation that the end user creates any normal user accounts that they need. If we want a user account for testing purposes to be available, it should be created during the test initialization phase.
| export const hashObj = (objToHash: any, algorithhm: string = 'sha256'): string =>{ | ||
| const jsonString = JSON.stringify(objToHash, Object.keys(objToHash).sort()); | ||
|
|
||
| export const hashObj = (objToHash: any, algorithhm: string = "sha256"): string => { |
There was a problem hiding this comment.
check if this is dead code, if so, remove
| hmac.update(password); | ||
| const hmacResult = hmac.digest(); | ||
|
|
||
| return crypto.pbkdf2Sync(hmacResult, salt, 600000, 64, "sha256").toString("hex"); |
There was a problem hiding this comment.
replace numbers with constants if possible
| return storedHash === inputHash; | ||
| } | ||
|
|
||
| export function generateSalt(length: number = 16): string { |
There was a problem hiding this comment.
add a reference to the doc or website or guidance or whatever that explains the current source of all of these default / actually used values
There was a problem hiding this comment.
Add documentation explaining why the models are the way they are / the actual tables are the way they are / why some of the code is weird as it is to handle the sqlite vs postgres differences (usually where postgres offers some built-in functionality that sqlite doesn't have so we gotta do it by hand there)
db/models/token.ts
Outdated
There was a problem hiding this comment.
we discussed 'project tokens' as opposed to just 'user tokens' since there is a likelihood of a user story where someone wants to have an apikey/token that doesn't have visibility to every single thing that they have visibility to but instead just visibility into one or two projects/boundaries.
something to keep in mind is that it probably still needs to be associated with a user, not just the boundary itself, so that its always possible to connect actions that occur in a system to a user.
| declare Stig?: NonAttribute<Stig>; | ||
| declare succeededByAssessmentId: ForeignKey<Assessment["id"]> | null; |
There was a problem hiding this comment.
Can we determine why the circular reference here is linking forward whereas in the other one it's linking backwards?
Can we change the db structure so that both link in the same direction (doesn't matter which)?
| declare Stig?: NonAttribute<Stig>; | ||
| declare succeededByAssessmentId: ForeignKey<Assessment["id"]> | null; |
There was a problem hiding this comment.
Please add an explanation as to the necessity for the circular references?
There was a problem hiding this comment.
Is the variable name not sufficient here?
| declare Stig?: NonAttribute<Stig>; | ||
| declare succeededByAssessmentId: ForeignKey<Assessment["id"]> | null; |
There was a problem hiding this comment.
Please add a wiki page or other artifact explaining how the ingested data gets mapped onto the database.
| declare Stig?: NonAttribute<Stig>; | ||
| declare succeededByAssessmentId: ForeignKey<Assessment["id"]> | null; |
There was a problem hiding this comment.
Please completely type all optional attributes
There was a problem hiding this comment.
allowNull: true is the default behavior on all columns aside from those defining a primary key. IMO we should only explicitly define allowNull when it is false to show how that field is the exception to the rule. It also doesn't help that we don't have a linter (unless you can find one) to enforce defining allowNull one way or the other on every field. I feel like my solution is the more consistent solution unless we can find a linter.
db/models/cciItem.ts
Outdated
There was a problem hiding this comment.
@aaronlippold question - should we split 'typePolicy' and 'typeTechnical' into their own 'type' enum table and then have some like join table between the two instead of having booleans like this?
db/models/cciItem.ts
Outdated
There was a problem hiding this comment.
naming consistency (snake / camel / etc)? seems like some of the fields come from the source data files and some come from column headers that we create. even if we don't rename everything to camelcase everywhere, can we at least group the attributes so that it's easier to determine which naming system applies to which block of attributes?
| } from "sequelize"; | ||
| import type { Milestone } from "."; | ||
| import type { Milestone, User } from "."; | ||
|
|
||
| type levels = "Very High" | "High" | "Moderate" | "Low" | "Very Low"; |
There was a problem hiding this comment.
rename to whatever the source document is so that it's easier to know that these are like the POAM kind of levels
| @@ -33,11 +34,15 @@ export class EvaluationItem extends Model< | |||
| declare Recommendations: levels; | |||
| declare lastUpdate: CreationOptional<string>; | |||
| declare creationDate: CreationOptional<string>; | |||
| declare Users?: NonAttribute<User[]>; | |||
There was a problem hiding this comment.
another example where reworking the grouping might make things clearer
db/models/index.ts
Outdated
db/models/index.ts
Outdated
There was a problem hiding this comment.
Maybe we can find out a better way to order/organize the code, but we couldn't figure out a good solution for this during the review time.
db/models/override.ts
Outdated
There was a problem hiding this comment.
Rename to clarify that this is the status override.
During the discussion, it turns out that this work is being done in another branch.
db/models/policyDocument.ts
Outdated
There was a problem hiding this comment.
Leave a comment saying that the schema allows for this to be any policy or guidance documentation but in practice it's basically just defining the NIST Control Family revision.
db/models/stig.ts
Outdated
There was a problem hiding this comment.
Another comment about the attribute naming. We should just make sure its consistent even if it has multiple different types of consistency depending on if its a variable name that comes from an external document vs something that's internal to the db. In this case, the double underscore meaning XML attribute as opposed to node is coming out of left field. However we make it consistent, we should have some document explaining the naming patterns.
db/models/stig.ts
Outdated
There was a problem hiding this comment.
Can we rename the MAC profiles to not be abbreviations
db/models/stigAlias.ts
Outdated
There was a problem hiding this comment.
We should probably have a description for every table not just the document ingest ones. In this case one or two two sentences to explain how the stig names don't always line up with the file names or whatever so we need some aliases to match everything properly. Then link out to a wiki page where you can go more in-depth on one or two specific examples of this issue (like the google chrome stig iirc).
| } from "sequelize"; | ||
|
|
||
| import { AssessmentItem } from "./assessmentItem"; | ||
| import { Stig } from "./stig"; | ||
| import { StigIdent } from "./stigIdent"; | ||
| import { EvaluationItem } from "./evaluationItem"; | ||
| import type { Override } from "."; | ||
| import type { Override, StigReference, StigResponsibility } from "."; |
There was a problem hiding this comment.
Why are we pulling from index.ts instead of their files where they're actually defined? And also why are we importing them as types?
| declare StigResponsibilities?: NonAttribute<StigResponsibility[]>; | ||
| declare StigReferences?: NonAttribute<StigReference[]>; | ||
|
|
||
| declare addStigResponsibility: BelongsToManyAddAssociationMixin<StigResponsibility, number>; |
There was a problem hiding this comment.
Maybe group the functions with the variables? Or at least put them in the same order as the variables are defined.
|
|
||
| declare static associations: { | ||
| Stigs: Association<StigLibrary, Stig>; | ||
| }; | ||
| } | ||
|
|
||
| StigLibrary.init( |
There was a problem hiding this comment.
Related to the 'allowNull' discussion, I believe that 'unique: true' is automatically applied to anything that has primaryKey: true, so let's get rid of the duplication.
db/models/theme.ts
Outdated
There was a problem hiding this comment.
Not a blocker, but was wondering if you wanted to do hardcoded colors in the code vs having the colors be defined here. The other SAF products don't have multiple themes (at least anymore) so it's not as much of a concern for them, but it seems like yall are interested in having at least a light/dark mode which raises the question of potentially supporting custom themes as well in which case having that stuff listed in the db would probably help.
db/models/theme.ts
Outdated
There was a problem hiding this comment.
Reminder about our conversation w/r to accessibility considerations (colors/contrasts/font sizes/labels/alt text/etc).
db/models/tier.ts
Outdated
There was a problem hiding this comment.
As discussed, probably worthwhile to change Tier -> Company to match the default text used everywhere since it's a lot simpler to change the migration scripts now (ie without customers/users) instead of having to make a new migration to do it.
There was a problem hiding this comment.
As discussed, 'System' will be added
db/umzug.ts
Outdated
There was a problem hiding this comment.
postgresconnectionobject - port is hardcoded to 5432 but should probably come from an envvar
db/umzug.ts
Outdated
There was a problem hiding this comment.
My personal preference is to use 'for..of' as opposed to any of the other looping syntax options since it's usually the safest and most accurate. However, this is a nitpick so feel free to ignore it if you like.
db/umzug.ts
Outdated
There was a problem hiding this comment.
Maybe it's possible to set up the logger from umzug to do both console and file based at the same time. Otherwise I guess just the console is fine.
lib/template/checklistv2.xml
Outdated
There was a problem hiding this comment.
Seems like this will require review to compare it against the Ckl->HDF->Ckl flow checklists to confirm that all the fields are mapped appropriately. Once discrepancy already found is that TIR automatically updates to match it against the STIG Library version which is fine I think. Another discrepancy is CHECK_CONTENT_REF. Another is the escaping for the xml/html areas, but I think those are semantically equivalent even if they're syntactically different. Another is that the 'legacy_id' field is missing, etc.
There was a problem hiding this comment.
maybe remove the alias section / commented out line
| @@ -16,6 +16,7 @@ export default defineNuxtConfig({ | |||
| database_name: process.env.DATABASE_NAME, | |||
| temp_folder: process.env.TEMP_FOLDER || "./tmp", | |||
| jwt_key: process.env.JWT_KEY, | |||
| secret_key: process.env.SECRET_KEY, | |||
There was a problem hiding this comment.
rename to reference it being related to the HMAC stuff
| @@ -16,6 +16,7 @@ export default defineNuxtConfig({ | |||
| database_name: process.env.DATABASE_NAME, | |||
| temp_folder: process.env.TEMP_FOLDER || "./tmp", | |||
| jwt_key: process.env.JWT_KEY, | |||
| secret_key: process.env.SECRET_KEY, | |||
| usesqlite: process.env.SQLITE, | |||
There was a problem hiding this comment.
it least for heimdall we try to avoid using verbs in the envvars. imo this could be renamed to just 'sqlite'.
| "pg": "~8.11.3", | ||
| "pg-hstore": "~2.3.4", | ||
| "pinia": "^2.1.7", | ||
| "postgres": "~3.3.5", | ||
| "sequelize": "~6.32.1", | ||
| "sqlite3": "~5.1.6", | ||
| "tar": "^6.2.0", | ||
| "ts-node": "^10.9.2", |
There was a problem hiding this comment.
I thought we were getting rid of ts-node? If it's no longer explicitly used by us then we can rid of it here I think.
There was a problem hiding this comment.
Not a blocker, but we will want to consider seeing which dependencies we can open up (^) vs keeping more locked down (~). I think it might be helped in the future by dependabot automating some of these validation/upgrade processes.
| @@ -4,6 +4,6 @@ export default defineEventHandler(async (event) => { | |||
| console.log( | |||
There was a problem hiding this comment.
reminder to swap out all the console logs for the logger
public/favicon.ico
Outdated
There was a problem hiding this comment.
Talk with artists/branding folks again w/r to coming up with a new logo.
Relatedly, still need to add the mitre branding.
There was a problem hiding this comment.
Add a section to this to make it clearer that you need to read the license/notice files.
Also should add like an authors section or something to clearly attribute work done by both the mitre and lm teams
| @@ -0,0 +1,79 @@ | |||
| import { Assessment, AssessmentItem, Stig, StigData } from "../../db/models"; | |||
|
|
|||
| // changed to make a blank but function actually checks for existing. need to split this logs for checklist.ts and stigLibrary | |||
There was a problem hiding this comment.
review these TODO comments to see if they're still applicable
| const newAssessment = await Assessment.create({ | ||
| SystemId: systemId, | ||
| StigId: stigId, | ||
| comment: "TIR", |
There was a problem hiding this comment.
where is this supposed to be in the ckl? is it intended on being different from the xml comment that's on like line 2 of the mustache template which says 'tir' and which version of tir is being used?
|
|
||
| export default defineEventHandler(async (event) => { | ||
| const body = await readBody(event); | ||
|
|
||
| const assessmentItem = await AssessmentItem.findByPk(body.id); | ||
| const rawToken = getCookie(event, "tirtoken"); |
There was a problem hiding this comment.
As based off of our discussion during this review / for the ldap stuff, we should be transitioning to an external auth package for nuxt that will make manually mucking around with cookies like this unnecessary.
| const isOwner = boundary?.dataValues.ownerId === userId; | ||
| const isMember = | ||
| boundary?.dataValues.Boundary_Users.find((o: { UserId: number }) => o.UserId === userId) !== | ||
| undefined; |
There was a problem hiding this comment.
not sure if you need to convert the undefined into an explicit boolean. since undefined is a falsy value, it was behave as equivalent to a boolean called false when used in conditionals and stuff.
| if ( | ||
| !isOwner && | ||
| boundary?.dataValues.Boundary_Users.find((o: { UserId: number }) => o.UserId === userId) | ||
| .BoundaryRoleId === 3 |
There was a problem hiding this comment.
having an enum here or something more clearer than '3' would be good.
| const assessmentItem = await AssessmentItem.findByPk(body.id); | ||
|
|
||
| if (assessmentItem) { | ||
| for (const key in body) { |
There was a problem hiding this comment.
for..in has issues (ex. inherited attributes will be iterated). my preference here would be to do a for..of of Object.entries(body) so that way you get both the key and the value at the same time and don't accidentally iterate over something you weren't intending to.
| }, | ||
| ], | ||
| }); | ||
| const isOwner = boundary?.dataValues.ownerId === userId; |
There was a problem hiding this comment.
As discussed, might want to do a research spike into the CASL stuff for not just page access but also for actions that you can do.
https://github.com/mitre/heimdall2/blob/master/apps/backend/src/casl/casl-ability.factory.ts
|
|
||
| if (assessmentItem) { | ||
| for (const key in body) { | ||
| assessmentItem.setDataValue(key, body[key]); |
There was a problem hiding this comment.
maybe we should validate the input here (body) to ensure that it has the shape that we need - sanitization, etc.
| import jwt from "jsonwebtoken"; | ||
| import { User, UserRole, Theme } from "../../../db/models"; | ||
|
|
||
| // import TokenExpiredError from 'jsonwebtoken' |
There was a problem hiding this comment.
Unauthorized - typo
Might be worthwhile doing a single pass of a spellchecker through the repo to catch this stuff before we merge it
| import jwt from "jsonwebtoken"; | ||
| import { User } from "../../../../db/models"; | ||
| import { verifyPassword } from "~/server/utils/hash"; |
There was a problem hiding this comment.
let's be consistent for pathing (tilda or doing the dots) but i think in this case it's a global func cause of how nuxt works with the utils so we could probably just delete this line
|
|
||
| const config = useRuntimeConfig(); | ||
|
|
||
| if (!config.jwt_key) { | ||
| throw new Error("jwt_key is not set."); | ||
| } | ||
|
|
||
| const SECRET_KEY = config.jwt_key as string; | ||
| if (!config.secret_key) { |
There was a problem hiding this comment.
maybe we should consider crashing on startup if the user has not supplied all the envvars it needs to run. the hope is that the rest of the code can be DRYer if we don't need to worry about these envvars not being initialized in multiple locations. maybe there's some way to force the nuxt config to state that the vars are required? it'd need some research.
| throw new Error("secret_key is not set."); | ||
| } | ||
|
|
||
| const JWT_KEY = config.jwt_key as string; |
There was a problem hiding this comment.
do not need to narrow/define the type since it knows that these are strings
| } | ||
|
|
||
| const JWT_KEY = config.jwt_key as string; | ||
| const SECRET_KEY = config.secret_key as string; | ||
|
|
||
| export default defineEventHandler(async (event) => { | ||
| console.log("entered local flow"); |
There was a problem hiding this comment.
logger if we're gonna keep this around, but i think this will be all reworked with the replacement auth stuff
| const cookieName = "tirtoken"; | ||
| setCookie(event, cookieName, jwt.sign({ userId: user.id }, SECRET_KEY, { expiresIn: "2h" }), { | ||
| setCookie(event, cookieName, jwt.sign({ userId: user.id }, JWT_KEY, { expiresIn: "2h" }), { |
There was a problem hiding this comment.
seems like you can get rid of the expires attribute down there. the secure attribute should probably be that commented out code.
|
|
||
| return { sucesss: true }; | ||
| await user.save(); |
There was a problem hiding this comment.
There's no error handling in the case that the transaction failed. It's fine that we don't cause the error gets bubbled up, but we should at least make sure that the UI shows that its a 500 failure instead of gaslighting the user into thinking that it's an issue on their end or not providing any info at all (the error modal pops up with no message inside).
| import { TirAlias } from "../../../db/models/tirAlias"; | ||
|
|
||
| export default defineEventHandler(async () => { | ||
| const tirAlias = await TirAlias.findAll(); |
There was a problem hiding this comment.
nitpick: should probably be 'aliases' or something else plural
There was a problem hiding this comment.
should probably figure out alias.put as well
| @@ -0,0 +1,291 @@ | |||
| import * as fs from "fs"; | |||
| import * as path from "path"; | |||
| import AdmZip from "adm-zip"; | |||
There was a problem hiding this comment.
might be worthwhile investigating if we want to swap to 'archiver'
questions: does it only work in a node context or can it work from a browser context too? can it reduce the number of dependencies that you have (since it has built in support for both zip and tar)?
| }, | ||
| }); | ||
| for (const assessment of currentAssessments) { | ||
| const oldStig = await Stig.findOne({ where: { id: assessment.StigId } }); |
There was a problem hiding this comment.
would help if we could self document / just actually straight up document that assessment.stigid is like the db id and then oldstig.stigid is the id coming from the stig itself (firefox blah blah)
| }); | ||
|
|
||
| for (const check of newChecks) { | ||
| const matchingCheck = findMatch(check, oldChecks); |
There was a problem hiding this comment.
maybe explore doing preprocessing here instead of doing a find in each iteration here. it'd be something that'd return the full union set as well as the set that remains unique to each assessment. you can iterate over all of that in one go then.
| } | ||
| }; | ||
|
|
||
| export const findMatch = (check: AssessmentItem, array: AssessmentItem[]) => { |
There was a problem hiding this comment.
nitpick: i like having functions that are depended upon be above their users, so in this case 'findmatch' is depended upon by 'migrateboundary' so it would be above 'migrateboundary'.
| let outputDirPath = baseOutputPath; | ||
|
|
||
| if (originalName) { | ||
| outputDirPath = path.join(baseOutputPath, originalName.substring(0, originalName.length - 4)); |
There was a problem hiding this comment.
instead of doing a '-4' on the filename, we could probably use some paths built-in to extract the basename of the file
| return array.find((item) => item.StigDatum?.rule_id === check.StigDatum?.rule_id); | ||
| }; | ||
|
|
||
| export const processLibrary = async ( |
There was a problem hiding this comment.
would be nice to have a diagram or docs or something explaining the user upload stig library -> gets saved to disk -> gets processed from disk and the content inserted into the db -> gets deleted from disk flow
| return array.find((item) => item.StigDatum?.rule_id === check.StigDatum?.rule_id); | ||
| }; | ||
|
|
||
| export const processLibrary = async ( |
There was a problem hiding this comment.
i think it would still be interesting to see if we could make this completely in memory so that we don't need to handle all the potential failures from doing disk io, but it would need to be a measured approach to determine how much memory usage spikes on the application. if the spikes are too large, then we might just be stuck.
| errorMessage?: string; | ||
| } | ||
|
|
||
| export const parseLibraryName = (filename: string): parseResults => { |
There was a problem hiding this comment.
as discussed, disa has changed the naming format so we can no longer extract out the information we need. we're going to need to explore if it can be extracted from some internal data structure.
| return fileList; | ||
| }; | ||
|
|
||
| interface parseResults { |
There was a problem hiding this comment.
i think interfaces are usually PascalCase not camelCase.
| error.errors.forEach((element) => { | ||
| logger.error(`[ERROR] ${libraryNameAttributes.filename} ${element.message}`); | ||
| }); | ||
| fs.rmSync(sourceZip); |
There was a problem hiding this comment.
as discussed, this delete is because we know for sure that the library has already been processed in full and so we no longer need the zip file. for all the other ones, we apparently want to keep the zip around since we aren't sure what could be causing the issues with saving it to the db.
if we're going to keep this stuff on disk, we should probably have a way for an admin to view these zipfiles and delete/clean them up if necessary so that way disk usage doesn't just grow indefinitely over time. it probably won't since nominally we will only need to upload a stig library on a quarterly basis and this area should be cleared out regularly enough when the application (if containerized) is redeployed but just to be on the safe side.
| const temporaryExtraction = path.join(outputDirectory, "tempExtraction"); | ||
| const mainZip = new AdmZip(sourceZip); | ||
| const fileList: string[] = []; | ||
| fs.mkdirSync(temporaryExtraction); |
There was a problem hiding this comment.
since we've already paid the 'coloring' price and marked this function as async, we can probably just use the async functions here and await them as necessary. used to be that they only had a callback variant or a sync variant, but now promise based variants exist so it's ergonomic enough.
https://nodejs.org/dist/latest-v16.x/docs/api/fs.html#fspromisesmkdirpath-options
| const mainZip = new AdmZip(sourceZip); | ||
| const fileList: string[] = []; | ||
| fs.mkdirSync(temporaryExtraction); | ||
| mainZip.extractAllTo(temporaryExtraction, true); |
| fs.mkdirSync(temporaryExtraction); | ||
| mainZip.extractAllTo(temporaryExtraction, true); | ||
|
|
||
| fs.readdirSync(temporaryExtraction).forEach((nestedFile) => { |
There was a problem hiding this comment.
This seems to be a completely independent set of operations so we can probably split this up into a collection of promises that are awaited as a group instead of having to sequentially do all of them like this
| xmlsExtracted: filelist.length, | ||
| }; | ||
|
|
||
| for (const xmlFile of filelist) { |
There was a problem hiding this comment.
this feels like it's going to be one of the slowest parts of the ingest just cause we're forced to await on each stig finishing being processed before we can move on. we discussed that the ingestion of these stigs needed to be done in order so as to prevent some conflicts that occasionally occurred.
i think we should spend some research time into seeing if we can as discussed do some pre-processing to figure out the dependency problem and then be able to go full async on upload into the db.
| @@ -16,6 +16,7 @@ export default defineNuxtConfig({ | |||
| database_name: process.env.DATABASE_NAME, | |||
| temp_folder: process.env.TEMP_FOLDER || "./tmp", | |||
| jwt_key: process.env.JWT_KEY, | |||
| secret_key: process.env.SECRET_KEY, | |||
There was a problem hiding this comment.
HMAC Rename to match previous comments.
| const fileHash = await hashFile(xmlFilePath); | ||
| stigLibrary: StigLibrary, | ||
| ): Promise<ParseStigResults> { | ||
| // const fileHash = await hashFile(xmlFilePath); |
There was a problem hiding this comment.
Remove unused code
| // const fileHash = await hashFile(xmlFilePath); |
| import { hashFile } from "./hash"; | ||
| import { Transaction, UniqueConstraintError } from "sequelize"; | ||
| import { Stig, StigIdent, StigLibrary, StigReference, StigResponsibility } from "../../db/models"; | ||
| // import { hashFile } from "./hash"; |
There was a problem hiding this comment.
Unused import
| // import { hashFile } from "./hash"; |
| import { Stig, StigLibrary, StigLibraryInterface } from "../../db/models"; | ||
| import { hashFile } from "./hash"; | ||
| import { Transaction, UniqueConstraintError } from "sequelize"; | ||
| import { Stig, StigIdent, StigLibrary, StigReference, StigResponsibility } from "../../db/models"; |
There was a problem hiding this comment.
Suggest removing relative pathing on import
| import { Stig, StigIdent, StigLibrary, StigReference, StigResponsibility } from "../../db/models"; | |
| import { Stig, StigIdent, StigLibrary, StigReference, StigResponsibility } from "~/db/models"; |
| populateStigModel(newStig, jsonObj); | ||
| newStig.dataValues.filename = path.basename(xmlFilePath); | ||
| newStig.dataValues.hash = fileHash; | ||
| // console.log("Trying to save to db"); | ||
| // newStig.dataValues.hash = fileHash; |
There was a problem hiding this comment.
Remove unused code
| // newStig.dataValues.hash = fileHash; |
| // const addRequestResult = await addStigToLibrary(newStig.dataValues.id, stigLibraryId); | ||
| // console.log(addRequestResult); |
There was a problem hiding this comment.
Unused code
| // const addRequestResult = await addStigToLibrary(newStig.dataValues.id, stigLibraryId); | |
| // console.log(addRequestResult); |
|
|
||
| let groupArray = jsonObj.Benchmark.Group; | ||
| if (!Array.isArray(groupArray)) { | ||
| groupArray = [groupArray]; | ||
| } | ||
| console.log("Starting Group processing for :", newStig.dataValues.title); | ||
| const perfTimer = new PerfTimer(); | ||
| perfTimer.enable(); |
There was a problem hiding this comment.
perfTimer should probably be turned off for production
| console.log(addRequestResult); | ||
| await stigLibrary.addStig(newStig, { transaction: stigImportTransaction }); | ||
| // const addRequestResult = await addStigToLibrary(newStig.dataValues.id, stigLibraryId); | ||
| // console.log(addRequestResult); | ||
|
|
||
| let groupArray = jsonObj.Benchmark.Group; | ||
| if (!Array.isArray(groupArray)) { | ||
| groupArray = [groupArray]; | ||
| } | ||
| console.log("Starting Group processing for :", newStig.dataValues.title); |
There was a problem hiding this comment.
Migrate to Winston
There was a problem hiding this comment.
Maybe we should rename importStig and importStigData to get rid of the verb and just be a noun like the other util modules (stigLibrary, etc)
| @@ -2,54 +2,96 @@ import * as fs from "fs"; | |||
| import { readFileSync } from "fs"; | |||
| import path from "path"; | |||
| import { parseStringPromise } from "xml2js"; | |||
There was a problem hiding this comment.
xml2js doesn't seem to be under as active maintenance as fast-xml-parser so you might want to transition to that. we currently use both xml2js and fast-xml-parser in the saf, but I want to consolidate on fast-xml-parser where possible.
the other thing that you might be interested in doing is that since xccdf is a format that has a schema, you could consider using mitre's fork of jsonix to do a more accurate mapping onto json via the xccdf schema so that you can sidestep issues that the parsers have to deal with like the singleton vs array of one question
| const fileHash = await hashFile(xmlFilePath); | ||
| stigLibrary: StigLibrary, | ||
| ): Promise<ParseStigResults> { | ||
| // const fileHash = await hashFile(xmlFilePath); | ||
| const xmlContent = readFileSync(xmlFilePath, "utf8"); |
There was a problem hiding this comment.
in functions that are already marked async, it might be worthwhile to just do an await on the async versions of these functions as opposed to using the sync versions
| unchangedCheckCount: 0, | ||
| errorCheckCount: 0, | ||
| }; | ||
|
|
||
| let jsonObj; |
There was a problem hiding this comment.
Instead of treating jsonObj as type 'any', might be worthwhile following along with my suggestion to use JSONIX to have it create a typescript type definition for you from the xccdf schema. If you don't want to do that, then you could just manually write out a type definition to match what the output of your parser should be giving you.
|
|
||
| const [newStig, created] = await Stig.findOrBuild({ | ||
| where: { filename: path.basename(xmlFilePath) }, | ||
| }); | ||
|
|
||
| try { | ||
| if (created) { | ||
| parseResults.newStig = true; | ||
| populateStigModel(newStig, jsonObj); |
There was a problem hiding this comment.
Usually I like trying to take a functional approach, but based off of our discussion it doesn't seem like it'd be that feasible to do here due to performance reasons. This is fine; however, my hope is that this is one of the few exceptions to the rule.
| processLibraryResults.updatedStigCount++; | ||
| } | ||
| } catch { | ||
| logger.error(`Error Parsig STIG: ${path.basename(xmlFile)}`); |
There was a problem hiding this comment.
| logger.error(`Error Parsig STIG: ${path.basename(xmlFile)}`); | |
| logger.error(`Error Parsing STIG: ${path.basename(xmlFile)}`); |
| const perfTimer = new PerfTimer(); | ||
| perfTimer.enable(); | ||
|
|
||
| const currentStigResponsibilities = await StigResponsibility.findAll(); |
There was a problem hiding this comment.
Could probably take all three of these do a Promise.all around them instead of individually awaiting them
| } | ||
|
|
||
| stigImportTransaction.commit(); | ||
|
|
||
| perfTimer.globalSummaryPrint(); | ||
| fs.rm(xmlFilePath, (err) => { |
There was a problem hiding this comment.
Discussion that happened here:
Notification system (that originally would just populate some application inbox, but could have reporters added to integrate with email or IRC equivalents) could provide a summary on the successes and failures of the import process. How many STIGs were ingested. How many files' deletions failed (which is a problem that the admin then needs to resolve at some point cause it could be a disk usage leak). etc.
| @@ -59,6 +101,7 @@ export async function parseXmlStig( | |||
| return { error: false, new: true }; | |||
There was a problem hiding this comment.
Another example of mismatch between return type defined in the function vs what is actually done
| } | ||
| console.log(`Adding stig: ${stig.dataValues.id} to library: ${stigLibrary.id}`); | ||
|
|
||
| // const addResult = addStigToLibrary(stig.dataValues.id, stigLibraryId); |
There was a problem hiding this comment.
remove commented out code
There was a problem hiding this comment.
Line 174 - StigLibraryInterface - is this interface still necessary? couldn't we adjust the modal to declare whatever attributes we needed?
|
|
||
| if (created) { | ||
| // console.log("No match found. Creating new stig data:", stigGroupObj.Rule.$["id"]); | ||
| console.log("No match found. Creating new stig data:", stigGroupObj.Rule.$["id"]); |
There was a problem hiding this comment.
Probably should not be doing console logs and logger stuff at the same time
| @@ -254,7 +307,7 @@ function sanitizeTextWithinTags(tagName: string | string[], xmlString: string): | |||
| function sanitizeForTag(tag: string, str: string): string { | |||
There was a problem hiding this comment.
might be worthwhile using a library to handle the sanitization just cause they are probably catching all the cases that we might be missing
| @@ -50,23 +50,56 @@ export async function parseStigData( | |||
| logger.error("Unknown error:", error); | |||
There was a problem hiding this comment.
It seems like it's fine with a partial upload of a stiglibrary succeeding, but we should have some sort of warning assigned to stigs that didn't get pulled through properly to explain that there might be some data loss or corruption that happened.
| import { TirAlias } from "../../../db/models/tirAlias"; | ||
|
|
||
| export default defineEventHandler(async () => { | ||
| const tirAlias = await TirAlias.findAll(); |
There was a problem hiding this comment.
should probably figure out alias.put as well
| @@ -0,0 +1,7 @@ | |||
| import { migrateBoundary } from "../../../server/utils/stigLibrary"; | |||
There was a problem hiding this comment.
Based off of our discussion, I think we are in agreement that it's probably best to just import the functionality from the util module every time instead of relying on it being a global.
| return vulnNumData ? vulnNumData.ATTRIBUTE_DATA : undefined; | ||
| } | ||
|
|
||
| type CklStigInfo = { |
There was a problem hiding this comment.
https://github.com/mitre/heimdall2/tree/master/libs/hdf-converters/src/ckl-mapper
might be worthwhile comparing the types that yall rolled out here with the types that we're using in the mapper. both the types spit out by jsonix which directly match the schema in all its painful ambiguity as well as the hand crafted types we derived from them.
| stigs: CklStig[]; | ||
| }; | ||
|
|
||
| export async function buildCklString<T extends boolean>( |
There was a problem hiding this comment.
I respect this, but I dunno how necessary it is to do the generics here.
| }); | ||
|
|
||
| if (!system) { | ||
| return "" as T extends true ? string[] : string; |
There was a problem hiding this comment.
why are we returning an empty string at all here?
in any case, you could just check the value of singlestigperckl to know if you needed to return a singleton or an array
There was a problem hiding this comment.
actually thinking about this a bit more, doesn't it make more sense to return an error here if we can't find a system that this ckl should apply to?
| } | ||
| } | ||
|
|
||
| if (singleStigPerCkl) { |
There was a problem hiding this comment.
i think the generics usage here could probably be simplified to just checking if it's singlestigperckl or not.
| return { result: false, error: false, errormsg: "" }; | ||
| } | ||
| } | ||
| logger.error("Unkown result from cci list validation.", { validationResult }); |
There was a problem hiding this comment.
| logger.error("Unkown result from cci list validation.", { validationResult }); | |
| logger.error("Unknown result from cci list validation.", { validationResult }); |
There was a problem hiding this comment.
also this is ckl verification, not anything to do with ccis
| import * as path from "path"; | ||
| import AdmZip from "adm-zip"; | ||
|
|
||
| export function zipDirectoryContents(sourceDir: string, outputFile: string) { |
There was a problem hiding this comment.
swap all of this to async / promises as opposed to nothing / callbacks
| if (!query.BoundaryId) { | ||
| throw createError({ | ||
| statusCode: 400, | ||
| statusMessage: `BoundaryId required.`, |
There was a problem hiding this comment.
linter might complain here about how you have a template string but nothing templated inside of it
| }); | ||
| } | ||
|
|
||
| if (query.SingleStigPerCkl === "true") { |
There was a problem hiding this comment.
move singlestigperckl variable definition here to where it's actually being used. could also probs simplify this to a ternary thing.
| const boundary = await Boundary.findOne({ | ||
| include: [ | ||
| { | ||
| model: Boundary_User, |
There was a problem hiding this comment.
After discussion, we probably shouldn't be pulling data from the join table.
There was a problem hiding this comment.
Will be handled in Issue #11
| } | ||
|
|
||
| if ( | ||
| !boundary.Boundary_Users.find((o: { UserId: number }) => o.UserId === userId) && |
There was a problem hiding this comment.
Probably also wanna remove the join table usages here as well
There was a problem hiding this comment.
Will be handled in issue #11
| logger.error(`Unabled to create dir in ${config.temp_folder}`); | ||
| throw createError({ | ||
| statusCode: 503, | ||
| statusMessage: "Unable to create Checklist.", |
There was a problem hiding this comment.
do we want to modify this status message to clarify that we've run outta space on the backend or is that information we want to obscure from the user / potential attacker?
| } | ||
|
|
||
| for (const system of boundary.Systems ?? []) { | ||
| const systemPath = path.join(dirPath, system.name); |
There was a problem hiding this comment.
considerations to have from user supplied names like this is a) invalid characters (ex. slashes, escaped characters ex \0, etc for both linux/windows/wherever else it might be deployed) and b) length of filepath (I think windows' max file length is surprisingly short relatively speaking).
we tested the \0 in the system name and it broke stuff so we gotta be careful about it.
| const zipFileName = `${config.temp_folder}/${boundary.name}.zip`; | ||
| zipDirectoryContents(dirPath, zipFileName); | ||
|
|
||
| const data: Buffer = await fs.readFile(zipFileName); |
There was a problem hiding this comment.
Seems like adm-zip has a function to return a zip object as a buffer instead of just to a zipfile on disk. This means that we could potentially avoid having to write the zip file to disk and then immediately read it again. Relevant since we forgot to do any checking on if the file was successfully written or read before execution proceeded - if we could avoid that being a necessity by everything just staying in memory then life is a bit easier.
Additionally, zipdirectorycontents should probably be doing validation itself alongside swapping to return the buffer.
Signed-off-by: Mark Rivera <[email protected]>
| }); | ||
| const isOwner = tier?.dataValues.ownerId === userId; | ||
| const isMember = | ||
| tier?.dataValues.Tier_Users.find((o: { UserId: number }) => o.UserId === userId) !== undefined; |
There was a problem hiding this comment.
Another join table usage that we probably want to remove
| } else { | ||
| throw createError({ | ||
| statusCode: 401, | ||
| statusMessage: "Not a Member of this Company", |
| ) { | ||
| throw createError({ | ||
| statusCode: 401, | ||
| statusMessage: "Reviewers are unable to create Boundaries", |
There was a problem hiding this comment.
join table usage here should be removed
There was a problem hiding this comment.
I prefer using guard clauses which I think in this case will also help with making the if statements easier to read.
On (what is currently) line 34, instead of doing if (i am not the owner AND i am not an admin) { blah } else { delete boundary } and then eventually doing the return with the success code, my preference would be if (i am the owner OR i am an admin) {delete boundary; return early with success code} and then the subsequent checks which themselves now no longer need to be indented up to the third level.
Probably also want to make the code DRYer by ideally having all the clauses that allow for one to delete a boundary within the same if statement with the early return, and then you have the error logging + throwing at the end of the function just the one time as well.
| @@ -66,11 +68,11 @@ export default defineEventHandler(async (event) => { | |||
| "ownerId", | |||
There was a problem hiding this comment.
I think we can extract out the types from sequelize and might not necessarily need to do this inferattributes thing. Additionally, even if we keep it, I think that updateboundaryrequest can be defined on this or directly on sequelize as well instead of being written by hand. The assumption is that both types are more or less the exact same, at least based off of my understanding of the cast that happens in line 84, but if a change happens to the model, there's nothing forcing an update to happen for the types in this file as well.
| @@ -86,14 +88,24 @@ export default defineEventHandler(async (event) => { | |||
| }); | |||
There was a problem hiding this comment.
missing building up the edit message here
| "PolicyDocumentId", | ||
| "ClassificationId", | ||
| "caveats", | ||
| ]; | ||
| let editMsg = "Changed:"; | ||
|
|
||
| if (boundary?.dataValues.ownerId !== userId && user?.dataValues.UserRole.id !== 1) { |
There was a problem hiding this comment.
same as for delete w/r to simplifying this section via guard clauses and DRY
| @@ -103,11 +115,18 @@ export default defineEventHandler(async (event) => { | |||
| attributesToUpdate.forEach((attr) => { | |||
There was a problem hiding this comment.
I think there was an accidental commit of this file. Providing an example file or response is fine, but this feels like it was something used for testing.
There was a problem hiding this comment.
Probably want to add two envvars: one is the debug level for the debugger output and the second is for dis/enabling the performance timer.
Incidentally, could probably pass the value of the dis/enable variable in the constructor of the timer object so that it turns any other functions you run into no-ops so that your usage of that object can be simpler by not having to remember to check if it is undefined first. Alternatively you could more or less keep what you're doing with the conditional instantiation of the object, but use the optional chaining operator wherever you're using the timer.
There was a problem hiding this comment.
https://github.com/mitre/tir/blob/Update-1.0/server/utils/cci.ts#L161 - just iterate through in batches as opposed to using a random cci as the breakpoint
https://github.com/mitre/tir/blob/Update-1.0/server/utils/cci.ts#L182 - probably want to move the reading of that file (and instantiation of that variable) closer to where it's being used (i'd put it right above line 190 where we define xsdDoc)
https://github.com/mitre/tir/blob/Update-1.0/server/utils/cci.ts#L182 - probably a good idea to have error checking here to make sure all the file read stuff and xml parsing stuff would be fine
There was a problem hiding this comment.
Delete the commented out code
There was a problem hiding this comment.
FindingCounts.arbitrary_key (as defined on line 24) is apparently not used according to our discussion. We should probably remove it since it's probably an error if we see something outside of the enum of expected values anyways.
There was a problem hiding this comment.
FindingsSheetItem might be a good target for another dev doc to explain where the different fields are sourced from
Might also be worthwhile having an additional tab or something within the generated spreadsheet that acts as a legend or guide so that the end user also knows where the stuff is sourced from (stig vs poam worksheet area vs wherever)
There was a problem hiding this comment.
We worked through an alternative implementation for the whereclause stuff on line 62 so that we didn't need to explicitly use the "is keyof" stuff by just doing object.entries.filter([,v] => v).map([k,] => k)
There was a problem hiding this comment.
Since the sequelize findAll will always return an array, you can get rid of the wrapping if statement on line 160
There was a problem hiding this comment.
Seems like you use that if (probablyanarray) { for (const thing of probablyanarray) } paradigm a lot but you could simplify to just for (const thing of probablyanarray || []) and then for (const thing of alwaysanarray) like for finding.assessmentitems.
There was a problem hiding this comment.
Currently we only have a client side validation that the system name is not an empty string. We should add that validation to the backend api / sequalize definition as well. Probably will hold true for other fields as well (boundary name?).
There was a problem hiding this comment.
Might be worthwhile swapping from procedural approaches to more functional approaches for iterating over the arrays for finding.assessmentitems and finding.stigidents.
Ex. for stigidents you could probs just do const identList = finding.StigIdents.map(i => i.text); and call it a day instead of constantly appending to the end of that array.
There was a problem hiding this comment.
For addFindings, another opportunity to potentially have used lodash (_.mergeWith). Anyways, you can get rid of the if statement in there I think. Checking if the value is non-zero before doing the addition seems pointless.
There was a problem hiding this comment.
For catFromSeverity,
- I would probably call it
catSeverityorcategorySeverityinstead ofromanSeverity. - Line 272 you defined the variable as
Catinstead of it being lowercase - Could probably simplify the instantiation of
catby using the square brace access into the map, ex.const cat = severityMap[rawSeverity] || "";.
There was a problem hiding this comment.
We need to determine if this is a public facing api endpoint or not.
There was a problem hiding this comment.
We spent some time and eventually discovered import {PassThrough} from "stream"; which is more than enough to handle our simple usecase and thereby not need to have the "into-stream" import here. Seems like it is probable that we can remove the import from the project entirely since we're doing the same thing in other locations.
This works because the xlsx workbook thing can write to a stream not just a buffer.
| @@ -21,6 +23,9 @@ export default defineEventHandler(async (event) => { | |||
|
|
|||
| setResponseHeader(event, "Content-Disposition", 'attachment; filename="Findings.txt"'); | |||
There was a problem hiding this comment.
might be worthwhile to have this match the frontend where it's ${boundaryname}-findings.xlsx or something like that as the default value but then have the option to use a provided value as the name
| @@ -2,14 +2,17 @@ import intoStream from "into-stream"; | |||
| import { sendStream } from "h3"; | |||
| import jwt from "jsonwebtoken"; | |||
| import { generatePoam } from "../../utils/excelExport/poamExport"; | |||
| import { User, Boundary, BoundaryInterface } from "../../../db/models"; | |||
|
|
|||
| export default defineEventHandler(async (event) => { | |||
| const rawToken = getCookie(event, "tirtoken"); | |||
There was a problem hiding this comment.
Another endpoint where we are unsure if it will be public or not. During the discussion it seemed like the intent was for it to be public, so we're going to need to replace the cookie thing with the actual auth solution using apikeys or some such.
After further discussion, revised auth is on the way but might not be here for this PR so should probably ensure that this endpoint and others (ex. the findingsdownload one) are still locked down to just application only.
| import { getIndexesByCciIds } from "../cci"; | ||
| import { text } from "stream/consumers"; |
There was a problem hiding this comment.
Delete the unused? export that might've been auto-added.
| @@ -9,6 +10,7 @@ export async function generatePoam( | |||
| const workbook = new ExcelJS.Workbook(); | |||
| const sheet = workbook.addWorksheet("Sheet1"); | |||
| sheet.views = [{ zoomScale: 80 }]; | |||
|
|
|||
| // const baseFont = {name: 'Calibri', size: 10}; | |||
There was a problem hiding this comment.
remove commented out code throughout the file
|
|
||
| export default defineEventHandler(async (event) => { | ||
| const rawToken = getCookie(event, "tirtoken"); | ||
| const config = useRuntimeConfig(); | ||
| const decodedToken = jwt.verify(rawToken, config.jwt_key) as { [key: string]: any }; | ||
|
|
||
| const userId = decodedToken.userId; | ||
| const user = await User.findByPk(userId); |
There was a problem hiding this comment.
If we need to do the userid validation, then we should do it here before we look up the user.
| const body = await readBody(event); | ||
| const boundary = (await Boundary.findByPk(body.BoundaryId)) as BoundaryInterface; | ||
|
|
||
| const poamWorkBook = await generatePoam(body.BoundaryId, isNaN(userId) ? undefined : userId); |
There was a problem hiding this comment.
If we move the validation up, then we can just pass the valid User object through to the generatePoam function and save on a db call. Alternatively, we could pass through precisely whatever attributes we need.
| @@ -374,7 +378,31 @@ export async function generatePoam( | |||
| const recommendations = 20; | |||
|
|
|||
| const results = await getEvaluationSummary(boundaryId, undefined, true); | |||
| const boundary = await Boundary.findByPk(boundaryId); | |||
|
|
|||
| interface BoundaryWithClassification extends Boundary { | |||
There was a problem hiding this comment.
usually one should not define an interface in-line in a function
after further discussion, yall are in the process of phasing these out because the sequelize models now include the join table info so you don't need to do it manually here.
there are other locations where this is happening
| @@ -374,7 +378,31 @@ export async function generatePoam( | |||
| const recommendations = 20; | |||
There was a problem hiding this comment.
This looks like you're trying to handroll an enum. Enums under the hood are integer indexed I believe, so you could just make an enum for this and grab the values that way.
https://bobbyhadz.com/blog/typescript-convert-enum-to-string
Don't forget that enums are PascalCase for both their own name as well as the items defined in them.
| @@ -374,7 +378,31 @@ export async function generatePoam( | |||
| const recommendations = 20; | |||
|
|
|||
| const results = await getEvaluationSummary(boundaryId, undefined, true); | |||
There was a problem hiding this comment.
The 'undefined' in the middle looks kinda wonky. I would recommend leaving the required parameter as it is (boundaryId), but moving the other two parameters to an options (or whatever you wanna call it) object that would have those parameters as optional attributes.
This would change this function call to look like getEvaluationSummary(boundaryId, {poam: true}).
| @@ -14,6 +14,7 @@ import { | |||
| import { FindingCounts } from "../../utils/findings"; | |||
| import { PerfTimer } from "../../utils/perfTimer"; | |||
| import { decodeToken } from "../../utils/currentUser"; | |||
There was a problem hiding this comment.
currentUser.ts has some commented out code in it
There was a problem hiding this comment.
Continue stripping out the join table stuff
| @@ -116,6 +117,7 @@ export default defineEventHandler(async (event) => { | |||
| model: Assessment, | |||
There was a problem hiding this comment.
for the variable allFindingsOrdered, is the variable name wrong and there's no explicit ordering other than what the db gives us or is the code wrong and something that should be ordered isn't?
There was a problem hiding this comment.
I prefer defining types outside of functions, and also if the type is derivative of another then I usually like to represent that via the various typescript utility types even if we then have to union a couple fields on top since at least it shows the lineage.
In this case, we might be interested in this one: https://www.typescriptlang.org/docs/handbook/utility-types.html#picktype-keys
| @@ -14,6 +14,7 @@ import { | |||
| import { FindingCounts } from "../../utils/findings"; | |||
There was a problem hiding this comment.
We want to explicitly import all the stuff from utils so will want to add initializeCounts here
| @@ -188,6 +197,10 @@ export default defineEventHandler(async (event) => { | |||
| }); | |||
| } | |||
|
|
|||
| if (!uniqueFinding.Stigs[0].id) { | |||
There was a problem hiding this comment.
After discussion, seems like this hack is going to be removed once the functionality is changed to create the assessment information upon the addition of a stig to a system.
There was a problem hiding this comment.
line 213, that variable definition could be made const and a one liner by doing a ternary statement.
There was a problem hiding this comment.
Might be worthwhile reviewing all the perftimer usages so that they're only for relevant / performance sensitive parts of the application. The usage right below line 213 to create the single object does not seem worthwhile to time.
There was a problem hiding this comment.
You have some closures defined at the bottom of this function definition (i.e. past the return statement but before the function closes) that have side effects for example adding on to the boundaryView list. I would separate them out into their own functions and at least pass in the boundaryView variable as a parameter so that it's clearer something is happening even if you make the modifications to that same variable as opposed to returning a new copy.
There was a problem hiding this comment.
Commented out code mostly at the bottom
Additional features added from Phase 2