Subject: Re: [lamps] WG Last Call for draft-ietf-lamps-rfc7030est-clarify-02

From: Alexey Melnikov <[email-masked]>
Date: Wed, 1 Apr 2020 12:53:58 +0100

Hi,

On 30/03/2020 17:11, Russ Housley wrote:
> This is the LAMPS WG Last Call for "Clarification of Enrollment over Secure Transport (EST): transfer encodings and ASN.1” <draft-ietf-lamps-rfc7030est-clarify-02>.  Please review the document and send your comments to the list by 19 April 2020.  This is longer than usual to accommodate the vast number of virtual interim sessions that are taking place right now.
>
> The datatracker page for the document is https://datatracker.ietf.org/doc/draft-ietf-lamps-rfc7030est-clarify/

I reviewed the document and it is basically ready, modulo a few minor 
things:

5.1.  CSR Attributes Response

    Responses to attribute request messages MUST be encoded as the
    content-type of "application/csrattrs", and are to be "base64"
    [RFC2045] encoded.

I think you should use "[RFC4648] section 4 Base64 encoded" here the 
same way you already do in Section 4.

I would also ask you to add some text that this is a deviation from HTTP 
1.1 [RFC7231] definition needed for backward compatibility.

6.2.  Updating section 4.4.2: Server-Side Key Generation Response

    Replace:

        If the content-type is not set, the response data MUST be a
        plaintext human-readable error message.

    with:

        If the content-type is not set, the response data must be a
        plaintext human-readable error message.
        Servers MAY use the "text/plain" content-type [RFC2046]
        for human-readable errors.

While the new text is not wrong, may I suggest rewording it to something 
like:

        If the content-type is not set or explicitly set to 
"text/plain", the response data must be a
        plaintext human-readable error message.

       Note that [RFC2046] states that lack of content-type is treated 
the same as Content-Type: "text/plain".

Basically I am trying to point out that lack of Content-Type is the same 
as "text/plain" as specified in RFC 2046, as opposed to something 
invented by this document.

/*
*/

Best Regards,

Alexey

---

From: "Panos Kampanakis (pkampana)" <[email-masked]>
Date: Fri, 3 Apr 2020 15:25:47 +0000

Hi Michael,

It is ready imo as well. Two more nits:

- CTE and CTE-base64 are not used in the document, so I think they can be removed. 

- Make sure you add “Updates: 7030” in the header. 

Thanks,

Panos

From: Spasm <[email-masked]> On Behalf Of Alexey Melnikov
Sent: Wednesday, April 01, 2020 7:54 AM
To: Russ Housley <[email-masked]>; LAMPS WG <[email-masked]>
Subject: Re: [lamps] WG Last Call for draft-ietf-lamps-rfc7030est-clarify-02

Hi,

 

I reviewed the document and it is basically ready, modulo a few minor things:

I think you should use "[RFC4648] section 4 Base64 encoded" here the same way you already do in Section 4.

I would also ask you to add some text that this is a deviation from HTTP 1.1 [RFC7231] definition needed for backward compatibility.

   Replace:

   with:

While the new text is not wrong, may I suggest rewording it to something like:

       If the content-type is not set or explicitly set to "text/plain", the response data must be a
       plaintext human-readable error message.

      Note that [RFC2046] states that lack of content-type is treated the same as Content-Type: "text/plain".

Basically I am trying to point out that lack of Content-Type is the same as "text/plain" as specified in RFC 2046, as opposed to something invented by this document.

Best Regards,

Alexey

---

From: Russ Housley <[email-masked]>
Date: Tue, 21 Apr 2020 17:16:10 -0400

> The datatracker page for the document is https://datatracker.ietf.org/doc/draft-ietf-lamps-rfc7030est-clarify/ <https://datatracker.ietf.org/doc/draft-ietf-lamps-rfc7030est-clarify/>

1) The title pages should include "Updates: 7030 (if approved)" in the upper left corner.

2) Section 1: I think it would be more clear to merge this paragraphs:

   [RFC2616] and later [RFC7231] Appendix A.5 has text specifically
   deprecating Content-Transfer-Encoding.

   [RFC7030] calls it out this header incorrectly.

I suggest:

   [RFC2616] and later [RFC7231] Appendix A.5 specifically deprecates
   Content-Transfer-Encoding.  However, [RFC7030] incorrectly uses this header.

3) Section 1 talks about HTTP-related topics in the beginning and the middle of the section.  I think it would be better to put them near each other.

4) Section 1: please add a reference for IEC 62351.

5) Please use:

   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.

6) Please add a reference for RFC 8174.

7) Section 4, 2nd para: I find the wording confusing.  I suggest:

   This document updates [RFC7030] to require the POST request and
   payload response of all endpoints use Base64 encoding as specified in
   Section 4 of [RFC4648].  In both cases, the Distinguished Encoding
   Rules (DER) [X690] are used to produce the input for the Base64
   encoding routine. This format is to be used regardless of any
   Content-Transfer-Encoding header, and any value in
   such a header MUST be ignored.

8) Section 5.1:  Since there is only one ATTRIBUTE in the document, why not reduce the complexity by:

OLD:

  AttrSet ATTRIBUTE ::= { AttributesDefinedInRFC7030, ... }

NEW:

  AttrSet ATTRIBUTE ::= { aa-asymmDecryptKeyID, ... }

Note: If you accept this change, it needs to be made in the ASN.1 module too.

9) Section 5.1:  s/crypto system/cryptographic algorithm/ (more than one place)

Russ

---

From: Russ Housley <[email-masked]>
Date: Thu, 23 Apr 2020 17:12:31 -0400

I saw several reviews.  They were mostly a desire for clarify.  Please post an update to resolve the concerns that were raised.  Then, we will ask the reviewers whether the revision addresses their concerns.

Russ

> On Mar 30, 2020, at 12:11 PM, Russ Housley <[email-masked]> wrote:
> 
> Russ & Tim

---

From: Michael Richardson <[email-masked]>
Date: Sat, 25 Apr 2020 22:55:07 -0400

> I reviewed the document and it is basically ready, modulo a few minor

Thank you.

I've fixed all the things you suggested, and I also got rid of some
text that reflected a need to make an upward compatible system.
This included fixing up the Abstract.

    > Note: If you accept this change, it needs to be made in the ASN.1

I'm not certain that I got this right.  Here is just that diff:

https://github.com/mcr/ietf-lamps-rfc7030est-clarifications/commit/25fb70760e3755d06f73adba991ef1b8d791ca88

I'm looking back in the archives to make sure that I found all the reviews,
and then I'll post the -03.

--
Michael Richardson <[email-masked]>, Sandelman Software Works
 -= IPv6 IoT consulting =-

---

From: Russ Housley <[email-masked]>
Date: Sun, 26 Apr 2020 17:14:57 -0400

> https://github.com/mcr/ietf-lamps-rfc7030est-clarifications/commit/25fb70760e3755d06f73adba991ef1b8d791ca88

Yes, that looks fine to me.

Russ

---

From: Russ Housley <[email-masked]>
Date: Mon, 27 Apr 2020 12:31:43 -0400

Thanks for making the updates.  I compiled the ASN.1 module, and the compiler caught something that I read past.  There is a missing letter "a" in the ASN.1 module.  Also the text in Section 4.1 has a third spelling.  Please make them all match ...

OLD:

  AttrSet ATTRIBUTE ::= { aa-symmDecrytKeyID, ... }

NEW:

  AttrSet ATTRIBUTE ::= { aa-asymmDecrytKeyID, ... }

Russ

> On Apr 21, 2020, at 5:16 PM, Russ Housley <[email-masked]> wrote:
> 
>>

---

From: Michael Richardson <[email-masked]>
Date: Mon, 27 Apr 2020 14:05:54 -0400

Thank you for catching that.

BEFORE:
% grep aa- rfc7030est-clarify-03.txt
   AttrSet ATTRIBUTE ::= { aa-asymDecryptKeyId, ... }
  AttrSet ATTRIBUTE ::= { aa-symmDecrytKeyID, ... }
  aa-asymmDecryptKeyID ATTRIBUTE ::=
        IDENTIFIED BY id-aa-asymmDecryptKeyID }
  id-aa-asymmDecryptKeyID OBJECT IDENTIFIER ::= { iso(1) member-body(2)

AFTER (-04):
%grep aa- rfc7030est-clarify-04.txt
   AttrSet ATTRIBUTE ::= { aa-asymmDecryptKeyId, ... }
  AttrSet ATTRIBUTE ::= { aa-asymmDecrytKeyID, ... }
  aa-asymmDecryptKeyID ATTRIBUTE ::=
        IDENTIFIED BY id-aa-asymmDecryptKeyID }
  id-aa-asymmDecryptKeyID OBJECT IDENTIFIER ::= { iso(1) member-body(2)

I think that they ALL match now.

---

From: Russ Housley <[email-masked]>
Date: Mon, 27 Apr 2020 14:37:44 -0400

Not quite.  I see two spellings of KeyID and KeyId.  RFC 7030 uses KeyID.

Russ

> On Apr 27, 2020, at 2:05 PM, Michael Richardson <[email-masked]> wrote:
> 
> Signed PGP part
>

---

From: Michael Richardson <[email-masked]>
Date: Mon, 27 Apr 2020 15:21:16 -0400

> Not quite.  I see two spellings of KeyID and KeyId.  RFC 7030 uses

cancelled my submission and trying again.

---

From: Russ Housley <[email-masked]>
Date: Thu, 30 Apr 2020 17:24:49 -0400

I have two last minute questions about some of the normative references.

(1) The Introduction states:

   [I-D.ietf-anima-bootstrapping-keyinfra] extends [RFC7030], adding new
   functionality, and interop testing of the protocol has revealed that
   unusual processing called out in [RFC7030] causes confusion.

   EST is currently specified as part of [IEC62351], and is widely used
   in Government, Utilities and Financial markets today.

It is my understanding that the extensions in [I-D.ietf-anima-bootstrapping-keyinfra] are not required for the clarifications that appear in this document. There is no MUST or SHOULD relationship to either of the referenced documents, so I wonder if they should be an informational references.  What do others think?

(2) Several errata are listed in the Introduction:

   This document deals with errata numbers [errata4384], [errata5107],
   [errata5108], and [errata5904].

I think these are informative references.  What do others think?

Russ

---

From: Russ Housley <[email-masked]>
Date: Thu, 30 Apr 2020 17:39:19 -0400

I have a last minute questions about a missing normative reference.

I see [RFC6268] is referenced in the ASN.1 Module.  I think it needs to be a normative reference.

Also, the PKIX-CommonTypes-2009 module is defined in RFC 5912.  I think that should be noted as a comment in the ASN.1 module and included as a normative reference.

Russ

---

From: Michael Richardson <[email-masked]>
Date: Thu, 30 Apr 2020 18:37:29 -0400

> It is my understanding that the extensions in
    > [I-D.ietf-anima-bootstrapping-keyinfra] are not required for the
    > clarifications that appear in this document. There is no MUST or SHOULD
    > relationship to either of the referenced documents, so I wonder if they
    > should be an informational references.  What do others think?

I agree, it should be informative, and I've adjusted the markdown.

    > [errata5108], and [errata5904].

I asked that question... I thought that I heard that they should be normative.

    > be a normative reference.

okay.

    > Also, the PKIX-CommonTypes-2009 module is defined in RFC 5912.  I think
    > that should be noted as a comment in the ASN.1 module and included as a
    > normative reference.

okay, done.