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

Better error message for missing intermediates #182

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

csstaub
Copy link
Contributor

@csstaub csstaub commented Apr 4, 2019

If verification fails, retries using intermediate retrieved via AIA fetching and updates error message to make it clearer what the misconfiguration is.

Used to print:
Screen Shot 2019-04-04 at 15 25 25

But now shows:
Screen Shot 2019-04-04 at 15 25 05

This is best-effort.


if len(intermediates.Subjects()) == 0 {
// No intermediates found, maybe broken chain. Let's try AIA fetching?
intermediate := fetchAIA(certs[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're only trying the first one in the chain? I guess it's pretty rare to have more than 3 certs in a chain.

for _, url := range cert.IssuingCertificateURL {
resp, err := http.Get(url)
if err != nil {
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we log errors?

@@ -109,6 +111,30 @@ func caBundle(caPath string) *x509.CertPool {
return bundle
}

func fetchAIA(cert *x509.Certificate) *x509.Certificate {
for _, url := range cert.IssuingCertificateURL {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rfc4325 says we should look at the file extension to see if it's .cer or .p7c. We could return if it's .p7c and only support .cer fetching

@mbyczkowski
Copy link
Contributor

should this be merged @mcpherrinm or does it need more work?

@mcpherrinm
Copy link
Contributor

I think we should do the p7c/cer and recursive AIA fetching stuff in comments

Copy link
Contributor

@mcpherrinm mcpherrinm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to handle updating this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants