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

Open
dnephin opened this issue May 8, 2021 · 11 comments
Open

crypto/x509: add CertPool.Equal #46057

dnephin opened this issue May 8, 2021 · 11 comments

Comments

@dnephin
Copy link
Contributor

@dnephin 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 this to Incoming in Proposals May 8, 2021
@bradfitz
Copy link
Contributor

@bradfitz bradfitz 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.

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 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 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 May 12, 2021
aminjam added 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 rsc commented May 19, 2021

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

@rsc
Copy link
Contributor

@rsc 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 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 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 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 Jun 2, 2021
@tmthrgd
Copy link
Contributor

@tmthrgd 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 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 Jun 9, 2021
@rsc
Copy link
Contributor

@rsc 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants