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

ceremony: Distinguish between intermediate and cross-sign ceremonies #7005

Merged
merged 56 commits into from
Aug 23, 2023
Merged
Show file tree
Hide file tree
Changes from 43 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
076abd3
ceremony: Handle distinctions between intermediate and cross-sign cer…
pgporada Jul 19, 2023
8b5b434
Merge remote-tracking branch 'origin/main' into ceremony-cross-cert-r…
pgporada Jul 20, 2023
4163d40
Make a specific cross-cert profile and ceremony function
pgporada Jul 20, 2023
a7a1cfb
Add cross-cert config validation unit test
pgporada Jul 21, 2023
59c470b
Split linting certificate into separate function
pgporada Jul 24, 2023
21d737d
Don't throwaway linting certificate bytes from //linter package
pgporada Jul 24, 2023
a91c75f
Extra validations on RawIssuer and RawSubject fields
pgporada Jul 25, 2023
881389a
More verifications
pgporada Jul 25, 2023
3fe4e96
Perform checks on root and intermediate ceremony too
pgporada Jul 25, 2023
21ed896
Wire up GoodKey checking
pgporada Jul 26, 2023
11638af
Fix self-signed root linting and move utility functions into cert.go
pgporada Jul 26, 2023
46fe904
Fix context lint warning by using an empty non-nil context
pgporada Jul 26, 2023
9c57814
intermediate and crossCert ceremony profiles should check the same fi…
pgporada Jul 26, 2023
09aa2f2
Add cross-certificate ceremonies integration tests
pgporada Jul 26, 2023
b50b22d
Use correct InputCertPath for cross sign ceremonies and output more i…
pgporada Jul 27, 2023
ab89802
Skip lints not relevant to cross-signed subordinate CAs
pgporada Jul 27, 2023
6786449
Remove arg from crossCert.validate
pgporada Jul 27, 2023
177461b
Add cross-signed intermediates to CA configs
pgporada Jul 27, 2023
454a4c0
Use term subordinate CA instead of intermediate
pgporada Jul 27, 2023
618bc0e
Add test cases for cross signed certs and update expected error messages
pgporada Jul 27, 2023
63a4cb7
Increase startup timeout and add additional log line about it
pgporada Jul 27, 2023
2a71a8d
Add function to load public key and do more testing
pgporada Jul 28, 2023
8a11f16
Fix cross-certificate generation for integration tests
pgporada Jul 28, 2023
6697bd5
Add cross-signed intermediates to the issuerCerts list
pgporada Jul 28, 2023
95cc539
Fix lints
pgporada Jul 28, 2023
4f404b7
Only need to serve the issuer chains from the WFE
pgporada Jul 31, 2023
9608749
Only create ECDSA cross-signed by an RSA root to match the 2023 ceremony
pgporada Jul 31, 2023
043a39c
Fix issuer ordering problem that doesnt exist in staging/prod
pgporada Aug 2, 2023
f9730d5
Remove redundant text
pgporada Aug 2, 2023
fdde489
Add helper functions to edit the running ECDSA allow list and to down…
pgporada Aug 2, 2023
fe0aed7
Integration test for cross-signed intermediates
pgporada Aug 2, 2023
3d67b19
Remove deubg line
pgporada Aug 2, 2023
07475fa
Check that a client is served the expected certificate chains
pgporada Aug 2, 2023
e16bb18
Remove commented out code
pgporada Aug 2, 2023
3e49add
Use correct cross-sign cert filepath
pgporada Aug 2, 2023
04af1f9
Add extra information to log
pgporada Aug 2, 2023
b06e5e1
Remove debug line
pgporada Aug 2, 2023
8c82c2c
Change boolean name
pgporada Aug 2, 2023
659958d
Update PKI.md
pgporada Aug 2, 2023
daee81a
Merge branch 'main' into ceremony-cross-cert-rework
pgporada Aug 3, 2023
a0f8423
Musical functions
pgporada Aug 7, 2023
1211161
Address comments
pgporada Aug 7, 2023
e99b427
Been awake too many hours and removed too much code
pgporada Aug 7, 2023
64f8b94
Address stylistic comments
pgporada Aug 11, 2023
ca1f430
Still working on more BR and CPS verifications
pgporada Aug 11, 2023
567e1f2
Still addressing comments
pgporada Aug 14, 2023
9978b82
Fix ineffectual assignment
pgporada Aug 14, 2023
c20dcc5
Allow genCert to display command output
pgporada Aug 16, 2023
fc6fc4c
Copy EKUs from to-be-cross-signed cert over to the lint cert
pgporada Aug 16, 2023
9d38688
Test that EKUs match between lintcert and to-be-cross-signed
pgporada Aug 16, 2023
4e7a822
Update cmd/ceremony/cert.go
pgporada Aug 17, 2023
979ee46
Address comments
pgporada Aug 17, 2023
72b7d76
Prevent issuance if linting hasn't been done
pgporada Aug 17, 2023
950333e
Fix error message
pgporada Aug 17, 2023
308a6d0
I should learn how to use structs and types
pgporada Aug 17, 2023
676f793
I'm of the type that fails at types
pgporada Aug 17, 2023
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
37 changes: 32 additions & 5 deletions cmd/ceremony/cert.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,25 @@ import (
"errors"
"fmt"
"io"
"log"
"math/big"
"strconv"
"strings"
"time"

"github.com/letsencrypt/boulder/goodkey"
)

var kp goodkey.KeyPolicy

func init() {
var err error
kp, err = goodkey.NewKeyPolicy(&goodkey.Config{FermatRounds: 100}, nil)
if err != nil {
log.Fatal("Could not create goodkey.KeyPolicy")
}
}

type policyInfoConfig struct {
OID string
// Deprecated: we do not include the id-qt-cps policy qualifier in our
Expand Down Expand Up @@ -147,15 +160,18 @@ func (profile *certProfile) verifyProfile(ct certType) error {
}
}

if ct == intermediateCert {
if ct == intermediateCert || ct == crossCert {
if profile.CRLURL == "" {
return errors.New("crl-url is required for intermediates")
return errors.New("crl-url is required for subordinate CAs")
}
if profile.IssuerURL == "" {
return errors.New("issuer-url is required for intermediates")
return errors.New("issuer-url is required for subordinate CAs")
}

// BR 7.1.2.10.5 CA Certificate Certificate Policies
// OID 2.23.140.1.2.1 is an anyPolicy
if len(profile.Policies) != 1 || profile.Policies[0].OID != "2.23.140.1.2.1" {
return errors.New("policy should be exactly BRs domain-validated for intermediates")
return errors.New("policy should be exactly BRs domain-validated for subordinate CAs")
}
}

Expand Down Expand Up @@ -282,7 +298,18 @@ func makeTemplate(randReader io.Reader, profile *certProfile, pubKey []byte, ct
}

switch ct {
// rootCert and crossCert do not get EKU or MaxPathZero
// rootCert and crossCert do not get EKU or MaxPathZero. This tool currently
// only produces "unrestricted" cross-signs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this just made me realize a complicated thing: I think that the "crossCert" ceremony needs to match the existing cert's EKU and MaxPathLen.

If we're cross-signing a root (e.g. X1 signing X2), then there should be no MaxPathLen and no EKUs, since it's an Unrestricted cross-sign. This is exactly the case that the "unrestricted" carve-out in 7.1.2.2.3 is designed for. The self-signed root has no such restrictions, so the cross-sign doesn't need them either.

But if we're cross-signing an intermediate (e.g. X1 signing E5), there need to be EKUs. Fundamentally, E5-by-X1 and E5-by-X2 are the same class of object: they're both TLS Subordinate CA Certificates, and they're both Cross-Certified Subordinate CA Certificates. Neither one is more "real" or "original" than the other. Therefore, IMO, they both need to comply with 7.1.2.6 TLS Subordinate CA Certificate Profile, which says that EKUs are a MUST.

Honestly I think this is a bug in the new version of the BRs. It would appear that they allow cross-signed intermediates to have no EKUs. I don't think we should follow this interpretation, and should instead restrict ourselves to the more conservative interpretation where cross-signed intermediates need to retain the same restrictions as the "original" intermediate.

// https://github.com/cabforum/servercert/blob/a0360b61e73476959220dc328e3b68d0224fa0b3/docs/BR.md#71223-cross-certified-subordinate-ca-extensions
//
// 7.1.2.1.2 Root CA Extensions
// Extension Presence Critical Description
// extKeyUsage MUST NOT N -
//
// 7.1.2.2.3 Cross-Certified Subordinate CA Extensions
// Extension Presence Critical Description
// extKeyUsage SHOULD N See Section 7.1.2.2.4
//
case ocspCert:
cert.ExtKeyUsage = []x509.ExtKeyUsage{x509.ExtKeyUsageOCSPSigning}
// ASN.1 NULL is 0x05, 0x00
Expand Down
112 changes: 73 additions & 39 deletions cmd/ceremony/cert_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"encoding/hex"
"errors"
"fmt"
"io/fs"
"testing"

"github.com/letsencrypt/boulder/pkcs11helpers"
Expand Down Expand Up @@ -59,7 +60,7 @@ func TestMakeSubject(t *testing.T) {
test.AssertDeepEquals(t, profile.Subject(), expectedSubject)
}

func TestMakeTemplate(t *testing.T) {
func TestMakeTemplateRoot(t *testing.T) {
s, ctx := pkcs11helpers.NewSessionWithMock()
profile := &certProfile{}
randReader := newRandReader(s)
Expand Down Expand Up @@ -146,8 +147,8 @@ func TestMakeTemplateCrossCertificate(t *testing.T) {
OCSPURL: "ocsp",
CRLURL: "crl",
IssuerURL: "issuer",
NotAfter: "2018-05-18 11:31:00",
NotBefore: "2018-05-18 11:31:00",
NotAfter: "2020-10-10 11:31:00",
NotBefore: "2020-10-10 11:31:00",
}

ctx.GenerateRandomFunc = realRand
Expand Down Expand Up @@ -228,27 +229,27 @@ func TestMakeTemplateCRL(t *testing.T) {
func TestVerifyProfile(t *testing.T) {
for _, tc := range []struct {
profile certProfile
certType certType
certType []certType
expectedErr string
}{
{
profile: certProfile{},
certType: intermediateCert,
certType: []certType{intermediateCert, crossCert},
expectedErr: "not-before is required",
},
{
profile: certProfile{
NotBefore: "a",
},
certType: intermediateCert,
certType: []certType{intermediateCert, crossCert},
expectedErr: "not-after is required",
},
{
profile: certProfile{
NotBefore: "a",
NotAfter: "b",
},
certType: intermediateCert,
certType: []certType{intermediateCert, crossCert},
expectedErr: "signature-algorithm is required",
},
{
Expand All @@ -257,7 +258,7 @@ func TestVerifyProfile(t *testing.T) {
NotAfter: "b",
SignatureAlgorithm: "c",
},
certType: intermediateCert,
certType: []certType{intermediateCert, crossCert},
expectedErr: "common-name is required",
},
{
Expand All @@ -267,7 +268,7 @@ func TestVerifyProfile(t *testing.T) {
SignatureAlgorithm: "c",
CommonName: "d",
},
certType: intermediateCert,
certType: []certType{intermediateCert, crossCert},
expectedErr: "organization is required",
},
{
Expand All @@ -278,7 +279,7 @@ func TestVerifyProfile(t *testing.T) {
CommonName: "d",
Organization: "e",
},
certType: intermediateCert,
certType: []certType{intermediateCert, crossCert},
expectedErr: "country is required",
},
{
Expand All @@ -291,8 +292,22 @@ func TestVerifyProfile(t *testing.T) {
Country: "f",
OCSPURL: "g",
},
certType: intermediateCert,
expectedErr: "crl-url is required for intermediates",
certType: []certType{intermediateCert, crossCert},
expectedErr: "crl-url is required for subordinate CAs",
},
{
profile: certProfile{
NotBefore: "a",
NotAfter: "b",
SignatureAlgorithm: "c",
CommonName: "d",
Organization: "e",
Country: "f",
OCSPURL: "g",
CRLURL: "h",
},
certType: []certType{intermediateCert, crossCert},
expectedErr: "issuer-url is required for subordinate CAs",
},
{
profile: certProfile{
Expand All @@ -304,9 +319,10 @@ func TestVerifyProfile(t *testing.T) {
Country: "f",
OCSPURL: "g",
CRLURL: "h",
IssuerURL: "i",
},
certType: intermediateCert,
expectedErr: "issuer-url is required for intermediates",
certType: []certType{intermediateCert, crossCert},
expectedErr: "policy should be exactly BRs domain-validated for subordinate CAs",
},
{
profile: certProfile{
Expand All @@ -319,9 +335,10 @@ func TestVerifyProfile(t *testing.T) {
OCSPURL: "g",
CRLURL: "h",
IssuerURL: "i",
Policies: []policyInfoConfig{{OID: "1.2.3"}, {OID: "4.5.6"}},
},
certType: intermediateCert,
expectedErr: "policy should be exactly BRs domain-validated for intermediates",
certType: []certType{intermediateCert, crossCert},
expectedErr: "policy should be exactly BRs domain-validated for subordinate CAs",
},
{
profile: certProfile{
Expand All @@ -332,7 +349,7 @@ func TestVerifyProfile(t *testing.T) {
Organization: "e",
Country: "f",
},
certType: rootCert,
certType: []certType{rootCert},
},
{
profile: certProfile{
Expand All @@ -345,7 +362,7 @@ func TestVerifyProfile(t *testing.T) {
IssuerURL: "g",
KeyUsages: []string{"j"},
},
certType: ocspCert,
certType: []certType{ocspCert},
expectedErr: "key-usages cannot be set for a delegated signer",
},
{
Expand All @@ -359,7 +376,7 @@ func TestVerifyProfile(t *testing.T) {
IssuerURL: "g",
CRLURL: "i",
},
certType: ocspCert,
certType: []certType{ocspCert},
expectedErr: "crl-url cannot be set for a delegated signer",
},
{
Expand All @@ -373,7 +390,7 @@ func TestVerifyProfile(t *testing.T) {
IssuerURL: "g",
OCSPURL: "h",
},
certType: ocspCert,
certType: []certType{ocspCert},
expectedErr: "ocsp-url cannot be set for a delegated signer",
},
{
Expand All @@ -386,7 +403,7 @@ func TestVerifyProfile(t *testing.T) {
Country: "f",
IssuerURL: "g",
},
certType: ocspCert,
certType: []certType{ocspCert},
},
{
profile: certProfile{
Expand All @@ -399,7 +416,7 @@ func TestVerifyProfile(t *testing.T) {
IssuerURL: "g",
KeyUsages: []string{"j"},
},
certType: crlCert,
certType: []certType{crlCert},
expectedErr: "key-usages cannot be set for a delegated signer",
},
{
Expand All @@ -413,7 +430,7 @@ func TestVerifyProfile(t *testing.T) {
IssuerURL: "g",
CRLURL: "i",
},
certType: crlCert,
certType: []certType{crlCert},
expectedErr: "crl-url cannot be set for a delegated signer",
},
{
Expand All @@ -427,7 +444,7 @@ func TestVerifyProfile(t *testing.T) {
IssuerURL: "g",
OCSPURL: "h",
},
certType: crlCert,
certType: []certType{crlCert},
expectedErr: "ocsp-url cannot be set for a delegated signer",
},
{
Expand All @@ -440,72 +457,74 @@ func TestVerifyProfile(t *testing.T) {
Country: "f",
IssuerURL: "g",
},
certType: crlCert,
certType: []certType{crlCert},
},
{
profile: certProfile{
NotBefore: "a",
},
certType: requestCert,
certType: []certType{requestCert},
expectedErr: "not-before cannot be set for a CSR",
},
{
profile: certProfile{
NotAfter: "a",
},
certType: requestCert,
certType: []certType{requestCert},
expectedErr: "not-after cannot be set for a CSR",
},
{
profile: certProfile{
SignatureAlgorithm: "a",
},
certType: requestCert,
certType: []certType{requestCert},
expectedErr: "signature-algorithm cannot be set for a CSR",
},
{
profile: certProfile{
OCSPURL: "a",
},
certType: requestCert,
certType: []certType{requestCert},
expectedErr: "ocsp-url cannot be set for a CSR",
},
{
profile: certProfile{
CRLURL: "a",
},
certType: requestCert,
certType: []certType{requestCert},
expectedErr: "crl-url cannot be set for a CSR",
},
{
profile: certProfile{
IssuerURL: "a",
},
certType: requestCert,
certType: []certType{requestCert},
expectedErr: "issuer-url cannot be set for a CSR",
},
{
profile: certProfile{
Policies: []policyInfoConfig{{OID: "1.2.3"}},
},
certType: requestCert,
certType: []certType{requestCert},
expectedErr: "policies cannot be set for a CSR",
},
{
profile: certProfile{
KeyUsages: []string{"a"},
},
certType: requestCert,
certType: []certType{requestCert},
expectedErr: "key-usages cannot be set for a CSR",
},
} {
err := tc.profile.verifyProfile(tc.certType)
if err != nil {
if tc.expectedErr != err.Error() {
t.Fatalf("Expected %q, got %q", tc.expectedErr, err.Error())
for _, ct := range tc.certType {
err := tc.profile.verifyProfile(ct)
if err != nil {
if tc.expectedErr != err.Error() {
t.Fatalf("Expected %q, got %q", tc.expectedErr, err.Error())
}
} else if tc.expectedErr != "" {
t.Fatalf("verifyProfile didn't fail, expected %q", tc.expectedErr)
}
} else if tc.expectedErr != "" {
t.Fatalf("verifyProfile didn't fail, expected %q", tc.expectedErr)
}
}
}
Expand All @@ -531,3 +550,18 @@ func TestGenerateCSR(t *testing.T) {
test.AssertEquals(t, csr.Subject.String(), fmt.Sprintf("CN=%s,O=%s,C=%s",
profile.CommonName, profile.Organization, profile.Country))
}

func TestLoadCert(t *testing.T) {
_, err := loadCert("../../test/hierarchy/int-e1.cert.pem")
test.AssertNotError(t, err, "should not have errored")

_, err = loadCert("/path/that/will/not/ever/exist/ever")
test.AssertError(t, err, "should have failed opening certificate at non-existent path")
test.AssertErrorIs(t, err, fs.ErrNotExist)

_, err = loadCert("../../test/hierarchy/int-e1.key.pem")
test.AssertError(t, err, "should have failed when trying to parse a private key")

_, err = loadCert("../../test/test-root.pubkey.pem")
test.AssertError(t, err, "should have failed when trying to parse a public key")
}
Loading