-
Notifications
You must be signed in to change notification settings - Fork 61
Don't require hashes and sizes of Targets objects in Snapshot metadata. #1162
Conversation
+1 for the idea, can't comment on the code. |
break; | ||
case Hash::Type::kSha512: | ||
if (Hash(Hash::Type::kSha512, boost::algorithm::hex(Crypto::sha512digest(canonical))) != it) { | ||
LOG_ERROR << "Hash verification for " << role.ToString() << " metadata failed"; | ||
return false; | ||
} | ||
hash_exists = true; | ||
break; | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous code version returns false in 'default' case while the PR version returns true. Is it desired behavior ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, hence the comment above. The goal of this PR is to make this very hash check non-required, hence it returns true by default. I could've removed it entirely, but for now I decided to leave it.
if (meta_version.isIntegral()) { | ||
role_version_[role_object] = meta_version.asInt(); | ||
} else { | ||
role_version_[role_object] = -1; | ||
} | ||
|
||
// Size and hashes are not required, but we may as well record them if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we assume that hashes_list and meta_size won't return true for isObject() if there is no "hashes" or "length" in the json/meta_list ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, isObject
is false if the field is not present.
Maybe have the commit title and/or message mention that it concerns the Snapshot metadata? |
Sure, I can do that. (Update: just saw that you approved it, thanks!) |
c8da0db
to
d0b705b
Compare
Codecov Report
@@ Coverage Diff @@
## master #1162 +/- ##
=========================================
- Coverage 78.01% 77.9% -0.12%
=========================================
Files 170 170
Lines 10006 9991 -15
=========================================
- Hits 7806 7783 -23
- Misses 2200 2208 +8
Continue to review full report at Codecov.
|
Uptane (and TUF) no longer require them. If they are there, we will use them, and if not, we skip them. This provides no real security benefit, but may help detect implementation faults. For more information, see: uptane/uptane-standard#90 uptane/uptane-standard#92 Signed-off-by: Patrick Vacek <[email protected]>
d0b705b
to
65921af
Compare
Uptane (and TUF) no longer require them. If they are there, we will use
them, and if not, we skip them. This provides no real security benefit,
but may help detect implementation faults.
For more information, see:
uptane/uptane-standard#90
uptane/uptane-standard#92
Please also review advancedtelematic/tuf-test-vectors#49. That needs to get merged first.