Integrate CertFetcher with flag-protection into AMP Packager. #349

banaag · 2019-09-18T17:15:51Z

Update certcache to use certfetcher if cert autorenew is turned on.
Update ACME config to include email adddress and acme challenge port.
Update certloader.PopulateCertCache to instantiate certfetcher and pass it on to certcache instance.
Update all relevant tests.

… Update certcache to use certfetcher if cert autorenew is turned on. Update certloader.PopulateCertCache to instantiate certfetcher and pass it on to certcache instance.

Gregable

only minor comments, overall the structure looks good to me.

Gregable · 2019-09-18T20:18:31Z

packager/certcache/certcache.go

+// How often to check if certs needs updating.
+const CertCheckInterval = 24 * time.Hour
+
+// Recommended renewal duration for certs.


Maybe clarify in the comments that this is the duration before the next expiry rather than the duration after the last renewal.

Gregable · 2019-09-18T20:24:17Z

packager/certcache/certcache.go

+	}
+	d, err := util.GetDurationToExpiry(this.certs[0], this.certs[0].NotAfter)
+	if err != nil {
+		// Current cert is already invalid. Check if renewal is available.


Consider using TODO to mark places for the follow-on changes.

I already try to do this, what you pointed out is code I missed updating and now fixed. Thanks!

…address comments from gregable@.

…p autorenewcert config parsing.

…e loaded by non-auto-renewing amppackager instances.

…e gateway server dir. Re-ran go mod tidy.

twifkak

Very very very very sorry for the delay. I did my best to understand the changes, but I don't think I fully succeeded at that, so comments are a bit more surface-level than I'd normally prefer to give.

go.sum

packager/certcache/certcache.go

twifkak · 2019-10-29T23:04:03Z

packager/certcache/certcache.go

+			// If renewedCerts is set, copy that over to certs
+			// and set renewedCerts to nil.
+			// TODO(banaag): do the same cert setting dance on disk.
+			// Purge OCSP cache? Make shouldUpdateOCSP() return true?


Ooh, don't forget TODO this (future PR OK).

I did the cert setting dance on disk. The Purge OCSP cache can be a future PR.

packager/certcache/certcache.go

twifkak · 2019-10-29T23:17:47Z

packager/certcache/certcache.go

+	certs    []*x509.Certificate
+	// If certFetcher is not set, that means cert auto-renewal is not available.
+	certFetcher       *certfetcher.CertFetcher
+	renewedCertsMu    sync.RWMutex


The certName should change on a cert renewal (the sha256 computed by util.CertName is different), I think? So I think we need a CertCache.renewedCertName field also.

I think we need to put any reads/writes of certName and renewedCertName under their respective RWMutex.

twifkak · 2019-10-29T23:37:10Z

packager/certcache/certcache.go

+			log.Println("Error trying to fetch new certificates from CA: ", err)
+			return
+		}
+		this.setNewCerts(certs)


This logic seems incorrect to me; sorry if I read it wrong. AFAICT:

8 days before expiry, this code runs and creates the new cert file. Nothing uses it yet.

1 day later, maintainCerts calls this function again, which will lead down here to another call to FetchNewCert. Ditto daily.

Finally, maintainCerts calls this function and the old cert is expired, at which point the new cert is copied on top of it.

Step 2 seems like it'll make multiple redundant ACME requests for a cert.

Step 3 seems like it's happening at the wrong time (both too soon and too late). It seems like we should wait until we can get a GOOD OCSP response for it, and then immediately start serving it and using it to sign SXGs, so that we don't enter that 7-day period when we can't sign SXGs that last long enough.

Let me think about this and our offline discussion about OCSP checks and send a follow-up PR when it's done. Thanks!

packager/certloader/certloader.go

… I will address in a later commit.

banaag · 2019-11-07T18:31:36Z

Fixed the comments I missed (thanks twifkak@). Please feel free to continue review. @twifkak @Gregable

twifkak

OK, I don't want to block you any more than I already have (sorry!) so I'll approve with a few remaining changes you can make before merging.

Before recommending this to users, though, I would like to see:

A lot more test coverage (future PR OK). This adds some new functions with a lot of branching paths, and also complex interactions between functions based on the timing of the various go routines, or when things happen on the ACME or OCSP server, or when things happen on another AMP Packager.
Go over your TODOs again, to see if there's anything we missed (future PR OK). For example, I recall we were supposed to present the TOS as part of the initial setup process (certfetcher.go).

twifkak · 2019-11-05T00:48:14Z

packager/certcache/certcache.go

@@ -234,6 +237,9 @@ func (this *CertCache) ocspMidpoint(bytes []byte, issuer *x509.Certificate) (tim

 func (this *CertCache) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
 	params := mux.Params(req)
+
+	this.certsMu.Lock()


Should this be RLock/RUnlock? The only use of this.certs is a read, down on line 333.

Also, should it be closer to the read (i.e. inside the for loop)?

This is already RLock and RUnlock in the current version (maybe you were looking at an older file?). Also, this is supposed to guard the read on this.certName. In the current version, I don't see anything on line 333 that needs to be guarded.

twifkak · 2019-11-05T00:50:04Z

packager/certcache/certcache.go

@@ -475,22 +481,50 @@ func (this *CertCache) findIssuer() *x509.Certificate {
 	return nil
 }

+// Finds the issuer of the specified cert (i.e. the second from the bottom of the
+// chain).
+func (this *CertCache) findIssuerUsingCerts(certs []*x509.Certificate) *x509.Certificate {


Eliminate the duplication: implement findIssuer() using this.

twifkak · 2019-11-05T01:14:28Z

packager/certcache/certcache.go

@@ -669,30 +718,61 @@ func (this *CertCache) updateCertIfNecessary() {
 		this.setCerts(certs)


Should this one have an OCSP cache purge, too? If so, maybe move the certloader.RemoveFile call inside the setCerts impl.

Good catch. Done.

twifkak · 2019-11-05T01:16:19Z

packager/certcache/certcache.go

+        d := time.Duration(0)
+        err := errors.New("")
+        if this.hasCert() {
+                d, err = util.GetDurationToExpiry(this.certs[0], time.Now())
+        }


s/spaces/tabs/

Ran go fmt certcache.go

twifkak · 2019-11-05T01:45:20Z

packager/certcache/certcache.go

+        d := time.Duration(0)
+        err := errors.New("")


I'm not a fan of having these incorrect default values. Perhaps it's worth extracting the first half of this function as a:

func (this *CertCache) doesCertNeedReloading() bool { if !this.hasCert() { return true } d, err := ... return err != nil || d < certRenewalInterval }

twifkak · 2019-11-12T06:04:43Z

packager/certloader/certloader.go

-	issuer := certToPEM(certs[1])
-	bundled := append(cert, issuer...)
+	certLen := len(certs)
+	bundled := make([]byte, 0, 0)


Just []byte{} perhaps?

twifkak · 2019-11-12T06:06:43Z

packager/certloader/certloader.go

-	bundled := append(cert, issuer...)
+	certLen := len(certs)
+	bundled := make([]byte, 0, 0)
+	for i := 0; i < certLen - 1; i++ {


Can you just change certLen - 1 to certLen and get rid of the special case issuer after the loop? Better still use for _, cert := range certs {.

twifkak · 2019-11-12T06:10:15Z

packager/certloader/certloader.go

+		cert := certToPEM(certs[i])
+		bundled = append(bundled, cert...)
+	}
+	issuer := certToPEM(certs[certLen])


Won't certs[certLen] cause an out-of-range error?

Also, FYI, other than the leaf cert being at position 0, there's no ordering requirement for the rest of the certs array. The issuer cert can be any of the subsequent certs. (Hence why findIssuer() exists.)

Ok fixed in previous comment. Good to know about issuer position.

twifkak · 2019-11-12T06:12:48Z

packager/certcache/certcache.go

+		} else {
+			var ocspUpdateAfter time.Time
+
+			ocsp, _, errorOcsp := this.readOCSP(true)


It's Go style to capitalize the initialisms inside camel-case, e.g. errorOCSP and newOCSP. (But keep it lowercase if it's the first one, like ocspUpdateAfter.)

twifkak · 2019-11-12T06:22:51Z

packager/certcache/certcache.go

+
+			ocsp, _, errorOcsp := this.readOCSP(true)
+			if errorOcsp != nil {
+				newOcsp := this.fetchOCSP(ocsp, this.renewedCerts, &ocspUpdateAfter, false)


AFAICT, this is going to try to fetch the OCSP response for the new cert once a day, rather than using the backoff retry logic you added to readOCSP. Is that desired? Maybe that's OK for now, but can you add a TODO to revisit?

I realize it's difficult to use readOCSP here, since it's hard-coded to use this.certs and friends, rather than this.renewedCerts and friends. That makes me think two things:

The extraction of the retry logic from readOCSP would be useful.

We should bundle certName, certs, certsMu, ocspFile, ocspFilePath, ocspUpdateAfter, and ocspUpdateAfterMu into a new struct type, and then have two copies of that in certcache - one for current certs and one for new certs.

OK for future PR... TODO? 🐑 😁

The logic for the certs is by design and is as follows:
updateCertIfNecessary() will be called:

On demand, during initialization or when GetLatestCert() is called and we already have an expired cert.

Every 'certCheckInterval' hours (currently every 24 hours).

The logic for retries in readOCSP is specific to just reading OCSP and retrying when we got a new cert because of Digicert's setup.

Inside updateCertIfNecessary, there's a case where we already got the renewalCert (waiting in this.renewedCerts) and in this case, we make sure we also get the OCSP by calling readOCSP with retries set to true.

I understand your suggestion of possibly reusing the readOCSP retry logic in this edge case, but given that the updateCertIfNecessary() calls are a day apart, chances are the fetchOCSP might fail the first time, but will succeed the next day. Given that, it will still be nice-to-have the refactor of readOCSP so I added this as a TODO.

Gregable · 2019-11-13T18:58:32Z

packager/certcache/certcache.go

+// Callers can use the uninitialized CertCache for testing certificates (without doing OCSP or
+// cert refreshes).
+func New(certs []*x509.Certificate, certFetcher *certfetcher.CertFetcher, domains []string,
+	certFile string, newCertFile string, ocspCache string) *CertCache {


Consider this comment optional.

The long argument list makes the callsites tricky to read and easy to get wrong, especially if several of the arguments have the same type.

An alternative pattern would be to create an IsInitialized() bool or similarly named function that verifies all of the required fields have been set. Then callers can just set fields in the struct by name and assert IsInitialized before doing anything with it.

Added TODO.

Gregable · 2019-11-13T19:09:51Z

packager/certcache/certcache.go

+				continue
+			}
+		}
+		if len(ocsp) == 0 {


You have 3 places in this method that catch different error cases and trigger the numTries++;continue logic.

Consider extracting a method that does the ocsp read, checks all of the conditions (read error, empty file, unhealthy ocsp) and returns an error on any. This method and for loop then just implements the retry on error logic, but only needs a single condition/continue block.

Gregable · 2019-11-13T19:10:47Z

packager/certcache/certcache.go

+		} else {
+			break
+		}
+		numTries++


Is this reachable?

The previous condition block starting l.367 always exists I think.

Fixed with refactor from comment above.

Gregable · 2019-11-13T19:14:56Z

packager/certcache/certcache.go

+	// Wait using exponential backoff.
+	log.Printf("Waiting for %d minute(s)\n", waitTimeInMinutes)
+	waitTimeDuration := time.Duration(waitTimeInMinutes) * time.Minute
+	// For exponential backoff.


There is no "right" read pattern, so feel free to ignore as bikeshedding.

I think that the cost of making an OCSP request to the CA is reasonably low enough that we could do it every minute or 10 until it succeeds or we time out.

With this backoff, the last lookup is 8hrs apart? Seems excessively long to wait. I'd consider something like every 30s for 5 minutes, and then every 5 minutes for the next 12 hrs.

I changed the logic so that it caps the wait time at 10 minutes, max.

Gregable · 2019-11-13T19:21:57Z

packager/certfetcher/certfetcher.go

-	domains []string, acmeChallengePort int, shouldRegister bool) (*CertFetcher, error) {
+func New(email string, certSignRequest *x509.CertificateRequest, privateKey crypto.PrivateKey,
+	acmeDiscoURL string, httpChallengePort int, httpChallengeWebRoot string,
+	tlsChallengePort int, dnsProvider string, shouldRegister bool) (*CertFetcher, error) {


optional again. Same thought here about breaking this up. Callsite could have some structure like:

fetcher := CertFetcher()
fetcher.setUser(email, privateKey)
fetcher.bindToPort(port)

Added TODO.

banaag · 2019-11-13T21:35:12Z

Fixed @Gregable comments. PTAL.

twifkak · 2019-11-14T00:19:21Z

packager/certcache/certcache.go

@@ -107,6 +107,12 @@ type CertCache struct {
 // Callers need to call Init() on the returned CertCache before the cache can auto-renew certs.
 // Callers can use the uninitialized CertCache for testing certificates (without doing OCSP or
 // cert refreshes).
+//
+// TODO(banaag): per greigable@ comments:


Will address in follow-up PR #369

twifkak · 2019-11-14T00:20:05Z

packager/certcache/certcache.go

+
+	return ocsp, ocspUpdateAfter, nil
+}
+
 // Returns the OCSP response and expiry, refreshing if necessary.
 // TODO(banaag): Per twifkak's suggestion, consider:
 // It may also be interesting to try to separate the retry logic from the fetch logic. One approach comes to mind:


I think you just did this TODO, essentially?

Will address in follow-up PR #369

twifkak · 2019-11-14T00:21:25Z

packager/certcache/certcache.go

@@ -396,6 +411,10 @@ func waitForSpecifiedTime(waitTimeInMinutes int, numRetries int) int {
 	waitTimeDuration := time.Duration(waitTimeInMinutes) * time.Minute
 	// For exponential backoff.
 	newWaitTimeInMinutes := 2 * waitTimeInMinutes
+	if newWaitTimeInMinutes > 10 {
+		// Cap the wait time at 10 minutes.
+		newWaitTimeInMinutes = 10


Per this change, the comment on maxOCSPTries needs to be updated.

Will address in follow-up PR #369

First pass at updating amppkg.example.toml to document new experimental ACME support, implemented in #349 and others.

banaag added 2 commits September 17, 2019 14:21

Update ACME config to include email adddress and acme challenge port.…

3eae168

… Update certcache to use certfetcher if cert autorenew is turned on. Update certloader.PopulateCertCache to instantiate certfetcher and pass it on to certcache instance.

Update ACME config to include email adddress and acme challenge port.…

6e6936f

… Update certcache to use certfetcher if cert autorenew is turned on. Update certloader.PopulateCertCache to instantiate certfetcher and pass it on to certcache instance.

banaag requested review from twifkak and Gregable September 18, 2019 17:15

googlebot added the cla: yes label Sep 18, 2019

banaag added 2 commits September 18, 2019 12:12

Fix build errors in last commit.

da28431

Add config file error checking for PopulateCertCache.

53252ce

Gregable requested changes Sep 18, 2019

View reviewed changes

banaag added 9 commits September 18, 2019 14:33

Add more logic to handle initial conditions with invalid cert and to …

848dbff

…address comments from gregable@.

Code refactor/cleanup involving certs.

bfd3a6d

Add DNS and TLS challenges, added them to load from config, cleaned u…

058fdff

…p autorenewcert config parsing.

go mod tidy, go mod vendor updates

5d0800d

remove debug log statements

3869517

Fixed CSR Loading, added it to config

e793128

Fixed bugs with checking for cert expiry.

f5ba299

Added support for saving the fetched certs to disk and for certs to b…

7e81054

…e loaded by non-auto-renewing amppackager instances.

Fixed gateway server call to certcache. Removed go module files insid…

f2cdf94

…e gateway server dir. Re-ran go mod tidy.

twifkak mentioned this pull request Oct 23, 2019

Fix certcache to allow self-signed certs. #360

Merged

banaag added 5 commits October 23, 2019 13:14

Merge branch 'master' into certfetcher-integrate

234a353

Fixed certcache_test.go after merge.

f9bf520

Fixed bugs in certcache, also fixed unit test.

9633713

Added locking for reading/writing certs.

86f850b

Ran go fmt on files that have incorrect formatting

99e49b2

twifkak requested changes Oct 29, 2019

View reviewed changes

twifkak mentioned this pull request Oct 30, 2019

Support multiple certs in configuration #363

Open

Fix twifkak first-pass comments except the logic change comment which…

eeedb8f

… I will address in a later commit.

banaag requested review from Gregable and twifkak October 31, 2019 17:02

banaag added 2 commits October 31, 2019 13:39

Fix twifkak comments for ocsp refresh logic and ocsp cache purge.

eeae624

Fix additional twifkak comments.

8ac5827

twifkak approved these changes Nov 12, 2019

View reviewed changes

Fix 2nd round of twifkak comments.

1539d48

twifkak approved these changes Nov 13, 2019

View reviewed changes

Gregable approved these changes Nov 13, 2019

View reviewed changes

Fix gregable@ comments.

2a668e1

Gregable approved these changes Nov 13, 2019

View reviewed changes

banaag merged commit e416691 into ampproject:master Nov 13, 2019

twifkak reviewed Nov 14, 2019

View reviewed changes

banaag added a commit to banaag/amppackager that referenced this pull request Nov 15, 2019

Fix twifkak@ last comments from ampproject#349.

2e71592

banaag added a commit that referenced this pull request Nov 15, 2019

Fix twifkak@ last comments from #349. (#369)

0755ca4

twifkak pushed a commit that referenced this pull request Dec 5, 2019

Updating amppackage.example.toml documentation. (#361)

d88c537

First pass at updating amppkg.example.toml to document new experimental ACME support, implemented in #349 and others.

banaag added the acme All ACME related PRs label Feb 24, 2020

banaag deleted the certfetcher-integrate branch March 5, 2020 22:06

twifkak mentioned this pull request Jul 14, 2021

Add version information #545

Merged

		@@ -669,30 +718,61 @@ func (this *CertCache) updateCertIfNecessary() {
		this.setCerts(certs)

Integrate CertFetcher with flag-protection into AMP Packager. #349

Integrate CertFetcher with flag-protection into AMP Packager. #349

Conversation

banaag commented Sep 18, 2019

Gregable 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

twifkak 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

banaag commented Nov 7, 2019

twifkak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banaag Nov 12, 2019 • edited Loading

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

banaag commented Nov 13, 2019

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

twifkak left a comment •

edited

Loading

banaag Nov 12, 2019 •

edited

Loading