Add UnauthorizedException when kid is not provided

[theRealSuperMario]

Hello,

We noticed the following problem in our setup recently: We have a bot calling our API without providing the kid in the header. Unfortunately, this crashes the endpoint, because it expects that kid is in unverified_header. I sketched a fix below BUT I don't know if this is sufficient. Also, I don't know how to build a test case for this right now. Might figure it out but don't have the time currently.

Please let me know how I can get this across the finish line.

[dorinclisu]

Interesting case, that was indeed a small oversight and it seems to be the only remaining place where an assumption is (incorrectly) made about the token.

Your commit should be sufficient to fix it. But I would swap the unauthorized exception for the unauthenticated exception (technically more correct).

As for testing, it's a matter of creating a function (ex.: get_missing_kid_token()) and then putting it to use with another assertion in test_token(). https://github.com/dorinclisu/fastapi-auth0/blob/0a361065c922328be988262f21068afc88487f6b/tests/test_auth.py#L238

[theRealSuperMario]

Ok, I changed the code. Running the tests locally did not work (most likely because I did not invest enough time), so I could not verify the tests are passing.

Am 03.03.2023 um 21:30 schrieb Dorin @.***>:

Interesting case, that was indeed a small oversight and it seems to be the only remaining place where an assumption is (incorrectly) made about the token.

Your commit should be sufficient to fix it. But I would swap the unauthorized exception for the unauthenticated exception (technically more correct).

As for testing, it's a matter of creating a function (ex.: get_missing_kid_token()) and then putting it to use with another assertion in test_token()

— Reply to this email directly, view it on GitHub https://github.com/dorinclisu/fastapi-auth0/pull/30#issuecomment-1454090580, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRXFWPEQKGWERCDQXDUXK3W2JIFFANCNFSM6AAAAAAVOQUSOE. You are receiving this because you authored the thread.

[theRealSuperMario]

Any more change requests?

[dorinclisu]

Well for running the tests an auth0 tenant needs to be configured accordingly (it's documented here https://github.com/dorinclisu/fastapi-auth0/tree/master/tests).

I have one tenant configured for github actions but initially I did not allow pull requests to access it because it would enable any user to extract secrets. But I'll reevaluate the threat model and possibly enable it so that the tests can run in a PR.

I tried your commits locally and the test is not passing, because of an error in the test code itself.

Furthermore, I thought a bit more about the handling of when the token header has no 'kid', and I think we should simply write 'Malformed token' as message, because only a malicious user would craft such a token, so why help them with a specific message such as 'kid header not provided'? Sure, in theory anyhow they can't break the crypthographic security built into the token verification, but as security best practice the less they know, the better. I see virtually no case in which a (genuine) developer error would result in such a bad token.

[theRealSuperMario]

Yes, I had the same thought when I wrote the code. Just "malformed-token" should be enough.

Regarding the tests - Is there any way you could build the test case yourself? I honestly don’t really know what I am doing with the tokens and headers there 😅 (yet). Otherwise, we have to wait to let me figure it out...

Am 06.03.2023 um 21:37 schrieb Dorin @.***>:

Well for running the tests an auth0 tenant needs to be configured accordingly (it's documented here https://github.com/dorinclisu/fastapi-auth0/tree/master/tests).

I have one tenant configured for github actions but initially I did not allow pull requests to access it because it would enable any user to extract secrets. But I'll reevaluate the threat model and possibly enable it so that the tests can run in a PR.

I tried your commits locally and the test is not passing, because of an error in the test code itself.

Furthermore, I thought a bit more about the handling of when the token header has no 'kid', and I think we should simply write 'Malformed token' as message, because only a malicious user would craft such a token, so why help them with a specific message such as 'kid header not provided'? Sure, in theory anyhow they can't break the crypthographic security built into the token verification, but as security best practice the less they know, the better. I see virtually no case in which a (genuine) developer error would result in such a bad token.

— Reply to this email directly, view it on GitHub https://github.com/dorinclisu/fastapi-auth0/pull/30#issuecomment-1456940508, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRXFWL4HCMTFINOHL76B2TW2ZDIZANCNFSM6AAAAAAVOQUSOE. You are receiving this because you authored the thread.

[dorinclisu]

I fixed it but cannot push commits to this PR until you enable "Allow edits from maintainers".

[theRealSuperMario]

If I see this correctly, I will have to create a new PR from a user-owned fork to enable this. https://github.com/community/community/discussions/9921

I'll try that, sorry for the inconvenience.

[theRealSuperMario]

Yep, so you it should work over there

https://github.com/dorinclisu/fastapi-auth0/pull/31

Little bit annoying that a differentiation is made based on the namespace here.