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

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

Merged
merged 23 commits into from
Nov 13, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
3eae168
Update ACME config to include email adddress and acme challenge port.…
banaag Sep 17, 2019
6e6936f
Update ACME config to include email adddress and acme challenge port.…
banaag Sep 18, 2019
da28431
Fix build errors in last commit.
banaag Sep 18, 2019
53252ce
Add config file error checking for PopulateCertCache.
banaag Sep 18, 2019
848dbff
Add more logic to handle initial conditions with invalid cert and to …
banaag Sep 18, 2019
bfd3a6d
Code refactor/cleanup involving certs.
banaag Sep 23, 2019
058fdff
Add DNS and TLS challenges, added them to load from config, cleaned u…
banaag Sep 27, 2019
5d0800d
go mod tidy, go mod vendor updates
banaag Sep 27, 2019
3869517
remove debug log statements
banaag Sep 27, 2019
e793128
Fixed CSR Loading, added it to config
banaag Sep 27, 2019
f5ba299
Fixed bugs with checking for cert expiry.
banaag Sep 30, 2019
7e81054
Added support for saving the fetched certs to disk and for certs to b…
banaag Oct 21, 2019
f2cdf94
Fixed gateway server call to certcache. Removed go module files insid…
banaag Oct 22, 2019
234a353
Merge branch 'master' into certfetcher-integrate
banaag Oct 23, 2019
f9bf520
Fixed certcache_test.go after merge.
banaag Oct 23, 2019
9633713
Fixed bugs in certcache, also fixed unit test.
banaag Oct 24, 2019
86f850b
Added locking for reading/writing certs.
banaag Oct 25, 2019
99e49b2
Ran go fmt on files that have incorrect formatting
banaag Oct 25, 2019
eeedb8f
Fix twifkak first-pass comments except the logic change comment which…
banaag Oct 31, 2019
eeae624
Fix twifkak comments for ocsp refresh logic and ocsp cache purge.
banaag Oct 31, 2019
8ac5827
Fix additional twifkak comments.
banaag Nov 7, 2019
1539d48
Fix 2nd round of twifkak comments.
banaag Nov 13, 2019
2a668e1
Fix gregable@ comments.
banaag Nov 13, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion cmd/amppkg/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ var flagConfig = flag.String("config", "amppkg.toml", "Path to the config toml f
var flagDevelopment = flag.Bool("development", false, "True if this is a development server.")
var flagInvalidCert = flag.Bool("invalidcert", false, "True if invalid certificate intentionally used in production.")

// IMPORTANT: do not turn on this flag for now, it's still under development.
var flagAutoRenewCert = flag.Bool("autorenewcert", false, "True if amppackager is to attempt cert auto-renewal.")

// Prints errors returned by pkg/errors with stack traces.
func die(err interface{}) { log.Fatalf("%+v", err) }

Expand Down Expand Up @@ -82,7 +85,7 @@ func main() {
die(errors.Wrap(err, "loading key file"))
}

certCache, err := certloader.PopulateCertCache(config, key, *flagDevelopment || *flagInvalidCert);
certCache, err := certloader.PopulateCertCache(config, key, *flagDevelopment || *flagInvalidCert, *flagAutoRenewCert)
if err != nil {
die(errors.Wrap(err, "building cert cache"))
}
Expand Down
11 changes: 11 additions & 0 deletions cmd/gateway_server/go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
module github.com/ampproject/amppackager/cmd/gateway_server

go 1.13

require (
github.com/WICG/webpackage v0.0.0-20190904045429-ab0e18a2113c // indirect
github.com/ampproject/amppackager v0.0.0-20190911170541-c9993b8ac4d1 // indirect
github.com/golang/protobuf v1.3.2 // indirect
golang.org/x/crypto v0.0.0-20190911031432-227b76d455e7 // indirect
google.golang.org/grpc v1.23.1 // indirect
)
45 changes: 45 additions & 0 deletions cmd/gateway_server/go.sum
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
github.com/WICG/webpackage v0.0.0-20190904045429-ab0e18a2113c h1:z+hJKXGCAdk9H/JKAmACY2bKyLhAInkjMh/VVlKNxEI=
github.com/WICG/webpackage v0.0.0-20190904045429-ab0e18a2113c/go.mod h1:RmCSqjSsrBtTrYiO+OKMBfMgztnTnFXVSapi/mNB7IA=
github.com/ampproject/amppackager v0.0.0-20190911170541-c9993b8ac4d1 h1:vwqF8hQdUaCP6u9FzVXwwFKJ+5kdQDOkp2vYlTbTSGE=
github.com/ampproject/amppackager v0.0.0-20190911170541-c9993b8ac4d1/go.mod h1:kqUX/cvxB07nOtlh8/35T4sXNOos7/kqOJTN34XAvF8=
github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q=
github.com/golang/mock v1.1.1/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A=
github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/protobuf v1.3.2 h1:6nsPYzhq5kReh6QImI3k5qWzO4PEbvbIW2cwSfR/6xs=
github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5aqRK0M=
github.com/influxdata/influxdb v1.6.3/go.mod h1:qZna6X/4elxqT3yI9iZYdZrWWdeFOOprn86kgg4+IzY=
github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ=
github.com/mrichman/hargo v0.1.2-0.20190117125451-162adce4527e/go.mod h1:ycD51zRGXcO6ak4DnFPjHv4xzbgRU5tYyWDzbMzFYKw=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/sirupsen/logrus v1.3.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo=
github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/ugorji/go/codec v0.0.0-20181209151446-772ced7fd4c2/go.mod h1:VFNgLljTbGfSG7qAOspJ7OScBnGdDN/yBr0sguwnwf0=
golang.org/x/crypto v0.0.0-20180904163835-0709b304e793/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
golang.org/x/crypto v0.0.0-20180910181607-0e37d006457b/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
golang.org/x/crypto v0.0.0-20190911031432-227b76d455e7 h1:0hQKqeLdqlt5iIwVOBErRisrHJAN57yOiPRQItI20fU=
golang.org/x/crypto v0.0.0-20190911031432-227b76d455e7/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc=
golang.org/x/net v0.0.0-20190110200230-915654e7eabc/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs=
golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135/go.mod h1:RgjU9mgBXZiqYHBnxXauZ1Gv1EHHAz9KjViQ78xBX0Q=
google.golang.org/appengine v1.1.0/go.mod h1:EbEs0AVv82hx2wNQdGPgUI5lhzA/G0D9YwlJXL52JkM=
google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8/go.mod h1:JiN7NxoALGmiZfu7CAH4rXhgtRTLTxftemlI0sWmxmc=
google.golang.org/grpc v1.23.1 h1:q4XQuHFC6I28BKZpo6IYyb3mNO+l7lSOxRuYTCiDfXk=
google.golang.org/grpc v1.23.1/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyacEbxg=
gopkg.in/urfave/cli.v1 v1.20.0/go.mod h1:vuBzUtMdQeixQj8LVd+/98pzhxNGQoyuPBlsXHOQNO0=
honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
2 changes: 1 addition & 1 deletion cmd/gateway_server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func (s *gatewayServer) GenerateSXG(ctx context.Context, request *pb.SXGRequest)
}

// Note: do not initialize certCache, we just want it to hold the certs for now.
certCache := certcache.New(certs, "");
certCache := certcache.New(certs, nil, "");

privateKey, err := util.ParsePrivateKey(request.PrivateKey)
if err != nil {
Expand Down
138 changes: 129 additions & 9 deletions packager/certcache/certcache.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"time"

"github.com/WICG/webpackage/go/signedexchange/certurl"
"github.com/ampproject/amppackager/packager/certfetcher"
"github.com/ampproject/amppackager/packager/mux"
"github.com/ampproject/amppackager/packager/util"
"github.com/pkg/errors"
Expand All @@ -46,10 +47,19 @@ var infiniteFuture = time.Unix(1<<63-62135596801, 999999999)
// 1MB is the maximum used by github.com/xenolf/lego/acmev2 in GetOCSPForCert.
// Alternatively, here's a random documented example of a 20K limit:
// https://www.ibm.com/support/knowledgecenter/en/SSPREK_9.0.0/com.ibm.isam.doc/wrp_stza_ref/reference/ref_ocsp_max_size.html
const maxOCSPResponseBytes = 1024 * 1024
const MaxOCSPResponseBytes = 1024 * 1024
twifkak marked this conversation as resolved.
Show resolved Hide resolved

// How often to check if OCSP stapling needs updating.
const ocspCheckInterval = 1 * time.Hour
const OcspCheckInterval = 1 * time.Hour

// How often to check if certs needs updating.
const CertCheckInterval = 24 * time.Hour

// Recommended renewal duration for certs.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

// 8 days is recommended duration to start requesting new certs to allow for ACME server outages.
// It's 6 days + 2 days renewal grace period.
twifkak marked this conversation as resolved.
Show resolved Hide resolved
// TODO(banaag): make 2 days renewal grace period configurable in toml.
const CertRenewalInterval = 8 * 24 * time.Hour

type CertHandler interface {
GetLatestCert() (*x509.Certificate)
Expand All @@ -58,25 +68,35 @@ type CertHandler interface {
type CertCache struct {
// TODO(twifkak): Support multiple cert chains (for different domains, for different roots).
certName string
certsMu sync.RWMutex
certs []*x509.Certificate
// If certFetcher is not set, that means cert auto-renewal is not available.
certFetcher *certfetcher.CertFetcher
renewedCertsMu sync.RWMutex
renewedCerts []*x509.Certificate
ocspUpdateAfterMu sync.RWMutex
ocspUpdateAfter time.Time
// TODO(twifkak): Implement a registry of Updateable instances which can be configured in the toml.
ocspFile Updateable
client http.Client
ocspFile Updateable
client http.Client

// "Virtual methods", exposed for testing.
// Given a certificate, returns the OCSP responder URL for that cert.
extractOCSPServer func(*x509.Certificate) (string, error)
// Given an HTTP request/response, returns its cache expiry.
httpExpiry func(*http.Request, *http.Response) time.Time
// Is CertCache initialized to do cert renewal or OCSP refreshes?
isInitialized bool
twifkak marked this conversation as resolved.
Show resolved Hide resolved
}

// Must call Init() on the returned CertCache before you can use it.
func New(certs []*x509.Certificate, ocspCache string) *CertCache {
// Callers need to call Init() on the returned CertCache before the cache can auto-renew certs.
// Callers can use the uninitialized CertCache either for testing certificates (without doing OCSP or
twifkak marked this conversation as resolved.
Show resolved Hide resolved
// cert refreshes).
func New(certs []*x509.Certificate, certFetcher *certfetcher.CertFetcher, ocspCache string) *CertCache {
return &CertCache{
certName: util.CertName(certs[0]),
certs: certs,
certFetcher: certFetcher,
ocspUpdateAfter: infiniteFuture, // Default, in case initial readOCSP successfully loads from disk.
// Distributed OCSP cache to support the following sleevi requirements:
// 1. Support for keeping a long-lived (disk) cache of OCSP responses.
Expand Down Expand Up @@ -104,6 +124,7 @@ func New(certs []*x509.Certificate, ocspCache string) *CertCache {
return expiry
}
},
isInitialized: false,
}
}

Expand All @@ -122,15 +143,40 @@ func (this *CertCache) Init(stop chan struct{}) error {
// to raise an alert if something has gone really wrong.
// 7. The ability to serve old responses while fetching new responses.
go this.maintainOCSP(stop)

if this.certFetcher != nil {
// Update Certs in the background.
go this.maintainCerts(stop)
}

this.isInitialized = true
return nil
}

// For now this just returns the first entry in the certs field in the cache.
// For follow-on changes, we will transform this to a lambda so that anything
// that needs a cert can do the cert refresh logic (if needed) on demand.
func (this *CertCache) GetLatestCert() (*x509.Certificate) {
// TODO(banaag): check if cert is still valid, refresh if not.
return this.certs[0]
if !this.isInitialized || this.certFetcher == nil {
// If certcache is not initialized or certFetcher is not set,
// just return cert without checking if it needs auto-renewal.
return this.certs[0]
}
d, err := util.GetDurationToExpiry(this.certs[0], this.certs[0].NotAfter)
if err != nil {
// Current cert is already invalid. Check if renewal is available.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

log.Println("Current cert is expired, attempting to renew.")
return nil
}
if d >= time.Duration(CertRenewalInterval) {
// Cert is still valid.
return this.certs[0]
} else if d < time.Duration(CertRenewalInterval) {
// Cert is still valid, but we need to start process of requesting new cert.
log.Println("Current cert is close to expiry threshold, attempting to renew in the background.")
return this.certs[0]
}
return nil
}

func (this *CertCache) createCertChainCBOR(ocsp []byte) ([]byte, error) {
Expand Down Expand Up @@ -263,7 +309,7 @@ func (this *CertCache) maintainOCSP(stop chan struct{}) {
// has trouble getting a request, hopefully it does something
// smarter than just retry in a busy loop, hammering the OCSP server
// into further oblivion.
ticker := time.NewTicker(ocspCheckInterval)
ticker := time.NewTicker(OcspCheckInterval)

for {
select {
Expand Down Expand Up @@ -440,3 +486,77 @@ func (this *CertCache) fetchOCSP(orig []byte, ocspUpdateAfter *time.Time) []byte
}
return respBytes
}


// Checks for cert updates every certCheckInterval hours. Never terminates.
Copy link
Member

Choose a reason for hiding this comment

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

"Never terminates" is (technically) untrue. Change to "Terminates only when stop receives a message." (Also for the maintainOCSP doc string.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

func (this *CertCache) maintainCerts(stop chan struct{}) {
// Only make one request per certCheckInterval, to minimize the impact
// on servers that are buckling under load.
ticker := time.NewTicker(CertCheckInterval)

for {
select {
case <-ticker.C:
this.updateCertIfNecessary()
case <-stop:
ticker.Stop()
return
}
}
}

// Set current cert with mutex protection.
func (this *CertCache) setCerts(certs []*x509.Certificate) {
this.certsMu.Lock()
Copy link
Member

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 make certsMu an RWMutex, and RLock-protect all reads also.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

certsMU is already an RWMutex (you maybe looking at an older version of the code when you commented), added RLock-protect for all reads also. Did the same for renewedCerts.

defer this.certsMu.Unlock()
this.certs = certs
}

// Set new cert with mutex protection.
func (this *CertCache) setNewCerts(certs []*x509.Certificate) {
this.renewedCertsMu.Lock()
defer this.renewedCertsMu.Unlock()
this.renewedCerts = certs
}

// Update the cert in the cache if necessary.
func (this *CertCache) updateCertIfNecessary() {
if this.certFetcher == nil {
// Do nothing if certFetcher is not set.
return
}
d, err := util.GetDurationToExpiry(this.certs[0], this.certs[0].NotAfter)
if err != nil {
if this.renewedCerts != nil {
// If renewedCerts is set, copy that over to certs
// and set renewedCerts to nil.
// TODO(banaag): do the same cert setting dance on disk.
Copy link
Member

Choose a reason for hiding this comment

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

Is this TODO still accurate? setCerts/setNewCerts seem to write to disk.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

// Purge OCSP cache? Make shouldUpdateOCSP() return true?
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

this.setCerts(this.renewedCerts)
this.setNewCerts(nil)
return
}
// Current cert is already invalid. Try refreshing.
log.Println("Warning current cert is expired, attempting to renew: ", err)
certs, err := this.certFetcher.FetchNewCert()
if err != nil {
log.Println("Error trying to fetch new certificates from CA: ", err)
return
}
this.setCerts(certs)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Done.

return
}
if d >= time.Duration(CertRenewalInterval) {
// Cert is still valid, don't do anything.
} else if d < time.Duration(CertRenewalInterval) {
// Cert is still valid, but we need to start process of requesting new cert.
log.Println("Warning: Current cert crossed threshold for renewal, attempting to renew.")
certs, err := this.certFetcher.FetchNewCert()
if err != nil {
log.Println("Error trying to fetch new certificates from CA: ", err)
return
}
// TODO(banaag): save this cert to disk.
this.setNewCerts(certs)
Copy link
Member

Choose a reason for hiding this comment

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

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

  1. 8 days before expiry, this code runs and creates the new cert file. Nothing uses it yet.
  2. 1 day later, maintainCerts calls this function again, which will lead down here to another call to FetchNewCert. Ditto daily.
  3. 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

}
}
4 changes: 3 additions & 1 deletion packager/certcache/certcache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ type CertCacheSuite struct {

func (this *CertCacheSuite) New() (*CertCache, error) {
// TODO(twifkak): Stop the old CertCache's goroutine.
certCache := New(pkgt.B3Certs, filepath.Join(this.tempDir, "ocsp"))
// TODO(banaag): Consider adding a test with certfetcher set.
// For now, this tests certcache without worrying about certfetcher.
certCache := New(pkgt.B3Certs, nil, filepath.Join(this.tempDir, "ocsp"))
certCache.extractOCSPServer = func(*x509.Certificate) (string, error) {
return this.ocspServer.URL, nil
}
Expand Down
30 changes: 27 additions & 3 deletions packager/certloader/certloader.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,24 +24,48 @@ import (
"github.com/pkg/errors"

"github.com/ampproject/amppackager/packager/certcache"
"github.com/ampproject/amppackager/packager/certfetcher"
"github.com/ampproject/amppackager/packager/util"
)

// Creates cert cache by loading certs and keys from disk, doing validation
// and populating the cert cache with current set of certificate related information.
// If development mode is true, prints a warning for certs that can't sign HTTP exchanges.
func PopulateCertCache(config *util.Config, key crypto.PrivateKey, developmentMode bool) (*certcache.CertCache, error) {
func PopulateCertCache(config *util.Config, key crypto.PrivateKey,
developmentMode bool, autoRenewCert bool) (*certcache.CertCache, error) {

certs, err := loadCertsFromFile(config, developmentMode)
if err != nil {
return nil, err
}
domain := ""
for _, urlSet := range config.URLSet {
domain := urlSet.Sign.Domain
domain = urlSet.Sign.Domain
if err := util.CertificateMatches(certs[0], key, domain); err != nil {
return nil, errors.Wrapf(err, "checking %s", config.CertFile)
}
}
certCache := certcache.New(certs, config.OCSPCache)

certFetcher := (*certfetcher.CertFetcher)(nil)
if autoRenewCert {
emailAddress := config.ACMEConfig.Production.EmailAddress
acmeDiscoveryURL := config.ACMEConfig.Production.DiscoURL
challengePort := config.ACMEConfig.Production.ChallengePort
if developmentMode {
emailAddress = config.ACMEConfig.Development.EmailAddress
acmeDiscoveryURL = config.ACMEConfig.Development.DiscoURL
challengePort = config.ACMEConfig.Development.ChallengePort
}

// Create the cert fetcher that will auto-renew the cert.
certFetcher, err = certfetcher.NewFetcher(emailAddress, key, acmeDiscoveryURL,
[]string{domain}, challengePort, !developmentMode)
if err != nil {
return nil, errors.Wrap(err, "creating certfetcher")
}
}

certCache := certcache.New(certs, certFetcher, config.OCSPCache)

return certCache, nil
}
Expand Down
5 changes: 3 additions & 2 deletions packager/certloader/certloader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,11 @@ func TestPopulateCertCache(t *testing.T) {
}},
},
pkgt.B3Key,
true)
true,
false)
assert.Nil(t, err)
assert.NotNil(t, certCache)
assert.Equal(t, pkgt.B3Certs[0], certCache.GetLatestCert())
assert.Nil(t, err)
}

func TestLoadCertsFromFile(t *testing.T) {
Expand Down
2 changes: 2 additions & 0 deletions packager/util/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ type ACMEServerConfig struct {
DiscoURL string // ACME Directory Resource URL
AccountURL string // ACME Account URL. If non-empty, we
// will auto-renew cert via ACME.
EmailAddress string // Email address registered with ACME CA.
ChallengePort int // ACME challenge port.
}

// TODO(twifkak): Extract default values into a function separate from the one
Expand Down
Loading