Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1452)

Issue 22020045: code review 22020045: crypto/x509: add non-cgo darwin system anchor certs (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 7 months ago by josharian
Modified:
4 years, 6 months ago
Reviewers:
CC:
golang-dev, minux1, agl1
Visibility:
Public.

Description

crypto/x509: add non-cgo darwin system anchor certs The set of certs fetched via exec'ing `security` is not quite identical to the certs fetched via the cgo call. The cgo fetch includes any trusted root certs that the user may have added; exec does not. The exec fetch includes an Apple-specific root cert; the cgo fetch does not. Other than that, they appear to be the same. Unfortunately, os/exec depends on crypto/x509, via net/http. Break the circular dependency by moving the exec tests to their own package. This will not work in iOS; we'll cross that bridge when we get to it.

Patch Set 1 #

Patch Set 2 : diff -r 0685a9549d5a https://code.google.com/p/go #

Patch Set 3 : diff -r 0685a9549d5a https://code.google.com/p/go #

Patch Set 4 : diff -r c0c2d0b05a77 https://code.google.com/p/go #

Patch Set 5 : diff -r c0c2d0b05a77 https://code.google.com/p/go #

Patch Set 6 : diff -r c0c2d0b05a77 https://code.google.com/p/go #

Patch Set 7 : diff -r c0c2d0b05a77 https://code.google.com/p/go #

Patch Set 8 : diff -r c0c2d0b05a77 https://code.google.com/p/go #

Total comments: 4

Patch Set 9 : diff -r c0c2d0b05a77 https://code.google.com/p/go #

Patch Set 10 : diff -r c0c2d0b05a77 https://code.google.com/p/go #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+89 lines, -98 lines) Patch
M src/pkg/crypto/x509/root_cgo_darwin.go View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -4 lines 0 comments Download
M src/pkg/crypto/x509/root_darwin.go View 1 2 3 4 5 6 7 8 9 1 chunk +10 lines, -68 lines 2 comments Download
A src/pkg/crypto/x509/root_darwin_test.go View 1 2 3 4 5 6 7 8 9 1 chunk +50 lines, -0 lines 0 comments Download
A src/pkg/crypto/x509/root_nocgo_darwin.go View 1 2 3 4 5 6 7 8 9 1 chunk +11 lines, -0 lines 0 comments Download
R src/pkg/crypto/x509/root_stub.go View 1 1 chunk +0 lines, -14 lines 0 comments Download
M src/pkg/os/exec/exec_test.go View 1 2 3 4 10 chunks +16 lines, -12 lines 0 comments Download

Messages

Total messages: 9
josharian
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
4 years, 6 months ago (2013-12-11 22:17:09 UTC) #1
minux1
why not test roots fetched via the non-cgo version is consistent with roots fetched via ...
4 years, 6 months ago (2013-12-11 22:47:12 UTC) #2
josharian
Yes, that's better, thanks. Will do. On Wed, Dec 11, 2013 at 2:47 PM, <minux.ma@gmail.com> ...
4 years, 6 months ago (2013-12-11 22:50:48 UTC) #3
agl1
https://codereview.appspot.com/22020045/diff/120001/src/pkg/crypto/x509/root_darwin_test.go File src/pkg/crypto/x509/root_darwin_test.go (right): https://codereview.appspot.com/22020045/diff/120001/src/pkg/crypto/x509/root_darwin_test.go#newcode13 src/pkg/crypto/x509/root_darwin_test.go:13: if want, have := 200, len(roots.certs); have < want ...
4 years, 6 months ago (2013-12-11 22:57:14 UTC) #4
josharian
Removed changes to run.bash; added a lightweight consistency test in its place. (The test passes ...
4 years, 6 months ago (2013-12-12 01:11:04 UTC) #5
agl1
https://codereview.appspot.com/22020045/diff/160001/src/pkg/crypto/x509/root_darwin.go File src/pkg/crypto/x509/root_darwin.go (right): https://codereview.appspot.com/22020045/diff/160001/src/pkg/crypto/x509/root_darwin.go#newcode9 src/pkg/crypto/x509/root_darwin.go:9: func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate, err error) ...
4 years, 6 months ago (2013-12-12 16:29:47 UTC) #6
josharian
On 2013/12/12 16:29:47, agl1 wrote: > https://codereview.appspot.com/22020045/diff/160001/src/pkg/crypto/x509/root_darwin.go > File src/pkg/crypto/x509/root_darwin.go (right): > > https://codereview.appspot.com/22020045/diff/160001/src/pkg/crypto/x509/root_darwin.go#newcode9 > ...
4 years, 6 months ago (2013-12-12 17:27:39 UTC) #7
josharian
ping
4 years, 6 months ago (2013-12-18 14:50:35 UTC) #8
agl1
4 years, 6 months ago (2013-12-18 15:57:26 UTC) #9
*** Submitted as https://code.google.com/p/go/source/detail?r=3df1b0ec6119 ***

crypto/x509: add non-cgo darwin system anchor certs

The set of certs fetched via exec'ing `security` is not quite identical
to the certs fetched via the cgo call. The cgo fetch includes
any trusted root certs that the user may have added; exec does not.
The exec fetch includes an Apple-specific root cert; the cgo fetch
does not. Other than that, they appear to be the same.

Unfortunately, os/exec depends on crypto/x509, via net/http. Break the
circular dependency by moving the exec tests to their own package.

This will not work in iOS; we'll cross that bridge when we get to it.

R=golang-dev, minux.ma, agl
CC=golang-dev
https://codereview.appspot.com/22020045

Committer: Adam Langley <agl@golang.org>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b