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.

Sign up for GitHub

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

Jump to bottom

Task: Improve _validate_threshold, so it tells how many more signatures are required #367

Open
MVrachev opened this issue Aug 24, 2023 · 2 comments
Open

Task: Improve _validate_threshold, so it tells how many more signatures are required #367

MVrachev opened this issue Aug 24, 2023 · 2 comments

Comments

@MVrachev
Copy link
Member

MVrachev commented Aug 24, 2023 •
edited
Loading

Improve _validate_threshold, so it tells how many signatures are required.
This will give more information why we fail validation of a metadata.
Please keep in mind this will most likely require changes in python-tuf method Metadata.verify_delegate as well.

Originally posted by @lukpueh in #355 (comment):

That would be great, but it's a bit tricky, because `_validate_threshold` is just a wrapper around 
`Metadata.verify_delegate`, which does not tell us how many signatures 
that counted towards the threshold were verified. 
I suggest to ticketize this. It might actually be an interesting feature in python-tuf.
@MVrachev MVrachev changed the title Task: Improve _validate_threshold, so it tells how many signatures are required Task: Improve _validate_threshold, so it tells how many more signatures are required Aug 24, 2023
@MVrachev MVrachev mentioned this issue Aug 24, 2023
Merged
@lukpueh lukpueh mentioned this issue Aug 24, 2023
Closed
@lukpueh
Copy link
Collaborator

lukpueh commented Oct 4, 2023 •
edited
Loading

Let's replace _validate_threshold with python-tuf's new get_verification_result for this, once it's released!

theupdateframework/python-tuf#2481

lukpueh added a commit to lukpueh/repository-service-tuf-worker that referenced this issue Oct 5, 2023
@lukpueh
Support partially signed metadata in metadata_update
48e8189 
Update `_root_metadata_update` subroutine of `metadata_update` task
interface to accept partially signed metadata. If the required threshold
is not met, the passed metadata is written to the "ROOT_SIGNING"
repository setting and the task returns with a "pending signatures"
message

Missing signatures can then be added using the `sign_metadata` task
interface, which also finalizes the metadata update, as soon as the
threshold is reached.

NOTE: Currently, there is no sanity check of signatures below the
threshold.  A useful check might be, that passed metadata has at least 1
initial and only valid signatures, akin to bootstrap. repository-service-tuf#367 will make
this a lot easier.

This change also includes a reordering of the validation routine to check
the version increment prior to signature threshold. Otherwise, a bad
version would only be detected after all signatures have been added.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh added a commit to lukpueh/repository-service-tuf-worker that referenced this issue Oct 20, 2023
@lukpueh
Support partially signed metadata in metadata_update
c7792d3 
Update `_root_metadata_update` subroutine of `metadata_update` task
interface to accept partially signed metadata. If the required threshold
is not met, the passed metadata is written to the "ROOT_SIGNING"
repository setting and the task returns with a "pending signatures"
message

Missing signatures can then be added using the `sign_metadata` task
interface, which also finalizes the metadata update, as soon as the
threshold is reached.

NOTE: Currently, there is no sanity check of signatures below the
threshold.  A useful check might be, that passed metadata has at least 1
initial and only valid signatures, akin to bootstrap. repository-service-tuf#367 will make
this a lot easier.

This change also includes a reordering of the validation routine to check
the version increment prior to signature threshold. Otherwise, a bad
version would only be detected after all signatures have been added.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
kairoaraujo pushed a commit that referenced this issue Nov 6, 2023
@lukpueh @MVrachev
Support 'metadata update' in 'sign_metadata' task (#355)
17d8dcb 
* Support 'metadata update' in 'sign_metadata' task

Implement support for distributed asynchronous root metadata signing
in the course of a "metadata update" event.

Other than the already supported "bootstrap" signing event, signatures
added to root during "metadata update" must validate with keys from
trusted OR new root, and meet the signature threshold of trusted AND new
root.

*Related changes:*
- Ignore obsolete "rolename" in sign_metadata payload. We only support
  root, and check the type when loading "ROOT_SIGNING".
- Refactor `_validate_{signature, threshold}` helpers to accept an
  optional delegator (e.g. trusted root).
- Add local `_result` helper to return a "sign metadata"-specific
  task result.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Fix failing tests for sign_metadata

- remove obsolete "test_sign_metadata_root_signing_no_bootstrap"
  Now, if there is no ongoing "bootstrap", we just assume there is an
  ongoing "metadata update".
- adopt changes in "test_sign_metadata_invalid_role_type"
  - new expected error message
  - fail earlier, before consulting with "BOOTSTRAP" state variable

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Add test_sign_metadata__update__invalid_signature

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Add test_sign_metadata__update__invalid_threshold

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Add test_sign_metadata__update__finalize

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Refactor test_sign_metadata__update* and add cases

Combine existing sign_metadata/update metadata tests in a single
parametrized test method and add additional test cases for different
return values from internal `_validate_{signature, threshold}` calls, in order
to test correct sage of OR/AND operators:
- signature must be valid according to trusted OR new root
- threshold must be met according to trusted AND new root

Note: The test removes asserts for internal method calls, which don't
seem so interesting, as long as we get the expected result.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Address misc review comments in sign_metadata

- Use elif instead of elsewhere appropriate
- Remove blank line in docstrings
- Clarify comment about signature/threshold validation

Co-authored-by: Martin Vrachev <martin.vrachev@gmail.com>
Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Assert _root_metadata_update result in sign_metadata

This is a temporary measure until after _root_metadata_update has been
refactored (see code comment) to not fail silently e.g in in tests that
mock the argument passed to _root_metadata_update.

This commit also updates the related tests to now mock the
`_root_metadata_update` result too. As the wrong result would no longer
fail silently.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Check 'role' field in 'sign_metadata' payload

'sign_metadata' only supports root, thus the role in the payload is
not relevant, and was ignored previously.

For consistency, this commit adds a check that the role is indeed
root and fails otherwise.

This is also tested by adding another column to the test table of
test_sign_metadata__update, used to patch the default payload in test
runs.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Re-use part of _root_metadata_update

Factor out "finalize" part of _root_metadata_update to re-use in
sign_metadata.

Prior to this commit, sign_metadata would call _root_metadata_update
duplicating much of the verification behavior, although it only cared
for the finalization part. Now, it can call into the desired subroutine
only.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Support partially signed metadata in metadata_update

Update `_root_metadata_update` subroutine of `metadata_update` task
interface to accept partially signed metadata. If the required threshold
is not met, the passed metadata is written to the "ROOT_SIGNING"
repository setting and the task returns with a "pending signatures"
message

Missing signatures can then be added using the `sign_metadata` task
interface, which also finalizes the metadata update, as soon as the
threshold is reached.

NOTE: Currently, there is no sanity check of signatures below the
threshold.  A useful check might be, that passed metadata has at least 1
initial and only valid signatures, akin to bootstrap. #367 will make
this a lot easier.

This change also includes a reordering of the validation routine to check
the version increment prior to signature threshold. Otherwise, a bad
version would only be detected after all signatures have been added.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Rename two boolean variables in sign_metadata

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Change test_sign_metadata__update test style

Use test copy pasta instead of @parametrize.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

* Remove unused mocks in test_sign_metadata__update

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

---------

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Co-authored-by: Martin Vrachev <martin.vrachev@gmail.com>
Co-authored-by: Kairo Araujo <kdearaujo@vmware.com>
@MVrachev
Copy link
Member Author

This issue is ready to be tackled and it's important.
I suggest we do it as early as we can @kairoaraujo and @KAUTH.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@lukpueh @MVrachev