ACME client integration into webpkgserver. #53

banaag · 2020-09-05T00:05:00Z

Integrate support for ACME client into webpkgserver start-up.

banaag · 2020-09-05T00:05:17Z

yuizumi

Mostly good. Sorry for slow response.

yuizumi · 2020-09-07T03:36:03Z

certchain/certchainutil/certchainutil.go

 	"io/ioutil"
 	"net/http"
 	"net/url"
 	"os"

 	"github.com/WICG/webpackage/go/signedexchange"
 	"github.com/google/webpackager/certchain"
+	"github.com/pkg/errors"


We don't use pkg/errors. Use xerrors instead.

Apologies, I forgot about this. Fixed.

yuizumi · 2020-09-07T03:37:08Z

server/from_toml.go

-	mc := certmanager.Config{
-		RawChainSource: certmanager.NewLocalCertFile(
+	var rcs certmanager.RawChainSource
+	rcs = certmanager.NewLocalCertFile(


This looks to be unused.

Yes, already removed but perhaps you saw an older copy.

yuizumi · 2020-09-11T03:35:13Z

certchain/certchainutil/certchainutil.go

@@ -77,6 +80,24 @@ func ReadPrivateKeyFile(filename string) (crypto.PrivateKey, error) {
 	return signedexchange.ParsePrivateKey(pem)
 }

+// ReadCertificateRequestFile reads a PEM file and returns an x508 Certificate


"x508" looks like typo. It should be "x509" or "X.509", but I guess we can just remove it given (IIRC) we call it just "Certificate Request" elsewhere.

yuizumi · 2020-09-11T03:48:47Z

server/from_toml.go

+			return nil, err
+		}
+
+		rcs, err = acmeclient.NewClient(


Consistency:

rcs, err = acmeclient.NewClient( acmeclient.Config{ // ... }, // Note the trailing comma. )

OR

rcs, err = acmeclient.NewClient(acmeclient.Config{ // ... })

Fixed (2nd choice).

yuizumi · 2020-09-11T03:49:24Z

server/from_toml.go

+			return nil, err
+		}
+	} else {
+		rcs = certmanager.NewLocalCertFile(


yuizumi · 2020-09-11T04:05:49Z

certchain/certchainutil/certchainutil.go

+func ReadCertificateRequestFile(filename string) (*x509.CertificateRequest, error) {
+	data, err := ioutil.ReadFile(filename)
+	if err != nil {
+		return nil, errors.Wrapf(err, "reading %s", filename)


No need to wrap. ioutil.ReadFile (naturally) calls os.Open internally, os.Open always returns *os.PathError, thus the message already contains the filename.

yuizumi · 2020-09-11T04:07:27Z

certchain/certchainutil/certchainutil.go

+	}
+	block, _ := pem.Decode(data)
+	if block == nil {
+		return nil, errors.Errorf("pem decode: no key found in %s", filename)


Does "no key found" describe the problem correctly?

banaag · 2020-09-11T20:27:54Z

certchain/certmanager/augmentor.go

@@ -130,13 +127,18 @@ func (a *Augmentor) daemon(rcNext, orNext futureevent.Event) {
 				orNext, err = a.maintainOCSPResp()
 				if err != nil {
 					log.Printf("cannot update the OCSP response: %v", err)
+				} else {


Note that I added these log statements to indicate that the OCSP retrieval was successful after several failed retries.

banaag · 2020-09-11T20:28:36Z

certchain/certmanager/ocsp_client.go

@@ -107,23 +107,23 @@ func (c OCSPClient) Fetch(chain *certchain.RawChain, now func() time.Time) (ocsp

 	raw, expiry, err := c.fetch(chain)
 	if err != nil {
-		err = fmt.Errorf("cannot fetch OCSP response: %v", err)
+		err = fmt.Errorf("cannot fetch OCSP response: %v, retrying in %v", err, retryWait)


I also added the amount of time before next retry in the error, because it was useful to know how long the next retry would be.

banaag · 2020-09-11T20:31:43Z

certchain/certmanager/augmentor.go

@@ -107,9 +107,6 @@ func (a *Augmentor) Start() error {
 		return err
 	}
 	orNext, err := a.maintainOCSPResp()
-	if err != nil {


Yusuke, I had to remove this check and return statement. In the event that during start-up, we had an expired certificate and ACME retrieved it, it is very common for DigiCert OCSP response to fail for a period of time (due to updates of their data in the CDN). In this case, it will be beneficial to retry until we get the proper OCSP response. In the previous state of the code, no retry was attempted and the server exits with an error.

I understand there is a problem but I'm not fully sure if you had to ― I don't think dropping this if statement is the only option. At the very least the code is now violating the contract written in GoDoc: "It produces the first AugmentedChain before starting the goroutine and blocks until it is ready." This change can introduce subtle bugs since the rest of codebase is written with relying on that contract.

I have to say removing this check is over-focused on the DigiCert+ACME case. With this change Augmentor won't be able to handle some bad certificates in a proper way, including:

those truly revoked, for which the OCSP responder will never return okay.

those with a bogus OCSP URI, which can't be accessed in the first place.

The most straightforward approach is to do a "white-true" loop to wait until the function receives a valid ocsp response:

var orNext futureevent.Event for { orNext,err = a.maintainOCSPResp() if err == nil { break } log.Print(err) <-orNext.Chan() // I don't mean a busy loop. }

If possible, it's nicer to determine whether the failure is transient or permanent, and exit the loop on a permanent error.

Thanks for the comments. Updated to your suggested logic but added a retry counter so that it can break out of the loop after N retries.

yuizumi · 2020-09-17T11:08:31Z

certchain/certmanager/augmentor.go

+
+	var orNext futureevent.Event
+	i := 0
+	maxRetries := 3


Make it a const.

yuizumi · 2020-09-17T11:09:15Z

certchain/certmanager/augmentor.go

+		}
+		if i >= maxRetries {
+			log.Println("Exceeded number of retries for OCSP.")
+			break


return err. otherwise my point is not (fully) addressed.

yuizumi · 2020-09-24T17:21:34Z

certchain/certmanager/augmentor.go

-		return err
+
+	var orNext futureevent.Event
+	i := 0


Give a little more meaningful name, say, retryCount.

yuizumi · 2020-09-24T17:23:27Z

server/from_toml.go

 		OCSPRespSource: certmanager.NewOCSPClient(
 			certmanager.OCSPClientConfig{
 				AllowTestCert: c.SXG.Cert.AllowTestCert,
 			},
 		),
 	}
-	if c.SXG.Cert.CacheDir == "" {
+	if c.SXG.Cert.CacheDir != "" {
+		fmt.Printf("Creating SXG Cert Cache Directory: %s\n", c.SXG.Cert.CacheDir)


"Creating SXG cert cache directory" ― sentence case rather than title case. Maybe s/cert/certificate/.

yuizumi · 2020-09-25T06:05:47Z

certchain/certchainutil/certchainutil.go

@@ -77,6 +80,24 @@ func ReadPrivateKeyFile(filename string) (crypto.PrivateKey, error) {
 	return signedexchange.ParsePrivateKey(pem)
 }

+// ReadCertificateRequestFile reads a PEM file and returns a Certificate
+// Request usable for requesting Signed Exchange certificates.


GoDoc should be just "... returns a Certificate Request." This function checks nothing of SXG compatibility.

yuizumi · 2020-09-25T06:09:10Z

certchain/certmanager/augmentor.go

}
+

If you find this empty line increase the readability, add another above the line case <-orNext.Chan() for consistency. Otherwise, remove this empty line.

yuizumi · 2020-09-25T06:11:38Z

certchain/certmanager/augmentor.go

+
+	var orNext futureevent.Event
+	retryCount := 0
+	const maxRetries = 3


Maybe maxRetryCount (for consistency with retryCount). Alternative is to rename retryCount to retries, but I think retryCount is more descriptive.

banaag requested a review from yuizumi September 5, 2020 00:05

banaag force-pushed the acme branch 2 times, most recently from a6c457f to 64d31ef Compare September 10, 2020 15:46

yuizumi reviewed Sep 11, 2020

View reviewed changes

banaag force-pushed the acme branch from 64d31ef to 7dab02a Compare September 11, 2020 20:26

banaag commented Sep 11, 2020

View reviewed changes

banaag requested a review from yuizumi September 11, 2020 20:32

banaag force-pushed the acme branch 2 times, most recently from 96d516f to 95abb3a Compare September 16, 2020 18:15

yuizumi reviewed Sep 24, 2020

View reviewed changes

banaag force-pushed the acme branch from 95abb3a to 95dd047 Compare September 24, 2020 17:36

banaag requested a review from yuizumi September 24, 2020 17:37

yuizumi approved these changes Sep 25, 2020

View reviewed changes

Integrate support for ACME client into webpkgserver start-up.

d1a5d72

banaag force-pushed the acme branch from 95dd047 to d1a5d72 Compare September 25, 2020 07:00

banaag merged commit adf1984 into master Sep 25, 2020

banaag deleted the acme branch September 25, 2020 07:07

ACME client integration into webpkgserver. #53

ACME client integration into webpkgserver. #53

Conversation

banaag commented Sep 5, 2020

banaag commented Sep 5, 2020

yuizumi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment