# Andy Newton, ART AD, comments for DRAFT
CC @anewton1998

* line numbers:
  - https://author-tools.ietf.org/api/idnits?url=https://www.ietf.org/archive/id/draft-ietf-lamps-rfc7030-csrattrs-20.txt&submitcheck=True

* comment syntax:
  - https://github.com/mnot/ietf-comments/blob/main/format.md

* "Handling Ballot Positions":
  - https://ietf.org/about/groups/iesg/statements/handling-ballot-positions/


## Comments

This looks good. Thanks for the work.

## Nits

### Markdown Reference Syntax

For your next revision: the following Markdown references appear to use the wrong syntax.

218        When not using the template-based approach of {#csrtemplate},

and

228        {#examples}

This document does not appear to raise transport-related concerns.

I do though have a number of comments - intended to help readability of the next revision.

The abstract says: “RFC7030 (EST) and clarifies how the CSR Attributes Response can be used by an EST server to specify both CSR attribute OIDs”
- EST, CSR and OID : please define or expand all acronyms on their first usage for easier reading of the abstract.

“As a result of some of the implementation challenges, it came to light that the particular way of that RFC7030 (EST) says to use the CSR attributes was not universally agreed upon.”
- I liked the context, buit this is a little hard to read, could it please be written simpler?

===

Introduction:

I suggest RFC7030 is referenced when first mentioned, and EST expanded at the start.

The following text was not clear to me: “it was suggested that it went contrary to section 2.6”. - I assumed this was a reference to another RFC? The language could be improved and please cite RFC 7030, if this is being referenced? or whatever reference if something else?

====

Section 3.2 refers to something that I was unsure about, is this the text that appears in RFC70303 as:
“The syntax for application/csrattrs body is as follows:” part of the RFC, it would help to clarify in the text what was being changed.

NiT: There is a missing period after:
“ Many examples for this are given in {#examples}“
- what is #examples?

RDN is used before being defined.
CMRF is not defined.

“If the 'extnValue' is present (which is always the case when type Extension is used), the client is required to use the given extension value.“
- This seems like a requirement: Could this be an RFC2119 “MUST” requirement?
====

What does this require: “Otherwise it is  expected to fill in the extension value.", could this be written in RFC2119 language?

What does this require: “If the 'subjectAltName' extension contains the 'directoryName' choice containing the NULL-DN (i.e., an empty sequence of RDNs) or the'iPAddress' choice with an empty OCTET STRING, this means that the client is expected to fill in the respective GeneralName value.”, could this be written using RFC2119 language?

Thanks to the authors and the WG for their work on this document. I have only a few minor comments/suggestions provided inline below with the idnits output of v20 of the document.


153	3.1.  Extensions to RFC 7030 section 2.6

155	   Replace the second paragraph with the following text:

<minor> Would be easier for the reader if the old paragraph was also listed
here - as in old/new style of patching.


218	   When not using the template-based approach of {#csrtemplate},

<minor> I did not understand what "{#csrtemplate}" refers to. There is also a
"{#examples}". I get the impression that they are some references?

219	   specifying the requirement for a public key of a specific type and
220	   optionally its size and other parameters MUST be done as follows:


782	7.  IANA Considerations

784	   IANA is asked to allocate two new Object Identifiers:

<minor> I see three below.

<EoRv20>

I am not a CSR expert, so most of these comments come from reading the document and seeing where some clarity could be provided.

Section 3.1, paragraph 1
>    These attributes can provide additional descriptive information that
>    the EST server cannot access itself, such as the Media Access Control
>    (MAC) address of an interface of the EST client. The EST server can
>    also provide concrete values that it tells the client to include in
>    the CSR, such as a specific X.509 Subject Alternative Name extension.
>    Moreover, these attributes can indicate the type of the included
>    public key or which crypto algorithms to use for the self-signature,
>    such as a specific elliptic curve or a specific hash function that
>    the client is expected to use when generating the CSR.

I know the first sentence comes from RFC 7030, but since we are updating the paragraph ...

Is it that the attributes are providing "descriptive" information? Later in Section 5.2.2 there is mention of how the MAC address is expected to be included in the CSR. In what way is that just a "descriptive" information?

Possible DOWNREF from this Standards Track doc to [X.680]. If so, the IESG
needs to approve it.

Found terminology that should be reviewed for inclusivity; see
https://www.rfc-editor.org/part2/#inclusive_language for background and more
guidance:

 * Term "dummy"; alternatives might be "placeholder", "sample", "stand-in",
   "substitute"

-------------------------------------------------------------------------------
NIT
-------------------------------------------------------------------------------

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

Section 1, paragraph 1
>    Enrollment over Secure Transport [RFC7030] (EST) has been used in a
>    wide variety of applications.  In particular, [RFC8994] and [RFC8995]
>    describe a way to use it in order to build out an autonomic control
>    plane (ACP) [RFC8368].

s/automatic control plane (ACP)/Automatic Control Plane (ACP)/

Section 3.2, paragraph 14
>    When not using the template-based approach of {#csrtemplate},
>    specifying the requirement for a public key of a specific type and
>    optionally its size and other parameters MUST be done as follows:
>    Include exactly one Attribute with the type field being the OID
>    specifying the type of the key, such as ecPublicKey or rsaEncryption.
>    The 'values' field MAY be empty to indicate no further requirements
>    on the key.  Otherwise it MUST contain suitable parameters for the
>    given key type, such as a singleton set containing the OID of an EC
>    curve such as secp384r1 or containing an integer value for the RSA
>    key size such as 4096.  Many examples for this are given in
>    {#examples}

Is {#examples} meant to be a cross-reference to the Examples section? If so in both the HTML and TXT version of the document, it is not expanding correctly. Similar issue exists with other cross-references, e.g., {#csrtemplate}.

Section 3.2, paragraph 10
> further requirements on the key. Otherwise it MUST contain suitable paramete
>                                  ^^^^^^^^^
A comma may be missing after the conjunctive/linking adverb "Otherwise".

Section 3.3, paragraph 9
> laces no requirements on the key. Otherwise it MUST be present, and the 'algo
>                                   ^^^^^^^^^
A comma may be missing after the conjunctive/linking adverb "Otherwise".

Section 3.3, paragraph 14
> o use the given extension value. Otherwise it is expected to fill in the ext
>                                  ^^^^^^^^^
A comma may be missing after the conjunctive/linking adverb "Otherwise".

Hi Michael, Owen, David, and Dan,

Thank you for taking care of the DISCUSS/COMMENT points in my initial ballot [1] and follow-ups [2]. I trust the agreed changes will make to the the public version.

Cheers,
Med

[1] https://mailarchive.ietf.org/arch/msg/spasm/oSM0A0mktO2sKeM3DZuc2jPv_Ig/

[2] https://mailarchive.ietf.org/arch/msg/spasm/b2XBt1fGYXtux6KFo0ub0m_9B2Y/

A few minor comments:

        the 'subjectAltName' extension with DNS name "www.myServer.com" and an empty IP address to be filled in,

I had to read this a number of times to understand it. How about:


        the 'subjectAltName' extension with two entries; one DNS entry with name "www.myServer.com" and IP 
        entry that is empty for the IP address to be filled in.

Section 5.1.2

I was puzzled by the ac@  sample.com until I remembered a warning from my co-AD
about a hidden scrollbar. It would be good if this can be avoided.

I found at least three markdown notations in the text that failed to expand properly while
generating xml from markdown that need to be fixed.b

Thank you to Lars Eggert for the GENART review.

# Éric Vyncke, INT AD, comments for draft-ietf-lamps-rfc7030-csrattrs-20
CC @evyncke

Thank you for the work put into this document.

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education), and one nit.

Special thanks to Russ Housley for the shepherd's write-up (albeit using an old template) including the WG consensus and the justification of the intended status.

I hope that this review helps to improve the document,

Regards,

-éric

## COMMENTS (non-blocking)

### Abstract (and Introduction)

`EST` is used before its expansion.

`way of using the CSR attributes was not universally agreed upon` is ambiguous as it could be read as this RFC lacked IETF consensus.

### Section 3.3

s/some useless fields have to be included/some *unused* fields have to be included/ or restrict the scope of this sentence just to SET ?

Who is the "we" in `We avoid these drawbacks as follows` ? The authors ? the LAMPS WG ? the IETF community ? Please avoid using "we" or be specific.

### Section 5.1.2

Please note that "acample.com" should probably replaced by "example.com".

## NITS (non-blocking / cosmetic)

### Section 1

s/autonomic control plane (ACP)/Autonomic Control Plane (ACP)/