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: fix flaky key pair generation test #22980

Closed
wants to merge 1 commit into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Sep 20, 2018

There is a very small chance (about 0.4%) that OpenSSL will successfully decrypt a key without the correct passphrase and will then fail while parsing its ASN.1 structure. In those rare cases, the error message will be different.

Fixes: #22978

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 20, 2018
@tniessen
Copy link
Member Author

Suggesting to fast-track, please add 👍 here to approve.

@tniessen tniessen added the fast-track PRs that do not need to wait for 48 hours to land. label Sep 20, 2018
@addaleax
Copy link
Member

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 20, 2018
@Trott
Copy link
Member

Trott commented Sep 20, 2018

Would it make sense to repeat the encryption/decryption when that happens rather than allow the test to pass? (Not blocking. Just a suggestion. Like, if the decryption fails 100% of the time, we want the test to fail. That sort of thing.)

@tniessen
Copy link
Member Author

@Trott Short answer: No.

Longer answer: We expect the signing / decryption operation using the generated private key to fail since the private key was encrypted with a custom passphrase.
We don't give the passphrase to OpenSSL, so OpenSSL will attempt to decrypt the private key without a password. When that happens, there are two possible outcomes:

  1. OpenSSL detects that the decrypted message has an invalid padding (that's how people usually distinguish between "correctly decrypted" and "incorrectly decrypted") and throws the bad decrypt error, or
  2. the padding is valid by chance (said 0.4%). The default padding is valid if the last n bytes of the decrypted plaintext have a value of n (for 1 <= n <= 16). So in those cases where the padding is considered correct by OpenSSL, it invokes the ASN1 parser which attempts to parse the (supposedly decrypted) private key. The parser fails, however, because the private key was not decrypted correctly, thus resulting in the asn1 encoding routines error.

Both results meet our expectations, but one is much more likely than the other.

@tniessen
Copy link
Member Author

Thanks for reviewing! Landed in bad670c.

@tniessen tniessen closed this Sep 20, 2018
tniessen added a commit that referenced this pull request Sep 20, 2018
There is a very small chance (about 0.4%) that OpenSSL will
successfully decrypt a key without the correct passphrase and will
then fail while parsing its ASN.1 structure. In those rare cases,
the error message will be different.

PR-URL: #22980
Fixes: #22978
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Sep 21, 2018
There is a very small chance (about 0.4%) that OpenSSL will
successfully decrypt a key without the correct passphrase and will
then fail while parsing its ASN.1 structure. In those rare cases,
the error message will be different.

PR-URL: #22980
Fixes: #22978
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky parallel/test-crypto-keygen
4 participants