-
Notifications
You must be signed in to change notification settings - Fork 18k
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
x/crypto/ocsp: Support multiple OCSP responses/requests #30651
Comments
The last bug was closed due to a belief that there was no real life use-case. The fact this is triggering on a real life CA, duplicate-able on a production website is reason enough to me that there is a real life use-case for multiple response parsing. |
Would a simple |
Very much so yes! |
Hey @paultag I want help out with the fix but I am a little unclear on the steps to reproduce, could you make small code example of what you did to get the error? Thank you! |
Cert, CA, for each do an package main
import (
"bytes"
"crypto"
"crypto/x509"
"encoding/pem"
"fmt"
"io/ioutil"
"log"
"net/http"
"net/url"
"os"
"golang.org/x/crypto/ocsp"
)
func loadCert(path string) (*x509.Certificate, error) {
fd, err := os.Open(path)
if err != nil {
return nil, err
}
defer fd.Close()
bytes, err := ioutil.ReadAll(fd)
if err != nil {
return nil, err
}
block, _ := pem.Decode(bytes)
_ = block
// return x509.ParseCertificate(bytes)
return x509.ParseCertificate(bytes)
}
func main() {
cert, err := loadCert(os.Args[1])
if err != nil {
panic(err)
}
ca, err := loadCert(os.Args[2])
if err != nil {
panic(err)
}
revoked := isCertificateRevokedByOCSP(cert, ca, "http://ocsp.disa.mil")
fmt.Printf("Revoked: %t\n", revoked)
}
func isCertificateRevokedByOCSP(clientCert, issuerCert *x509.Certificate, ocspServer string) bool {
opts := &ocsp.RequestOptions{Hash: crypto.SHA1}
buffer, err := ocsp.CreateRequest(clientCert, issuerCert, opts)
if err != nil {
fmt.Printf("err: %s\n", err)
return false
}
httpRequest, err := http.NewRequest(http.MethodPost, ocspServer, bytes.NewBuffer(buffer))
if err != nil {
fmt.Printf("err: %s\n", err)
return false
}
ocspUrl, err := url.Parse(ocspServer)
if err != nil {
fmt.Printf("err: %s\n", err)
return false
}
httpRequest.Header.Add("Content-Type", "application/ocsp-request")
httpRequest.Header.Add("Accept", "application/ocsp-response")
httpRequest.Header.Add("host", ocspUrl.Host)
httpClient := &http.Client{}
httpResponse, err := httpClient.Do(httpRequest)
if err != nil {
fmt.Printf("err: %s\n", err)
return false
}
defer httpResponse.Body.Close()
output, err := ioutil.ReadAll(httpResponse.Body)
if err != nil {
fmt.Printf("err: %s\n", err)
return false
}
ocspResponse, err := ocsp.ParseResponse(output, issuerCert)
if err != nil {
fmt.Printf("err: %s\n", err)
return false
}
if ocspResponse.Status == ocsp.Revoked {
log.Printf("certificate has been revoked by OCSP server %s, refusing connection", ocspServer)
return true
} else {
return false
}
} |
Also maybe save the response to avoid hammering the OCSP server :) Output:
|
Would it be odd to introduce I gave this a shot, but I am not sure I am on the right path or have the OSCP expertise for this one :-/
|
@bennapp I'm not sure (I'm not a Go developer) but maybe they would appreciate a Pull Request and refactor of the old method to use this new one (and maintain old behavior if len > 1) Thanks for working on this! |
To add a little context: While the example behavior is quite rare (the DISA case is a real... doozy) it is still technically standards compliant (although strongly suggested against). |
So this is only needed to parse responses that violate a SHOULD NOT? |
Yup, I think probably the best improvement here would just be clarifying the documentation for |
Great, I'd appreciate a documentation CL. I personally also find "If the response contains a certificate then the signature over the response is checked." confusing. |
…nseForCert This change clarifies the behaviors of ParseResponse and ParseResponseForCert, particularly when parsing responses that contain multiple certificate statuses. Fixes golang/go#30651 Change-Id: Ia632c8c2a69d1b0c17d71f9f9dcb59ddb0be401b
Change https://golang.org/cl/220353 mentions this issue: |
…nseForCert This change clarifies the behaviors of ParseResponse and ParseResponseForCert, particularly when parsing responses that contain multiple certificate statuses. Fixes golang/go#30651 Change-Id: Ifa3c51583fa94a3e6e0c3f874213b8a702b7f82a
This change clarifies the behaviors of ParseResponse and ParseResponseForCert, particularly when parsing responses that contain multiple certificate statuses. Fixes golang/go#30651 Change-Id: Ia632c8c2a69d1b0c17d71f9f9dcb59ddb0be401b GitHub-Last-Rev: 481f613438c91521fd90078cc0915cf01cf44b4e GitHub-Pull-Request: golang/crypto#122 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353 Reviewed-by: Katie Hockman <katie@golang.org> Trust: Roland Shoemaker <roland@golang.org>
This change clarifies the behaviors of ParseResponse and ParseResponseForCert, particularly when parsing responses that contain multiple certificate statuses. Fixes golang/go#30651 Change-Id: Ia632c8c2a69d1b0c17d71f9f9dcb59ddb0be401b GitHub-Last-Rev: 481f613438c91521fd90078cc0915cf01cf44b4e GitHub-Pull-Request: golang/crypto#122 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353 Reviewed-by: Katie Hockman <katie@golang.org> Trust: Roland Shoemaker <roland@golang.org>
This change clarifies the behaviors of ParseResponse and ParseResponseForCert, particularly when parsing responses that contain multiple certificate statuses. Fixes golang/go#30651 Change-Id: Ia632c8c2a69d1b0c17d71f9f9dcb59ddb0be401b GitHub-Last-Rev: 481f613438c91521fd90078cc0915cf01cf44b4e GitHub-Pull-Request: golang/crypto#122 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353 Reviewed-by: Katie Hockman <katie@golang.org> Trust: Roland Shoemaker <roland@golang.org>
This change clarifies the behaviors of ParseResponse and ParseResponseForCert, particularly when parsing responses that contain multiple certificate statuses. Fixes golang/go#30651 Change-Id: Ia632c8c2a69d1b0c17d71f9f9dcb59ddb0be401b GitHub-Last-Rev: 481f613438c91521fd90078cc0915cf01cf44b4e GitHub-Pull-Request: golang/crypto#122 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353 Reviewed-by: Katie Hockman <katie@golang.org> Trust: Roland Shoemaker <roland@golang.org>
This change clarifies the behaviors of ParseResponse and ParseResponseForCert, particularly when parsing responses that contain multiple certificate statuses. Fixes golang/go#30651 Change-Id: Ia632c8c2a69d1b0c17d71f9f9dcb59ddb0be401b GitHub-Last-Rev: 481f613438c91521fd90078cc0915cf01cf44b4e GitHub-Pull-Request: golang/crypto#122 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353 Reviewed-by: Katie Hockman <katie@golang.org> Trust: Roland Shoemaker <roland@golang.org>
This change clarifies the behaviors of ParseResponse and ParseResponseForCert, particularly when parsing responses that contain multiple certificate statuses. Fixes golang/go#30651 Change-Id: Ia632c8c2a69d1b0c17d71f9f9dcb59ddb0be401b GitHub-Last-Rev: 481f613 GitHub-Pull-Request: golang#122 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353 Reviewed-by: Katie Hockman <katie@golang.org> Trust: Roland Shoemaker <roland@golang.org>
This change clarifies the behaviors of ParseResponse and ParseResponseForCert, particularly when parsing responses that contain multiple certificate statuses. Fixes golang/go#30651 Change-Id: Ia632c8c2a69d1b0c17d71f9f9dcb59ddb0be401b GitHub-Last-Rev: 481f613438c91521fd90078cc0915cf01cf44b4e GitHub-Pull-Request: golang/crypto#122 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353 Reviewed-by: Katie Hockman <katie@golang.org> Trust: Roland Shoemaker <roland@golang.org>
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
Send an OCSP to a server that replies with multiple responses
What did you expect to see?
One or all of the responses
What did you see instead?
An error,
OCSP response contains bad number of responses
Actual bug report
I'm basically reopening #17950
Some servers I've tried (such as
http://ocsp.disa.mil
orhttp://ocsp.managed.entrust.co m/OCSP/EMSSSPCAResponder
) reply with a payload that crashesx/crypto/ocsp
with an errorOCSP response contains bad number of responses
.Having a function that returns all responses received, or something else sensible would be ideal. I don't appear to be able to work around this as a library user without doing some serious asn1 mundging and repacking the ASN1 back to the OCSP library, which seems like it defeats the purpose.
This error appears to be triggering if you click on
crt.sh
asking for an OCSP check on the following certThe text was updated successfully, but these errors were encountered: