Hi Dale, Thanks for the review. Responses inline below; changes in this PR:
https://github.com/ietf-wg-acme/acme/pull/424 On Wed, Apr 18, 2018 at 9:03 PM, Dale Worley <wor...@ariadne.com> wrote: > Reviewer: Dale Worley > Review result: Ready with Issues > > I am the assigned Gen-ART reviewer for this draft. The General Area > Review Team (Gen-ART) reviews all IETF documents being processed > by the IESG for the IETF Chair. Please treat these comments just > like any other last call comments. > > For more information, please see the FAQ at > <https://wiki.tools.ietf.org/area/gen/wiki/GenArtfaq>. > > Document: draft-ietf-acme-acme-11 > Reviewer: Dale R. Worley > Review Date: 2018-04-18 > IETF LC End Date: 2018-04-18 > IESG Telechat date: 2018-05-10 > > Summary: > > This draft is on the right track but has open issues, described > in the review. > > This draft is much better than the version (-08) that I previously was > the assigned Gen-ART reviewer for. The overall work and exposition > are quite solid, though there are some open technical issues that need > to be resolved. > > * Technical issues > > What is the versioning and extensibility system? -- It seems that the > natural approach is that structures returned by fetches of Acme > resources may contain elements that are not defined in this document, > and a client should ignore them if it doesn't understand them. This > allows plenty of flexibility for extending the Acme protocol; in > particular, by adding further resources to the directory object. > The WG discussed versioning / extensibility several times, and concluded that on the one hand, there are enough extension points in the protocol to cover many cases (e.g., new challenge types), and on the other hand, extensions that are not compatible with that approach can be handled with a new API endpoint. Clients have always needed to be configured with an HTTP URL for the directory; this just notes that they also need to know what flavor of ACME is running there. In other words, there's no need for version negotiation in-band to the protocol. > The handling of "terms of service agreement" seems to be insufficient, > in that none of the information passed around says *what* agreement > the client has agreed to. The client only sends > "termsOfServiceAgreed:true" in a request, leaving what has been agreed > to unspecified -- and unauditable. One possibility is to add an > argument for the client to provide the URL from which it fetched the > ToS (so the server knows what agreement the client is referring to) > and the hash of the ToS document (so the client must attest to what > the agreement text was). > This mechanism has been reviewed by multiple CAs and found to be sufficient for their needs. A prior version had an explicit URL, and it was found to cause interop problems. > 6.6.1. Subproblems > > What error type should be used in a problem document when there are > subproblem documents? It seems that in this situation, the intention > is that the top-level problem document is not intended to carry error > information itself, so you want to define a "subproblems" error type, > to use when there is no natural overall error type. > > * Editorial issues > > 7.1. Resources > > The position of "newNonce" in the diagram is strange. Does it have a > different relationship to the directory resource than newAccount, > etc.? Similarly for the "finalize" and "cert" URLs of an order > object. -- Reading further in the draft suggests that the graphical > difference indicates that that these values are optional in the > respective objects, although the text doesn't identify that. > I don't think it's all that useful to parse close semantics into this diagram. "newNonce" is different, in that it serves an underlying transport purpose, rather than a certificate management purpose. > 7.1.2. Account Objects > > contact (optional, array of string): An array of URLs that the > server can use to contact the client for issues related to this > account. For example, the server may wish to notify the client > about server-initiated revocation or certificate expiration. > > I mentioned this in my review of -08, but it hasn't been fixed: > Strictly, you want "URIs" here, as tel:, sip:, and mailto: URIs are > not URLs [RFC 6068]. > For readability in the implementer community, where "URL" is much more widely used than "URI", the convention in this document is to use "URL". It seems unlikely this will lead to interop problems. > 7.3.5. External Account Binding > > To enable ACME account binding, a CA needs to provide the ACME client > with a MAC key and a key identifier, using some mechanism outside of > ACME. The key identifier MUST be an ASCII string. The MAC key > SHOULD be provided in base64url-encoded form, to maximize > compatibility between non-ACME provisioning systems and ACME clients. > > I *think* what this means is that the service providing the external > account provides the ACME client with a MAC key and a key identifier, > which the client then uses in constructing its request to the ACME > server. If that is correct, this text is not making clear (to me) the > distinction between the CA that operates the ACME server (which I take > as the default meaning of "CA" in this document) and the service that > provides the "external account". I think two different terms are > needed for the two services so as to make the processing described in > this section clear. > You seem to be envisioning a scenario where a CA relies on accounts established with some third party service, which AFAIK no CA actually does. Changed "a CA" to "the CA operating the ACME server". > > 7.4. Applying for Certificate Issuance > > This section seems to describe both the process of creating an order > and the process of finalizing an order. The initial paragraphs regard > creating an order, but the text starting with "Once the client > believes it has fulfilled the server's requirements..." it talks about > finalization. The text continues to discuss finalization until it > gets to this confusing item: > > o "ready": The server agrees that the requirements have been > fulfilled, and is awaiting finalization. Submit a finalization > request. > > I *think* what is going on is that the text from "The status of the > order will indicate..." through the 5 following items are a *general* > description of the status field which is limited to neither the order > creation step nor the order finalization step. > > It seems to me that this section should be divided into three parts, > one describing creating an order, one describing finalizing an order, > and the generalized description of the status values and the > next-steps that each value implies. The first two parts might be > better expressed as two subsections of 7.4, to clarify which part > of the order life cycle contains which actions. It's not clear to me > the best way to handle the third part; it may be most useful placed > somewhere else in the document. > As you point out, there's no ideal place for the text in question. Either it's at the front, in which case finalization hasn't been discussed yet, or it's at the end, where some things are redundant. I think it's OK where it is. > > 7.6. Certificate Revocation > > It is implicit but might be worth mentioning that the server should > only accept revocation requests for certificates that it itself has > issued. I mention this because the revocation request signed by the > certificate's key does not directly reference an account in the > server, but (I think) provides enough information that the server > could construct an apparently legitimate CRL entry for the certificate > using just the information in the revocation request. Thus, a sloppy > implementation might skip verifying that it is dealing with a > certificate that it issued. > A CA cannot revoke certificates it has not issued. > 8.3. HTTP Challenge > > On receiving a response, the server constructs and stores the key > authorization from the challenge "token" value and the current client > account key. > > I'm not sure this storage step is necessary, or even visible in the > protocol operation. (E.g., the server can calculate the key > authorization at any time that it needs to know the value.) So you > might want to remove this sentence. > There's no harm in storing it; servers can make their own decisions. > Similarly for section 8.4. > > 9.1. MIME Type: application/pem-certificate-chain > > Each following certificate SHOULD directly certify one > preceding it. > > This is grammatical, but it leaves me wondering if a typo was made, > because "certify one preceding" and "certify the one preceding" are > both grammatical but have different meanings. I suggest you replace > it with one of the following: > > Each following certificate SHOULD directly certify the one > preceding it. > > Each following certificate SHOULD directly certify some certificate > preceding it. > Yeah there's a missing "the". > > 9.7.8. Validation Methods > > | tls-sni-01 | RESERVED | N | N/A | > | | | | | > | tls-sni-02 | RESERVED | N | N/A | > > It seems pointless to insert two entries into a registry with no > documentation reference, unless the intention is to reserve these > identifiers from future use. But in that case, this document is > actively prescribing that the identifiers are reserved, and this > document is the reference for that action. > I've updated the reference, but the objectief here is precisely to reserve these identifiers from future use.
_______________________________________________ Acme mailing list Acme@ietf.org https://www.ietf.org/mailman/listinfo/acme
