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

fix(api-gateway): support proxy when fetching jwk for token validation #9013

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

johnsca
Copy link

@johnsca johnsca commented Dec 3, 2024

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Description of Changes Made (if issue reference is not provided)

When fetching the JWK data needed to validate the auth token, the environment proxy settings are not honored.

When fetching the JWK data needed to validate the auth token, the
environment proxy settings are not honored.
@johnsca johnsca requested a review from a team as a code owner December 3, 2024 20:19
@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Dec 3, 2024
@igorlukanin
Copy link
Member

@johnsca Thanks for this contribution!

the environment proxy settings are not honored

Just to double check, which proxy settings exactly are not honored? Also, which platform are we talking about here?

@johnsca
Copy link
Author

johnsca commented Jan 10, 2025

@igorlukanin

Just to double check, which proxy settings exactly are not honored? Also, which platform are we talking about here?

No proxy settings were being honored when using JWT authentication (specifically, when retrieving the JWK keys to validate the token). This was encountered when deploying Cube in an environment with restricted outbound traffic which requires request to the public internet to go out via a proxy for security reasons.

This PR uses ProxyAgent because it supports the HTTPS_PROXY and NO_PROXY env vars, which the lower-level HttpsProxyAgent used by getHttpAgentForProxySettings() doesn't appear to. It might be worth moving that code to ProxyAgent as well, as recommended by the proxy-agents README, but I wanted to keep this PR minimal.

@KSDaemon
Copy link
Member

Hi, @johnsca. Thanks for finding this issue and suggesting a fix. However, the right way to fix this, as you already mentioned, is to patch getProxySettings(), making it aware of common env vars like HTTTP_PROXY / HTTPS_PROXY. And then reuse it in api-gateway

@@ -51,7 +52,7 @@ export type JWKsFetcherOptions = Pick<BackgroundMemoizeOptions<any, any>, 'onBac

export const createJWKsFetcher = (jwtOptions: JWTOptions, options: JWKsFetcherOptions) => {
const fetchJwkUrl = asyncMemoizeBackground(async (url: string) => {
const response = await asyncRetry(() => fetch(url), {
const response = await asyncRetry(() => fetch(url, { agent: new ProxyAgent() }), {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to reuse getHttpAgentForProxySettings() from backend-shared here instead.

@johnsca
Copy link
Author

johnsca commented Jan 27, 2025

@KSDaemon It doesn't seem like a good idea to reimplement what proxy-from-env does, which is what ProxyAgent uses, but proxy-from-env doesn't have an interface that can be used to directly convert getProxySettings to use (it unfortunately puts all of the proxy settings logic directly into getProxyForUrl which is unfortunate). So I instead switched getHttpAgentForProxySettings to use ProxyAgent instead and just dropped the use of getProxySettings. That should be a drop-in replacement, and it also aligns better with the recommendation from the proxy-agents monorepo to prefer using proxy-agent rather than the lower-level implementations directly, such as https-proxy-agent.

I don't see anywhere that getProxySettings is used other than that one file but since it's exported, I didn't feel comfortable removing it even though it's unused now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:community Contribution from Cube.js community members.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants