Closed ssddanbrown closed 6 months ago
Hi Dan, I heard about this issue from a colleague and while I don't have the detailed knowledge myself, I hang out in a channel with auth oracles and tried to gather some information. I will just add some hypotheses to be verified:
code
which can leak via access logs/token
if it doesn't contain the PKCE verifierIf you have more questions about that topic I can recommend the community channel of kanidm. Kani is the idp I use and the community and maintainers have strong knowledge about all the auth things.
Thanks for the valuable insight @tumbl3w33d.
It looks like PKCE is the way to go and would cover the same (and greater) concerns than nonce
does.
I've just been reading through the PKCE RFC. It looks like we should be able to add this into BookStack by default without optional control, which would be nice, although I'd need to test that across a variety my of test auth systems/platforms to be sure that they do play nice as per the RFC, and don't create compatibility issues (Always suspicious of Microsoft Azure doing something awkward).
We'll need to be sure to update our OIDC guidance if adding by default, to advise enforcing PKCE on the auth system server side, otherwise it's of limited benefit.
PR now open for adding PKCE, targeted for the next feature release: https://github.com/BookStackApp/BookStack/pull/4804
Alrighty, #4804 is now merged. Tested a bunch of OIDC auth systems there. Surprisingly most did not seem to provide options to force PKCE, but all supported PKCE and failed when invalid PKCE data was provided. Maybe enforcing PKCE is as important as I originally thought.
Either way, there were no issues with enabling by default, and it's now in as an enhanced layer of security for OIDC where supported. It will be included as part of the next feature release.
With the intention to provide an extra layer of security. Looks like there may be some overlap between these potential options, so may need to assess benefits relative to support and requirement by Identity providers.
Relevant articles: