Support short-term cert expiry

ampproject / amppackager

Tool to improve AMP URLs via Signed Exchanges

https://amp.dev/documentation/guides-and-tutorials/optimize-and-measure/signed-exchange/

Apache License 2.0

140 stars 55 forks source link

Support short-term cert expiry #93

Open twifkak opened 6 years ago

twifkak commented 6 years ago

Currently, amppkg only loads the cert file at startup. If it expires while the packager is running, the packager continues to sign with it and serve it. Instead, it should attempt to reload automatically starting a few days before expiry, and continuing at some regular interval until no longer imminently expiring. If the cert is expired, it should stop signing exchanges, and log a warning.

In addition, it should serve the cert-url with an http expiry no longer than the cert expiry (as a follow-up to #85).

twifkak commented 5 years ago

This will be much more important as of WICG/webpackage#383.

twifkak commented 5 years ago

Restarting the server every 90 days is an option. Perhaps not a particularly pleasant one, but perhaps not so bad in this world of coordinated containers.

twifkak commented 5 years ago

Cert renewals will have a different cert-sha256 and hence a different cert-url and generate different signatures. If so, we should decide whether to continue serving the old cert at the old URL (up until expiry). We may get a timeline like:

AMP cache requests SXG, amppkg provides.
amppkg has new cert.
AMP cache requests cert-url, amppkg 404s.

There may be an arbitrary amount of time between (1) and (3), though likely usually small. Though the AMP cache is free to respond to the 404 by doing a GOTO 1.

I'm leaning to 'no' for simplicity, since AMP Caches have some workarounds available.

Alternatively, maybe we should reconsider data: cert-urls.

twifkak commented 5 years ago

Update:

We should support hosting old and new cert at same time. (Does this mean we'll need to change the toml to allow a list of cert paths?)
Optional: Allow adding new cert without restarting. (Either inotify or SIGUSR1.)
Required, but later is ok: Integrate an ACME client library.

twifkak commented 4 years ago

Sorry for the noise; changing things back since I see from the comments there is other stuff than just the tests. Split the tests off into #433 since that seems fixit-sized.