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 new "create" command to CVElib for MITRE CVE Services 2.1.0 #17

Merged
merged 5 commits into from
Apr 19, 2022

Conversation

juspence
Copy link
Contributor

@juspence juspence commented Mar 15, 2022

@mprpic Probably shouldn't be merged until after the new API version is released, but this should be ready to go.
EDIT: And per yesterday's AWG meeting the release has been delayed to address findings uncovered during testing, so no rush to review this.

Tested against a local (Docker container) instance of CVEProject/cve-services using the "staging" branch, per docs in that repo: https://github.com/CVEProject/cve-services/blob/dev/docker/README.md

This reverts commit 8f0967e.

The change introduced by this commit has not been released to currently
deployed production version of CVE Services. It will be made available when
CVE Services 2.x is generally available.
@juspence juspence changed the title Add new "create" command to CVELib for MITRE CVE Services 2.1.0 Add new "create" command to CVElib for MITRE CVE Services 2.1.0 Mar 15, 2022
Copy link
Contributor

@mprpic mprpic left a comment

Choose a reason for hiding this comment

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

Can you also please add some examples of how to use this new functionality into the readme file?

cvelib/cli.py Outdated Show resolved Hide resolved
cvelib/cli.py Outdated Show resolved Hide resolved
cvelib/cve_api.py Outdated Show resolved Hide resolved
cvelib/cve_api.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
@mprpic mprpic changed the base branch from master to cve-services-2.1.0 March 21, 2022 13:13
@mprpic
Copy link
Contributor

mprpic commented Mar 21, 2022

I switched this to merge to the 2.1.0 branch instead of master. We can merge this branch into master once 2.1.0 is live and we are ready to publish a cvelib version that supports it.

@juspence juspence requested a review from mprpic March 22, 2022 18:26
Copy link
Contributor

@mprpic mprpic left a comment

Choose a reason for hiding this comment

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

LGTM!

@mprpic
Copy link
Contributor

mprpic commented Mar 23, 2022

Actually, couple more comments before this gets merged:

  • Can the input be validated with a simple json.loads so that we don't end up throwing a non-intuitive traceback on something like:

    $ cve create CVE-2021-0003 ''
    json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
  • The help text for cve create -h needs to make it clear that this is about CVE records in the JSON format. The argument name CVE_TEXT does not make that very clear. Perhaps we could call it cve_json_data? The text you added in the readme could also be shown in the -h output actually.

  • If we use an argument to specify the JSON string, it may be difficult to extend this to also support reading input from files or standard input (think echo <json_data>" | cve create CVE-123). If we used an option instead (e.g. -i for input and later -f for file) it would also be consistent with other subcommands that already use options (e.g. cve user create). Wdyt?

  • Testing this out against a locally running cve-services instance, I get this error:

    $ cve create CVE-2021-0003 '{"affected": [{"cpes": ["cpe:/o:redhat:linux:7.1"], "defaultStatus": "affected", "product": "Red Hat Linux", "vendor": "Red Hat"}], "descriptions": [{"lang": "en", "value": "There would be words here if this was real data."}], "providerMetadata": {"orgId": "19f229d4-f3d5-4605-bf93-521fa4499c06", "shortName": "test_org"}, "references": [{"name": "CVE-2001-0635", "url": "https://access.redhat.com/security/cve/CVE-2001-0635"}, {"name": "bz#1616605: CVE-2001-0635 security flaw", "url": "https://bugzilla.redhat.com/show_bug.cgi?id=1616605"}]}'
    ERROR: 400 Client Error: Bad Request for url: http://localhost:3000/api/cve/CVE-2021-0003/cna
    DETAILS: {'error': 'INVALID_JSON_SCHEMA', 'message': 'CVE cnaContainer JSON schema validation FAILED.', 'details': {'errors': [{'instancePath': '', 'schemaPath': '#/required', 'keyword': 'required', 'params': {'missingProperty': 'cnaContainer'}, 'message': "must have required property 'cnaContainer'"}]}}

    I guess it expects a full CVE record, not just the cna container, right? The test test with the cna container only though.

Couple of improvements we should file as issues and address in separate MR:

  • Add support for supplying CNA container in a file.
  • Automatically fill in all cveMetadata values in a record being submitted based on user's data (there are endpoints to look up UUIDs for orgs/users if I recall correctly).
  • Add more verbose docs on the topic of CVE IDs vs records and how to construct a record (perhaps point to vulnogram).
  • (maybe) Add optional integration tests against a locally running version of cve-services

@juspence
Copy link
Contributor Author

juspence commented Mar 23, 2022

Actually, couple more comments before this gets merged:

* Can the input be validated with a simple `json.loads` so that we don't end up throwing a non-intuitive traceback on something like:
  ```shell
  $ cve create CVE-2021-0003 ''
  json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
  ```

It already uses json.loads, so I wrapped it in a try-except block to print a friendly message about invalid CVE JSON, plus the traceback with the exact error message.

* The help text for `cve create -h` needs to make it clear that this is about CVE records in the JSON format. The argument name `CVE_TEXT` does not make that very clear. Perhaps we could call it `cve_json_data`? The text you added in the readme could also be shown in the `-h` output actually.

Fixed.

* If we use an argument to specify the JSON string, it may be difficult to extend this to also support reading input from files or standard input (think `echo <json_data>" | cve create CVE-123`). If we used an option instead (e.g. `-i` for input and later `-f` for file) it would also be consistent with other subcommands that already use options (e.g. `cve user create`). Wdyt?

You can pipe in arguments using "echo '{json_body}' | xargs cve create id" to do this, but it's not very user-friendly. I tried switching the argument to a --json option, which would let us add --file in the future. But using --option makes it...optional.

So the error message when it's missing isn't as clear, and this also breaks "echo '{json_body}' | xargs cve create id". It seems like options have to be passed to the command directly and can't be piped in. Even "echo '--json {json_body}' | xargs cve create id" and many other variations don't work.

* Testing this out against a locally running cve-services instance, I get this error:
  ```shell
  $ cve create CVE-2021-0003 '{"affected": [{"cpes": ["cpe:/o:redhat:linux:7.1"], "defaultStatus": "affected", "product": "Red Hat Linux", "vendor": "Red Hat"}], "descriptions": [{"lang": "en", "value": "There would be words here if this was real data."}], "providerMetadata": {"orgId": "19f229d4-f3d5-4605-bf93-521fa4499c06", "shortName": "test_org"}, "references": [{"name": "CVE-2001-0635", "url": "https://access.redhat.com/security/cve/CVE-2001-0635"}, {"name": "bz#1616605: CVE-2001-0635 security flaw", "url": "https://bugzilla.redhat.com/show_bug.cgi?id=1616605"}]}'
  ERROR: 400 Client Error: Bad Request for url: http://localhost:3000/api/cve/CVE-2021-0003/cna
  DETAILS: {'error': 'INVALID_JSON_SCHEMA', 'message': 'CVE cnaContainer JSON schema validation FAILED.', 'details': {'errors': [{'instancePath': '', 'schemaPath': '#/required', 'keyword': 'required', 'params': {'missingProperty': 'cnaContainer'}, 'message': "must have required property 'cnaContainer'"}]}}
  ```
    
  I guess it expects a full CVE record, not just the cna container, right? The test test with the cna container only though.

It does take only the CNA container, not the full CVE record, but the API changed so now we have to wrap the container. Added a fix.

Couple of improvements we should file as issues and address in separate MR:

* Add support for supplying CNA container in a file.

* Automatically fill in all cveMetadata values in a record being submitted based on user's data (there are endpoints to look up UUIDs for orgs/users if I recall correctly).

* Add more verbose docs on the topic of CVE IDs vs records and how to construct a record (perhaps point to vulnogram).

* (maybe) Add optional integration tests against a locally running version of cve-services

Filed #18, #19, #20, and #21 for these.

@mprpic
Copy link
Contributor

mprpic commented Mar 24, 2022

You can pipe in arguments using "echo '{json_body}' | xargs cve create id" to do this, but it's not very user-friendly. I tried switching the argument to a --json option, which would let us add --file in the future. But using --option makes it...optional.

So the error message when it's missing isn't as clear, and this also breaks "echo '{json_body}' | xargs cve create id". It seems like options have to be passed to the command directly and can't be piped in. Even "echo '--json {json_body}' | xargs cve create id" and many other variations don't work.

You can mark the option as required=True to make it mandatory. Let's not worry about the echo '' | cve... use case then, I doubt many people would use it for raw input.

@mprpic mprpic merged commit 39d58aa into RedHatProductSecurity:cve-services-2.1.0 Apr 19, 2022
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