Thank you very much Sean! Great points, I balloted DISCUSS as these should be fixed before the document is moved forward, and will be looking out for the authors and wg’s answers.
Francesca
From: last-call <last-call-bounces@xxxxxxxx> on behalf of Sean Turner via Datatracker <noreply@xxxxxxxx>
Date: Tuesday, 30 November 2021 at 05:15
To: art@xxxxxxxx <art@xxxxxxxx>
Cc: draft-ietf-acme-authority-token-tnauthlist.all@xxxxxxxx <draft-ietf-acme-authority-token-tnauthlist.all@xxxxxxxx>, last-call@xxxxxxxx <last-call@xxxxxxxx>, acme@xxxxxxxx <acme@xxxxxxxx>
Subject: [Last-Call] Artart telechat review of draft-ietf-acme-authority-token-tnauthlist-08
Reviewer: Sean Turner
Review result: Ready with Issues
Hi! 2nd ARTART review so I am not sure I am yet attuned to all the ART hot
buttons. Drawing on the times I have been through the ARTART process as an I-D
author ;)
ADs NOTE: I picked "Ready with Issues" because, while my comments seem long, I
do not think any of the things that I noted are insurmountable but they
non-nits are worthy of a response.
0) I found some problems with the JSON in this I-D:
ASIDE: It seems perfectly okay to have “base64url({foo})” in the JSON; RFC
8739s and 9115 do. But, validators won't return a valid check for the JSON
examples (at least not the validators I used). I did get them to return valid
after I made the corrections below and then ran the validate twice. Once for
the foo and for the rest by replacing base64url({foo}) with "". A pain but it
did uncover:
0.a) s3, 1st and 2nd examples (missing closing " on type:
OLD:
{"type:"TNAuthList",
NEW:
{"type":"TNAuthList",
0.b) s4, 1st example (drop ,):
OLD:
"url": "https://example.com/acme/authz/1234",
NEW:
"url": "https://example.com/acme/authz/1234"
0.c) s4, 2nd example (add “)
OLD:
{"type:"TNAuthList",
NEW:
{"type":"TNAuthList",
0.3) s4, 2nd example (add ,)
OLD:
"url": "https://boulder.example.com/authz/asdf/0"
NEW:
"url": "https://boulder.example.com/authz/asdf/0",
0.44) s5.4, quote the URL:
OLD:
"x5u":https://protect2.fireeye.com/v1/url?k=848acae2-db11f3e7-848a8a79-86d2114eab2f-1d2b6327a9ee43b5&q=1&e=1964e0ec-af8b-4226-9ec3-22ebc0dbc063&u=https%3A%2F%2Fauthority.example.org%2Fcert
NEW:
"x5u":"https://protect2.fireeye.com/v1/url?k=9ab17434-c52a4d31-9ab134af-86d2114eab2f-1ece6dcafafe9403&q=1&e=1964e0ec-af8b-4226-9ec3-22ebc0dbc063&u=https%3A%2F%2Fauthority.example.org%2Fcert"
1) base64 reference/encoding:
1.a) RFC 4648 defines a number of alphabets. I assume that you are referring to
the “Base 64 Encoding with URL and Filename Safe Alphabet” because you use
base64url in the JSON? I think you need to make this clear. You maybe could
steal the text from RFC 8555 (and yeah this is wordy):
OLD:
The format of the string that represents the TNAuthList MUST be
constructed as a base64 [RFC4648] encoding of the TN Authorization
List certificate extension ASN.1 object.
New:
The format of the string that represents the TNAuthList MUST be
constructed as a base64url encoding, as per [RFC8555] base64url
encoding is described in Section 5 of RFC4648 [RFC4648] according
to the profile specified in JSON Web Signature in Section 2 of
[RFC7515] , of the TN Authorization List certificate extension
ASN.1 object.
1.b) Assuming that you are using the URL safe format, I would suggest updating
the references in the text to “base64” match the examples that use base64url:
1.b.0) s3, 3rd para:
OLD:
base64 encoded string.
NEW:
base64url encoded string.
1.c.1) s5.4, 2nd bullet:
OLD:
base64 encoded
NEW:
base64url encoded
1.c.2) s5.5, 3rd to last para:
OLD:
base64 encoded
NEW:
base64url encoded
1.c.3) s6, 5th bullet:
OLD:
equivalent base64 encoded
NEW:
equivalent base64url encoded
1.c) You will note in the requirement RFC 8555 that trailing "=" be stripped
and encoded values that trailing = MUST be rejected. The identifiers in s3, the
identifier in s4, and tkvalue in s5.4 and s5.5 include "=". Is the JSON wrong
or is padding included?
2) s5.1-3: It seems like you need a normative reference to RFC 7519 for the
iss, exp, and jti claims.
3) s5.4: fingerprint
3.a) So this specification defines fingerprint. Then locks in SHA256 and says
the number of bytes is defined by the hash function. Since you locked in
SHA256, why don’t you just say it MUST be 32 bytes? I guess I am a little
confused how one would support a different algorithm in the future.
3.b) Was there any thought to providing more than one fingerprint (certificates
sometimes do)? I.e., would it be better to be an array of two fields:
"fingerprint":" [{alg=“SHA256”, value=
“56:3E:CF:AE:83:CA:4D:15:B0:29:FF:1B:71:D3:BA:B9:19:81:F8:50:9B:DF:4A:D4:39:72:E2:B1:F0:B9:38:E3”}]
NITS:
0) In s1, s/Certification Authority/Certification Authority (CA)
to abbreviate CA on 1st use. CA is used lated but there is no abbreviation
provided elsewhere in the I-D.
1) s5.4 1st para includes the following:
It contains a JSON object of three elements.
What follows is four bullets. Maybe just future proof it by "It contains a JSON
object with the following elements:"
2) s5.5, please expand CSP on 1st use.
3) s5.5, last para: s/should/SHOULD
4) s5.6, please expand SPC, OCN, and SPID on 1st use.
5) s6, last bullet: uses “CSR request”, but elsewhere it’s been “new-order”
should you use that here too for consistency. If not the R is CSR is request so
you can drop request after expanding CSR ;)
6) Shouldn't the RFC 2119 reference be normative?
>From ID-NITS:
== Outdated reference: A later version (-07) exists of
draft-ietf-acme-authority-token-05
== Outdated reference: draft-ietf-stir-cert-delegation has
been published as RFC 9060
--
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call
|
