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 CertPool.Equal #46057

Closed
dnephin opened this issue May 8, 2021 · 14 comments
Closed

crypto/x509: add CertPool.Equal #46057

dnephin opened this issue May 8, 2021 · 14 comments
Labels
Milestone

Comments

@dnephin
Copy link
Contributor

dnephin commented May 8, 2021

What version of Go are you using (go version)?

go 1.16.x

What did you expect to see?

In response to #45891, x509.CertPool structs can no longer be compared with reflect.DeepEqual, and there is no way to export the certificates, so there is no longer any way to fully compare these structs.

CertPool.Subjects can be used for a shallow comparison, but #45891 (comment) shows that this is not a complete comparison.

By adding an Equal(other CertPool) bool these types can be compared directly, and go-cmp will automatically use the method for comparison.

@ianlancetaylor ianlancetaylor changed the title crypto/x509: add CertPool.Equal proposal: crypto/x509: add CertPool.Equal May 8, 2021
@gopherbot gopherbot added this to the Proposal milestone May 8, 2021
@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label May 8, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) May 8, 2021
@bradfitz
Copy link
Contributor

In recent versions of Go, CertPool now internally represents a lazy set of certificates that might not be fully loaded yet.

What does "Equal" mean in this proposal? Would you want it to load them all from disk if they weren't yet?

Can you give some background on when you'd use this?

@dnephin
Copy link
Contributor Author

dnephin commented May 12, 2021

In recent versions of Go, CertPool now internally represents a lazy set of certificates that might not be fully loaded yet.

Yes, I believe this change is what prevents reflect.DeepEqual from working as it did before (#45891).

#45891 has some context. We have tests that used reflect.DeepEqual to compare two x509.CertPool, which worked fine in go1.15.x. In Go 1.16.x those tests no longer work because two effectively equivalent cert pools are no longer considered equal.

I would expect Equal to load all the content from disk and compare the content using reflect.DeepEqual, which I think should make it equivalent to using reflect.DeepEqual in go1.15.x.

@rsc
Copy link
Contributor

rsc commented May 12, 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) May 12, 2021
aminjam pushed a commit to cloudfoundry/diego-ssh that referenced this issue May 13, 2021
for right now we are only comparing the Subject until the issue is
resolved

Context: golang/go#46057
@rsc
Copy link
Contributor

rsc commented May 19, 2021

/cc @FiloSottile for whether this is a reasonable API addition

@rsc
Copy link
Contributor

rsc commented May 26, 2021

Talked to Filippo and he said this was reasonable to do. We are planning to keep the cert pools opaque when they are derived from the system pool, but it is easy to tell if two different pools both include the system pool or not.

Does anyone object to this?

@tmthrgd
Copy link
Contributor

tmthrgd commented May 26, 2021

Does anyone actually have a non-test use for this? Or is this something that comes up at all commonly? It sounds like it could end up being an unexpectedly slow and heavy-weight function.

@rsc
Copy link
Contributor

rsc commented Jun 2, 2021

@tmthrgd Filippo says it can be implemented efficiently. The definition of CertPool.Equal would be "same 'use system pool' setting and same additional certificates". So the system pool itself need not be consulted for implementing Equal.

@rsc
Copy link
Contributor

rsc commented Jun 2, 2021

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

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Jun 2, 2021
@tmthrgd
Copy link
Contributor

tmthrgd commented Jun 2, 2021

@tmthrgd Filippo says it can be implemented efficiently. The definition of CertPool.Equal would be "same 'use system pool' setting and same additional certificates". So the system pool itself need not be consulted for implementing Equal.

More than happy to be wrong, but I was basing that off of this request/requirement above: “I would expect Equal to load all the content from disk and compare the content using reflect.DeepEqual[.]” That sounded deceptively expensive to me. Though if it’s not expensive, there doesn’t seem to be any real reason to oppose it.

@rsc
Copy link
Contributor

rsc commented Jun 9, 2021

I would expect Equal to load all the content from disk and compare the content using reflect.DeepEqual, which I think should make it equivalent to using reflect.DeepEqual in go1.15.x.

To be clear, it won't. It will just compare whether two CertPools both say "include what's on disk" or not.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Jun 9, 2021
@rsc
Copy link
Contributor

rsc commented Jun 9, 2021

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 CertPool.Equal crypto/x509: add CertPool.Equal Jun 9, 2021
@rsc rsc modified the milestones: Proposal, Backlog Jun 9, 2021
@FiloSottile FiloSottile removed this from the Backlog milestone Mar 2, 2022
@gopherbot
Copy link

Change https://go.dev/cl/388915 mentions this issue: crypto/x509: add CertPool.Equal

@bcmills bcmills reopened this Apr 5, 2022
@gopherbot
Copy link

Change https://go.dev/cl/398237 mentions this issue: api: add x509.CertPool.Equal to next/46057.txt

gopherbot pushed a commit that referenced this issue Apr 5, 2022
CL 388915 added an exported API but was authored (and tested)
before the API check became stricter.

Updates #46057.

Change-Id: Iee6e4969ade77fb0539fa97fcef0047389fb2cf6
Reviewed-on: https://go-review.googlesource.com/c/go/+/398237
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
@dmitshur dmitshur closed this as completed Apr 5, 2022
@ericlagergren
Copy link
Contributor

// Equal reports whether s and other are equal.

@rolandshoemaker Under what conditions are two CertPools equal? The docs are a truism. I understand wanting to keep it a bit vague, but being too vague just causes people to look at the implementation.

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
No open projects
Development

No branches or pull requests