Use enums for TLS key exchange group params

20 files changed, 290 insertions, 266 deletions

diff --git a/src/lib/tls/msg_client_hello.cpp b/src/lib/tls/msg_client_hello.cpp
index 158238de2..a9da82f07 100644
--- a/src/lib/tls/msg_client_hello.cpp
+++ b/src/lib/tls/msg_client_hello.cpp
@@ -129,14 +129,15 @@ Client_Hello::Client_Hello(Handshake_IO& io,
       }
 #endif
 
-   Supported_Groups* supported_groups = new Supported_Groups(policy.allowed_groups());
-   m_extensions.add(supported_groups);
+   std::unique_ptr<Supported_Groups> supported_groups(new Supported_Groups(policy.key_exchange_groups()));
 
-   if(!supported_groups->curves().empty())
+   if(supported_groups->ec_groups().size() > 0)
       {
       m_extensions.add(new Supported_Point_Formats(policy.use_ecc_point_compression()));
       }
 
+   m_extensions.add(supported_groups.release());
+
    cb.tls_modify_extensions(m_extensions, CLIENT);
 
    if(policy.send_fallback_scsv(client_settings.protocol_version()))
@@ -175,13 +176,16 @@ Client_Hello::Client_Hello(Handshake_IO& io,
    m_extensions.add(new Renegotiation_Extension(reneg_info));
    m_extensions.add(new Server_Name_Indicator(session.server_info().hostname()));
    m_extensions.add(new Session_Ticket(session.session_ticket()));
-   m_extensions.add(new Supported_Elliptic_Curves(policy.allowed_ecc_curves()));
 
-   if(!policy.allowed_ecc_curves().empty())
+   std::unique_ptr<Supported_Groups> supported_groups(new Supported_Groups(policy.key_exchange_groups()));
+
+   if(supported_groups->ec_groups().size() > 0)
       {
       m_extensions.add(new Supported_Point_Formats(policy.use_ecc_point_compression()));
       }
 
+   m_extensions.add(supported_groups.release());
+
    if(session.supports_encrypt_then_mac())
       m_extensions.add(new Encrypt_then_MAC);
 
@@ -324,18 +328,18 @@ std::vector<Signature_Scheme> Client_Hello::signature_schemes() const
    return schemes;
    }
 
-std::vector<std::string> Client_Hello::supported_ecc_curves() const
+std::vector<Group_Params> Client_Hello::supported_ecc_curves() const
    {
    if(Supported_Groups* groups = m_extensions.get<Supported_Groups>())
-      return groups->curves();
-   return std::vector<std::string>();
+      return groups->ec_groups();
+   return std::vector<Group_Params>();
    }
 
-std::vector<std::string> Client_Hello::supported_dh_groups() const
+std::vector<Group_Params> Client_Hello::supported_dh_groups() const
    {
    if(Supported_Groups* groups = m_extensions.get<Supported_Groups>())
       return groups->dh_groups();
-   return std::vector<std::string>();
+   return std::vector<Group_Params>();
    }
 
 bool Client_Hello::prefers_compressed_ec_points() const
diff --git a/src/lib/tls/msg_client_kex.cpp b/src/lib/tls/msg_client_kex.cpp
index 6e767d4d6..b94e9839e 100644
--- a/src/lib/tls/msg_client_kex.cpp
+++ b/src/lib/tls/msg_client_kex.cpp
@@ -112,30 +112,31 @@ Client_Key_Exchange::Client_Key_Exchange(Handshake_IO& io,
               kex_algo == Kex_Algo::ECDHE_PSK)
          {
          const uint8_t curve_type = reader.get_byte();
-
          if(curve_type != 3)
             throw Decoding_Error("Server sent non-named ECC curve");
 
          const uint16_t curve_id = reader.get_uint16_t();
+         const std::vector<uint8_t> peer_public_value = reader.get_range<uint8_t>(1, 1, 255);
 
-         const std::string curve_name = Supported_Elliptic_Curves::curve_id_to_name(curve_id);
-
-         if(curve_name == "")
-            throw Decoding_Error("Server sent unknown named curve " + std::to_string(curve_id));
-
-         if(!policy.allowed_ecc_curve(curve_name))
+         if(policy.choose_key_exchange_group({static_cast<Group_Params>(curve_id)}) == Group_Params::NONE)
             {
             throw TLS_Exception(Alert::HANDSHAKE_FAILURE,
                                 "Server sent ECC curve prohibited by policy");
             }
 
-         const std::vector<uint8_t> peer_public_value = reader.get_range<uint8_t>(1, 1, 255);
-         const std::pair<secure_vector<uint8_t>, std::vector<uint8_t>> ecdh_result =
+         const std::string curve_name = Supported_Groups::curve_id_to_name(curve_id);
+
+         if(curve_name == "")
+            throw Decoding_Error("Server sent unknown named curve " + std::to_string(curve_id));
+
+               const std::pair<secure_vector<uint8_t>, std::vector<uint8_t>> ecdh_result =
             state.callbacks().tls_ecdh_agree(curve_name, peer_public_value, policy, rng,
                                              state.server_hello()->prefers_compressed_ec_points());
 
          if(kex_algo == Kex_Algo::ECDH)
+            {
             m_pre_master = ecdh_result.first;
+            }
          else
             {
             append_tls_length_value(m_pre_master, ecdh_result.first, 2);
diff --git a/src/lib/tls/msg_server_kex.cpp b/src/lib/tls/msg_server_kex.cpp
index 56ff3017f..4cb204c68 100644
--- a/src/lib/tls/msg_server_kex.cpp
+++ b/src/lib/tls/msg_server_kex.cpp
@@ -57,27 +57,31 @@ Server_Key_Exchange::Server_Key_Exchange(Handshake_IO& io,
 
    if(kex_algo == Kex_Algo::DH || kex_algo == Kex_Algo::DHE_PSK)
       {
-      const std::vector<std::string>& dh_groups =
-               state.client_hello()->supported_dh_groups();
+      const std::vector<Group_Params> dh_groups = state.client_hello()->supported_dh_groups();
 
-      std::string group_name;
+      Group_Params shared_group = Group_Params::NONE;
 
-      // if the client does not send any DH groups in
-      // the supported groups extension, but does offer DH ciphersuites,
-      // we select a group arbitrarily
-      if (dh_groups.empty())
+      /*
+      If the client does not send any DH groups in the supported groups
+      extension, but does offer DH ciphersuites, we select a group arbitrarily
+      */
+
+      if(dh_groups.empty())
          {
-         group_name = policy.dh_group();
+         shared_group = policy.default_dh_group();
          }
       else
          {
-         group_name = policy.choose_dh_group(dh_groups);
+         shared_group = policy.choose_key_exchange_group(dh_groups);
          }
 
-      if (group_name.empty())
+      if(shared_group == Group_Params::NONE)
          throw TLS_Exception(Alert::HANDSHAKE_FAILURE,
                "Could not agree on a DH group with the client");
 
+      BOTAN_ASSERT(group_param_is_dh(shared_group), "DH groups for the DH ciphersuites god");
+
+      const std::string group_name = group_param_to_string(shared_group);
       std::unique_ptr<DH_PrivateKey> dh(new DH_PrivateKey(rng, DL_Group(group_name)));
 
       append_tls_length_value(m_params, BigInt::encode(dh->get_domain().get_p()), 2);
@@ -87,25 +91,19 @@ Server_Key_Exchange::Server_Key_Exchange(Handshake_IO& io,
       }
    else if(kex_algo == Kex_Algo::ECDH || kex_algo == Kex_Algo::ECDHE_PSK)
       {
-      const std::vector<std::string>& curves =
-         state.client_hello()->supported_ecc_curves();
+      const std::vector<Group_Params> ec_groups = state.client_hello()->supported_ecc_curves();
 
-      if(curves.empty())
+      if(ec_groups.empty())
          throw Internal_Error("Client sent no ECC extension but we negotiated ECDH");
 
-      const std::string curve_name = policy.choose_curve(curves);
-
-      if(curve_name == "")
-         throw TLS_Exception(Alert::HANDSHAKE_FAILURE,
-                             "Could not agree on an ECC curve with the client");
+      Group_Params shared_group = policy.choose_key_exchange_group(ec_groups);
 
-      const uint16_t named_curve_id = Supported_Elliptic_Curves::name_to_curve_id(curve_name);
-      if(named_curve_id == 0)
-         throw Internal_Error("TLS does not support ECC with " + curve_name);
+      if(shared_group == Group_Params::NONE)
+         throw TLS_Exception(Alert::HANDSHAKE_FAILURE, "No shared ECC group with client");
 
       std::vector<uint8_t> ecdh_public_val;
 
-      if(curve_name == "x25519")
+      if(shared_group == Group_Params::X25519)
          {
 #if defined(BOTAN_HAS_CURVE_25519)
          std::unique_ptr<Curve25519_PrivateKey> x25519(new Curve25519_PrivateKey(rng));
@@ -117,6 +115,10 @@ Server_Key_Exchange::Server_Key_Exchange(Handshake_IO& io,
          }
       else
          {
+         Group_Params curve = policy.choose_key_exchange_group(ec_groups);
+
+         const std::string curve_name = group_param_to_string(curve);
+
          EC_Group ec_group(curve_name);
          std::unique_ptr<ECDH_PrivateKey> ecdh(new ECDH_PrivateKey(rng, ec_group));
 
@@ -128,6 +130,7 @@ Server_Key_Exchange::Server_Key_Exchange(Handshake_IO& io,
          m_kex_key.reset(ecdh.release());
          }
 
+      const uint16_t named_curve_id = static_cast<uint16_t>(shared_group);
       m_params.push_back(3); // named curve
       m_params.push_back(get_byte(0, named_curve_id));
       m_params.push_back(get_byte(1, named_curve_id));
diff --git a/src/lib/tls/tls_algos.cpp b/src/lib/tls/tls_algos.cpp
index 33b00d519..ce32963b7 100644
--- a/src/lib/tls/tls_algos.cpp
+++ b/src/lib/tls/tls_algos.cpp
@@ -115,6 +115,43 @@ Auth_Method auth_method_from_string(const std::string& str)
    throw Invalid_Argument("Bad signature method " + str);
    }
 
+bool group_param_is_dh(Group_Params group)
+   {
+   uint16_t group_id = static_cast<uint16_t>(group);
+   return (group_id >= 256 && group_id < 512);
+   }
+
+Group_Params group_param_from_string(const std::string& group_name)
+   {
+   if(group_name == "secp256r1")
+      return Group_Params::SECP256R1;
+   if(group_name == "secp384r1")
+      return Group_Params::SECP384R1;
+   if(group_name == "secp521r1")
+      return Group_Params::SECP521R1;
+   if(group_name == "brainpool256r1")
+      return Group_Params::BRAINPOOL256R1;
+   if(group_name == "brainpool384r1")
+      return Group_Params::BRAINPOOL384R1;
+   if(group_name == "brainpool512r1")
+      return Group_Params::BRAINPOOL512R1;
+   if(group_name == "x25519")
+      return Group_Params::X25519;
+
+   if(group_name == "ffdhe/ietf/2048")
+      return Group_Params::FFDHE_2048;
+   if(group_name == "ffdhe/ietf/3072")
+      return Group_Params::FFDHE_3072;
+   if(group_name == "ffdhe/ietf/4096")
+      return Group_Params::FFDHE_4096;
+   if(group_name == "ffdhe/ietf/6144")
+      return Group_Params::FFDHE_6144;
+   if(group_name == "ffdhe/ietf/8192")
+      return Group_Params::FFDHE_8192;
+
+   return Group_Params::NONE; // unknown
+   }
+
 std::string group_param_to_string(Group_Params group)
    {
    switch(group)
diff --git a/src/lib/tls/tls_algos.h b/src/lib/tls/tls_algos.h
index 4f7a35ec8..9ad1a7a85 100644
--- a/src/lib/tls/tls_algos.h
+++ b/src/lib/tls/tls_algos.h
@@ -118,6 +118,8 @@ std::string signature_algorithm_of_scheme(Signature_Scheme scheme);
 * Matches with wire encoding
 */
 enum class Group_Params : uint16_t {
+   NONE = 0,
+
    SECP256R1 = 23,
    SECP384R1 = 24,
    SECP521R1 = 25,
@@ -139,6 +141,8 @@ enum class Group_Params : uint16_t {
 };
 
 std::string group_param_to_string(Group_Params group);
+Group_Params group_param_from_string(const std::string& group_name);
+bool group_param_is_dh(Group_Params group);
 
 enum class Kex_Algo {
    STATIC_RSA,
diff --git a/src/lib/tls/tls_extensions.cpp b/src/lib/tls/tls_extensions.cpp
index 6d69d7b45..f796a39df 100644
--- a/src/lib/tls/tls_extensions.cpp
+++ b/src/lib/tls/tls_extensions.cpp
@@ -28,8 +28,8 @@ Extension* make_extension(TLS_Data_Reader& reader, uint16_t code, uint16_t size)
          return new SRP_Identifier(reader, size);
 #endif
 
-      case TLSEXT_USABLE_ELLIPTIC_CURVES:
-         return new Supported_Elliptic_Curves(reader, size);
+      case TLSEXT_SUPPORTED_GROUPS:
+         return new Supported_Groups(reader, size);
 
       case TLSEXT_CERT_STATUS_REQUEST:
          return new Certificate_Status_Request(reader, size);
@@ -295,20 +295,30 @@ std::vector<uint8_t> Application_Layer_Protocol_Notification::serialize() const
    return buf;
    }
 
-Supported_Groups::Supported_Groups(const std::vector<std::string>& groups) :
-      m_groups(groups)
+Supported_Groups::Supported_Groups(const std::vector<Group_Params>& groups) : m_groups(groups)
    {
-   for(const auto& group : m_groups)
+   }
+
+std::vector<Group_Params> Supported_Groups::ec_groups() const
+   {
+   std::vector<Group_Params> ec;
+   for(auto g : m_groups)
       {
-      if(is_dh_group(group))
-         {
-         m_dh_groups.push_back(group);
-         }
-      else
-         {
-         m_curves.push_back(group);
-         }
+      if(group_param_is_dh(g) == false)
+         ec.push_back(g);
       }
+   return ec;
+   }
+
+std::vector<Group_Params> Supported_Groups::dh_groups() const
+   {
+   std::vector<Group_Params> dh;
+   for(auto g : m_groups)
+      {
+      if(group_param_is_dh(g) == true)
+         dh.push_back(g);
+      }
+   return dh;
    }
 
 std::string Supported_Groups::curve_id_to_name(uint16_t id)
@@ -409,9 +419,9 @@ std::vector<uint8_t> Supported_Groups::serialize() const
    {
    std::vector<uint8_t> buf(2);
 
-   for(size_t i = 0; i != m_groups.size(); ++i)
+   for(auto g : m_groups)
       {
-      const uint16_t id = name_to_curve_id(m_groups[i]);
+      const uint16_t id = static_cast<uint16_t>(g);
 
       if(id > 0)
          {
@@ -427,9 +437,9 @@ std::vector<uint8_t> Supported_Groups::serialize() const
    }
 
 Supported_Groups::Supported_Groups(TLS_Data_Reader& reader,
-      uint16_t extension_size)
+                                   uint16_t extension_size)
    {
-   uint16_t len = reader.get_uint16_t();
+   const uint16_t len = reader.get_uint16_t();
 
    if(len + 2 != extension_size)
       throw Decoding_Error("Inconsistent length field in supported groups list");
@@ -437,28 +447,10 @@ Supported_Groups::Supported_Groups(TLS_Data_Reader& reader,
    if(len % 2 == 1)
       throw Decoding_Error("Supported groups list of strange size");
 
-   len /= 2;
-
-   for(size_t i = 0; i != len; ++i)
+   for(size_t i = 0; i != len / 2; ++i)
       {
       const uint16_t id = reader.get_uint16_t();
-      const Group_Params group_id = static_cast<Group_Params>(id);
-
-      const bool is_dh = (id >= 256 && id <= 511);
-      const std::string name = group_param_to_string(group_id);
-
-      if(!name.empty())
-         {
-         m_groups.push_back(name);
-         if(is_dh)
-            {
-            m_dh_groups.push_back(name);
-            }
-         else
-            {
-            m_curves.push_back(name);
-            }
-         }
+      m_groups.push_back(static_cast<Group_Params>(id));
       }
    }
 
diff --git a/src/lib/tls/tls_extensions.h b/src/lib/tls/tls_extensions.h
index a1ed3f858..27917a145 100644
--- a/src/lib/tls/tls_extensions.h
+++ b/src/lib/tls/tls_extensions.h
@@ -1,8 +1,8 @@
 /*
 * TLS Extensions
-* (C) 2011,2012,2016 Jack Lloyd
-*     2016 Juraj Somorovsky
-*     2016 Matthias Gierlings
+* (C) 2011,2012,2016,2018 Jack Lloyd
+* (C) 2016 Juraj Somorovsky
+* (C) 2016 Matthias Gierlings
 *
 * Botan is released under the Simplified BSD License (see license.txt)
 */
@@ -29,7 +29,7 @@ enum Handshake_Extension_Type {
    TLSEXT_CERT_STATUS_REQUEST    = 5,
 
    TLSEXT_CERTIFICATE_TYPES      = 9,
-   TLSEXT_USABLE_ELLIPTIC_CURVES = 10,
+   TLSEXT_SUPPORTED_GROUPS       = 10,
    TLSEXT_EC_POINT_FORMATS       = 11,
    TLSEXT_SRP_IDENTIFIER         = 12,
    TLSEXT_SIGNATURE_ALGORITHMS   = 13,
@@ -234,34 +234,32 @@ class Supported_Groups final : public Extension
    {
    public:
       static Handshake_Extension_Type static_type()
-         { return TLSEXT_USABLE_ELLIPTIC_CURVES; }
+         { return TLSEXT_SUPPORTED_GROUPS; }
 
       Handshake_Extension_Type type() const override { return static_type(); }
 
       static std::string curve_id_to_name(uint16_t id);
       static uint16_t name_to_curve_id(const std::string& name);
 
-      static bool is_dh_group( const std::string& group_name );
+      static bool is_dh_group(const std::string& group_name);
 
-      const std::vector<std::string>& curves() const { return m_curves; }
-      const std::vector<std::string>& dh_groups() const { return m_dh_groups; }
+      std::vector<Group_Params> ec_groups() const;
+      std::vector<Group_Params> dh_groups() const;
 
       std::vector<uint8_t> serialize() const override;
 
-      explicit Supported_Groups(const std::vector<std::string>& groups);
+      explicit Supported_Groups(const std::vector<Group_Params>& groups);
 
       Supported_Groups(TLS_Data_Reader& reader,
-                                uint16_t extension_size);
+                       uint16_t extension_size);
 
       bool empty() const override { return m_groups.empty(); }
    private:
-      std::vector<std::string> m_groups;
-      std::vector<std::string> m_curves;
-      std::vector<std::string> m_dh_groups;
+      std::vector<Group_Params> m_groups;
    };
 
 // previously Supported Elliptic Curves Extension (RFC 4492)
-using Supported_Elliptic_Curves = Supported_Groups;
+//using Supported_Elliptic_Curves = Supported_Groups;
 
 /**
 * Supported Point Formats Extension (RFC 4492)
diff --git a/src/lib/tls/tls_messages.h b/src/lib/tls/tls_messages.h
index 03b07fe27..98c46dfa8 100644
--- a/src/lib/tls/tls_messages.h
+++ b/src/lib/tls/tls_messages.h
@@ -106,9 +106,9 @@ class BOTAN_UNSTABLE_API Client_Hello final : public Handshake_Message
 
       std::vector<Signature_Scheme> signature_schemes() const;
 
-      std::vector<std::string> supported_ecc_curves() const;
+      std::vector<Group_Params> supported_ecc_curves() const;
 
-      std::vector<std::string> supported_dh_groups() const;
+      std::vector<Group_Params> supported_dh_groups() const;
 
       bool prefers_compressed_ec_points() const;
 
diff --git a/src/lib/tls/tls_policy.cpp b/src/lib/tls/tls_policy.cpp
index 2c63aa840..7fd7af235 100644
--- a/src/lib/tls/tls_policy.cpp
+++ b/src/lib/tls/tls_policy.cpp
@@ -9,9 +9,8 @@
 
 #include <botan/tls_policy.h>
 #include <botan/tls_ciphersuite.h>
-#include <botan/tls_magic.h>
+#include <botan/tls_algos.h>
 #include <botan/tls_exceptn.h>
-#include <botan/tls_extensions.h>
 #include <botan/internal/stl_util.h>
 #include <botan/pk_keys.h>
 #include <sstream>
@@ -123,90 +122,58 @@ bool Policy::allowed_signature_hash(const std::string& sig_hash) const
    return value_exists(allowed_signature_hashes(), sig_hash);
    }
 
-std::vector<std::string> Policy::allowed_ecc_curves() const
-   {
-   // Default list is ordered by performance
-
-   return {
-      "x25519",
-      "secp256r1",
-      "secp521r1",
-      "secp384r1",
-      "brainpool256r1",
-      "brainpool384r1",
-      "brainpool512r1",
-      };
-   }
-
-bool Policy::allowed_ecc_curve(const std::string& curve) const
-   {
-   if(!allowed_ecc_curves().empty())
-      {
-      return value_exists(allowed_ecc_curves(), curve);
-      }
-   return value_exists(allowed_groups(), curve);
-   }
-
 bool Policy::use_ecc_point_compression() const
    {
    return false;
    }
 
-/*
-* Choose an ECC curve to use
-*/
-std::string Policy::choose_curve(const std::vector<std::string>& curve_names) const
+Group_Params Policy::choose_key_exchange_group(const std::vector<Group_Params>& peer_groups) const
    {
-   const std::vector<std::string> our_groups = allowed_groups();
+   if(peer_groups.empty())
+      return Group_Params::NONE;
 
-   for(size_t i = 0; i != our_groups.size(); ++i)
-      if(!Supported_Groups::is_dh_group(our_groups[i])
-         && value_exists(curve_names, our_groups[i]))
-         return our_groups[i];
+   const std::vector<Group_Params> our_groups = key_exchange_groups();
 
-   return ""; // no shared curve
-   }
-
-/*
-* Choose an FFDHE group to use
-*/
-std::string Policy::choose_dh_group(const std::vector<std::string>& dh_groups) const
-   {
-   if(dh_groups.empty())
-      return dh_group();
-
-   const std::vector<std::string> our_groups = allowed_groups();
-
-   for(size_t i = 0; i != our_groups.size(); ++i)
-      if(Supported_Groups::is_dh_group(our_groups[i])
-            && value_exists(dh_groups, our_groups[i]))
-         return our_groups[i];
+   for(auto g : our_groups)
+      {
+      if(value_exists(peer_groups, g))
+         return g;
+      }
 
-   return ""; // no shared ffdhe group
+   return Group_Params::NONE;
    }
 
-std::string Policy::dh_group() const
+Group_Params Policy::default_dh_group() const
    {
-   // We offer 2048 bit DH because we can
-   return "modp/ietf/2048";
+   /*
+   * Return the first listed or just default to 2048
+   */
+   for(auto g : key_exchange_groups())
+      {
+      if(group_param_is_dh(g))
+         return g;
+      }
+
+   return Group_Params::FFDHE_2048;
    }
 
-std::vector<std::string> Policy::allowed_groups() const
+std::vector<Group_Params> Policy::key_exchange_groups() const
    {
    // Default list is ordered by performance
    return {
-      "x25519",
-      "secp256r1",
-      "secp521r1",
-      "secp384r1",
-      "brainpool256r1",
-      "brainpool384r1",
-      "brainpool512r1",
-      "ffdhe/ietf/2048",
-      "ffdhe/ietf/3072",
-      "ffdhe/ietf/4096",
-      "ffdhe/ietf/6144",
-      "ffdhe/ietf/8192"
+      Group_Params::X25519,
+      Group_Params::SECP256R1,
+      Group_Params::SECP521R1,
+      Group_Params::SECP384R1,
+      Group_Params::BRAINPOOL256R1,
+      Group_Params::BRAINPOOL384R1,
+      Group_Params::BRAINPOOL512R1,
+
+      Group_Params::FFDHE_2048,
+      Group_Params::FFDHE_3072,
+      Group_Params::FFDHE_4096,
+      Group_Params::FFDHE_6144,
+      Group_Params::FFDHE_8192,
       };
    }
 
@@ -448,7 +415,7 @@ class Ciphersuite_Preference_Ordering final
 }
 
 std::vector<uint16_t> Policy::ciphersuite_list(Protocol_Version version,
-                                             bool have_srp) const
+                                               bool have_srp) const
    {
    const std::vector<std::string> ciphers = allowed_ciphers();
    const std::vector<std::string> macs = allowed_macs();
@@ -503,8 +470,11 @@ std::vector<uint16_t> Policy::ciphersuite_list(Protocol_Version version,
       removal of x25519 from the ECC curve list as equivalent to
       saying they do not trust CECPQ1
       */
-      if(suite.kex_method() == Kex_Algo::CECPQ1 && allowed_ecc_curve("x25519") == false)
-         continue;
+      if(suite.kex_method() == Kex_Algo::CECPQ1)
+         {
+         if(value_exists(key_exchange_groups(), Group_Params::X25519) == false)
+            continue;
+         }
 
       // OK, consider it
       ciphersuites.push_back(suite);
@@ -540,6 +510,20 @@ void print_vec(std::ostream& o,
    o << '\n';
    }
 
+void print_vec(std::ostream& o,
+               const char* key,
+               const std::vector<Group_Params>& v)
+   {
+   o << key << " = ";
+   for(size_t i = 0; i != v.size(); ++i)
+      {
+      o << group_param_to_string(v[i]);
+      if(i != v.size() - 1)
+         o << ' ';
+      }
+   o << '\n';
+   }
+
 void print_bool(std::ostream& o,
                 const char* key, bool b)
    {
@@ -560,8 +544,7 @@ void Policy::print(std::ostream& o) const
    print_vec(o, "signature_hashes", allowed_signature_hashes());
    print_vec(o, "signature_methods", allowed_signature_methods());
    print_vec(o, "key_exchange_methods", allowed_key_exchange_methods());
-   print_vec(o, "ecc_curves", allowed_ecc_curves());
-   print_vec(o, "groups", allowed_groups());
+   print_vec(o, "key_exchange_groups", key_exchange_groups());
 
    print_bool(o, "allow_insecure_renegotiation", allow_insecure_renegotiation());
    print_bool(o, "include_time_in_hello_random", include_time_in_hello_random());
@@ -571,7 +554,6 @@ void Policy::print(std::ostream& o) const
    print_bool(o, "negotiate_encrypt_then_mac", negotiate_encrypt_then_mac());
    print_bool(o, "support_cert_status_message", support_cert_status_message());
    o << "session_ticket_lifetime = " << session_ticket_lifetime() << '\n';
-   o << "dh_group = " << dh_group() << '\n';
    o << "minimum_dh_group_size = " << minimum_dh_group_size() << '\n';
    o << "minimum_ecdh_group_size = " << minimum_ecdh_group_size() << '\n';
    o << "minimum_rsa_bits = " << minimum_rsa_bits() << '\n';
diff --git a/src/lib/tls/tls_policy.h b/src/lib/tls/tls_policy.h
index a3d175201..c483770f8 100644
--- a/src/lib/tls/tls_policy.h
+++ b/src/lib/tls/tls_policy.h
@@ -81,23 +81,10 @@ class BOTAN_PUBLIC_API(2,0) Policy
       bool allowed_signature_hash(const std::string& hash) const;
 
       /**
-      * Return list of ECC curves we are willing to use in order of preference.
-      * Allowed values: x25519, secp256r1, secp384r1, secp521r1,
-      *                 brainpool256r1, brainpool384r1, brainpool512r1
+      * Return list of ECC curves and FFDHE groups we are willing to
+      * use in order of preference.
       */
-      virtual std::vector<std::string> allowed_ecc_curves() const;
-
-      bool allowed_ecc_curve(const std::string& curve) const;
-
-      /**
-      * Return list of ECC curves and FFDHE groups
-      * we are willing to use in order of preference.
-      * Allowed values: x25519, secp256r1, secp384r1, secp521r1,
-      *                 brainpool256r1, brainpool384r1, brainpool512r1,
-      *                 ffdhe/ietf/2048, ffdhe/ietf/3072, ffdhe/ietf/4096,
-      *                 ffdhe/ietf/6144, ffdhe/ietf/8192
-      */
-      virtual std::vector<std::string> allowed_groups() const;
+      virtual std::vector<Group_Params> key_exchange_groups() const;
 
       /**
       * Request that ECC curve points are sent compressed
@@ -105,14 +92,10 @@ class BOTAN_PUBLIC_API(2,0) Policy
       virtual bool use_ecc_point_compression() const;
 
       /**
-      * Choose an elliptic curve to use
-      */
-      virtual std::string choose_curve(const std::vector<std::string>& curve_names) const;
-
-      /**
-      * Choose an FFHDE group to use
+      * Select a key exchange group to use, from the list of groups sent by the
+      * peer. If none are acceptable, return Group_Params::NONE
       */
-      virtual std::string choose_dh_group(const std::vector<std::string>& dh_group_names) const;
+      virtual Group_Params choose_key_exchange_group(const std::vector<Group_Params>& peer_groups) const;
 
       /**
       * Allow renegotiation even if the counterparty doesn't
@@ -166,7 +149,7 @@ class BOTAN_PUBLIC_API(2,0) Policy
       */
       virtual bool allow_dtls12() const;
 
-      virtual std::string dh_group() const;
+      virtual Group_Params default_dh_group() const;
 
       /**
       * Return the minimum DH group size we're willing to use
@@ -291,7 +274,7 @@ class BOTAN_PUBLIC_API(2,0) Policy
       * Return allowed ciphersuites, in order of preference
       */
       virtual std::vector<uint16_t> ciphersuite_list(Protocol_Version version,
-                                                   bool have_srp) const;
+                                                     bool have_srp) const;
 
       /**
       * @return the default MTU for DTLS
@@ -323,6 +306,8 @@ class BOTAN_PUBLIC_API(2,0) Policy
       virtual ~Policy() = default;
    };
 
+typedef Policy Default_Policy;
+
 /**
 * NSA Suite B 128-bit security level (RFC 6460)
 */
@@ -344,11 +329,8 @@ class BOTAN_PUBLIC_API(2,0) NSA_Suite_B_128 : public Policy
       std::vector<std::string> allowed_signature_methods() const override
          { return std::vector<std::string>({"ECDSA"}); }
 
-      std::vector<std::string> allowed_ecc_curves() const override
-         { return std::vector<std::string>({"secp256r1"}); }
-
-      std::vector<std::string> allowed_groups() const override
-         { return allowed_ecc_curves(); }
+      std::vector<Group_Params> key_exchange_groups() const override
+         { return {Group_Params::SECP256R1}; }
 
       size_t minimum_signature_strength() const override { return 128; }
 
@@ -390,15 +372,20 @@ class BOTAN_PUBLIC_API(2,0) BSI_TR_02102_2 : public Policy
          return std::vector<std::string>({"ECDSA", "RSA", "DSA"});
          }
 
-      std::vector<std::string> allowed_ecc_curves() const override
+      std::vector<Group_Params> key_exchange_groups() const override
          {
-         return std::vector<std::string>({"brainpool512r1", "brainpool384r1", "brainpool256r1", "secp384r1", "secp256r1"});
-         }
-
-      std::vector<std::string> allowed_groups() const override
-         {
-         return std::vector<std::string>({"brainpool512r1", "brainpool384r1", "brainpool256r1", "secp384r1",
-            "secp256r1", "ffdhe/ietf/8192", "ffdhe/ietf/6144", "ffdhe/ietf/4096", "ffdhe/ietf/3072", "ffdhe/ietf/2048"});
+         return std::vector<Group_Params>({
+            Group_Params::BRAINPOOL512R1,
+               Group_Params::BRAINPOOL384R1,
+               Group_Params::BRAINPOOL256R1,
+               Group_Params::SECP384R1,
+               Group_Params::SECP256R1,
+               Group_Params::FFDHE_8192,
+               Group_Params::FFDHE_6144,
+               Group_Params::FFDHE_4096,
+               Group_Params::FFDHE_3072,
+               Group_Params::FFDHE_2048
+               });
          }
 
       bool allow_insecure_renegotiation() const override { return false; }
@@ -475,9 +462,7 @@ class BOTAN_PUBLIC_API(2,0) Text_Policy : public Policy
 
       std::vector<std::string> allowed_signature_methods() const override;
 
-      std::vector<std::string> allowed_ecc_curves() const override;
-
-      std::vector<std::string> allowed_groups() const override;
+      std::vector<Group_Params> key_exchange_groups() const;
 
       bool use_ecc_point_compression() const override;
 
@@ -504,8 +489,6 @@ class BOTAN_PUBLIC_API(2,0) Text_Policy : public Policy
 
       bool support_cert_status_message() const override;
 
-      std::string dh_group() const override;
-
       size_t minimum_ecdh_group_size() const override;
 
       size_t minimum_ecdsa_group_size() const override;
diff --git a/src/lib/tls/tls_server.cpp b/src/lib/tls/tls_server.cpp
index 61a360ba9..b4e74c775 100644
--- a/src/lib/tls/tls_server.cpp
+++ b/src/lib/tls/tls_server.cpp
@@ -166,7 +166,7 @@ uint16_t choose_ciphersuite(
                           "Policy forbids us from negotiating any ciphersuite");
 
    const bool have_shared_ecc_curve =
-      (policy.choose_curve(client_hello.supported_ecc_curves()) != "");
+      (policy.choose_key_exchange_group(client_hello.supported_ecc_curves()) != Group_Params::NONE);
 
    /*
    Walk down one list in preference order
diff --git a/src/lib/tls/tls_text_policy.cpp b/src/lib/tls/tls_text_policy.cpp
index 97a6da31a..28783a430 100644
--- a/src/lib/tls/tls_text_policy.cpp
+++ b/src/lib/tls/tls_text_policy.cpp
@@ -40,11 +40,6 @@ std::vector<std::string> Text_Policy::allowed_signature_methods() const
    return get_list("signature_methods", Policy::allowed_signature_methods());
    }
 
-std::vector<std::string> Text_Policy::allowed_ecc_curves() const
-   {
-   return get_list("ecc_curves", Policy::allowed_ecc_curves());
-   }
-
 bool Text_Policy::use_ecc_point_compression() const
    {
    return get_bool("use_ecc_point_compression", Policy::use_ecc_point_compression());
@@ -89,6 +84,7 @@ bool Text_Policy::allow_client_initiated_renegotiation() const
    {
    return get_bool("allow_client_initiated_renegotiation", Policy::allow_client_initiated_renegotiation());
    }
+
 bool Text_Policy::allow_server_initiated_renegotiation() const
    {
    return get_bool("allow_server_initiated_renegotiation", Policy::allow_server_initiated_renegotiation());
@@ -109,14 +105,36 @@ bool Text_Policy::support_cert_status_message() const
    return get_bool("support_cert_status_message", Policy::support_cert_status_message());
    }
 
-std::string Text_Policy::dh_group() const
+std::vector<Group_Params> Text_Policy::key_exchange_groups() const
    {
-   return get_str("dh_group", Policy::dh_group());
-   }
+   std::string group_str = get_str("key_exchange_groups");
 
-std::vector<std::string> Text_Policy::allowed_groups() const
-   {
-   return get_list("groups", Policy::allowed_groups());
+   if(group_str.empty())
+      {
+      // fall back to previously used name
+      group_str = get_str("ecc_curves");
+      }
+
+   if(group_str.empty())
+      {
+      return Policy::key_exchange_groups();
+      }
+
+   std::vector<Group_Params> groups;
+   for(std::string group_name : split_on(group_str, ' '))
+      {
+      Group_Params group_id = group_param_from_string(group_name);
+
+      if(group_id == Group_Params::NONE)
+         {
+         // TODO accept hex codes in text file
+         continue;
+         }
+
+      groups.push_back(group_id);
+      }
+
+   return groups;
    }
 
 size_t Text_Policy::minimum_ecdh_group_size() const
diff --git a/src/tests/data/tls-policy/bsi.txt b/src/tests/data/tls-policy/bsi.txt
index 9879b87f5..c62777472 100644
--- a/src/tests/data/tls-policy/bsi.txt
+++ b/src/tests/data/tls-policy/bsi.txt
@@ -1,23 +1,22 @@
-allow_tls10=false
-allow_tls11=false
-allow_tls12=true
-allow_dtls10=false
-allow_dtls12=false
+allow_tls10 = false
+allow_tls11 = false
+allow_tls12 = true
+allow_dtls10 = false
+allow_dtls12 = false
 
-ciphers=AES-256/GCM AES-128/GCM AES-256 AES-128
-signature_hashes=SHA-384 SHA-256
-macs=AEAD SHA-384 SHA-256
-key_exchange_methods=ECDH DH PSK ECDHE_PSK DHE_PSK 
-signature_methods=ECDSA RSA DSA
-ecc_curves=brainpool512r1 brainpool384r1 brainpool256r1 secp384r1 secp256r1
-groups=brainpool512r1 brainpool384r1 brainpool256r1 secp384r1 secp256r1 ffdhe/ietf/8192 ffdhe/ietf/6144 ffdhe/ietf/4096 ffdhe/ietf/3072 ffdhe/ietf/2048
-minimum_dh_group_size=2000
-minimum_dsa_group_size=2000
-minimum_ecdh_group_size=250
-minimum_ecdsa_group_size=250
-minimum_rsa_bits=2000
+ciphers = AES-256/GCM AES-128/GCM AES-256 AES-128
+signature_hashes = SHA-384 SHA-256
+macs = AEAD SHA-384 SHA-256
+key_exchange_methods = ECDH DH PSK ECDHE_PSK DHE_PSK
+signature_methods = ECDSA RSA DSA
+key_exchange_groups = brainpool512r1 brainpool384r1 brainpool256r1 secp384r1 secp256r1 ffdhe/ietf/8192 ffdhe/ietf/6144 ffdhe/ietf/4096 ffdhe/ietf/3072 ffdhe/ietf/2048
+minimum_dh_group_size = 2000
+minimum_dsa_group_size = 2000
+minimum_ecdh_group_size = 250
+minimum_ecdsa_group_size = 250
+minimum_rsa_bits = 2000
 
-allow_insecure_renegotiation=false
-allow_server_initiated_renegotiation=true
-server_uses_own_ciphersuite_preferences=true
-negotiate_encrypt_then_mac=true
+allow_insecure_renegotiation = false
+allow_server_initiated_renegotiation = true
+server_uses_own_ciphersuite_preferences = true
+negotiate_encrypt_then_mac = true
diff --git a/src/tests/data/tls-policy/compat.txt b/src/tests/data/tls-policy/compat.txt
index 473453ab0..cd419e91a 100644
--- a/src/tests/data/tls-policy/compat.txt
+++ b/src/tests/data/tls-policy/compat.txt
@@ -14,7 +14,7 @@ macs = AEAD SHA-256 SHA-384 SHA-1
 signature_hashes = SHA-512 SHA-384 SHA-256 SHA-1
 signature_methods = ECDSA RSA IMPLICIT
 key_exchange_methods = CECPQ1 ECDH DH RSA
-ecc_curves = x25519 secp256r1 secp521r1 secp384r1 brainpool256r1 brainpool384r1 brainpool512r1
+key_exchange_groups = x25519 secp256r1 secp521r1 secp384r1 brainpool256r1 brainpool384r1 brainpool512r1 ffdhe/ietf/2048
 allow_insecure_renegotiation = false
 include_time_in_hello_random = true
 allow_client_initiated_renegotiation = true
@@ -23,7 +23,6 @@ hide_unknown_users = false
 server_uses_own_ciphersuite_preferences = true
 negotiate_encrypt_then_mac = true
 session_ticket_lifetime = 86400
-dh_group = modp/ietf/1024
 minimum_dh_group_size = 1024
 minimum_ecdh_group_size = 255
 minimum_rsa_bits = 1024
diff --git a/src/tests/data/tls-policy/datagram.txt b/src/tests/data/tls-policy/datagram.txt
index 6a9819aff..d6071a906 100644
--- a/src/tests/data/tls-policy/datagram.txt
+++ b/src/tests/data/tls-policy/datagram.txt
@@ -8,7 +8,7 @@ macs = AEAD
 signature_hashes = SHA-512 SHA-384 SHA-256
 signature_methods = ECDSA RSA
 key_exchange_methods = CECPQ1 ECDH DH
-ecc_curves = x25519 secp256r1 secp521r1 secp384r1 brainpool256r1 brainpool384r1 brainpool512r1
+key_exchange_groups = x25519 secp256r1 secp521r1 secp384r1 brainpool256r1 brainpool384r1 brainpool512r1 ffdhe/ietf/2048 ffdhe/ietf/3072 ffdhe/ietf/4096 ffdhe/ietf/6144 ffdhe/ietf/8192
 allow_insecure_renegotiation = false
 include_time_in_hello_random = true
 allow_server_initiated_renegotiation = false
diff --git a/src/tests/data/tls-policy/default.txt b/src/tests/data/tls-policy/default.txt
index c96f91d96..0cf3dbbf8 100644
--- a/src/tests/data/tls-policy/default.txt
+++ b/src/tests/data/tls-policy/default.txt
@@ -8,7 +8,7 @@ macs = AEAD SHA-256 SHA-384 SHA-1
 signature_hashes = SHA-512 SHA-384 SHA-256
 signature_methods = ECDSA RSA
 key_exchange_methods = CECPQ1 ECDH DH
-ecc_curves = x25519 secp256r1 secp521r1 secp384r1 brainpool256r1 brainpool384r1 brainpool512r1
+key_exchange_groups = x25519 secp256r1 secp521r1 secp384r1 brainpool256r1 brainpool384r1 brainpool512r1 ffdhe/ietf/2048 ffdhe/ietf/3072 ffdhe/ietf/4096 ffdhe/ietf/6144 ffdhe/ietf/8192
 allow_insecure_renegotiation = false
 include_time_in_hello_random = true
 allow_server_initiated_renegotiation = false
@@ -16,7 +16,6 @@ hide_unknown_users = false
 server_uses_own_ciphersuite_preferences = true
 negotiate_encrypt_then_mac = true
 session_ticket_lifetime = 86400
-dh_group = modp/ietf/2048
 minimum_dh_group_size = 2048
 minimum_ecdh_group_size = 255
 minimum_rsa_bits = 2048
diff --git a/src/tests/data/tls-policy/strict.txt b/src/tests/data/tls-policy/strict.txt
index f59aaf271..7cb55bb83 100644
--- a/src/tests/data/tls-policy/strict.txt
+++ b/src/tests/data/tls-policy/strict.txt
@@ -8,7 +8,7 @@ macs = AEAD
 signature_hashes = SHA-512 SHA-384
 signature_methods = ECDSA RSA
 key_exchange_methods = CECPQ1 ECDH
-ecc_curves = x25519 secp256r1 secp521r1 secp384r1 brainpool256r1 brainpool384r1 brainpool512r1
+key_exchange_groups = x25519 secp256r1 secp521r1 secp384r1 brainpool256r1 brainpool384r1 brainpool512r1 ffdhe/ietf/2048 ffdhe/ietf/3072 ffdhe/ietf/4096 ffdhe/ietf/6144 ffdhe/ietf/8192
 allow_insecure_renegotiation = false
 include_time_in_hello_random = true
 allow_server_initiated_renegotiation = false
@@ -16,7 +16,6 @@ hide_unknown_users = false
 server_uses_own_ciphersuite_preferences = true
 negotiate_encrypt_then_mac = true
 session_ticket_lifetime = 86400
-dh_group = modp/ietf/2048
 minimum_dh_group_size = 2048
 minimum_ecdh_group_size = 255
 minimum_rsa_bits = 2048
diff --git a/src/tests/data/tls-policy/suiteb.txt b/src/tests/data/tls-policy/suiteb.txt
index 7c0b3e7d8..90ef68f4a 100644
--- a/src/tests/data/tls-policy/suiteb.txt
+++ b/src/tests/data/tls-policy/suiteb.txt
@@ -8,7 +8,7 @@ macs = AEAD
 signature_hashes = SHA-256
 signature_methods = ECDSA
 key_exchange_methods = ECDH
-ecc_curves = secp256r1
+key_exchange_groups = secp256r1
 allow_insecure_renegotiation = false
 include_time_in_hello_random = true
 allow_server_initiated_renegotiation = false
@@ -16,8 +16,6 @@ hide_unknown_users = false
 server_uses_own_ciphersuite_preferences = true
 negotiate_encrypt_then_mac = true
 session_ticket_lifetime = 86400
-dh_group = modp/ietf/2048
-groups = secp256r1
 minimum_dh_group_size = 2048
 minimum_ecdh_group_size = 255
 minimum_rsa_bits = 2048
diff --git a/src/tests/test_tls.cpp b/src/tests/test_tls.cpp
index 4762653ac..728c13735 100644
--- a/src/tests/test_tls.cpp
+++ b/src/tests/test_tls.cpp
@@ -108,7 +108,7 @@ class Test_TLS_Alert_Strings : public Test
 
 BOTAN_REGISTER_TEST("tls_alert_strings", Test_TLS_Alert_Strings);
 
-class Test_TLS_Policy_Test : public Test
+class Test_TLS_Policy_Text : public Test
    {
    public:
       std::vector<Test::Result> run() override
@@ -119,9 +119,10 @@ class Test_TLS_Policy_Test : public Test
 
          for(std::string policy : policies)
             {
-            result.test_eq("Values for TLS " + policy + " policy",
-                           tls_policy_string(policy),
-                           read_tls_policy(policy));
+            const std::string from_policy_obj = tls_policy_string(policy);
+            const std::string from_file = read_tls_policy(policy);
+
+            result.test_eq("Values for TLS " + policy + " policy", from_file, from_policy_obj);
             }
 
          return {result};
@@ -174,7 +175,7 @@ class Test_TLS_Policy_Test : public Test
          }
    };
 
-BOTAN_REGISTER_TEST("tls_policy_test", Test_TLS_Policy_Test);
+BOTAN_REGISTER_TEST("tls_policy_text", Test_TLS_Policy_Text);
 
 class Test_TLS_Ciphersuites : public Test
    {
diff --git a/src/tests/unit_tls.cpp b/src/tests/unit_tls.cpp
index 0a5739c3d..186822d2f 100644
--- a/src/tests/unit_tls.cpp
+++ b/src/tests/unit_tls.cpp
@@ -686,13 +686,20 @@ class TLS_Unit_Tests final : public Test
          {
          Botan::RandomNumberGenerator& rng = Test::rng();
 
-         for(auto const& version : versions)
+         try
             {
-            TLS_Handshake_Test test(
-               version.to_string() + " " + test_descr,
-               version, creds, policy, policy, rng, client_ses, server_ses, client_auth);
-            test.go();
-            results.push_back(test.results());
+            for(auto const& version : versions)
+               {
+               TLS_Handshake_Test test(
+                  version.to_string() + " " + test_descr,
+                  version, creds, policy, policy, rng, client_ses, server_ses, client_auth);
+               test.go();
+               results.push_back(test.results());
+               }
+            }
+         catch(std::exception& e)
+            {
+            results.push_back(Test::Result::Failure(test_descr, e.what()));
             }
          }