Fix OCSP checking (#274)

Fixes #272. The root of the problem is that we're asking the wrong OCSP servers. The current code does something like:

leaf, issuers := chain[0], chain[1:]
for _, issuer := range issuers {
  // check for revocation of leaf via issuer.OCSPServer
}

The main problem is that OCSPServer is the URI of the OCSP server for that certificate, and by asking issuer.OCSPServer about leaf, we're requesting OCSP responses from the wrong server. We should be checking leaf.OCSPServer in this example instead of issuer.OCSPServer.

The second problem is that the loop makes no sense. One would not expect any OCSP server in a given chain to be authoritative for a given leaf.

If you do certigo connect google.com, you'll see

Certificate has OCSP extension, but was unable to check status:
	ocsp: error from server: unauthorized

The "unauthorized" means the OCSP server we asked doesn't know anything about the cert we asked about.

lib/ocsp.go

@@ -22,7 +22,7 @@ import (
 	"encoding/base64"
 	"errors"
 	"fmt"
-	"io/ioutil"
+	"io"
 	"net/http"
 	"strings"
 	"time"
@@ -32,7 +32,7 @@ import (
 )
 
 var (
-	skippedRevocationCheck = errors.New("skipped revocation check")
+	errSkippedRevocationCheck = errors.New("skipped revocation check")
 
 	revocationStatusColor = map[int]*color.Color{
 		ocsp.Good:    green,
@@ -71,18 +71,14 @@ const (
 )
 
 func checkOCSP(chain []*x509.Certificate, ocspStaple []byte) (status *ocsp.Response, err error) {
-	if len(chain) <= 1 {
+	if len(chain) < 2 {
 		// Nothing to check here
-		return nil, skippedRevocationCheck
+		return nil, errSkippedRevocationCheck
 	}
 
-	// Skip if there are no OCSP servers in the chain.
-	numServers := 0
-	for _, cert := range chain[1:] {
-		numServers += len(cert.OCSPServer)
-	}
-	if numServers == 0 {
-		return nil, skippedRevocationCheck
+	leaf, issuer := chain[0], chain[1]
+	if len(leaf.OCSPServer) == 0 {
+		return nil, errSkippedRevocationCheck
 	}
 
 	retries := maxOCSPValidationRetries
@@ -91,11 +87,10 @@ func checkOCSP(chain []*x509.Certificate, ocspStaple []byte) (status *ocsp.Respo
 		retries = 1
 	}
 
-	var issuer *x509.Certificate
 	for i := 0; i < retries; i++ {
 		encoded := ocspStaple
 		if len(encoded) == 0 {
-			encoded, _, err = fetchOCSP(chain)
+			encoded, err = fetchOCSP(leaf, issuer)
 			if err != nil {
 				return nil, err
 			}
@@ -110,64 +105,61 @@ func checkOCSP(chain []*x509.Certificate, ocspStaple []byte) (status *ocsp.Respo
 	return status, err
 }
 
-func fetchOCSP(chain []*x509.Certificate) ([]byte, *x509.Certificate, error) {
+func fetchOCSP(cert, issuer *x509.Certificate) ([]byte, error) {
+	encoded, err := ocsp.CreateRequest(cert, issuer, nil)
+	if err != nil {
+		return nil, fmt.Errorf("failure building request: %s", err)
+	}
+
+	// Try all the OCSP servers listed in the certificate
 	var lastError error
-	for _, issuer := range chain[1:] {
-		encoded, err := ocsp.CreateRequest(chain[0], issuer, nil)
-		if err != nil {
-			return nil, nil, fmt.Errorf("failure building request: %s", err)
-		}
-
-		// Try all the OCSP servers listed in the certificate
-		for _, server := range issuer.OCSPServer {
-			// We try both GET and POST requests, because some servers are janky.
-			reqs := []*http.Request{}
-			if len(encoded) < 255 {
-				// GET only supported for requests with small payloads, so we can stash
-				// them in the path. RFC says 255 bytes encoded, but doesn't mention if that
-				// refers to the DER-encoded payload before or after base64 is applied. We
-				// just assume it's the former and try both GET and POST in case one fails.
-				req, err := buildOCSPwithGET(server, encoded)
-				if err != nil {
-					lastError = err
-					continue
-				}
-				reqs = append(reqs, req)
-			}
-
-			// POST should always be supported, but some servers don't like it
-			req, err := buildOCSPwithPOST(server, encoded)
+	for _, server := range cert.OCSPServer {
+		// We try both GET and POST requests, because some servers are janky.
+		reqs := []*http.Request{}
+		if len(encoded) < 255 {
+			// GET only supported for requests with small payloads, so we can stash
+			// them in the path. RFC says 255 bytes encoded, but doesn't mention if that
+			// refers to the DER-encoded payload before or after base64 is applied. We
+			// just assume it's the former and try both GET and POST in case one fails.
+			req, err := buildOCSPwithGET(server, encoded)
 			if err != nil {
 				lastError = err
 				continue
 			}
 			reqs = append(reqs, req)
+		}
 
-			for _, req := range reqs {
-				resp, err := ocspHttpClient.Do(req)
-				if err != nil {
-					lastError = err
-					continue
-				}
+		// POST should always be supported, but some servers don't like it
+		req, err := buildOCSPwithPOST(server, encoded)
+		if err != nil {
+			lastError = err
+			continue
+		}
+		reqs = append(reqs, req)
 
-				if resp.StatusCode != http.StatusOK {
-					lastError = fmt.Errorf("unexpected status code, got: %s", resp.Status)
-					continue
-				}
-
-				body, err := ioutil.ReadAll(resp.Body)
-				defer resp.Body.Close()
-				if err != nil {
-					lastError = err
-					continue
-				}
-
-				return body, issuer, nil
+		for _, req := range reqs {
+			resp, err := ocspHttpClient.Do(req)
+			if err != nil {
+				lastError = err
+				continue
 			}
+
+			if resp.StatusCode != http.StatusOK {
+				lastError = fmt.Errorf("unexpected status code, got: %s", resp.Status)
+				continue
+			}
+
+			body, err := io.ReadAll(resp.Body)
+			defer resp.Body.Close()
+			if err != nil {
+				lastError = err
+				continue
+			}
+			return body, nil
 		}
 	}
 
-	return nil, nil, lastError
+	return nil, lastError
 }
 
 func buildOCSPwithPOST(server string, encoded []byte) (*http.Request, error) {

lib/verify.go

@@ -20,6 +20,7 @@ import (
 	"crypto/x509"
 	"encoding/json"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -167,7 +168,7 @@ func VerifyChain(certs []*x509.Certificate, ocspStaple []byte, expectedName, caP
 		if err == nil {
 			result.OCSPStatus = status
 		}
-		if err != nil && err != skippedRevocationCheck {
+		if err != nil && !errors.Is(err, errSkippedRevocationCheck) {
 			result.OCSPError = err.Error()
 		}