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

luks2: don't time tests to verify KDF settings #275

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

chrisccoulson
Copy link
Collaborator

There are a few tests where we time the KDF operation because there
isn't another way to determine if it was configured correctly by
cryptsetup. These tests have always been a little bit flaky, and they
have been significantly more so recently.

Stop timing them, and instead:

  • in internal/luks2, just rely on the fact that we verify the expected
    commandline arguments were passed to cryptsetup. We have to assume
    that these options are correct, which is what the existing tests
    attempted to confirm.
  • in crypt_test.go, ensure that we pass the correct target time to
    internal/luks2.

There are a few tests where we time the KDF operation because there
isn't another way to determine if it was configured correctly by
cryptsetup. These tests have always been a little bit flaky, and they
have been significantly more so recently.

Stop timing them, and instead:
- in internal/luks2, just rely on the fact that we verify the expected
  commandline arguments were passed to cryptsetup. We have to assume
  that these options are correct, which is what the existing tests
  attempted to confirm.
- in crypt_test.go, ensure that we pass the correct target time to
  internal/luks2.
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

+1

@chrisccoulson chrisccoulson merged commit 0474c53 into canonical:master Nov 22, 2023
2 checks passed
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.

2 participants