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 Question ooi model and create the first bit that generates a question #1407

Merged
merged 22 commits into from
Jul 25, 2023

Conversation

Donnype
Copy link
Contributor

@Donnype Donnype commented Jul 17, 2023

Changes

Add Question ooi model and create the first bit that generates a question.

Issue link

(Partially) closes #288

Proof

image


Code Checklist

  • All the commits in this PR are properly PGP-signed and verified;
  • This PR only contains functionality relevant to the issue; tickets have been created for newly discovered issues.
  • I have written unit tests for the changes or fixes I made.
  • For any non-trivial functionality, I have added integration and/or end-to-end tests.
  • I have performed a self-review of my code and refactored it to the best of my abilities.

Communication

  • I have informed others of any required .env changes files if required and changed the .env-dist accordingly.
  • I have made corresponding changes to the documentation, if necessary.

Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.


Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

@Donnype Donnype requested a review from a team as a code owner July 17, 2023 12:57
@Donnype Donnype changed the title Add Question ooi model and create the first bit that generates a question. Add Question ooi model and create the first bit that generates a question Jul 17, 2023
ammar92
ammar92 previously approved these changes Jul 18, 2023
Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

Just a few remarks, but looks good to me 👍

Comment on lines +17 to +18
with (Path(__file__).parent / "question_schema.json").open() as f:
schema = json.load(f)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering. If this schema is not expected to change during a worker's lifetime or (statically) shipped with a bit, then you might want to do this reading outside of the function. The performance gain is obviously not significant, but it would make more sense if this schema variable is static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is quite nice to get the feedback of the changes without restarting Octopoes though

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes agreed, but it's unlikely that this file be altered during development or production time right? And if it's changed during development, restarting shouldn't take that long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loading the file also doesn't take too long of course. And if there are issues with the file, perhaps we prefer a bit runtime error instead of an import-time error?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer not to do unnecessary IO calls for things that never change. Changing bit (or plugins in general) code or input should always yield a newer version of that plugin.

Runtime errors are worse because they're much harder to catch. In case of the import-time error, that would mean someone developing an invalid plugin build which of course will be caught during a review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shoudn't we just put the schema in the questionOOI? As this would allow us to also support other questions in our next iterations of this functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

That QuestionOOI would then in turn need to be created by another bit which we can run on each 'network' being created. The flow would then be:
Bit: For each network, there must be a Ports configOOI, of not, create a QuestionOOI with the jsonschema from this PR. The Form is rendered using the schema value from this QuestionOOI.
The input on this form is then ingested by a normlizer specific for this schema, and creates the configOOI, which then also signals the question to be removed.

Bit: The config is used by the OpenPorts bit(s) to see if there are any issues.

Copy link
Contributor Author

@Donnype Donnype Jul 19, 2023

Choose a reason for hiding this comment

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

Shoudn't we just put the schema in the questionOOI? As this would allow us to also support other questions in our next iterations of this functionality.

@underdarknl We are putting the schema in the QuestionOOI? We can just add new bits that parse different schema files or parse other schema files in this bit if we want to?

That QuestionOOI would then in turn need to be created by another bit which we can run on each 'network' being created. The flow would then be: Bit: For each network, there must be a Ports configOOI, of not, create a QuestionOOI with the jsonschema from this PR. The Form is rendered using the schema value from this QuestionOOI. The input on this form is then ingested by a normlizer specific for this schema, and creates the configOOI, which then also signals the question to be removed.

Bit: The config is used by the OpenPorts bit(s) to see if there are any issues.

I am not sure I understand. The bits don't do checks such as "if X does not exist yet", instead they are mostly idempotent which makes sense as we can manually retrigger them in the interface as well. The current QuestionOOI natural key setup means that is not an issue.

The flow you describe is also visualized in the issue and this flow is implemented in the subsequent PRs, the only difference being that I made the normalizer generic for now. Or do you see differences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ammar92 If we load all schema's for bits, one can also argue that we are leaving a memory footprint for a bit that runs only when we add a network (=almost never)?

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea of loading schema's for future use is not relevant if you ask me, Its like asking us if we want to pre-load the values of other OOI's too.

octopoes/octopoes/models/ooi/question.py Show resolved Hide resolved
@Darwinkel
Copy link
Contributor

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset.
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • I confirmed that the PR's advertised feature or hotfix works as intended.

What works:

  • No apparent regressions
  • A new Question OOI is created when a network is added with clearance > L2

@dekkers dekkers merged commit 1e4f2ab into main Jul 25, 2023
@dekkers dekkers deleted the feature/add-question-ooi-model branch July 25, 2023 12:20
jpbruinsslot added a commit that referenced this pull request Jul 25, 2023
* main: (95 commits)
  Translations for release 1.11 - EN -> NL, PAP (#1439)
  Add Question ooi model and create the first bit that generates a question (#1407)
  make port classification configurable (#1418)
  KATalogus API filtering and pagination (#1405)
  Fix robot test (#1420)
  Use the correct clearance level variable in organization member list template (#1427)
  Fix translation in Debian package (#1432)
  Reschedule tasks when no results in bytes are found after grace period (#1410)
  Don't scan hostname nmap in nmap boefje (#1415)
  Add and use our own CVE API (#1383)
  Add `task_id` as a query parameter to the `GET /origins` endpoint (#1414)
  Remove member group checks and check for permission instead (#1275)
  Bump cryptography from 41.0.0 to 41.0.2 in /boefjes/boefjes/plugins/kat_ssl_certificates (#1396)
  Bump cryptography from 41.0.1 to 41.0.2 in /bytes (#1397)
  Build the Debian build image on the main branch (#1387)
  Add explicit `black` config to all modules (#1395)
  Fix <no title> in the user guide docs (#1391)
  Add configurable octpoes request timeout (#1382)
  Remove hardcoded clearance level in member list for superusers (#1390)
  Add Debian build depends for CVE API package (#1384)
  ...
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.

Question OOIs with json-schema
6 participants