# Gen-ART review of draft-ietf-lamps-rfc7030-csrattrs-08

CC @larseggert

## Comments

### "Abstract", paragraph 2
```
     This document updates RFC7030 (EST) and clarifies how the CSR
```
Document status does not indicate the update.

### Section 1, paragraph 5
```
     Section 2.6 says that the CSR attributes "can provide additional
     descriptive information that the EST server cannot access itself".
     This is extended to describe how the EST server can provide values
     that it demands to use.

     After significant discussion, it has been determined that Section 4.5
     of [RFC7030] specification is sufficiently difficult to read and
     ambiguous to interpret that clarification is needed.
```
This document is a "patch RFC" for RFC7030, which might be easy to
produce but puts the burden on the reader/implementer when it comes
to figuring out what the new updates specification actually is.
I'd much prefer an actual 7030bis, also given that there already is a
verified erratum for RFC7030. Yes, it's more work for the authors and
the WG.

### Section 3.2, paragraph 5
```
     The ASN.1 syntax for CSR Attributes as defined in EST section 4.5.2
     is as follows:

        CsrAttrs ::= SEQUENCE SIZE (0..MAX) OF AttrOrOID

        AttrOrOID ::= CHOICE (oid OBJECT IDENTIFIER, attribute Attribute }

        Attribute { ATTRIBUTE:IOSet } ::= SEQUENCE {
             type   ATTRIBUTE.&id({IOSet}),
             values SET SIZE(1..MAX) OF ATTRIBUTE.&Type({IOSet}{@type}) }

     This remains unchanged, such that bits-on-the-wire compatibility is
     maintained.
```
In a "patch RFC", it's not useful to include the bit's that don't
change, because doing so creates a copy of the text that can get out
of sync if 7030 is actually changed down the road. Omit?

### Section 3.3, paragraph 0
```
  3.3.  Alternative: Use of CSR templates
```
Why is this alternative included here? If there is any value to doing
so (e.g., for posterity) move it to an appendix so it's clear it's
not part of the normative content of this doc.

### Section 5.5.1, paragraph 1
```
     <CODE BEGINS>
     MD0GCSqGSIb3DQEJBzASBgcqhkjOPQIBMQcGBSuBBAAiMBIGCSqGSIb3DQEJDjEFBgNVBAUGCCqGSM49BAMD
     <CODE ENDS>
```
Line too long.

## Nits

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.

### Typos

#### Section 3.2, paragraph 11
```
-    Extension, MUST NOT include more than one element with a particiular
-                                                                   -
```

### Duplicate references

Duplicate normative references to: `https://www.itu.int/rec/T-REC-X.680`.
(The URL for [X.690] is incorrect.)

### Uncited references

Uncited references: `[RFC5652]`.

### Grammar/style

#### Section 3.2, paragraph 13
```
 faked with an empty big string. The avoid this drawback, this specification
                                 ^^^^^^^^^
```
After "The", the verb "avoid" doesn't fit. Is "avoid" spelled correctly? If
"avoid" is the first word in a compound adjective, use a hyphen between the two
words. Using the verb "avoid" as a noun may be non-standard.

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool