Skip to content

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 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.

3 participants