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

http proxy error after upgrading from 1.6.0 to 1.8.0 #144

Closed
shnplr opened this issue May 31, 2020 · 8 comments · Fixed by #176
Closed

http proxy error after upgrading from 1.6.0 to 1.8.0 #144

shnplr opened this issue May 31, 2020 · 8 comments · Fixed by #176
Assignees
Labels

Comments

@shnplr
Copy link

shnplr commented May 31, 2020

This library no longer works behind corporate proxy. Our proxy scheme is 'http:https://:80' for both http & https.

{
jwksUri: 'https://server.com/.well-known/jwks.json',
proxy: 'http:https://@proxy.example.com:80'
}

Current workaround is to use "1.7.0"

Note: A minor issue with "@" needed to prevent NPE request.js does not check proxy.auth for null.

@lubomir-haralampiev
Copy link
Contributor

I'm using this great module too and experiencing the same problem, which ended with pinning to 1.7.0.

Likely this is because a https request needs to go through the proxy only working with http. This is a reatively old problem axios users are experiencing:
axios/axios#925
axios/axios#2072 (comment)

As there are working workarounds suggested, I'm not sure when/if this be solved on axios side.

Maybe it could be an option, to provide the possibility to inject a complete axios instance, when initializing the jwks-rsa Client?

@tcvv
Copy link

tcvv commented Jul 13, 2020

We had this problem and we workaround it by overwritting the getKeys function of the JwksClient object instance we're using (I suppose you can also overwrite the prototype one). Since the function only performs the network request, you can perform it using a library of your choice.
Not sure how to fix it at a library level though, unless we could somehow (optionally) inject our own prepared request object, which fullfills the same api as the existing request.js wrapper, when creating the client.

@metamono
Copy link

Seems like we have the same problem #142

It's about time to do something here!!!

@davidpatrick davidpatrick self-assigned this Aug 12, 2020
@jimmyjames
Copy link
Contributor

This is likely related to migrating from the deprecated request module to axios, introduced in 1.8. As discussed above, the current workaround is to use 1.7 if you are impacted by this, while we work on a fix. Apologies for the unintended bug introduced here!

@jimmyjames
Copy link
Contributor

It appears this is related to axios/axios#925 when using an HTTP proxy to an HTTPS endpoint.

The common recommended fix is to configure an httpsAgent to handle the proxy, either using https-proxy-agent or tunnel. Because we do allow customization of the http(s) agent via requestAgentOptions, we’ll need to be careful how we can still support that agent customization while also supporting proxy (also, it appears that https-proxy-agent doesn't support some common agent features, such as keepAlive). If we can’t get a fix into axios itself, perhaps we could instead allow agent customization by accepting an Agent instance, instead of options that we use to construct the agent. This way those who need to configure this library with a proxy could do something like:

const client = jwksClient({
  jwksUri: 'https://YOUR-DOMAIN/.well-known/jwks.json',
  httpsAgent: new HttpsProxyAgent({
    host: '127.0.0.1', 
    port: '9090', 
    auth: 'username:password'
  })
});

We’d then need to deprecate the current proxy and requestAgentOptions and document how to configure an agent for proxy support.

As mentioned earlier, if you are using this library behind a proxy, it’s recommended to stay on v1.7 until we either (hopefully) get a fix on the Axios side or implement a workaround, perhaps as described above.

@ghislaincote
Copy link

ghislaincote commented Sep 11, 2020

Hi, I DID NOT find the current thread in time... and ran into axios/axios#2072 (comment)

So, I tried to fix it myself, over v 1.9, with a similar solution as above (using 'https-proxy-agent').
As a result, the code for the request wrapper would be simplified (the agent takes the proxy string as a whole)... but I ignore the possible consequences on non-https proxy traffic. We would have to test.

FYI, it works in our environnement. We are using a 'http' proxy, to get our jwks objects from a 'https://domain/path' url.

//THIS IS MY WHOLE src/wrappers/request.js REPLACEMENT FILE !

import http from 'http';
import https from 'https';
import httpsProxyAgent from 'https-proxy-agent';
import { request } from 'axios';

export default function(options, cb) {
  const requestOptions = {
    url: options.uri,
    headers: options.headers,
    timeout: options.timeout
  };

  if (options.proxy) {
    //Needed because of limitation of Axios with https traffic tunnelled into http corporate proxies
    //See : https://github.com/axios/axios/issues/2072
    requestOptions.proxy = false; //Oddly, this must be false, since we are using the agent
    requestOptions.httpsAgent = new httpsProxyAgent(options.proxy);
  } 

  if (options.agentOptions || options.strictSSL != undefined) {
    const agentOptions = {
      ...(options.strictSSL != undefined) && { rejectUnauthorized: options.strictSSL },
      ...options.agentOptions
    };
    requestOptions.httpAgent = new http.Agent(agentOptions);
    requestOptions.httpsAgent = new https.Agent(agentOptions);
  }

  request(requestOptions)
    .then(response => cb(null, response))
    .catch(err => cb(err));
};

What do you think ?

I am not a contributor to this project, and have no Idea how to be.
If you tell me how, I will submit the code above, after required tests (again, tell me which), of course.

In the meantime, as per @jimmyjames suggestion, we will most probably switch to 1.7, to fix an issue we have in production.

Thanks !

@catchin
Copy link

catchin commented Sep 23, 2020

I have a related problem with the new 1.10.0 version. As now it seems 'https-proxy-agent' is pulled in as a dependency, but in the package.json it is only a devDependency.
Therefore, I get the following error:

Error: Cannot find module 'https-proxy-agent'
Require stack:
- .../node_modules/jwks-rsa/lib/wrappers/request.js
- .../node_modules/jwks-rsa/lib/JwksClient.js
- .../node_modules/jwks-rsa/lib/index.js
...

@lubomir-haralampiev
Copy link
Contributor

Thanks for working on this issue.
We are still missing one aspect. Until 1.7 it was possible to define the proxy configuration in the environment variables HTTP_PROXY, HTTPS_PROXY and NO_PROXY. The current solution doesn't interpret the environment and the implemented fix doesn't apply.

I implemented a solution in #188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants