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

proposal: crypto/x509: add functions to download certificates from windows update, and retrieve certificates from windows x509stores #26950

Closed
jeet-parekh opened this issue Aug 13, 2018 · 11 comments
Labels
FrozenDueToAge OS-Windows Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@jeet-parekh
Copy link
Contributor

SystemCertPool() hasn't been working on Windows (#16736). That issue was fixed once. But since Windows does not ship with all of its root certificates installed, it caused some unexpected behavior (#18609).

The missing certificates can be downloaded from windows update using certutil.exe present on Windows.

There are also other multiple certificate stores on Windows. There are two store locations, and a bunch of stores in each location.

We could retrieve the certificates from windows update and all of the above locations. Then remove the disallowed certificates from that entire collection. That way SystemCertPool() can be fixed.

I have been working on it - CL 127577. While working on it, I created two new functions. One would retrieve the certificates from any location mentioned in the MSDN docs - windows509Store(). And the other would download the certificates from windows update - downloadWUCerts().

I opened this proposal to welcome a discussion regarding whether or not we should export those two new functions to the standard library. The code would be there regardless. Because there are multiple certificate stores on Windows, these functions could provide the end user a bit more control if needed.

@gopherbot gopherbot added this to the Proposal milestone Aug 13, 2018
@jeet-parekh
Copy link
Contributor Author

@gopherbot, please add label OS-Windows

@rasky rasky added the Proposal-Crypto Proposal related to crypto packages or other security issues label Aug 13, 2018
@adamdecaf
Copy link
Contributor

Should there be an environment variable to disable this? What if the network is down/slow?

@jeet-parekh
Copy link
Contributor Author

Should there be an environment variable to disable this?

Sorry, but I didn't get that. Disable what? If you mean the way Windows downloads the certificates on demand... then no, there isn't a way to disable that.

What if the network is down/slow?

downloadWUCerts() won't work if the network is down.

@adamdecaf
Copy link
Contributor

adamdecaf commented Aug 14, 2018

If the network is slow that means SystemCertPool() could block, for say a minute (or more)? That's not ideal.

What about environments without a network? Why can't we offer a way to disable this?

@jeet-parekh
Copy link
Contributor Author

I agree, blocking would not be ideal.

I think giving a way to enable/disable fetching certs from windows update would increase the complexity a bit. If we rely on an environment variable, the user would have to change it whenever their network status changes. Or whenever they want to enable/disable downloading certs from WU.

Instead, we could just return an error mentioning that certs were not downloaded from WU. I described a way to do it in the CL.

@rsc
Copy link
Contributor

rsc commented Jan 13, 2021

/cc @FiloSottile

@rsc
Copy link
Contributor

rsc commented Jan 13, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Jan 13, 2021
@jeet-parekh
Copy link
Contributor Author

I had opened this proposal a long time ago. And honestly, I had forgotten about this. For my work, I remember I made a tool to fetch the windows certs whenever needed. It's been a long time since I last worked with Go.

@rsc
Copy link
Contributor

rsc commented Jan 20, 2021

This issue is now pretty old, it hasn't gotten much additional enthusiasm that I see, and the original commenter no longer needs the functionality. It sounds like maybe we should decline this specific proposal until we have an ongoing need that we can use to evaluate solutions.

@rsc
Copy link
Contributor

rsc commented Jan 27, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jan 27, 2021
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Feb 3, 2021
@rsc
Copy link
Contributor

rsc commented Feb 3, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Feb 3, 2021
@golang golang locked and limited conversation to collaborators Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge OS-Windows Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
No open projects
Development

No branches or pull requests

5 participants