Metadata API: Implement threshold verification #1436

jku · 2021-06-04T14:23:14Z

#1417 is merged so this is good to go

The delegating Metadata (root or targets) verifies that the delegated metadata is signed by required threshold of keys for the delegated role.

This implementation raises UnsignedMetadataError if threshold is not met. Other possible exceptions are programming errors.

Fixes #1306

sechkova

Maybe I've become biased after looking at the code in the next-gen client for a while because I couldn't point out any issues :)

I was searching for a way to avoid adding role_name as an argument of verify_delegate but I saw you already opened #1425 about it so +1 from me on that.

MVrachev

I didn't found any issues either.
LGTM!

tests/test_api.py

MVrachev · 2021-06-08T11:19:30Z

tuf/api/metadata.py

+            try:
+                key.verify_signature(delegate, signed_serializer)
+                # keyids are unique. Try to make sure the public keys are too
+                signing_keys.add(key.keyval["public"])


Maybe making sure that we have keyval["public"] uniqueness can be done instead in Root and Targets?

yeah possible but I'd say it's a separate issue: #1429

I only very recently realised the TUF spec does not require the existence of "public" at all -- the three keys defined in the spec all happen to have it but it isn't required.

So this line needs to change. The spec does require that KEYIDs are unique so verify_delegate() will just respect that and will not try to poke into the internals.

tests/test_api.py

tuf/api/metadata.py

jku · 2021-06-11T18:36:03Z

tuf/api/metadata.py

+                keys = self.signed.delegations.keys
+                # Assume role names are unique in delegations.roles: #1426
+                roles = self.signed.delegations.roles
+                role = next((r for r in roles if r.name == role_name), None)


Also from trishank: "What does this line do? Looks like magic"
My response was:

it returns the first role in delegations with name role_name (or None if no match).

I agree it's not pretty and a bit too clever.
I could do something like this instead:

role = None for delegated_role in roles: if delegated_role.name == role_name: role = delegated_role break

but that's 5 lines of initialize-then-modify antipattern: not the end of the world but not perfect either.

the real fix is probably #1426 though (make roles a OrderedDict). Then this code becomes readable and efficient:
role = roles.get(role_name)

I think I'd leave it as is ... but if others think that's too clever I'm happy to change to the more verbose one

Perhaps add a one-liner comment for less familiar readers?

Find the first role in delegations with name role_name (or None if no match)

MVrachev · 2021-06-15T14:58:04Z

In our discussion with @jku in issue #1438 we agreed that we should make sure that the metadata is accepted even if it has a couple of invalid keys, but it accommodates the requirement placed by threshold.
I think I don't see a test for that. Do you plan to add one @jku?

jku · 2021-06-16T10:30:20Z

In our discussion with @jku in issue #1438 we agreed that we should make sure that the metadata is accepted even if it has a couple of invalid keys, but it accommodates the requirement placed by threshold.
I think I don't see a test for that. Do you plan to add one @jku?

I think there are two things that we should definitely test:

test that Key.verify_signature() raises UnsignedMetadataError (or potentially a derived error) if keys are in any way invalid. This should happen in Handle exceptions in verify #1435, I've added the tests that I think make sense there
test that verify_delegate() handles UnsignedMetadataErrors coming from Key.verify_signature(): catching that error is fine as long as threshold is eventually reached: this should happen in this PR. It does look like there is no test case with failing signatures that should eventually succeed (because threshold is still reached). That would make sense to add (but it does not need to be about invalid keys in particular).

Soo, yes I guess I plan to add one now...

jku · 2021-06-16T19:26:08Z

rebased on develop, added a commit to add a tests for

signature that fails to verify, but overall signatures reach threshold
threshold that actually contains multiple signatures

This also changes one error type as suggested by teodora

MVrachev

Thanks for adding the tests!
Seems good to me!

jku · 2021-06-22T07:56:30Z

Added a commit that removes the attempt to enforce key contents uniqueness:

I only very recently realised the TUF spec does not require the existence of "public" key within the Key.keyval dictionary at all -- the three keys defined in the spec all happen to have it but it isn't required.
We know that the attempt is not protection against maliciously crafted keys anyway: creating multiple representations of the same key is not difficult

Luckily the spec requires that KEYIDs are unique so verify_delegate() will just respect that and will not try to poke into the internals of the keys.

joshuagl

This looks good to me. I wonder if the parameter names in verify_delegate could be renamed to more clearly indicate that role_name and delegate are related (an identifier and an instance)?

tuf/api/metadata.py

joshuagl · 2021-06-29T08:52:20Z

tuf/api/metadata.py

+                keys = self.signed.delegations.keys
+                # Assume role names are unique in delegations.roles: #1426
+                roles = self.signed.delegations.roles
+                role = next((r for r in roles if r.name == role_name), None)


Perhaps add a one-liner comment for less familiar readers?

Find the first role in delegations with name role_name (or None if no match)

jku · 2021-07-05T12:09:50Z

I chose the more verbose names: it's easy to lose track of where the keys and thresholds really come from and what they are applied on so more verbose seems reasonable.

The delegating Metadata (root or targets) verifies that the delegated metadata is signed by required threshold of keys for the delegated role. Calling the function on non-delegator-metadata or giving a rolename that is not actually delegated by the delegator is considered a programming error and ValueError is raised. If the threshold is not reached, UnsignedMetadataError is raised. Tweak type annotation of Delegations.keys to match the one for Root.keys (so they can be assigned to same local variable). Signed-off-by: Jussi Kukkonen <[email protected]>

Make sure verify_delegate() succeeds when threshold is reached even if some signatures fail to verify. Make sure higher threshold (2/2) works. Change error type for "Call is valid only on delegator metadata" error. Signed-off-by: Jussi Kukkonen <[email protected]>

There was an attempt at ensuring key content uniqueness in verify_delegate() by making sure the values corresponding to "public" keys in Key.keyval dictionaries are unique. This had two issues: * it wasn't a security measure: it's not difficult to produce two different "public" values of the same key content * Spec does not actually guarantee the existence of "public" key in the keyval dictionary (the three keys included in the spec just all happen to have it) Luckily the spec does require KEYIDs to be unique so we do not need to do all this: Just count keyids of keys with verified signatures. Keep building a Set of keyids as a belt-and-suspenders strategy: Role keyids are currently guaranteed to be unique but we'd notice here if they weren't. Add a logger call for failed verifys: this might useful to figure out which keys exactly are the issue when a delegate can not be verified. Signed-off-by: Jussi Kukkonen <[email protected]>

jku · 2021-07-05T12:16:48Z

Someone made mypy rules stricter in the mean time 😬 so had to rebase and fix the annotation (no changes in existing commits, only in two new commit).

joshuagl

I really like how easy this is to follow, nice work.

tuf/api/metadata.py

We don't neeed those tests given that verify_with_threshold is a placeholder until theupdateframework#1436 is merged. Signed-off-by: Martin Vrachev <[email protected]>

* Rename arguments so connection between the role name and the metadata is stronger. * Also add a comment on the list comprehension + next() trick. * Add return value annotation * Raise early if delegations is None to make the flow more obvious (and modify test case so we have coverage for the new case) Signed-off-by: Jussi Kukkonen <[email protected]>

jku · 2021-07-11T08:37:55Z

Yes, agree with all suggestions. I've combined all the improvements from suggestions into one commit.

The changes since last push are

use if self.signed.delegations is None to make that path clearer as suggested
Add a test for this specific case (this required modifying the test data)
tweak comments as suggested

It's minor enough that I don't think this absolutely needs another approval (so I will merge with the one I have) but ... there's not a lot of code that's more critical for TUF so feel free to have another look

MVrachev

I looked at the new changes and it seems good to me!

joshuagl

LGTM, thanks!

jku force-pushed the verify-delegate branch from b9595f3 to af904b3 Compare June 4, 2021 14:30

MVrachev mentioned this pull request Jun 7, 2021

Metadata Attribute research: KEY #1438

Closed

jku mentioned this pull request Jun 8, 2021

Merge ngclient: a new client library implementation #1408

Merged

sechkova approved these changes Jun 8, 2021

View reviewed changes

MVrachev approved these changes Jun 8, 2021

View reviewed changes

tests/test_api.py Show resolved Hide resolved

MVrachev reviewed Jun 8, 2021

View reviewed changes

MVrachev reviewed Jun 9, 2021

View reviewed changes

tests/test_api.py Outdated Show resolved Hide resolved

MVrachev mentioned this pull request Jun 9, 2021

Metadata Attribute research: Threshold #1439

Closed

jku commented Jun 11, 2021

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

jku force-pushed the verify-delegate branch 2 times, most recently from 3e6def1 to 3185a30 Compare June 11, 2021 18:27

jku commented Jun 11, 2021

View reviewed changes

jku marked this pull request as ready for review June 11, 2021 18:39

jku mentioned this pull request Jun 14, 2021

Improve testing using generated metadata #1444

Closed

jku force-pushed the verify-delegate branch from 5f3e57e to 09e9bd8 Compare June 16, 2021 19:24

jku force-pushed the verify-delegate branch from 09e9bd8 to 1e54bed Compare June 17, 2021 07:18

MVrachev approved these changes Jun 21, 2021

View reviewed changes

joshuagl reviewed Jun 29, 2021

View reviewed changes

Jussi Kukkonen added 3 commits July 5, 2021 15:13

jku force-pushed the verify-delegate branch from b351b1e to eab2896 Compare July 5, 2021 12:15

jku mentioned this pull request Jul 6, 2021

Metadata API: Fix keyval "public" requirement #1480

Merged

3 tasks

joshuagl self-requested a review July 7, 2021 08:40

joshuagl mentioned this pull request Jul 7, 2021

ngclient: Use Metadata APIs verify_with_threshold() when possible #1413

Closed

jku mentioned this pull request Jul 8, 2021

ng client improve TrustedMetadataSet testing #1477

Merged

3 tasks

joshuagl approved these changes Jul 8, 2021

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

tuf/api/metadata.py Outdated Show resolved Hide resolved

tuf/api/metadata.py Outdated Show resolved Hide resolved

jku force-pushed the verify-delegate branch from eab2896 to 271d5b7 Compare July 11, 2021 08:28

MVrachev approved these changes Jul 12, 2021

View reviewed changes

joshuagl approved these changes Jul 12, 2021

View reviewed changes

joshuagl merged commit bd5912b into theupdateframework:develop Jul 12, 2021

jku deleted the verify-delegate branch December 30, 2024 09:10

Metadata API: Implement threshold verification #1436

Metadata API: Implement threshold verification #1436

Uh oh!

Conversation

jku commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sechkova left a comment

Choose a reason for hiding this comment

Uh oh!

MVrachev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MVrachev Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

jku Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

jku Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jku Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshuagl Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

MVrachev commented Jun 15, 2021

Uh oh!

jku commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jku commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MVrachev left a comment

Choose a reason for hiding this comment

Uh oh!

jku commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshuagl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshuagl Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

jku commented Jul 5, 2021

Uh oh!

jku commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshuagl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jku commented Jul 11, 2021

Uh oh!

MVrachev left a comment

Choose a reason for hiding this comment

Uh oh!

joshuagl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jku commented Jun 4, 2021 •

edited

Loading

jku Jun 22, 2021 •

edited

Loading

jku Jun 11, 2021 •

edited

Loading

jku commented Jun 16, 2021 •

edited

Loading

jku commented Jun 16, 2021 •

edited

Loading

jku commented Jun 22, 2021 •

edited

Loading

jku commented Jul 5, 2021 •

edited

Loading