public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "yi1 li" <yi1.li@intel.com>
To: "Yao, Jiewen" <jiewen.yao@intel.com>,
	"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>,
	"Gao, Liming" <gaoliming@byosoft.com.cn>
Subject: Re: [edk2-devel] [PATCH 1/1] MdePkg: Add WPA3 related TLS configure macro
Date: Wed, 4 May 2022 11:32:06 +0000	[thread overview]
Message-ID: <MWHPR11MB159782867F74DE13F4C8FC49C5C39@MWHPR11MB1597.namprd11.prod.outlook.com> (raw)
In-Reply-To: <MW4PR11MB58722A2FF0721A8C921C50648CC39@MW4PR11MB5872.namprd11.prod.outlook.com>

Hi Jiewen,

Thanks for feedback, I will check it.
For 7), I will submit relevant TLS function code together next patch.

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com> 
Sent: Wednesday, May 4, 2022 6:13 PM
To: devel@edk2.groups.io; Li, Yi1 <yi1.li@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Subject: RE: [edk2-devel] [PATCH 1/1] MdePkg: Add WPA3 related TLS configure macro

Thanks Yi.

Some feedback:

1) {0x13, *} is defined in TLS1.3 - https://datatracker.ietf.org/doc/html/rfc8446#appendix-B.4
The comment ">  /// TLS Cipher Suite, refers to A.5 of rfc-2246, rfc-4346 and rfc-5246." should be updated to include 8446 as well.

2) Although it is not absolutely required, I highly recommend to add specific value to TLS_HASH_ALGO, to align with definition in RFC.
> +  TlsHashAlgoNone = 0,
> +  TlsHashAlgoMd5 = 1,
> +  TlsHashAlgoSha1 = 2,
> +  TlsHashAlgoSha224 = 3,
> +  TlsHashAlgoSha256 = 4,
> +  TlsHashAlgoSha384 = 5,
> +  TlsHashAlgoSha512 = 6,
> +} TLS_HASH_ALGO;

3) Ditto, for TLS_SIGNATURE_ALGO.

> +  TlsSignatureAlgoAnonymous = 0,
> +  TlsSignatureAlgoRsa = 1,
> +  TlsSignatureAlgoDsa = 2,
> +  TlsSignatureAlgoEcdsa = 3,
> +} TLS_SIGNATURE_ALGO;

The value is assigned in the spec. It cannot be changed.

4) RFC4492 is obsoleted by RFC8422 - https://datatracker.ietf.org/doc/html/rfc8422#section-5.1.1

=================
   RFC 4492 defined 25 different curves in the NamedCurve registry (now
   renamed the "TLS Supported Groups" registry, although the enumeration
   below is still named NamedCurve) for use in TLS.  Only three have
   seen much use.  This specification is deprecating the rest (with
   numbers 1-22).
=================

I don't see a reason to define so many deprecated algorithms.
Would you please align with section 5.1.1 in RFC8422? You may consider to add x25519 and x448 as well.

============
           enum {
               deprecated(1..22),
               secp256r1 (23), secp384r1 (24), secp521r1 (25),
               x25519(29), x448(30),
               reserved (0xFE00..0xFEFF),
               deprecated(0xFF01..0xFF02),
               (0xFFFF)
           } NamedCurve;
============

5) Since you added TLS 1.3 cipher suit, I assume you also want to add definition for TLS 1.3.

Please aware that "signature_algorithms" is changed in TLS 1.3 - https://datatracker.ietf.org/doc/html/rfc8446#section-4.2.3.
I am not sure if you need define that as well.

============
      enum {
          /* RSASSA-PKCS1-v1_5 algorithms */
          rsa_pkcs1_sha256(0x0401),
          rsa_pkcs1_sha384(0x0501),
          rsa_pkcs1_sha512(0x0601),

          /* ECDSA algorithms */
          ecdsa_secp256r1_sha256(0x0403),
          ecdsa_secp384r1_sha384(0x0503),
          ecdsa_secp521r1_sha512(0x0603),

          /* RSASSA-PSS algorithms with public key OID rsaEncryption */
          rsa_pss_rsae_sha256(0x0804),
          rsa_pss_rsae_sha384(0x0805),
          rsa_pss_rsae_sha512(0x0806),

          /* EdDSA algorithms */
          ed25519(0x0807),
          ed448(0x0808),

          /* RSASSA-PSS algorithms with public key OID RSASSA-PSS */
          rsa_pss_pss_sha256(0x0809),
          rsa_pss_pss_sha384(0x080a),
          rsa_pss_pss_sha512(0x080b),
...
      } SignatureScheme;
============

6) Ditto. Please aware that "NamedCurve" is changed in TLS 1.3 - https://datatracker.ietf.org/doc/html/rfc8446#section-4.2.7
I am not sure if you need define that as well.

============
      enum {

          /* Elliptic Curve Groups (ECDHE) */
          secp256r1(0x0017), secp384r1(0x0018), secp521r1(0x0019),
          x25519(0x001D), x448(0x001E),
...
      } NamedGroup;
============

7) Last but not least, I hope to see how those new definition is used.
Without consumer, it is hard for me to understand why they are needed, or if we miss something else.


Thank you
Yao, Jiewen


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of yi1 li
> Sent: Wednesday, May 4, 2022 5:31 PM
> To: devel@edk2.groups.io
> Cc: Li, Yi1 <yi1.li@intel.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
> Subject: [edk2-devel] [PATCH 1/1] MdePkg: Add WPA3 related TLS 
> configure macro
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3892
> 
> Which are needed for SUITE-B and SUITE-B-192.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: yi1 li <yi1.li@intel.com>
> ---
>  MdePkg/Include/IndustryStandard/Tls1.h | 133 
> ++++++++++++++++++-------
>  1 file changed, 97 insertions(+), 36 deletions(-)
> 
> diff --git a/MdePkg/Include/IndustryStandard/Tls1.h
> b/MdePkg/Include/IndustryStandard/Tls1.h
> index cf67428b1129..6519afe15e78 100644
> --- a/MdePkg/Include/IndustryStandard/Tls1.h
> +++ b/MdePkg/Include/IndustryStandard/Tls1.h
> @@ -15,42 +15,49 @@
>  ///
>  /// TLS Cipher Suite, refers to A.5 of rfc-2246, rfc-4346 and rfc-5246.
>  ///
> -#define TLS_RSA_WITH_NULL_MD5                {0x00, 0x01}
> -#define TLS_RSA_WITH_NULL_SHA                {0x00, 0x02}
> -#define TLS_RSA_WITH_RC4_128_MD5             {0x00, 0x04}
> -#define TLS_RSA_WITH_RC4_128_SHA             {0x00, 0x05}
> -#define TLS_RSA_WITH_IDEA_CBC_SHA            {0x00, 0x07}
> -#define TLS_RSA_WITH_DES_CBC_SHA             {0x00, 0x09}
> -#define TLS_RSA_WITH_3DES_EDE_CBC_SHA        {0x00, 0x0A}
> -#define TLS_DH_DSS_WITH_DES_CBC_SHA          {0x00, 0x0C}
> -#define TLS_DH_DSS_WITH_3DES_EDE_CBC_SHA     {0x00, 0x0D}
> -#define TLS_DH_RSA_WITH_DES_CBC_SHA          {0x00, 0x0F}
> -#define TLS_DH_RSA_WITH_3DES_EDE_CBC_SHA     {0x00, 0x10}
> -#define TLS_DHE_DSS_WITH_DES_CBC_SHA         {0x00, 0x12}
> -#define TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA    {0x00, 0x13}
> -#define TLS_DHE_RSA_WITH_DES_CBC_SHA         {0x00, 0x15}
> -#define TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA    {0x00, 0x16}
> -#define TLS_RSA_WITH_AES_128_CBC_SHA         {0x00, 0x2F}
> -#define TLS_DH_DSS_WITH_AES_128_CBC_SHA      {0x00, 0x30}
> -#define TLS_DH_RSA_WITH_AES_128_CBC_SHA      {0x00, 0x31}
> -#define TLS_DHE_DSS_WITH_AES_128_CBC_SHA     {0x00, 0x32}
> -#define TLS_DHE_RSA_WITH_AES_128_CBC_SHA     {0x00, 0x33}
> -#define TLS_RSA_WITH_AES_256_CBC_SHA         {0x00, 0x35}
> -#define TLS_DH_DSS_WITH_AES_256_CBC_SHA      {0x00, 0x36}
> -#define TLS_DH_RSA_WITH_AES_256_CBC_SHA      {0x00, 0x37}
> -#define TLS_DHE_DSS_WITH_AES_256_CBC_SHA     {0x00, 0x38}
> -#define TLS_DHE_RSA_WITH_AES_256_CBC_SHA     {0x00, 0x39}
> -#define TLS_RSA_WITH_NULL_SHA256             {0x00, 0x3B}
> -#define TLS_RSA_WITH_AES_128_CBC_SHA256      {0x00, 0x3C}
> -#define TLS_RSA_WITH_AES_256_CBC_SHA256      {0x00, 0x3D}
> -#define TLS_DH_DSS_WITH_AES_128_CBC_SHA256   {0x00, 0x3E}
> -#define TLS_DH_RSA_WITH_AES_128_CBC_SHA256   {0x00, 0x3F}
> -#define TLS_DHE_DSS_WITH_AES_128_CBC_SHA256  {0x00, 0x40} -#define 
> TLS_DHE_RSA_WITH_AES_128_CBC_SHA256  {0x00, 0x67}
> -#define TLS_DH_DSS_WITH_AES_256_CBC_SHA256   {0x00, 0x68}
> -#define TLS_DH_RSA_WITH_AES_256_CBC_SHA256   {0x00, 0x69}
> -#define TLS_DHE_DSS_WITH_AES_256_CBC_SHA256  {0x00, 0x6A} -#define 
> TLS_DHE_RSA_WITH_AES_256_CBC_SHA256  {0x00, 0x6B}
> +#define TLS_RSA_WITH_NULL_MD5                  {0x00, 0x01}
> +#define TLS_RSA_WITH_NULL_SHA                  {0x00, 0x02}
> +#define TLS_RSA_WITH_RC4_128_MD5               {0x00, 0x04}
> +#define TLS_RSA_WITH_RC4_128_SHA               {0x00, 0x05}
> +#define TLS_RSA_WITH_IDEA_CBC_SHA              {0x00, 0x07}
> +#define TLS_RSA_WITH_DES_CBC_SHA               {0x00, 0x09}
> +#define TLS_RSA_WITH_3DES_EDE_CBC_SHA          {0x00, 0x0A}
> +#define TLS_DH_DSS_WITH_DES_CBC_SHA            {0x00, 0x0C}
> +#define TLS_DH_DSS_WITH_3DES_EDE_CBC_SHA       {0x00, 0x0D}
> +#define TLS_DH_RSA_WITH_DES_CBC_SHA            {0x00, 0x0F}
> +#define TLS_DH_RSA_WITH_3DES_EDE_CBC_SHA       {0x00, 0x10}
> +#define TLS_DHE_DSS_WITH_DES_CBC_SHA           {0x00, 0x12}
> +#define TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA      {0x00, 0x13}
> +#define TLS_DHE_RSA_WITH_DES_CBC_SHA           {0x00, 0x15}
> +#define TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA      {0x00, 0x16}
> +#define TLS_RSA_WITH_AES_128_CBC_SHA           {0x00, 0x2F}
> +#define TLS_DH_DSS_WITH_AES_128_CBC_SHA        {0x00, 0x30}
> +#define TLS_DH_RSA_WITH_AES_128_CBC_SHA        {0x00, 0x31}
> +#define TLS_DHE_DSS_WITH_AES_128_CBC_SHA       {0x00, 0x32}
> +#define TLS_DHE_RSA_WITH_AES_128_CBC_SHA       {0x00, 0x33}
> +#define TLS_RSA_WITH_AES_256_CBC_SHA           {0x00, 0x35}
> +#define TLS_DH_DSS_WITH_AES_256_CBC_SHA        {0x00, 0x36}
> +#define TLS_DH_RSA_WITH_AES_256_CBC_SHA        {0x00, 0x37}
> +#define TLS_DHE_DSS_WITH_AES_256_CBC_SHA       {0x00, 0x38}
> +#define TLS_DHE_RSA_WITH_AES_256_CBC_SHA       {0x00, 0x39}
> +#define TLS_RSA_WITH_NULL_SHA256               {0x00, 0x3B}
> +#define TLS_RSA_WITH_AES_128_CBC_SHA256        {0x00, 0x3C}
> +#define TLS_RSA_WITH_AES_256_CBC_SHA256        {0x00, 0x3D}
> +#define TLS_DH_DSS_WITH_AES_128_CBC_SHA256     {0x00, 0x3E}
> +#define TLS_DH_RSA_WITH_AES_128_CBC_SHA256     {0x00, 0x3F}
> +#define TLS_DHE_DSS_WITH_AES_128_CBC_SHA256    {0x00, 0x40}
> +#define TLS_DHE_RSA_WITH_AES_128_CBC_SHA256    {0x00, 0x67}
> +#define TLS_DH_DSS_WITH_AES_256_CBC_SHA256     {0x00, 0x68}
> +#define TLS_DH_RSA_WITH_AES_256_CBC_SHA256     {0x00, 0x69}
> +#define TLS_DHE_DSS_WITH_AES_256_CBC_SHA256    {0x00, 0x6A}
> +#define TLS_DHE_RSA_WITH_AES_256_CBC_SHA256    {0x00, 0x6B}
> +#define TLS_DHE_RSA_WITH_AES_256_GCM_SHA384    {0x00, 0x9F}
> +#define TLS_AES_128_GCM_SHA256                 {0x13, 0x01}
> +#define TLS_AES_256_GCM_SHA384                 {0x13, 0x02}
> +#define TLS_CHACHA20_POLY1305_SHA256           {0x13, 0x03}
> +#define TLS_ECDHE_ECDSA_AES128_GCM_SHA256      {0xC0, 0x2B}
> +#define TLS_ECDHE_ECDSA_AES256_GCM_SHA384      {0xC0, 0x2C}
> +#define TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384  {0xC0, 0x30}
> 
>  ///
>  /// TLS Version, refers to A.1 of rfc-2246, rfc-4346 and rfc-5246.
> @@ -95,6 +102,60 @@ typedef struct {
>  //
>  #define TLS_CIPHERTEXT_RECORD_MAX_PAYLOAD_LENGTH  18432
> 
> +///
> +/// TLS Hash algorithm, refers to section 7.4.1.4.1. of rfc-5246.
> +///
> +typedef enum {
> +  TlsHashAlgoNone = 0,
> +  TlsHashAlgoMd5,
> +  TlsHashAlgoSha1,
> +  TlsHashAlgoSha224,
> +  TlsHashAlgoSha256,
> +  TlsHashAlgoSha384,
> +  TlsHashAlgoSha512,
> +} TLS_HASH_ALGO;
> +
> +///
> +/// TLS Signature algorithm, refers to section 7.4.1.4.1. of rfc-5246.
> +///
> +typedef enum {
> +  TlsSignatureAlgoAnonymous = 0,
> +  TlsSignatureAlgoRsa,
> +  TlsSignatureAlgoDsa,
> +  TlsSignatureAlgoEcdsa,
> +} TLS_SIGNATURE_ALGO;
> +
> +///
> +/// TLS Supported Elliptic Curves Extensions, refers to section 5.1.1 
> +of rfc-4492 /// typedef enum {
> +  TlsEcNamedCurve_sect163k1 = 1,
> +  TlsEcNamedCurve_sect163r1,   // 2,
> +  TlsEcNamedCurve_sect163r2,   // 3,
> +  TlsEcNamedCurve_sect193r1,   // 4,
> +  TlsEcNamedCurve_sect193r2,   // 5,
> +  TlsEcNamedCurve_sect233k1,   // 6,
> +  TlsEcNamedCurve_sect233r1,   // 7,
> +  TlsEcNamedCurve_sect239k1,   // 8,
> +  TlsEcNamedCurve_sect283k1,   // 9,
> +  TlsEcNamedCurve_sect283r1,   // 10,
> +  TlsEcNamedCurve_sect409k1,   // 11,
> +  TlsEcNamedCurve_sect409r1,   // 12,
> +  TlsEcNamedCurve_sect571k1,   // 13,
> +  TlsEcNamedCurve_sect571r1,   // 14,
> +  TlsEcNamedCurve_secp160k1,   // 15,
> +  TlsEcNamedCurve_secp160r1,   // 16,
> +  TlsEcNamedCurve_secp160r2,   // 17,
> +  TlsEcNamedCurve_secp192k1,   // 18,
> +  TlsEcNamedCurve_secp192r1,   // 19,
> +  TlsEcNamedCurve_secp224k1,   // 20,
> +  TlsEcNamedCurve_secp224r1,   // 21,
> +  TlsEcNamedCurve_secp256k1,   // 22,
> +  TlsEcNamedCurve_secp256r1,   // 23,
> +  TlsEcNamedCurve_secp384r1,   // 24,
> +  TlsEcNamedCurve_secp521r1,   // 25,
> +} TLS_EC_NAMED_CUREVE;
> +
>  #pragma pack()
> 
>  #endif
> --
> 2.31.1.windows.1
> 
> 
> 
> 
> 


  reply	other threads:[~2022-05-04 11:32 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <cover.1651656533.git.yi1.li@intel.com>
2022-05-04  9:30 ` [PATCH 1/1] MdePkg: Add WPA3 related TLS configure macro yi1 li
2022-05-04 10:12   ` [edk2-devel] " Yao, Jiewen
2022-05-04 11:32     ` yi1 li [this message]
2022-05-06  7:12     ` yi1 li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MWHPR11MB159782867F74DE13F4C8FC49C5C39@MWHPR11MB1597.namprd11.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox