Hi Paul,

As discussed, I have completed my initial review of your draft. The main
purpose is to ensure that the document is in good condition to go to
external reviewers.

Have a look at my comments and please feel free to debate any of them with
me.

I think we'll need a new revision before progressing the draft any further.

Best,
Adrian
===

Title

Can you please expand "SM"

---

Abstract

Can you please expand "SM"

---

Abstract

Please add a second paragraph to help understand the purpose, intent,
and status of the work. Something like...

  The use of these cipher suites with TLS 1.3 is not endorsed or
  recommended by the IETF.  This document provides a description of
  how to use the SM cypher suites with TLS 1.3 so that implementers
  may produce interworking implementations.

We can negotiate these words if you like, in particular to strengthen
the reason why this document is published.

---

Global change s/draft/document/ except in the boilerplate text.

---

Can you add a similar additional paragraph (as added to the Abstract) to
the end of section 1 (before section 1.1).

---

Section 1

OLD
  This document describes two new cipher suites for the Transport Layer
  Security (TLS) protocol version 1.3 (a.k.a TLSv1.3, [RFC8446]).  The
  new cipher suites are listed as follows (or Section 2):
NEW
  This document describes two new cipher suites for the Transport Layer
  Security (TLS) protocol version 1.3 (TLSv1.3, [RFC8446]).  The
  new cipher suites are as follows (see also Section 2):
END

---

Section 1

s/cipher suites contains/cipher suites contain/

s/For the more/For a more/

s/meet the need of/meet the needs of/

s/encryption key and protect/encryption keys and protect/

---

Please expand abbreviations on first use. I see:

AEAD
CCM
ECDHE
GCM
SM
SM2
SM3
SM4

---

1.2

Please use the new boilerplate text and add your modifications as
follows:

  The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
  "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and
  "OPTIONAL" in this document are to be interpreted as described in BCP
  14 [RFC2119] [RFC8174] when, and only when, they appear in all
  capitals, as shown here, and indicate requirement levels for
  compliant TLSv1.3 implementations.

You will need to add a reference for RFC 8174.

---

Section 2. Title

OLD
2.  Proposed Cipher Suites
NEW
2.  Supported Cipher Suites
END

---

3.1

OLD
  The only capable version for the new cipher suites defined in this
  document is TLSv1.3.  Implementations of this document MUST NOT apply
  these cipher suites into any TLS protocols that have an older version
  than 1.3.
NEW
  The new cpher suites defined in this document are only applicable to
  TLSv1.3.  Implementations of this document MUST NOT apply these
  cipher suites to any older versions of TLS.
END

---

3.2.1

s/use SM2 signature/use the SM2 signature/

s/SM2 signature is defined/The SM2 signature is defined/

---

3.2.1

  In general, SM2 is a
  signature algorithm based on elliptic curves.

Why "in general"?

Maybe just write...

  SM2 is a
  signature algorithm based on elliptic curves.

---

3.2.1

OLD
  This curve has the name curveSM2 and IANA is
  requested to assign a value for it.
NEW
  This curve is named curveSM2 and has been assigned the value 41 as
  shown in Section 4.
END

---

3.2.1

OLD
  Unlike other elliptic curve
  based public key algorithm like ECDSA, SM2 cannot select other
  elliptic curves in practice, but it's allowed to write test cases by
  using other elliptic curve parameter sets for SM2, take Annex F.14 of
  [ISO-SM2] as a reference.
NEW
  Unlike other elliptic curve
  based public key algorithms like ECDSA, SM2 MUST NOT select other
  elliptic curves.  But it is acceptable to write test cases that use
  other elliptic curve parameter sets for SM2, take Annex F.14 of
  [ISO-SM2] as a reference.
END

---

3.2.1

s/SM2 signature algorithm requests/The SM2 signature algorithm requests/

s/when generate/when generating/

---

3.3.1.1.

s/is REQUIRED to include/MUST include/

---

3.3.1.2

  If a TLSv1.3 server receives a ClientHello message containing the new
  cipher suites defined in this document, it MAY choose to use the new
  cipher suites.  If so, then the server MUST put one of the new cipher
  suites defined in this document into its ServerHello's
  'cipher_suites' array and eventually sends it to the client side.

This is all fine, but since you have "MAY" can you also please include
some text to explain:
- how the server might choose to use or not use the cipher suites
- what would happen if it chose to not use the cipher suites

---

3.3.2

s/authentication purpose/authentication purposes/

---

3.3.3

s/When server sends/When a server sends/

---

3.3.4

OLD
signature algorithm MUST be SM2 signature algorithm.
NEW
signature algorithm MUST be SM2.
END

---

3.4

  Implementations of this
  document SHOULD always conform to what TLSv1.3 [RFC8446] and its
  successors require about the key derivation and related methods.

This use of "SHOULD" worries me!
I would prefer that you use "MUST", but if you really want "SHOULD" you
need to explain how/why an implementation might vary from the IETF
standards track documents.

---

3.5.1

s/and plaintext/plaintext/

s/authentication tag conformed/authentication tag conforming/

---

3.5.1

  which in details SHOULD be
  constructed by the TLS record header.

Again, I am worried by "SHOULD".
Either change to "MUST" or supply some description of the variance.

---

3.5.2

s/An authentication tag conformed/An authentication tag conforming/

---

There is an error in the IANA section:

      +--------+-------------+---------+-------------+-----------+
      |  Value | Description | DTLS-OK | Recommended | Reference |
      +--------+-------------+---------+-------------+-----------+
      | 0x0708 | sm2sig_sm3  | No      | No          | this RFC  |
      +--------+-------------+---------+-------------+-----------+

There is no "DTLS-OK" column in this registry.

---

5.

s/_MUST NOT_/MUST NOT/

---

You can delete Appendix B, I think.



-- 
Adrian Farrel (ISE),
[email protected]