Concurrency bug in NewOpenIDProvider() about oidc HOT 8 CLOSED

Please see https://gist.github.com/flyn-org/abb616eecd71b4531395e62d8e60f7b3 for an all-in-one source file.

This approach may seem odd, but it allows a student to run this with `nc -l -p 1024 -e ./service-oidc oidc http://127.0.0.1:1025/auth/callback SECRET student". (The assignment is at https://www.aquinas.dev/project/oidcC.)

Look for "FIXME: Kludge ..." for the portion in question.

from oidc.

Testing your change is on my list of things to do. I just have not had time to do it properly yet.

from oidc.

I ran my tests a number of times, and the bug didn't pop back up. If you are confident in the logic of you fix---namely that it introduces a synchronization that ensures the signing key is set before things proceed---then I think we can close this. I only hesitate to fully rely on my testing because concurrency bugs tend to be nondeterministic and thus difficult to catch conclusively with testing.

Will we see a new release that contains this fix soon? Thank you!

from oidc.

Thanks for reporting this. Is it possible to get a snippet of the code you used this lib?

This would make it easier for us to understand the issue 😎

from oidc.

@livio-a had you have time to check this one?

from oidc.

Hey @flyn-org

Thanks for the detailed description. Unfortunately, I still could not reproduce the exact error, but I think I've managed to find a solution anyway:
In PR #151 the go storage.GetSigningKey(ctx, keyCh) is now called before creating the signer, because the NewSigner func will wait for the key before returning the Signer.

I also published a pre-released version of it: https://github.com/caos/oidc/releases/tag/v1.0.2-beta.1
Does this solve your problem?

from oidc.

Hey @flyn-org did you have time to try it with the new version?

from oidc.

No stress, I totally understand. It’s just been on my list as well to check the current state 😃

from oidc.