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

Added support for using the root certificates of the os #229

Closed
wants to merge 6 commits into from

Conversation

DavyLandman
Copy link
Member

Sometimes, company firewalls like to inspect https traffic. To do that they insersect https traffic, and re-sign the traffic with a private certificate authority.

This code makes sure we import local certificate authorities.

Copy link
Member

@jurgenvinju jurgenvinju left a comment

Choose a reason for hiding this comment

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

ok. I've some questions. see review. Also want to learn from this what to do in the Java code, but couldn't see that yet.

@@ -102,7 +102,7 @@ function getJavaCandidates(): string[] {
}
}
}
return result;
return [];//result;
Copy link
Member

Choose a reason for hiding this comment

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

is this right?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, this is debugging code, I wanted to trigger it on my machine. good find.

@@ -20,9 +20,11 @@
"url": "https://github.com/usethesource/rascal-language-servers"
},
"dependencies": {
"mac-ca": "^1.0.6",
Copy link
Member

Choose a reason for hiding this comment

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

dependency added but not used?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's imported, that's enough.

see the comment:

import * as macca from 'mac-ca'; // just importing is is enough


// help typescript accept that it's all "any"
declare module 'win-ca';
declare module 'mac-ca';
Copy link
Member

Choose a reason for hiding this comment

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

don't see where this is used yet. pls help

Copy link
Member Author

Choose a reason for hiding this comment

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

well, typescript doesn't want to import modules without type definitions. but you can add a defintion for a module that roughly says: stop type-checking, everything is any. This file does that.

@DavyLandman
Copy link
Member Author

Thanks for the review, I'm waiting for some feedback on this downstream issue: ukoloff/win-ca#48

after that it either fixed or closed, we can choose to do this. else the downloader will sadly fail on any https-mitm-firewall-enterprise environments.

@jurgenvinju
Copy link
Member

Ok that's great. I need to learn what to do for Mac by reading the code of the import you did. Let's merge this branch asap.

@DavyLandman
Copy link
Member Author

I think we should move to this library: https://github.com/microsoft/vscode-windows-ca-certs in combination with https://github.com/microsoft/vscode-proxy-agent

@DavyLandman
Copy link
Member Author

DavyLandman commented Mar 8, 2023

After some discussion with VS developers, they told me that vscode is already patching all nodejs processes to import the root certificate. I've tested this by installing a custom CA root in my own machine. and indeed, it just works as you would expect.

I'm closing this PR. @jurgenvinju we need to ask the users that reported this to provide a bit more information about their corporate firewall setup.

@DavyLandman DavyLandman closed this Mar 8, 2023
@DavyLandman DavyLandman deleted the use-root-ca branch September 22, 2023 15:11
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.

2 participants