Iterate on SSL_CREDENTIAL_set_must_match_issuer a bit
First, simplify the API a bit:
- Just take a boolean param rather than having both set and clear
functions.
- Unless we need it, no need to bother with a getter. We generally
assume that the caller knows what they configured.
Next, expand on the docs and move it with other credential APIs, not
SSL_PRIVATE_KEY_METHOD.
Finally, fix a bug and test this in runner: the TLS 1.2 handshake forgot
to check the issuer, which meant that it assumed all credentials were
viable. Fix this and add tests to cover it all. In doing so, this pulls
in the MustMatchIssuer runner plumbing out of
https://boringssl-review.googlesource.com/c/boringssl/+/73087 to land a
little sooner.
Also test that issuer matching works with delegated credentials. May as
well.
Change-Id: I22aee148dd81fb9804d80b4243b68a5ecdead480
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/76708
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index eed2766..f06f2f2 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -774,8 +774,6 @@
// - Whether the peer supports the signature algorithms in the certificate chain
// - Whether the a server certificate is compatible with the server_name
// extension (SNI)
-// - Whether the peer supports the certificate authority that issued the
-// certificate
//
// Credentials may be configured before the handshake or dynamically in the
// early callback (see |SSL_CTX_set_select_certificate_cb|) and certificate
@@ -839,6 +837,25 @@
OPENSSL_EXPORT int SSL_CREDENTIAL_set1_signed_cert_timestamp_list(
SSL_CREDENTIAL *cred, CRYPTO_BUFFER *sct_list);
+// SSL_CREDENTIAL_set_must_match_issuer configures whether |cred| should check
+// if the peer supports the certificate chain's issuer.
+//
+// If |match| is non-zero, |cred| will only be applicable when the certificate
+// chain is issued by some CA requested by the peer, e.g. in the
+// certificate_authorities extension. This can be used for certificate chains
+// that may not be usable by all peers, e.g. chains with fewer cross-signs or
+// issued from a newer CA.
+//
+// If |match| is zero (default), |cred| will not be conditioned on the peer's
+// requested CAs. This can be used for certificate chains that are assumed to be
+// usable by most peers.
+//
+// The credential list is tried in order, so more specific credentials that
+// enable issuer matching should generally be ordered before less specific
+// credentials that do not.
+OPENSSL_EXPORT void SSL_CREDENTIAL_set_must_match_issuer(SSL_CREDENTIAL *cred,
+ int match);
+
// SSL_CTX_add1_credential appends |cred| to |ctx|'s credential list. It returns
// one on success and zero on error. The credential list is maintained in order
// of decreasing preference, so earlier calls are preferred over later calls.
@@ -1400,24 +1417,6 @@
OPENSSL_EXPORT int SSL_CREDENTIAL_set_private_key_method(
SSL_CREDENTIAL *cred, const SSL_PRIVATE_KEY_METHOD *key_method);
-// SSL_CREDENTIAL_set_must_match_issuer sets the flag that this credential
-// should be considered only when it matches a peer request for a particular
-// issuer via a negotiation mechanism (such as the certificate_authorities
-// extension).
-OPENSSL_EXPORT void SSL_CREDENTIAL_set_must_match_issuer(SSL_CREDENTIAL *cred);
-
-// SSL_CREDENTIAL_clear_must_match_issuer clears the flag requiring issuer
-// matching, indicating this credential should be considered regardless of peer
-// issuer matching requests. (This is the default).
-OPENSSL_EXPORT void SSL_CREDENTIAL_clear_must_match_issuer(
- SSL_CREDENTIAL *cred);
-
-// SSL_CREDENTIAL_must_match_issuer returns the value of the flag indicating
-// that this credential should be considered only when it matches a peer request
-// for a particular issuer via a negotiation mechanism (such as the
-// certificate_authorities extension).
-OPENSSL_EXPORT int SSL_CREDENTIAL_must_match_issuer(const SSL_CREDENTIAL *cred);
-
// SSL_can_release_private_key returns one if |ssl| will no longer call into the
// private key and zero otherwise. If the function returns one, the caller can
// release state associated with the private key.
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 7319df7..4ce8d18 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -1270,7 +1270,8 @@
// check the ECDSA curve. Prior to TLS 1.3, there is no way to determine which
// ECDSA curves are supported by the peer, so we must assume all curves are
// supported.
- return tls1_choose_signature_algorithm(hs, cred, out_sigalg);
+ return tls1_choose_signature_algorithm(hs, cred, out_sigalg) &&
+ ssl_credential_matches_requested_issuers(hs, cred);
}
static enum ssl_hs_wait_t do_send_client_certificate(SSL_HANDSHAKE *hs) {
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index 9594210..ff275e7 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -273,9 +273,12 @@
TLS12ServerParams params;
params.cipher = choose_cipher(hs, client_pref, mask_k, mask_a);
- if (params.cipher == nullptr) {
+ if (params.cipher == nullptr ||
+ (cred != nullptr &&
+ !ssl_credential_matches_requested_issuers(hs, cred))) {
return TLS12ServerParams();
}
+ // Only report the selected signature algorithm if it will be used.
if (ssl_cipher_requires_server_key_exchange(params.cipher) &&
ssl_cipher_uses_certificate_auth(params.cipher)) {
params.signature_algorithm = sigalg;
diff --git a/ssl/ssl_credential.cc b/ssl/ssl_credential.cc
index fcd8ca5..94eaeff 100644
--- a/ssl/ssl_credential.cc
+++ b/ssl/ssl_credential.cc
@@ -596,14 +596,6 @@
return CRYPTO_get_ex_data(&cred->ex_data, idx);
}
-void SSL_CREDENTIAL_set_must_match_issuer(SSL_CREDENTIAL *cred) {
- cred->must_match_issuer = true;
-}
-
-void SSL_CREDENTIAL_clear_must_match_issuer(SSL_CREDENTIAL *cred) {
- cred->must_match_issuer = false;
-}
-
-int SSL_CREDENTIAL_must_match_issuer(const SSL_CREDENTIAL *cred) {
- return cred->must_match_issuer ? 1 : 0;
+void SSL_CREDENTIAL_set_must_match_issuer(SSL_CREDENTIAL *cred, int match) {
+ cred->must_match_issuer = !!match;
}
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index e7d83e4..b7e0a65 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -4876,8 +4876,8 @@
ASSERT_TRUE(SSL_CREDENTIAL_set1_private_key(cred.get(), key.get()));
ASSERT_TRUE(SSL_CREDENTIAL_set1_private_key(cred2.get(), testkey.get()));
- SSL_CREDENTIAL_set_must_match_issuer(cred.get());
- SSL_CREDENTIAL_set_must_match_issuer(cred2.get());
+ SSL_CREDENTIAL_set_must_match_issuer(cred.get(), 1);
+ SSL_CREDENTIAL_set_must_match_issuer(cred2.get(), 1);
ASSERT_TRUE(SSL_CTX_add1_credential(ctx.get(), cred.get()));
ASSERT_TRUE(SSL_CTX_add1_credential(ctx.get(), cred2.get()));
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index daae594..1f24361 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -509,6 +509,10 @@
// If RootCAs is nil, TLS uses the host's root CA set.
RootCAs *x509.CertPool
+ // SendRootCAs, if true, causes the client to send the list of
+ // supported root CAs in the certificate_authorities extension.
+ SendRootCAs bool
+
// NextProtos is a list of supported, application level protocols.
NextProtos []string
@@ -2282,7 +2286,9 @@
Type CredentialType
// Certificate is a chain of one or more certificates, leaf first.
Certificate [][]byte
- PrivateKey crypto.PrivateKey // supported types: *rsa.PrivateKey, *ecdsa.PrivateKey
+ // RootCertificate is the certificate that issued this chain.
+ RootCertificate []byte
+ PrivateKey crypto.PrivateKey // supported types: *rsa.PrivateKey, *ecdsa.PrivateKey
// OCSPStaple contains an optional OCSP response which will be served
// to clients that request it.
OCSPStaple []byte
@@ -2313,6 +2319,9 @@
// SignSignatureAlgorithms, if not nil, overrides the default set of
// supported signature algorithms to sign with.
SignSignatureAlgorithms []signatureAlgorithm
+ // MustMatchIssuer, if set, causes the shim to only consider this
+ // credential when the issuer matches a peer-requested CA.
+ MustMatchIssuer bool
// The following fields are used for PAKE credentials. For simplicity,
// we specify the password directly and expect the shim and runner to
// compute the client- and server-specific halves as needed.
@@ -2346,6 +2355,12 @@
return &ret
}
+func (c *Credential) WithMustMatchIssuer(mustMatch bool) *Credential {
+ ret := *c
+ ret.MustMatchIssuer = mustMatch
+ return &ret
+}
+
func (c *Credential) signatureAlgorithms() []signatureAlgorithm {
if c != nil && c.SignatureAlgorithms != nil {
return c.SignatureAlgorithms
@@ -2568,12 +2583,13 @@
cert := generateTestCert(template, nil, key)
tmpCertPath, tmpKeyPath := writeTempCertFile([]*x509.Certificate{cert}), writeTempKeyFile(key)
return Credential{
- Certificate: [][]byte{cert.Raw},
- PrivateKey: key,
- Leaf: cert,
- ChainPath: tmpCertPath,
- KeyPath: tmpKeyPath,
- RootPath: tmpCertPath,
+ Certificate: [][]byte{cert.Raw},
+ RootCertificate: cert.Raw,
+ PrivateKey: key,
+ Leaf: cert,
+ ChainPath: tmpCertPath,
+ KeyPath: tmpKeyPath,
+ RootPath: tmpCertPath,
}
}
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 731902f..5fd5b7b 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -632,6 +632,10 @@
}
}
+ if c.config.SendRootCAs && c.config.RootCAs != nil {
+ hello.certificateAuthorities = c.config.RootCAs.Subjects()
+ }
+
if maxVersion >= VersionTLS13 {
// Use the same key shares between ClientHelloInner and ClientHelloOuter.
if innerHello != nil {
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index 651efb3..32a2732 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -205,6 +205,7 @@
pakeClientID []byte
pakeServerID []byte
pakeShares []pakeShare
+ certificateAuthorities [][]byte
outerExtensions []uint16
reorderOuterExtensionsWithoutCompressing bool
prefixExtensions []uint16
@@ -560,6 +561,18 @@
body: body.BytesOrPanic(),
})
}
+ if len(m.certificateAuthorities) > 0 {
+ body := cryptobyte.NewBuilder(nil)
+ body.AddUint16LengthPrefixed(func(certificateAuthorities *cryptobyte.Builder) {
+ for _, ca := range m.certificateAuthorities {
+ addUint16LengthPrefixedBytes(certificateAuthorities, ca)
+ }
+ })
+ extensions = append(extensions, extension{
+ id: extensionCertificateAuthorities,
+ body: body.BytesOrPanic(),
+ })
+ }
// The PSK extension must be last. See https://tools.ietf.org/html/rfc8446#section-4.2.11
if len(m.pskIdentities) > 0 {
pskExtension := cryptobyte.NewBuilder(nil)
@@ -1101,6 +1114,12 @@
m.pakeServerID = []byte(serverId)
m.pakeShares = append(m.pakeShares, pakeShare{id: id, msg: msg})
}
+ case extensionCertificateAuthorities:
+ if !parseCAs(&body, &m.certificateAuthorities) || len(body) != 0 ||
+ // If present, the CA extension may not be empty.
+ len(m.certificateAuthorities) == 0 {
+ return false
+ }
}
if isGREASEValue(extension) {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index fe59956..238e958 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -231,18 +231,21 @@
func initCertificates() {
for _, def := range []struct {
- key crypto.Signer
- out *Credential
+ name string
+ key crypto.Signer
+ out *Credential
}{
- {&rsa1024Key, &rsa1024Certificate},
- {&rsa2048Key, &rsaCertificate},
- {&ecdsaP224Key, &ecdsaP224Certificate},
- {&ecdsaP256Key, &ecdsaP256Certificate},
- {&ecdsaP384Key, &ecdsaP384Certificate},
- {&ecdsaP521Key, &ecdsaP521Certificate},
- {ed25519Key, &ed25519Certificate},
+ {"Test RSA-1024 Cert", &rsa1024Key, &rsa1024Certificate},
+ {"Test RSA-2048 Cert", &rsa2048Key, &rsaCertificate},
+ {"Test ECDSA P-224 Cert", &ecdsaP224Key, &ecdsaP224Certificate},
+ {"Test ECDSA P-256 Cert", &ecdsaP256Key, &ecdsaP256Certificate},
+ {"Test ECDSA P-384 Cert", &ecdsaP384Key, &ecdsaP384Certificate},
+ {"Test ECDSA P-521 Cert", &ecdsaP521Key, &ecdsaP521Certificate},
+ {"Test Ed25519 Cert", ed25519Key, &ed25519Certificate},
} {
- *def.out = generateSingleCertChain(nil, def.key)
+ template := *baseCertTemplate
+ template.Subject.CommonName = def.name
+ *def.out = generateSingleCertChain(&template, def.key)
}
channelIDBytes = make([]byte, 64)
@@ -267,12 +270,13 @@
rootCertPath, chainPath := writeTempCertFile([]*x509.Certificate{rootCert}), writeTempCertFile([]*x509.Certificate{leafCert, intermediateCert})
rsaChainCertificate = Credential{
- Certificate: [][]byte{leafCert.Raw, intermediateCert.Raw},
- PrivateKey: &rsa2048Key,
- Leaf: leafCert,
- ChainPath: chainPath,
- KeyPath: keyPath,
- RootPath: rootCertPath,
+ Certificate: [][]byte{leafCert.Raw, intermediateCert.Raw},
+ RootCertificate: rootCert.Raw,
+ PrivateKey: &rsa2048Key,
+ Leaf: leafCert,
+ ChainPath: chainPath,
+ KeyPath: keyPath,
+ RootPath: rootCertPath,
}
}
@@ -1496,6 +1500,9 @@
flags = append(flags, prefix+"-signing-prefs", strconv.Itoa(int(sigAlg)))
}
handleBase64Field("delegated-credential", cred.DelegatedCredential)
+ if cred.MustMatchIssuer {
+ flags = append(flags, prefix+"-must-match-issuer")
+ }
handleBase64Field("pake-context", cred.PAKEContext)
handleBase64Field("pake-client-id", cred.PAKEClientID)
handleBase64Field("pake-server-id", cred.PAKEServerID)
@@ -4632,16 +4639,21 @@
}
}
-func addClientAuthTests() {
- // Add a dummy cert pool to stress certificate authority parsing.
+func makeCertPoolFromRoots(creds ...*Credential) *x509.CertPool {
certPool := x509.NewCertPool()
- for _, cert := range []Credential{rsaCertificate, rsa1024Certificate} {
- cert, err := x509.ParseCertificate(cert.Certificate[0])
+ for _, cred := range creds {
+ cert, err := x509.ParseCertificate(cred.RootCertificate)
if err != nil {
panic(err)
}
certPool.AddCert(cert)
}
+ return certPool
+}
+
+func addClientAuthTests() {
+ // Add a dummy cert pool to stress certificate authority parsing.
+ certPool := makeCertPoolFromRoots(&rsaCertificate, &rsa1024Certificate)
caNames := certPool.Subjects()
for _, ver := range tlsVersions {
@@ -18394,6 +18406,10 @@
dcAlgo: signatureECDSAWithP256AndSHA256,
algo: signatureRSAPSSWithSHA256,
})
+ p256DCFromECDSA := createDelegatedCredential(&ecdsaP256Certificate, delegatedCredentialConfig{
+ dcAlgo: signatureECDSAWithP256AndSHA256,
+ algo: signatureECDSAWithP256AndSHA256,
+ })
testCases = append(testCases, testCase{
testType: serverTest,
@@ -18547,6 +18563,25 @@
peerCertificate: p384DC,
},
})
+
+ // Delegated credentials participate in issuer-based certificate selection.
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "DelegatedCredentials-MatchIssuer",
+ config: Config{
+ DelegatedCredentialAlgorithms: []signatureAlgorithm{signatureECDSAWithP256AndSHA256},
+ // The client requested p256DCFromECDSA's issuer.
+ RootCAs: makeCertPoolFromRoots(p256DCFromECDSA),
+ SendRootCAs: true,
+ },
+ shimCredentials: []*Credential{
+ p256DC.WithMustMatchIssuer(true), p256DCFromECDSA.WithMustMatchIssuer(true)},
+ flags: []string{"-expect-selected-credential", "1"},
+ expectations: connectionExpectations{
+ peerCertificate: p256DCFromECDSA,
+ },
+ })
+
}
type echCipher struct {
@@ -21708,6 +21743,11 @@
}
}
+func canBeShimCertificate(c *Credential) bool {
+ // Some options can only be set with the credentials API.
+ return c.Type == CredentialTypeX509 && !c.MustMatchIssuer
+}
+
func addCertificateSelectionTests() {
// Combinatorially test each selection criteria at different versions,
// protocols, and with the matching certificate before and after the
@@ -22106,6 +22146,52 @@
mismatch: rsaCertificate.WithSignatureAlgorithms(signatureRSAPSSWithSHA384),
expectedError: ":NO_COMMON_SIGNATURE_ALGORITHMS:",
},
+
+ // By default, certificate selection does not take issuers
+ // into account.
+ {
+ name: "Client-DontCheckIssuer",
+ testType: clientTest,
+ config: Config{
+ ClientAuth: RequestClientCert,
+ ClientCAs: makeCertPoolFromRoots(&rsaChainCertificate, &ecdsaP384Certificate),
+ },
+ match: &ecdsaP256Certificate,
+ },
+ {
+ name: "Server-DontCheckIssuer",
+ testType: serverTest,
+ config: Config{
+ RootCAs: makeCertPoolFromRoots(&rsaChainCertificate, &ecdsaP384Certificate),
+ SendRootCAs: true,
+ },
+ match: &ecdsaP256Certificate,
+ },
+
+ // If requested, certificate selection will match against the
+ // requested issuers.
+ {
+ name: "Client-CheckIssuer",
+ testType: clientTest,
+ config: Config{
+ ClientAuth: RequestClientCert,
+ ClientCAs: makeCertPoolFromRoots(&rsaChainCertificate, &ecdsaP384Certificate),
+ },
+ match: rsaChainCertificate.WithMustMatchIssuer(true),
+ mismatch: ecdsaP256Certificate.WithMustMatchIssuer(true),
+ expectedError: ":NO_MATCHING_ISSUER:",
+ },
+ {
+ name: "Server-CheckIssuer",
+ testType: serverTest,
+ config: Config{
+ RootCAs: makeCertPoolFromRoots(&rsaChainCertificate, &ecdsaP384Certificate),
+ SendRootCAs: true,
+ },
+ match: rsaChainCertificate.WithMustMatchIssuer(true),
+ mismatch: ecdsaP256Certificate.WithMustMatchIssuer(true),
+ expectedError: ":NO_MATCHING_ISSUER:",
+ },
}
for _, protocol := range []protocol{tls, dtls} {
@@ -22247,16 +22333,18 @@
flags: append([]string{"-expect-selected-credential", "1"}, test.flags...),
expectations: connectionExpectations{peerCertificate: test.match},
})
- testCases = append(testCases, testCase{
- name: fmt.Sprintf("CertificateSelection-%s-MatchDefault-%s", test.name, suffix),
- protocol: protocol,
- testType: test.testType,
- config: config,
- shimCredentials: []*Credential{test.mismatch},
- shimCertificate: test.match,
- flags: append([]string{"-expect-selected-credential", "-1"}, test.flags...),
- expectations: connectionExpectations{peerCertificate: test.match},
- })
+ if canBeShimCertificate(test.match) {
+ testCases = append(testCases, testCase{
+ name: fmt.Sprintf("CertificateSelection-%s-MatchDefault-%s", test.name, suffix),
+ protocol: protocol,
+ testType: test.testType,
+ config: config,
+ shimCredentials: []*Credential{test.mismatch},
+ shimCertificate: test.match,
+ flags: append([]string{"-expect-selected-credential", "-1"}, test.flags...),
+ expectations: connectionExpectations{peerCertificate: test.match},
+ })
+ }
testCases = append(testCases, testCase{
name: fmt.Sprintf("CertificateSelection-%s-MatchNone-%s", test.name, suffix),
protocol: protocol,
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index b3947fa..6d3613a 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -534,6 +534,8 @@
&TestConfig::signed_cert_timestamps),
Base64Flag("-signed-cert-timestamps",
&CredentialConfig::signed_cert_timestamps)),
+ CredentialFlag(BoolFlag("-must-match-issuer",
+ &CredentialConfig::must_match_issuer)),
CredentialFlag(
Base64Flag("-pake-context", &CredentialConfig::pake_context)),
CredentialFlag(
@@ -1480,6 +1482,10 @@
}
}
+ if (cred_config.must_match_issuer) {
+ SSL_CREDENTIAL_set_must_match_issuer(cred.get(), 1);
+ }
+
if (!SetCredentialInfo(cred.get(), std::move(info))) {
return nullptr;
}
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index 6ddf620..d573d21 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -39,6 +39,7 @@
std::vector<uint8_t> delegated_credential;
std::vector<uint8_t> ocsp_response;
std::vector<uint8_t> signed_cert_timestamps;
+ bool must_match_issuer = false;
std::vector<uint8_t> pake_context;
std::vector<uint8_t> pake_client_id;
std::vector<uint8_t> pake_server_id;
