feat: add support for pgp signing (#2577) #2882

b31ngd3v · 2023-04-02T19:50:39Z

part of #2577

terriko · 2023-04-10T18:12:51Z

Pinging @warthog9 -- does this look like the type of signing you'd expect to get as a mirror?

b31ngd3v · 2023-04-10T18:46:55Z

@terriko @warthog9 here is the public key and a sample file for testing purposes

warthog9 · 2023-04-12T03:17:07Z

The contents looks right to me at first glance.

The file extension should be .asc to follow RFC3156 section 9.2, not .sig.

I don't think I need to worry about the data in the .json, from the mirroring perspective (mostly in the realm of it's outside my personal scope on this), but is the "signed": true, even pertinent information in the json? The timestamp makes sense, there's value in that, but I'm not seeing the logic for 'signed' outside of knowing if the file was modified to indicate it wasn't signed when it was, just feels odd? (I.E. I can't think of a situation where it's inherently useful)

b31ngd3v · 2023-04-12T09:14:03Z

The contents looks right to me at first glance.

The file extension should be .asc to follow RFC3156 section 9.2, not .sig.

I don't think I need to worry about the data in the .json, from the mirroring perspective (mostly in the realm of it's outside my personal scope on this), but is the "signed": true, even pertinent information in the json? The timestamp makes sense, there's value in that, but I'm not seeing the logic for 'signed' outside of knowing if the file was modified to indicate it wasn't signed when it was, just feels odd? (I.E. I can't think of a situation where it's inherently useful)

@warthog9 The "signed": true will help the tool to know whether to fetch the .asc files or not. The value in signed is only used for this, nothing else. so if someone modifies the value in signed to false the tool will not fetch the .asc files and while importing the data it'll throw a error saying data is not signed and will not import the data. the tool will only import the data when the signature matches or the user explicitly uses --ignore-sig flag to ignore the signature.

codecov-commenter · 2023-04-12T12:55:16Z

Codecov Report

Merging #2882 (1878a68) into main (cbbdb92) will decrease coverage by 1.31%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             main    #2882      +/-   ##
==========================================
- Coverage   82.97%   81.66%   -1.31%     
==========================================
  Files         694      694              
  Lines       10754    10881     +127     
  Branches     1430     1476      +46     
==========================================
- Hits         8923     8886      -37     
- Misses       1465     1612     +147     
- Partials      366      383      +17

Flag	Coverage Δ
longtests	`81.66% <25.00%> (-0.77%)`	⬇️
win-longtests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
test/test_fetch_json_db.py	`100.00% <ø> (ø)`
cve_bin_tool/cvedb.py	`67.55% <18.60%> (-8.71%)`	⬇️
cve_bin_tool/fetch_json_db.py	`48.90% <26.22%> (-21.34%)`	⬇️
cve_bin_tool/cli.py	`68.29% <50.00%> (-0.42%)`	⬇️
cve_bin_tool/error_handler.py	`92.00% <100.00%> (+0.10%)`	⬆️
test/test_cvedb.py	`92.00% <100.00%> (ø)`

... and 15 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

b31ngd3v · 2023-04-18T00:13:24Z

@terriko requesting review, thanks!

warthog9 · 2023-04-18T00:25:56Z

I think my thinking with respect to "signed": true at all tends to lean towards "is it actually useful?" I can see the advantage of the data having that in it to trigger a downloading/checking, but since it doesn't really mean much if it's altered I'm wondering if that shouldn't be just a default in the program w/ a command line opt-in/out by default, or a check of "if there's a .asc present with the same name, blindly try to check" perspective.

I wouldn't want to give a false sense of something with it being there vs. not myself, and if it was me I'd probably vote to drop it in favor of something like an attempt to fetch and checking the return code if the file is present, etc.

terriko

Okay, thinking about the behaviour I'd expect:

if a .asc file exists, verify it. Halt everything if it fails, maybe after one or two wait-and-retry cycles in case the mirror just happened to be updating at that second.
if no .asc exists, log a warning but continue.
skip both these behaviours and don't download signatures or validate anything if you've passed in an ignore-signature option.
We might potentially also want a "permissive" mode, where it doesn't halt but any signature errors are logged prominently. I don't love this as an option but I can see an argument for "I'd rather have a partial scan and an error to tell me to re-run than nothing"

Adding signed:true into the json would just make two other possible failure cases that would complicate the user's understanding of what's going on:

json says it's signed but no .asc so things fail. I guess this could detect an attack where someone deleted the .asc file off the main mirror, but if they could delete those files why wouldn't they edit the json at the same time? Not what value this has in a reasonable threat model.
json says it's not signed but there is a .asc which then isn't used. Do we log this and hope users notice? Try to validate the .asc file and log a different error if it doesn't match? Does this leave a hole for attackers to modify the json on a mirror even if they can't edit the .asc signature on the main source?

So, I'm not seeing a huge value to the json signed:true entry, and I'm seeing some downsides. Let's take the json signed entry out; we can reconsider it if there's a viable attack it would prevent later.

b31ngd3v · 2023-04-18T22:55:25Z

Okay, thinking about the behaviour I'd expect:

if a .asc file exists, verify it. Halt everything if it fails, maybe after one or two wait-and-retry cycles in case the mirror just happened to be updating at that second.

if no .asc exists, log a warning but continue.

@terriko if no .asc file exists shouldn't it fail? and warn the user that the data is not signed and to ignore this error run it with --ignore-sig option? otherwise, if the data gets tampered then it might download tampered data.

terriko · 2023-04-19T00:22:54Z

Hm, you're right.

Failing if there's no .asc is safer, but if it comes up a lot it increases the chances that people will turn off signatures entirely, which would then be less safe.

So rethinking:

Default mode: if the signature check fails for any reason (.asc is wrong OR missing), retry (once?). If retrying doens't fix it, log error and halt program.
Log Signature Error mode (unsafe): Attempt to validate signatures. If signature fails, retry/log error as above but do not halt program.
Ignore Signature mode (extra unsafe but faster): Do not attempt to validate signatures.

I guess start with 1 and 3, and use data to see if signature errors come up enough that we want the middle road option of 2.

terriko

More of a code review with an eye to getting rid of the signed: true metadata option.

test/test_fetch_json_db.py

cve_bin_tool/cvedb.py

cve_bin_tool/fetch_json_db.py

b31ngd3v · 2023-04-23T21:20:28Z

@terriko requesting review, thanks!

terriko · 2023-04-24T18:13:47Z

Took a quick glance through and I think this is good enough that we can reasonably merge and iterate, but I think it deserves a more careful read-through than I can give it right now, so I'm going to flag it and try to come back later (likely tomorrow).

terriko

Okay, let's get this merged (finally!). And we'll see if we can get the mirrors doing something next.

b31ngd3v added 5 commits April 3, 2023 01:17

feat: add sign with pgp flag while exporting json data

93f0545

feat: verify sign while importing the json data

5d0328a

feat: update FETCH_JSON_DB to use pgp signing

aab997a

fix: update test_fetch_json_db.py

18aa1e8

Merge branch 'main' into add_sig

955a62e

b31ngd3v marked this pull request as ready for review April 5, 2023 09:07

fix: existing broken tests

dfdf7f8

terriko added the dependencies Pull requests that update a dependency file label Apr 10, 2023

b31ngd3v added 2 commits April 12, 2023 14:46

Merge branch 'main' into add_sig

5740723

fix: change the file extension to .asc

7f91497

Merge branch 'main' into add_sig

d52fe45

terriko requested changes Apr 18, 2023

View reviewed changes

terriko requested changes Apr 19, 2023

View reviewed changes

b31ngd3v added 2 commits April 24, 2023 00:40

fix: removed signed: true

cd13a99

Merge branch 'main' into add_sig

1976e8e

b31ngd3v requested a review from terriko April 23, 2023 19:13

b31ngd3v added 2 commits April 24, 2023 01:09

fix: update tests

82c4619

fix: update tests

e630076

terriko added the awaiting maintainer Need a maintainer to respond / help out label Apr 24, 2023

terriko mentioned this pull request Apr 26, 2023

discussion: feature wishlist brainstorming #2947

Closed

b31ngd3v added 3 commits May 31, 2023 22:03

Merge branch 'main' into add_sig

6c80636

Merge branch 'main' into add_sig

2607c57

Merge branch 'main' into add_sig

1878a68

terriko approved these changes Jun 1, 2023

View reviewed changes

terriko merged commit ba52b35 into intel:main Jun 1, 2023
18 of 22 checks passed

b31ngd3v deleted the add_sig branch June 2, 2023 00:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for pgp signing (#2577) #2882

feat: add support for pgp signing (#2577) #2882

b31ngd3v commented Apr 2, 2023

terriko commented Apr 10, 2023

b31ngd3v commented Apr 10, 2023

warthog9 commented Apr 12, 2023

b31ngd3v commented Apr 12, 2023 •

edited

codecov-commenter commented Apr 12, 2023 •

edited

b31ngd3v commented Apr 18, 2023

warthog9 commented Apr 18, 2023

terriko left a comment

b31ngd3v commented Apr 18, 2023 •

edited

terriko commented Apr 19, 2023

terriko left a comment

b31ngd3v commented Apr 23, 2023

terriko commented Apr 24, 2023

terriko left a comment

feat: add support for pgp signing (#2577) #2882

feat: add support for pgp signing (#2577) #2882

Conversation

b31ngd3v commented Apr 2, 2023

terriko commented Apr 10, 2023

b31ngd3v commented Apr 10, 2023

warthog9 commented Apr 12, 2023

b31ngd3v commented Apr 12, 2023 • edited

codecov-commenter commented Apr 12, 2023 • edited

Codecov Report

b31ngd3v commented Apr 18, 2023

warthog9 commented Apr 18, 2023

terriko left a comment

Choose a reason for hiding this comment

b31ngd3v commented Apr 18, 2023 • edited

terriko commented Apr 19, 2023

terriko left a comment

Choose a reason for hiding this comment

b31ngd3v commented Apr 23, 2023

terriko commented Apr 24, 2023

terriko left a comment

Choose a reason for hiding this comment

b31ngd3v commented Apr 12, 2023 •

edited

codecov-commenter commented Apr 12, 2023 •

edited

b31ngd3v commented Apr 18, 2023 •

edited