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.

Sign up for GitHub

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

Jump to bottom

crypto/x509: check Certificate.isValid on CertPool.Add #66166

Open
rolandshoemaker opened this issue Mar 7, 2024 · 3 comments
Open

crypto/x509: check Certificate.isValid on CertPool.Add #66166

rolandshoemaker opened this issue Mar 7, 2024 · 3 comments
Assignees
@rolandshoemaker
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@rolandshoemaker
Copy link
Member

We shouldn't be adding certificates to the pool which we cannot reasonably later use during chain building. CertPool.Add doesn't have an error return, so we should just silently discard the certificate when isValid returns a non-nil error.
@rolandshoemaker rolandshoemaker self-assigned this Mar 7, 2024
@rolandshoemaker rolandshoemaker mentioned this issue Mar 7, 2024
Open
@rittneje
Copy link

rittneje commented Mar 8, 2024

Would that not be a breaking change wrt CertPool.Subjects?

@rolandshoemaker
Copy link
Member Author

I see there is a behavioral change here (i.e. Subjects will return the subject of a cert, but will likely not actually use it during chain building), but I don't really see how its breaking? (also side note, CertPool.Subjects is already deprecated, further changing its behavior seems fine).

@rittneje
Copy link

rittneje commented Mar 8, 2024

If I call AddCert with such an invalid certificate, will Subjects still report it? If not I think it could be a breaking change.

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rolandshoemaker rolandshoemaker

Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rolandshoemaker @seankhliao @rittneje