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

Fix certcache to allow self-signed certs. #360

Merged
merged 3 commits into from
Dec 18, 2019

Conversation

twifkak
Copy link
Member

@twifkak twifkak commented Oct 23, 2019

If the cert doesn't have an AIA extension with an OCSP responder URL,
then the cache will just include a fake (invalid) OCSP response in its
cert-chain+cbor. This is sufficient for displaying the SXG in Chrome
with --ignore-certificate-errors-spki-list.

Fixes #352.

@twifkak twifkak requested a review from banaag October 23, 2019 19:55
@twifkak
Copy link
Member Author

twifkak commented Oct 23, 2019

Note to reviewer: Marking this as "draft" because I'd rather not add another merge conflict for you to deal with in #349, plus maybe you have a better suggestion than my sentinel value hack.

@@ -41,7 +41,7 @@ func PopulateCertCache(config *util.Config, key crypto.PrivateKey, developmentMo
return nil, errors.Wrapf(err, "checking %s", config.CertFile)
}
}
certCache := certcache.New(certs, config.OCSPCache)
certCache := certcache.New(certs, config.OCSPCache, developmentMode)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you need to modify the certcache instantiation in gateway_server as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -51,6 +51,10 @@ const maxOCSPResponseBytes = 1024 * 1024
// How often to check if OCSP stapling needs updating.
const ocspCheckInterval = 1 * time.Hour

// Sentinel value used to communicate that a returned OCSP was fake, and the caller should not attempt to parse it.
// No, I'm not proud.
var fakeOCSP = []byte("fake ocsp response")
Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible to create a valid OCSP response by using the private key of a self-signed cert such as gateway.go does. But it needs two certs to have the same private key.

var ocspDer []byte
if len(certs) > 1 {
// Attach an OCSP response, signed with the second cert in the
// chain (assumed to be the issuer and using the same private
// key as the leaf cert).
var err error
ocspDer, err = createOCSPResponse(certs[1], privateKey.(*ecdsa.PrivateKey))
if err != nil {
return errorToSXGResponse(err), nil
}
} else {
// Make up an invalid OCSP response.
ocspDer = []byte("ocsp")
}

Or we can add an additional config to specify the issuer private key file to create a valid OCSP response.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the pointer; I didn't remember that gateway server did that. However, it seems that the gateway server doesn't fully exercise the certcache code, so it succeeds, whereas my attempt to adopt this fails, with "error validating fake response: bad OCSP signature: x509: ECDSA verification failure". Can you take a look at https://play.golang.org/p/8owXw00LsRx and see if you can find the error in my code?

Copy link
Contributor

Choose a reason for hiding this comment

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

The error can be resolved with the fix of https://play.golang.org/p/CvVyUD-_tji . The private key needs to be the issuer key which corresponds to the self-signed cert key in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes, that makes perfect sense. Thanks for the bug fix; I've updated this to generate a GOOD OCSP response as suggested.

If the cert doesn't have an AIA extension with an OCSP responder URL,
then the cache will just include a fake (invalid) OCSP response in its
cert-chain+cbor. This is sufficient for displaying the SXG in Chrome
with --ignore-certificate-errors-spki-list.
@twifkak twifkak marked this pull request as ready for review December 9, 2019 23:02
@twifkak twifkak merged commit 7bf8c12 into ampproject:master Dec 18, 2019
@twifkak twifkak deleted the self_signed branch December 18, 2019 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants