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

Prevent log spamming when using a non-cached JWKS #8730

Closed
wants to merge 1 commit into from

Conversation

lovasoa
Copy link

@lovasoa lovasoa commented Jul 29, 2022

Multiple auth providers (keycloak, firebase, ...) do not allow caching on their JWK endpoints.
In this case, Hasura requests the endpoint every second.
This change avoids spamming the logs when this happens.

Description

Changelog

  • CHANGELOG.md is updated with user-facing content relevant to this PR. If no changelog is required, then add the no-changelog-required label.

Affected components

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System
  • Tests
  • Other (list it)

Related Issues

#8299

Limitations, known bugs & workarounds

Does this PR add a new Metadata feature?

  • No
  • Yes
    • Does run_sql auto manages the new metadata through schema diffing?
      • Yes
      • Not required
    • Does run_sql auto manages the definitions of metadata on renaming?
      • Yes
      • Not required
    • Does export_metadata/replace_metadata supports the new metadata added?
      • Yes
      • Not required

GraphQL

  • No new GraphQL schema is generated
  • New GraphQL schema is being generated:
    • New types and typenames are correlated

Breaking changes

  • No Breaking changes

Multiple auth providers (keycloak, firebase, ...) do not allow caching on their JWK endpoints.
In this case, Hasura requests the endpoint every second.
This change avoids spamming the logs when this happens.

See
hasura#8299
@lovasoa lovasoa requested a review from a team as a code owner July 29, 2022 15:50
@hasura-bot
Copy link
Contributor

Beep boop! 🤖

Hey @lovasoa, thanks for your PR!

One of my human friends will review this PR and get back to you as soon as possible.

Stay awesome! 😎

@CLAassistant
Copy link

CLAassistant commented Jul 29, 2022

CLA assistant check
All committers have signed the CLA.

@SamirTalwar
Copy link
Contributor

Hi @lovasoa, thanks for the patch!

This seems benign but there are definitely use cases where users might want to see these logs at INFO level too. We think it might be more prudent to make this configurable, so that you can customize it for yourself as needed, as outlined in #8611.

I see you've already spotted this issue. Would you be interested in submitting a patch along those lines?

@lovasoa
Copy link
Author

lovasoa commented Aug 2, 2022

Hi, and thanks for the response :)
Sorry, but I don't have the bandwidth for that now.
Are you sure it makes sense to log this as INFO by default for everyone ? I agree it should be made configurable, but by default, given that hasura will refresh keys every second for most jwks providers, I think it doesn't make sense to display something on every refresh.

@SamirTalwar
Copy link
Contributor

It really depends on the caching proposed by the auth provider. I took a look at a few (Auth0, AWS Cognito, Firebase) and they all seem to encourage some caching.

The Firebase documentation I found suggests that keys are cached, and uses a central, public keyset. I hit the URL, https://www.googleapis.com/robot/v1/metadata/x509/securetoken@system.gserviceaccount.com, and got the following headers back:

cache-control: public, max-age=24087, must-revalidate, no-transform
date: Fri, 05 Aug 2022 13:40:48 GMT
expires: Fri, 05 Aug 2022 20:22:15 GMT

Can you explain your situation in greater detail so I can understand why it's different?

@lovasoa
Copy link
Author

lovasoa commented Oct 28, 2022

Hey @SamirTalwar , I'm sorry I missed your message when you posted it. Thank you for investigating on this !
Our auth provider is the open-source Keycloak. It serves JWKs with the following headers :

< HTTP/2 200 
< date: Fri, 28 Oct 2022 13:48:49 GMT
< content-type: application/json
< content-length: 1466
< cache-control: no-cache
< x-xss-protection: 1; mode=block
< x-frame-options: SAMEORIGIN
< referrer-policy: no-referrer
< strict-transport-security: max-age=15724800; includeSubDomains
< x-content-type-options: nosniff
< x-forwarded-for: keycloak.***.com
< x-forwarded-proto: https

Unfortunately, that does not seem to be configurable, and we end up with very large amounts of logs from hasura, which are expensive to store and make debugging more difficult.

@SamirTalwar
Copy link
Contributor

That's interesting. My first thought was that we should probably let the auth provider specify the expiry, but a default of 1 second seems very arbitrary. I'll bring this up and see if we can make that configurable.

@lovasoa
Copy link
Author

lovasoa commented Oct 31, 2022

Thank you @SamirTalwar !

@pranshi06
Copy link
Contributor

Hi @lovasoa , thanks for taking the initiative to resolve this issue.
We have fixed it by making jwk-refresh-log type configurable as outlined in #8611 via fd6aa0a and 7b7845f.

@pranshi06 pranshi06 closed this Nov 10, 2022
@hasura-bot
Copy link
Contributor

Beep boop! 🤖

Hey @lovasoa!

Sorry that your PR wasn’t merged.

Do take a look at any of the other open issues to see if you’d like to take something up! We’re around on Discord if you have any questions 😄

@lovasoa lovasoa deleted the patch-1 branch November 11, 2022 14:55
@lovasoa
Copy link
Author

lovasoa commented Nov 11, 2022

Great, thank you @pranshi06 !

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.

5 participants