commit a5dcf35caf6857f08be29586fd41ce8349d9e857
author David Benjamin <davidben@google.com> Thu Jan 26 15:05:14 2023 -0500
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Fri Feb 24 15:31:37 2023 +0000
tree ce1981ce77f3634af96448db56e0a75b1984da80
parent 7fa0910a6511895119a4aa3f474920594e734564

Move the ASN.1-based SSLKeyShare serialization to handoff.cc.

We've got two layers of serialization. There's the lower-level
SerializePrivateKey/DeserializePrivateKey functions that just encode a
private key assuming you already know the group, and then there's
Serialize/Create which output an INTEGER and OCTET STRING pair.

The latter is only used by handoff.cc, so move them there. This trims
the SSLKeyShare abstraction slightly.

Change-Id: I1c901d7c16b082bfe1b6acd0a1711575e7f95c05
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57625
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

ssl/handoff.cc

diff --git a/ssl/handoff.cc b/ssl/handoff.cc
index 39f0bac..ccb0b5e 100644
--- a/ssl/handoff.cc
+++ b/ssl/handoff.cc
@@ -387,9 +387,14 @@
       !CBB_add_asn1(&seq, &key_share, CBS_ASN1_SEQUENCE)) {
     return false;
   }
-  if (type == handback_after_ecdhe &&
-      !s3->hs->key_shares[0]->Serialize(&key_share)) {
-    return false;
+  if (type == handback_after_ecdhe) {
+    CBB private_key;
+    if (!CBB_add_asn1_uint64(&key_share, s3->hs->key_shares[0]->GroupID()) ||
+        !CBB_add_asn1(&key_share, &private_key, CBS_ASN1_OCTETSTRING) ||
+        !s3->hs->key_shares[0]->SerializePrivateKey(&private_key) ||
+        !CBB_flush(&key_share)) {
+      return false;
+    }
   }
   if (type == handback_tls13) {
     early_data_t early_data;
@@ -714,9 +719,19 @@
   }
   s3->read_sequence = CRYPTO_load_u64_be(read_sequence);
   s3->write_sequence = CRYPTO_load_u64_be(write_sequence);
-  if (type == handback_after_ecdhe &&
-      (hs->key_shares[0] = SSLKeyShare::Create(&key_share)) == nullptr) {
-    return false;
+  if (type == handback_after_ecdhe) {
+    uint64_t group_id;
+    CBS private_key;
+    if (!CBS_get_asn1_uint64(&key_share, &group_id) ||  //
+        group_id > 0xffff ||
+        !CBS_get_asn1(&key_share, &private_key, CBS_ASN1_OCTETSTRING)) {
+      return false;
+    }
+    hs->key_shares[0] = SSLKeyShare::Create(group_id);
+    if (!hs->key_shares[0] ||
+        !hs->key_shares[0]->DeserializePrivateKey(&private_key)) {
+      return false;
+    }
   }
   return true;  // Trailing data allowed for extensibility.
 }

ssl/internal.h

diff --git a/ssl/internal.h b/ssl/internal.h
index b83d299..fac15b6 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1062,14 +1062,6 @@
   // nullptr on error.
   static UniquePtr<SSLKeyShare> Create(uint16_t group_id);
 
-  // Create deserializes an SSLKeyShare instance previously serialized by
-  // |Serialize|.
-  static UniquePtr<SSLKeyShare> Create(CBS *in);
-
-  // Serializes writes the group ID and private key, in a format that can be
-  // read by |Create|.
-  bool Serialize(CBB *out);
-
   // GroupID returns the group ID.
   virtual uint16_t GroupID() const PURE_VIRTUAL;
 

ssl/ssl_key_share.cc

diff --git a/ssl/ssl_key_share.cc b/ssl/ssl_key_share.cc
index c6e87a6..1f28493 100644
--- a/ssl/ssl_key_share.cc
+++ b/ssl/ssl_key_share.cc
@@ -376,31 +376,6 @@
   }
 }
 
-UniquePtr<SSLKeyShare> SSLKeyShare::Create(CBS *in) {
-  uint64_t group;
-  CBS private_key;
-  if (!CBS_get_asn1_uint64(in, &group) || group > 0xffff ||
-      !CBS_get_asn1(in, &private_key, CBS_ASN1_OCTETSTRING)) {
-    return nullptr;
-  }
-  UniquePtr<SSLKeyShare> key_share = Create(static_cast<uint16_t>(group));
-  if (!key_share || !key_share->DeserializePrivateKey(&private_key)) {
-    return nullptr;
-  }
-  return key_share;
-}
-
-bool SSLKeyShare::Serialize(CBB *out) {
-  CBB private_key;
-  if (!CBB_add_asn1_uint64(out, GroupID()) ||
-      !CBB_add_asn1(out, &private_key, CBS_ASN1_OCTETSTRING) ||
-      !SerializePrivateKey(&private_key) ||  //
-      !CBB_flush(out)) {
-    return false;
-  }
-  return true;
-}
-
 bool SSLKeyShare::Accept(CBB *out_public_key, Array<uint8_t> *out_secret,
                          uint8_t *out_alert, Span<const uint8_t> peer_key) {
   *out_alert = SSL_AD_INTERNAL_ERROR;