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

Fix flaky oidc tests #2000

Merged
merged 4 commits into from
Nov 20, 2024
Merged

Fix flaky oidc tests #2000

merged 4 commits into from
Nov 20, 2024

Conversation

jsdt
Copy link
Contributor

@jsdt jsdt commented Nov 18, 2024

Description of Changes

The test_oidc_flow test has had some spurious failures, so this adds the following:

  1. We have a few more debug logs that should help figure out whats happening if there are more failures.
  2. When starting up an OIDC server in the tests, we wait for a successful request before continuing. This is just in case there is some race condition around server startup.
  3. The actual fix is that we encode the key parameters correctly in our tests.

The cause for the failures is that when we encoded keys to JWKS in tests, we used openssl::bn::BigNum for our parameters, which is a dynamically sized number. If the 8 most significant digits of a parameter happened to be 8, our JWKs would have a 31 byte parameter. According to the spec, that is invalid section 6.2.1.2. This was tricky to debug because when I tested the failing keys with node/webcrypto, that library just left-padded the parameter, so it seemed valid.

The library that we are using doesn't reject the key or left-pad it. I'm not sure how or if it fills in missing bytes, but it was returning an invalid signature error instead of an invalid key error.

We should update the library at some point to handle this sort of invalid key better, in case this encoding bug is more common.

API and ABI breaking changes

If this is an API or ABI breaking change, please apply the
corresponding GitHub label.

Expected complexity level and risk

0

Testing

This is a test only change.

@jsdt jsdt marked this pull request as ready for review November 18, 2024 18:28
@jsdt jsdt changed the title Wait for server to start responding and add logging in tests Fix flaky oidc tests Nov 19, 2024
@jsdt jsdt requested a review from gefjon November 19, 2024 19:00
@gefjon gefjon assigned jsdt and unassigned gefjon Nov 20, 2024
@jsdt jsdt added this pull request to the merge queue Nov 20, 2024
Merged via the queue into master with commit 3f0e152 Nov 20, 2024
7 of 8 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