Skip to content

Sort signatures by keys IDs for deterministic signature order#155

Merged
trishankatdatadog merged 10 commits intotheupdateframework:masterfrom
ethan-lowman-dd:ethan.lowman/sorted-signatures
Oct 18, 2021
Merged

Sort signatures by keys IDs for deterministic signature order#155
trishankatdatadog merged 10 commits intotheupdateframework:masterfrom
ethan-lowman-dd:ethan.lowman/sorted-signatures

Conversation

@ethan-lowman-dd
Copy link
Copy Markdown
Contributor

@ethan-lowman-dd ethan-lowman-dd commented Sep 16, 2021

In the course of implementing delegations in the repo writer (WIP branch), I had to change how keys are stored internally, which introduced non-determinism in the order of signatures. This caused tests which relied on fixtures to become flaky, as the order of signatures was not consistent. In order to keep support for static fixtures (using deterministic signature schemes like Ed25519), this PR sorts signatures by key ID.

I regenerated the fixtures using regenerate-metadata.sh, which reordered signatures of root.json, and updated dependent hashes.

I also updated the two tests that made assertions about signature/key slices to disregard order. In the process I fixed a test on line 1174 of repo_test.go which was comparing expected to itself rather than comparing expected to actual.

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 16, 2021

Pull Request Test Coverage Report for Build 1338037733

  • 31 of 35 (88.57%) changed or added relevant lines in 2 files are covered.
  • 26 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.2%) to 67.809%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/signer/sort.go 22 26 84.62%
Files with Coverage Reduction New Missed Lines %
pkg/keys/keys.go 3 30.0%
data/types.go 11 59.02%
pkg/keys/rsa.go 12 61.8%
Totals Coverage Status
Change from base Build 1312286690: 0.2%
Covered Lines: 1959
Relevant Lines: 2889

💛 - Coveralls

@trishankatdatadog
Copy link
Copy Markdown
Contributor

Best for @hosseinsia to review as it might affect fixtures in his #143 PR

Comment thread internal/signer/sort.go
Comment thread repo.go
Comment thread repo_test.go Outdated
@rdimitrov
Copy link
Copy Markdown
Contributor

Apart from #143, it's also worth syncing with @asraa because of #148 as both of the PRs alter/depend on the same sign package.

In that sense, maybe it would be better to prioritize which of these should be merged first to avoid the burden of fixing conflicts afterwards?

Copy link
Copy Markdown
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks! I'm not opposed to this change, but could you point me to where the testdata needed to be ordered?

I'm seeing a few checks in the interop_test/client_test but they don't seem to require ordering.

Just wondering if it makes more sense to change tests to test equality/expectation without ordering

Comment thread repo_test.go
@ethan-lowman-dd
Copy link
Copy Markdown
Contributor Author

@asraa The tests that are flaky if signature order is nondeterministic are:

  1. File hash checks against generated fixtures:
  2. Checking key IDs in order:

I addressed (1) by sorting signatures by key IDs and regenerating these fixtures. I addressed (2) by sorting keys IDs in the tests before making comparisons.

Comment thread repo.go
Comment thread internal/signer/sort.go
Comment thread repo.go
@ethan-lowman-dd ethan-lowman-dd requested a review from asraa October 6, 2021 21:08
asraa
asraa previously approved these changes Oct 6, 2021
hosseinsia
hosseinsia previously approved these changes Oct 6, 2021
Copy link
Copy Markdown
Contributor

@hosseinsia hosseinsia left a comment

Choose a reason for hiding this comment

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

LGTM!

@trishankatdatadog
Copy link
Copy Markdown
Contributor

@ethan-lowman-dd need to resolve conflicts...

@ethan-lowman-dd
Copy link
Copy Markdown
Contributor Author

Conflicts resolved -- need re-approvals to merge, since the commit dismissed the existing ones.

Comment thread internal/signer/sort_test.go Outdated
Copy link
Copy Markdown
Contributor

@hosseinsia hosseinsia left a comment

Choose a reason for hiding this comment

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

LGTM!
Wonderful! Many thanks!

@trishankatdatadog trishankatdatadog merged commit 5908a16 into theupdateframework:master Oct 18, 2021
mnm678 pushed a commit to mnm678/go-tuf that referenced this pull request Oct 21, 2021
…ateframework#155)

* Sort signatures by key ID

* Improve slice comparison

* Fix assert comment

* Remove sortStrings test helper

* Add test for Signer sort

* Sort keys before usage

* Revert "Sort keys before usage"

This reverts commit 88d9340.

* Rename to getSortedSigningKeys

* Check sort in a different way
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.

6 participants