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.

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

test targets fast-forward attack recovery #1742

Conversation

kairoaraujo
Copy link
Collaborator

This test simulates the targets fast-forward attack recovery.
It simulates that the targets keys were compromised, the attacker
generated a new high version of the targets.

The repository generates new keys for targets and snapshot to
rollback the targets version to the initial version.

Signed-off-by: Kairo de Araujo kdearaujo@vmware.com

Please fill in the fields below to submit a pull request. The more information
that is provided, the better.

Part of #1713

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@kairoaraujo
Copy link
Collaborator Author

Two comments here.

  • Not 100% confident if my flow is correct.
  • I added a target file to be more explicit, giving more context to the test, but it can be removed if irrelevant.

@coveralls
Copy link

coveralls commented Dec 22, 2021

Pull Request Test Coverage Report for Build 1654002083

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.737%

Totals Coverage Status
Change from base Build 1611151931: 0.0%
Covered Lines: 4094
Relevant Lines: 4173

💛 - Coveralls

@jku
Copy link
Member

jku commented Jan 3, 2022

Test logic looks 100% correct to me. I would personally remove two things from the test to keep it as minimal as possible:

  • as you mentioned the target file is not needed for this test
  • the targets key rotation seems unnecessary as well: obviously if the targets key had been compromised in a production environment we'd want to rotate it but I think it's not necessary for this test and makes the test more complex

Basically I'm not opposed to adding tests with more steps and complications in them... but I would like the test suite to first contain the most condensed test cases, and only add more complicated ones if needed after that. Opinions on that?

@kairoaraujo
Copy link
Collaborator Author

IMHO, we should keep the targets key rotation.
This rotation will also be part of the scenario if the targets key is compromised.

@jku
Copy link
Member

jku commented Jan 4, 2022

This rotation will also be part of the scenario if the targets key is compromised.

Yes this is true ... however

  1. if we're strict about it this is a test for fast forward recovery not targets key compromise -- maybe there are other valid reasons to do this than key compromise (a software bug for example, one that bumps the version too many times, or some attack that allows attacker to trigger metadata generation but does not leak keys)
  2. I wanted the tests to really show what is actually required to do the version rewinds (this is something apparently no-one has ever written down before us): now reading the test it looks like targets key rotation is required to rewind the targets version, but that's not true

I can merge it like this but personally I'd prefer the smallest possible tests for these cases and then new tests (in other issues) for more complex scenarios if it looks like they are needed.

This test simulates the targets fast-forward attack recovery.
It simulates that the targets keys were compromised, the attacker
generated a new high version of the targets.

The repository generates new key for snapshot to rollback the
targets version to the initial version.

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>
@kairoaraujo kairoaraujo force-pushed the issue#1713/test_fast-forward_recovery_targets branch from 506902d to 5b4a47a Compare January 4, 2022 14:15
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

Thank you. These are great tests that will make it much easier to write a spec issue about making the key rotation requirements clearer

@jku jku merged commit d8591e7 into theupdateframework:develop Jan 7, 2022
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.

3 participants