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

crypto/x509: add android user trusted CA folder as a possible source for certificate retrieval #58922

Closed
odeke-em opened this issue Mar 7, 2023 · 11 comments
Labels
OS-Android Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@odeke-em
Copy link
Member

odeke-em commented Mar 7, 2023

For the purposes of getting what looks like a simple cryptography related change expediently but carefully reviewed and approved, I am coming here from @jbpin's CL https://go-review.googlesource.com/c/go/+/473035 and PR #50240, a new contributor sent a change adding the Android User Trusted Certificate Authority folder as a source of certificates. This change is to invite experts to comment and approve if we should support /data/misc/keychain/certs-added as a source of certificates

Kindly cc-ing @rolandshoemaker @FiloSottile

@odeke-em odeke-em added OS-Android Proposal Proposal-Crypto Proposal related to crypto packages or other security issues labels Mar 7, 2023
@gopherbot gopherbot added this to the Proposal milestone Mar 7, 2023
@ianlancetaylor
Copy link
Contributor

CC @golang/security

@rsc
Copy link
Contributor

rsc commented Mar 8, 2023

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
Copy link
Contributor

rsc commented Mar 15, 2023

The proposal is to read Android certs both from the system cert directory and what is apparently the user cert directory (/data/misc/keychain/certs-added). This seems reasonable and aligns with reading user-installed certs on Mac and Windows, which we do by querying the operating system for certs. Is there a user-installed cert directory on Linux too? If so should we add that?

/cc @FiloSottile @rolandshoemaker

@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

Given the similarity to Mac and Windows it sounds like we can accept this for Android.
There is a question of whether there's a non-root-owned user-specific cert place on Linux, but if so that can be a separate proposal.

@rolandshoemaker
Copy link
Member

Seems reasonable, ideally there would be a uniform place to look/API we could use, but that is wishful thinking. At some point the list of places we might slurp certs from is going to become unwieldy, but I'm not sure we are there yet.

@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

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

@rsc
Copy link
Contributor

rsc commented Apr 6, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: crypto/x509: add android user trusted CA folder as a possible source for certificate retrieval crypto/x509: add android user trusted CA folder as a possible source for certificate retrieval Apr 6, 2023
@rsc rsc modified the milestones: Proposal, Backlog Apr 6, 2023
@nightlyone
Copy link
Contributor

Can the implementation of the change be limited to Android?

/data is often used by system administrators to mount storage that is exported via CIFS/NFS, as a place to restore a backup from or do forensics on.
Those system administrators probably don't expect their certificate chain to be tampered with in those cases.

I admire the simplicity of the current implementation, but would prefer to limit Android specifics to the android build tag.

@FiloSottile
Copy link
Contributor

@nightlyone yes, good catch, mailed CL 531878.

@FiloSottile
Copy link
Contributor

Also, CL 473035 was submitted, so closing.

@gopherbot
Copy link

Change https://go.dev/cl/531878 mentions this issue: crypto/x509: avoid Android root store paths on other Linuxes

gopherbot pushed a commit that referenced this issue Oct 6, 2023
Updates #58922

Change-Id: I0eb2c97babb05b2d9bc36ed8af03579094bc02ac
Reviewed-on: https://go-review.googlesource.com/c/go/+/531878
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ingo Oeser <nightlyone@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS-Android Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Accepted
Development

No branches or pull requests

7 participants