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(cli): support CA bundle and proxy at the same time #16704

Closed
wants to merge 8 commits into from
Closed

fix(cli): support CA bundle and proxy at the same time #16704

wants to merge 8 commits into from

Conversation

zradlo1984
Copy link
Contributor

@zradlo1984 zradlo1984 commented Sep 29, 2021

fixes #5804
This is solution I proposed 21.6.2021.
I use this solution in my work for 4 months and it works.
While we plan to use cdk in devops pipeline we need #5804 to be fixed in official cdk cli tool.
We cannot create custom modified version after each upgrade of CDK nor check and modify sdk-provider after each npm install.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Sep 29, 2021

@zradlo1984 zradlo1984 closed this Sep 29, 2021
@zradlo1984 zradlo1984 deleted the feature/5804-support-both-ca-and-proxy-at-same-time branch September 29, 2021 11:07
@zradlo1984 zradlo1984 restored the feature/5804-support-both-ca-and-proxy-at-same-time branch September 29, 2021 12:46
@zradlo1984 zradlo1984 reopened this Sep 29, 2021
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Oct 21, 2021
@rix0rrr rix0rrr changed the title fix(cli): Allow use CA bundle and proxy at the same time (#5804) fix(cli): support CA bundle and proxy at the same time Oct 27, 2021
@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 27, 2021

I have merged from master and simplified somewhat. I hope this also works. Can you confirm if it does?

@rix0rrr rix0rrr added the pr-linter/exempt-test The PR linter will not require test changes label Oct 27, 2021
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 507b58f
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Marking as "changes requested" to get it out of my queue. Feel free to re-request review after you can confirm this change also works. Cheers!

@zradlo1984
Copy link
Contributor Author

zradlo1984 commented Nov 30, 2021

@rix0rrr
Your solution is not working because node-proxy-agent has validation on protocol.
https:/TooTallNate/node-proxy-agent/blob/3f4571097d22c9b9ac399d5b060c36b6e2caa993/index.js#L99
It fails with error: You must specify a "protocol" for the proxy type (http, https, socks, socks4, socks4a, socks5, socks5h, pac+data, pac+file, pac+ftp, pac+http, pac+https) when no proxy variable is specified. Did you tested it?
If you wanna rewrite it you cannot create ProxyAgent via object without fields like protocol slashes host or hostname and port. See sources of ProxyAgent module line 99.
If my solution is ugly for cdk, feel free to rewrite it but please test it.
But I would prefer my original solution. It works with proxy only also with ca only and both. I don't have options to test all possibilites. I did minimal changes to sdk-provider. If you wanna do refactoring do it in separate feature, not in bug fix.
#5804 is classified as feature request but it is really bug. And it is not fixed since Jan 2020.

And your solution tries to read HTTP_PROXY env variable only, but CDK has also command line parameters to set proxy. You cannot ignore command line parameters. You must read proxy settings like this:
const proxyAddress = options.proxyAddress || httpsProxyFromEnvironment();

@zradlo1984
Copy link
Contributor Author

I am looking for source of this. It seems that it was introduced by commit ceab036 when somebody fixed #16751. He removed command line parameters reading and changed to create ProxyAgent without parameters. Now it is even worse. CDK does not support CA bundle and proxy at the same time and ignores --proxy command line parameter and it does not warn about this.

@zradlo1984
Copy link
Contributor Author

So what should I do? It can't be merged as is now.
I need support for custom CA bundle and also proxy support.
Should I close this PR and start to try something else from beginning on current master?
I can, but after creation of PR I can't wait month for reply.
I am wondering if custom fork or custom patching after npm install will not be faster for me, than fixing this.

@zradlo1984
Copy link
Contributor Author

@rix0rrr
This code works for me, now. It does not break ceab036 but it changes load of CA bundle. I don't have direct connection so I can test proxy settings (via env and also command line) with and without CA bundle. I can test direct connection (via aws c9). But I can't test direct connection without proxy with custom CA bundle.

    const proxyAddress = options.proxyAddress;
    const caBundlePath = options.caBundlePath || caBundlePathFromEnvironment();

    if (proxyAddress) {
        logging_1.debug('Using proxy server: %s', proxyAddress);
    }

    if (caBundlePath) {
        logging_1.debug('Using CA bundle path: %s', caBundlePath);
        config.httpOptions.ca = readIfPossible(caBundlePath);
    }

    // Configure the proxy agent. By default, this will use HTTPS?_PROXY and
    // NO_PROXY environment variables to determine which proxy to use for each
    // request.
    //
    // eslint-disable-next-line @typescript-eslint/no-require-imports
    const ProxyAgent = require('proxy-agent');
    config.httpOptions.agent = new ProxyAgent(proxyAddress);

@zradlo1984 zradlo1984 closed this Dec 10, 2021
@zradlo1984 zradlo1984 deleted the feature/5804-support-both-ca-and-proxy-at-same-time branch December 10, 2021 15:30
mergify bot pushed a commit that referenced this pull request Jan 3, 2022
fixes #5804
This is reworked solution I proposed 30.11.2021 in PR #16704 on current master.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
fixes aws#5804
This is reworked solution I proposed 30.11.2021 in PR aws#16704 on current master.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package/tools Related to AWS CDK Tools or CLI pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: support custom CA bundles while using a proxy
3 participants