Skip to content

Commit

Permalink
Remove cert validity check for grace period and testdata/b1 (ampproje…
Browse files Browse the repository at this point in the history
…ct#331)

All SXG certificates that have a Validity Period longer than 90 days
are rejected after 2019-08-01. b1 test data is no longer needed.
  • Loading branch information
shigeki authored and twifkak committed Aug 2, 2019
1 parent 31e946d commit bb8ba2f
Show file tree
Hide file tree
Showing 21 changed files with 26 additions and 269 deletions.
2 changes: 1 addition & 1 deletion cmd/amppkg/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func main() {
if certs == nil || len(certs) == 0 {
die(fmt.Sprintf("no cert found in %s", config.CertFile))
}
if err := util.CanSignHttpExchanges(certs[0], time.Now()); err != nil {
if err := util.CanSignHttpExchanges(certs[0]); err != nil {
if *flagDevelopment || *flagInvalidCert {
log.Println("WARNING:", err)
} else {
Expand Down
2 changes: 1 addition & 1 deletion docker/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ FROM golang:1.11
RUN go get -v github.com/ampproject/amppackager/cmd/amppkg

# Seed the ocsp cache
WORKDIR /go/src/github.com/ampproject/amppackager/testdata/b1/
WORKDIR /go/src/github.com/ampproject/amppackager/testdata/b3/
RUN ./seedcache.sh

WORKDIR /go/src/app
Expand Down
4 changes: 2 additions & 2 deletions docker/amppkg.example.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
# See https://github.com/ampproject/amppackager/blob/master/amppkg.example.toml
# for more information and details on other possible config options.

CertFile = '/go/src/github.com/ampproject/amppackager/testdata/b1/fullchain.cert'
KeyFile = '/go/src/github.com/ampproject/amppackager/testdata/b1/server.privkey'
CertFile = '/go/src/github.com/ampproject/amppackager/testdata/b3/fullchain.cert'
KeyFile = '/go/src/github.com/ampproject/amppackager/testdata/b3/server.privkey'
OCSPCache = '/tmp/amppkg-ocsp'

[[URLSet]]
Expand Down
8 changes: 4 additions & 4 deletions packager/certcache/certcache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,21 @@ import (
)

var caCert = func() *x509.Certificate {
certPem, _ := ioutil.ReadFile("../../testdata/b1/ca.cert")
certPem, _ := ioutil.ReadFile("../../testdata/b3/ca.cert")
certs, _ := signedexchange.ParseCertificates(certPem)
return certs[0]
}()

var caKey = func() *rsa.PrivateKey {
keyPem, _ := ioutil.ReadFile("../../testdata/b1/ca.privkey")
keyPem, _ := ioutil.ReadFile("../../testdata/b3/ca.privkey")
key, _ := util.ParsePrivateKey(keyPem)
return key.(*rsa.PrivateKey)
}()

func FakeOCSPResponse(thisUpdate time.Time) ([]byte, error) {
template := ocsp.Response{
Status: ocsp.Good,
SerialNumber: pkgt.Certs[0].SerialNumber,
SerialNumber: pkgt.B3Certs[0].SerialNumber,
ThisUpdate: thisUpdate,
NextUpdate: thisUpdate.Add(7 * 24 * time.Hour),
RevokedAt: thisUpdate.AddDate( /*years=*/ 0 /*months=*/, 0 /*days=*/, 365),
Expand All @@ -74,7 +74,7 @@ type CertCacheSuite struct {

func (this *CertCacheSuite) New() (*CertCache, error) {
// TODO(twifkak): Stop the old CertCache's goroutine.
certCache := New(pkgt.Certs, filepath.Join(this.tempDir, "ocsp"))
certCache := New(pkgt.B3Certs, filepath.Join(this.tempDir, "ocsp"))
certCache.extractOCSPServer = func(*x509.Certificate) (string, error) {
return this.ocspServer.URL, nil
}
Expand Down
7 changes: 5 additions & 2 deletions packager/signer/signer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package signer

import (
"bytes"
"encoding/base64"
"encoding/binary"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -187,7 +188,8 @@ func (this *SignerSuite) TestSimple() {
this.Assert().Contains(exchange.SignatureHeaderValue, "validity-url=\""+this.httpSignURL()+"/amppkg/validity\"")
this.Assert().Contains(exchange.SignatureHeaderValue, "integrity=\"digest/mi-sha256-03\"")
this.Assert().Contains(exchange.SignatureHeaderValue, "cert-url=\""+this.httpSignURL()+"/amppkg/cert/"+pkgt.CertName+"\"")
this.Assert().Contains(exchange.SignatureHeaderValue, "cert-sha256=*"+pkgt.CertName+"=*")
certHash, _ := base64.RawURLEncoding.DecodeString(pkgt.CertName)
this.Assert().Contains(exchange.SignatureHeaderValue, "cert-sha256=*"+base64.StdEncoding.EncodeToString(certHash[:])+"*")
// TODO(twifkak): Control date, and test for expires and sig.
// The response header values are untested here, as that is covered by signedexchange tests.

Expand Down Expand Up @@ -239,7 +241,8 @@ func (this *SignerSuite) TestFetchSignWithForwardedRequestHeaders() {
this.Assert().Contains(exchange.SignatureHeaderValue, "validity-url=\""+this.httpSignURL_CertSubjectCN()+"/amppkg/validity\"")
this.Assert().Contains(exchange.SignatureHeaderValue, "integrity=\"digest/mi-sha256-03\"")
this.Assert().Contains(exchange.SignatureHeaderValue, "cert-url=\""+this.httpSignURL_CertSubjectCN()+"/amppkg/cert/"+pkgt.CertName+"\"")
this.Assert().Contains(exchange.SignatureHeaderValue, "cert-sha256=*"+pkgt.CertName+"=*")
certHash, _ := base64.RawURLEncoding.DecodeString(pkgt.CertName)
this.Assert().Contains(exchange.SignatureHeaderValue, "cert-sha256=*"+base64.StdEncoding.EncodeToString(certHash[:])+"*")
var payloadPrefix bytes.Buffer
binary.Write(&payloadPrefix, binary.BigEndian, uint64(miRecordSize))
this.Assert().Equal(append(payloadPrefix.Bytes(), transformedBody...), exchange.Payload)
Expand Down
4 changes: 2 additions & 2 deletions packager/testing/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,14 @@ import (

// A cert (with its issuer chain) for testing.
var Certs = func() []*x509.Certificate {
certPem, _ := ioutil.ReadFile("../../testdata/b1/fullchain.cert")
certPem, _ := ioutil.ReadFile("../../testdata/b3/fullchain.cert")
certs, _ := signedexchange.ParseCertificates(certPem)
return certs
}()

// Its corresponding private key.
var Key = func() crypto.PrivateKey {
keyPem, _ := ioutil.ReadFile("../../testdata/b1/server.privkey")
keyPem, _ := ioutil.ReadFile("../../testdata/b3/server.privkey")
// This call to ParsePrivateKey() is needed by util_test.go.
key, _ := util.ParsePrivateKey(keyPem)
return key
Expand Down
17 changes: 3 additions & 14 deletions packager/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,13 @@ import (
"encoding/asn1"
"encoding/base64"
"encoding/pem"
"time"

"github.com/WICG/webpackage/go/signedexchange"
"github.com/pkg/errors"
)

const CertURLPrefix = "/amppkg/cert"

// https://wicg.github.io/webpackage/draft-yasskin-http-origin-signed-responses.html#cross-origin-cert-req
// Clients MUST reject certificates with this extension that were issued after 2019-05-01 and have a Validity Period longer than 90 days.
// After 2019-08-01, clients MUST reject all certificates with this extension that have a Validity Period longer than 90 days.
var start90DayGracePeriod = time.Date(2019, time.May, 1, 0, 0, 0, 0, time.UTC)
var end90DayGracePeriod = time.Date(2019, time.August, 1, 0, 0, 0, 0, time.UTC)

// CertName returns the basename for the given cert, as served by this
// packager's cert cache. Should be stable and unique (e.g.
// content-addressing). Clients should url.PathEscape this, just in case its
Expand Down Expand Up @@ -86,16 +79,12 @@ func hasCanSignHttpExchangesExtension(cert *x509.Certificate) bool {
// CanSignHttpExchanges extension, and a valid lifetime per the SXG spec;
// otherwise it returns an error. These are not the only requirements for SXGs;
// it also needs to use the right public key type, which is not checked here.
func CanSignHttpExchanges(cert *x509.Certificate, now time.Time) error {
func CanSignHttpExchanges(cert *x509.Certificate) error {
if !hasCanSignHttpExchangesExtension(cert) {
return errors.New("Certificate is missing CanSignHttpExchanges extension")
}

// TODO: remove issue date and current time check after 2019-08-01
if cert.NotBefore.After(start90DayGracePeriod) || now.After(end90DayGracePeriod) {
if cert.NotBefore.AddDate(0,0,90).Before(cert.NotAfter) {
return errors.New("Certificate MUST have a Validity Period no greater than 90 days")
}
if cert.NotBefore.AddDate(0,0,90).Before(cert.NotAfter) {
return errors.New("Certificate MUST have a Validity Period no greater than 90 days")
}
return nil
}
Expand Down
36 changes: 8 additions & 28 deletions packager/util/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"crypto/ecdsa"
"crypto/elliptic"
"testing"
"time"

pkgt "github.com/ampproject/amppackager/packager/testing"
"github.com/ampproject/amppackager/packager/util"
Expand All @@ -20,23 +19,20 @@ func errorFrom(err error) string {
}

func TestCertName(t *testing.T) {
assert.Equal(t, "PJ1IwfP1igOlJd2oTUVs2mj4dWIZcOWHMk5jfJYS2Qc", util.CertName(pkgt.Certs[0]))
assert.Equal(t, "Qk83Jo8qB8cEtxfb_7eit0SWVt0pdj5e7oDCqEgf77o", util.CertName(pkgt.B3Certs[0]))
}

// ParsePrivateKey() is tested indirectly via the definition of pkgt.Key.
// ParsePrivateKey() is tested indirectly via the definition of pkgt.B3Key.
func TestParsePrivateKey(t *testing.T) {
require.IsType(t, &ecdsa.PrivateKey{}, pkgt.Key)
assert.Equal(t, elliptic.P256(), pkgt.Key.(*ecdsa.PrivateKey).PublicKey.Curve)
require.IsType(t, &ecdsa.PrivateKey{}, pkgt.B3Key)
assert.Equal(t, elliptic.P256(), pkgt.B3Key.(*ecdsa.PrivateKey).PublicKey.Curve)
}

func TestCanSignHttpExchangesExtension(t *testing.T) {
// Before grace period, to allow the >90-day lifetime.
now := time.Date(2019, time.July, 31, 0, 0, 0, 0, time.UTC)

// Leaf node has the extension.
assert.Nil(t, util.CanSignHttpExchanges(pkgt.Certs[0], now))
assert.Nil(t, util.CanSignHttpExchanges(pkgt.B3Certs[0]))
// CA node does not.
assert.EqualError(t, util.CanSignHttpExchanges(pkgt.Certs[1], now), "Certificate is missing CanSignHttpExchanges extension")
assert.EqualError(t, util.CanSignHttpExchanges(pkgt.B3Certs[1]), "Certificate is missing CanSignHttpExchanges extension")
}

func TestParseCertificate(t *testing.T) {
Expand All @@ -62,23 +58,7 @@ func TestParseCertificateNotMatchDomain(t *testing.T) {
pkgt.B3Key2, "amppackageexample.com")), "x509: certificate is valid for amppackageexample2.com, www.amppackageexample2.com, not amppackageexample.com")
}

func TestParse90DaysCertificateAfterGracePeriod(t *testing.T) {
now := time.Date(2019, time.August, 1, 0, 0, 0, 1, time.UTC)
assert.Nil(t, util.CanSignHttpExchanges(pkgt.B3Certs[0], now))
}

func TestParse91DaysCertificate(t *testing.T) {
assert.Contains(t, errorFrom(util.CanSignHttpExchanges(pkgt.B3Certs91Days[0],
time.Now())), "Certificate MUST have a Validity Period no greater than 90 days")
}

func TestParseCertificateIssuedBeforeMay1InGarcePeriod(t *testing.T) {
now := time.Date(2019, time.July, 31, 0, 0, 0, 0, time.UTC)
assert.Nil(t, util.CanSignHttpExchanges(pkgt.Certs[0], now))
}

func TestParseCertificateIssuedBeforeMay1AfterGracePeriod(t *testing.T) {
now := time.Date(2019, time.August, 1, 0, 0, 0, 1, time.UTC)
assert.Contains(t, errorFrom(util.CanSignHttpExchanges(pkgt.Certs[0],
now)), "Certificate MUST have a Validity Period no greater than 90 days")
assert.Contains(t, errorFrom(util.CanSignHttpExchanges(pkgt.B3Certs91Days[0])),
"Certificate MUST have a Validity Period no greater than 90 days")
}
55 changes: 0 additions & 55 deletions testdata/b1/README.md

This file was deleted.

21 changes: 0 additions & 21 deletions testdata/b1/blah

This file was deleted.

21 changes: 0 additions & 21 deletions testdata/b1/ca.cert

This file was deleted.

20 changes: 0 additions & 20 deletions testdata/b1/ca.ocsp.cert

This file was deleted.

27 changes: 0 additions & 27 deletions testdata/b1/ca.privkey

This file was deleted.

1 change: 0 additions & 1 deletion testdata/b1/ca.srl

This file was deleted.

35 changes: 0 additions & 35 deletions testdata/b1/fullchain.cert

This file was deleted.

Empty file removed testdata/b1/index.txt
Empty file.
Empty file removed testdata/b1/index.txt.attr
Empty file.
Loading

0 comments on commit bb8ba2f

Please sign in to comment.