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

tests: Updater key rotation tests #1607

Closed
jku opened this issue Oct 7, 2021 · 4 comments · Fixed by #1635
Closed

tests: Updater key rotation tests #1607

jku opened this issue Oct 7, 2021 · 4 comments · Fixed by #1635
Assignees
Labels
backlog Issues to address with priority for current development goals
Milestone

Comments

@jku
Copy link
Member

jku commented Oct 7, 2021

This is part of #1579, we want to test the Updater in various key/signature change scenarios in a setup where:

  • tests are readable / understandable
  • a new test scenario adds as little code as possible
  • adding new scenarios should be easy and the added test runtime should be negligible

I have been looking at existing tests (test_key_revocation_integration.py, test_root_versioning_integration.py and test_updater_root_rotation_integration.py) and I believe they all revolve around a few variables that we should be able to turn into a table testing setup where no additional code is needed for most new tests. The most interesting and complex case is root rotations (because root delegates not just to other roles but to "future root role") so I'll start there.

All root key/signature change cases can be defined by listing N sequential root states, each with

  • key list
  • threshold
  • signature list
  • expected result

In practice a table like this should do it:

@dataclass
class RootVersion:
    keys: list[int]
    threshold: int
    signatures: list[int]
    result: Optional[Exception] = None

root_cases = {
    "key rotation: 1-of-1 scenario" : [
        RootVersion([1], 1, [1]),
        RootVersion([2], 1, [2,1]),
        RootVersion([2], 1, [2]),
    ],
    "failing threshold decrease: 2-of-2 becomes 1-of-2" : [
        RootVersion([1,2], 2, [1,2]),
        RootVersion([1,2], 1, [2], UnsignedMetadataError),
    ]
}

The "key rotation" test case written out would be

  • root v1 contains root key1 with threshold of 1, signed by key1
  • root v2 contains root key2 with threshold of 1, signed by key1 and key2
  • root v2 contains root key2 with threshold of 1, signed by key2

There is no exception listed so Updater is expected to load all the versions correctly

The "failing" test case written out would be

  • root v1 contains root key1 and key2 with threshold of 2, signed by key1 and key2
  • root v2 contains root key1 and key2 with threshold of 1, signed by key2

Updater should fail to update v2 as it's not signed by both keys as v1 requires

This is not super readable (using numbers to represent keys is admittedly a bit confusing) but similar 3-5 lines can describe quite tricky situations like threshold increases while rotating keys etc

@jku jku changed the title tests: Updater key rotation / metadata validity tests: Updater key rotation tests Oct 7, 2021
@jku
Copy link
Member Author

jku commented Oct 7, 2021

So the potential issues here are:

  1. the test case format is more complex than is acceptable
  2. the code that actually turns the test case in to a repository is not trivial

I think I'll try to do a throwaway version of this to smoke test the second issue (also so others see what the idea is), then if it still seems reasonable we can try tweaking the test case format so it's understandable

@jku jku self-assigned this Oct 8, 2021
@jku
Copy link
Member Author

jku commented Oct 8, 2021

2. the code that actually turns the test case in to a repository is not trivial

It's fine: https://github.com/jku/python-tuf/blob/e6911d6ae4c5630ca44b97d62a692d11737af100/tests/test_updater_key_rotations.py#L94 shows an example: the actual test is just 30 lines

I think the only real issue is the test case format.

@jku jku added the backlog Issues to address with priority for current development goals label Oct 13, 2021
@jku jku added this to the Sprint 10 milestone Oct 13, 2021
@sechkova sechkova modified the milestones: Sprint 10, Sprint 11 Oct 27, 2021
@jku jku closed this as completed in #1635 Oct 27, 2021
@MVrachev
Copy link
Collaborator

MVrachev commented Oct 27, 2021

@jku I think probably we want to add a couple of test cases for the rest of the keys, right?
If yes, then maybe we should reopen this one?

Also, while I was reviewing #1635 it was little hard for me to remember the placement of the arguments for RootVersion considering they are 15 root rotations tested and the dataset becomes a larger piece of code.
Do you think it will be a good idea if instead of writing RootVersion([5,6,7,8,9], 3, [0,2,4,5,6,7]
we write RootVersion(keys=[5,6,7,8,9], threshold=3, signatures=[0,2,4,5,6,7])?
The only annoying thing is that the expressive version finishes at the 82-nd character in the file and will have to be stripped to 80 when we enable the linters on the test files.

@jku
Copy link
Member Author

jku commented Oct 27, 2021

@jku I think probably we want to add a couple of test cases for the rest of the keys, right? If yes, then maybe we should reopen this one?

I'll file another one, just forgot to do it (EDIT: see #1647)

Also, while I was reviewing #1635 it was little hard for me to remember the placement of the arguments for RootVersion considering they are 15 root rotations tested and the dataset becomes a larger piece of code. Do you think it will be a good idea if instead of writing RootVersion([5,6,7,8,9], 3, [0,2,4,5,6,7] we write RootVersion(keys=[5,6,7,8,9], threshold=3, signatures=[0,2,4,5,6,7])?

yeah WFM, replace signatures with sigs and it should even fit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants