Hi Peter,
Thank you for the review. Responses in line
On Fri, Mar 24, 2023 at 12:06 AM Peter Yee via Datatracker <noreply@xxxxxxxx> wrote:
Reviewer: Peter Yee
Review result: Has Issues
Reviewer: Peter Yee
Review result: Has Issues
I have reviewed this document as part of the security directorate's ongoing
effort to review all IETF documents being processed by the IESG. These
comments were written primarily for the benefit of the security area directors.
Document editors and WG chairs should treat these comments just like any other
last call comments.
The document specifies an HTTP integrity mechanism that replaces a prior one
specified in RFC 3230. This is done with 4 new HTTP header fields and a new
digest algorithm registry to be used with those header fields.
The reason that the review result is "Has Issues" is that I disagree with the
algorithm statuses assigned in Table 2 that are used to populate the Hash
Algorithms for HTTP Digest Fields Registry. It's possible I'm way off base
here. In the face of a malicious actor who is able to change the header fields
as an HTTP message is transferred along the way to an endpoint, none of these
algorithms is strong. That's fine. As noted in sections 1.2 and section 6.1, a
secondary mechanism such as HTTP Message Signatures
(draft-ietf-httpbis-message-signatures) would be needed to protect the field
values in an end-to-end manner. If such a mechanism is employed, then the
strength of the hash algorithm used in this specification is largely
irrelevant, as the output of the hash algorithm here is not the direct input to
the digital signature algorithm. Perhaps I do not understand how
draft-ietf-httpbis-message-signatures is used to protect the header fields
specified in this document, but I have to think the field values generated
through this specification are merely data to be input to the combined digital
signature and hash algorithms specified in
draft-ietf-httpbis-message-signatures. To reiterate, I think the statuses
specified for the registry are not terribly useful: in the case of a malicious
actor, any of the hash values can be recalculated and substituted easily. In
the case of non-malicious value mismatches, any of the hash algorithms is
likely good enough, particularly if the underlying transport attempts to ensure
that it correctly delivers data.
You're correct that digest on its own provides weak protection from purposeful or malicious intent to manipulate it. You're also correct that signatures can help protect against that and the digest fields are just another piece of input to the signature (rather than having any special role or distinct meaning).
I disagree that the algorithm status is not useful. Common HTTP deployment models involve many interactions where there is no decent secondary integration mechanism. For example, a cache that persists to disk, or "just in time" processing at the edge. The choice of algorithm is dependent on the needs of the application using HTTP, which includes the data in content and the context of its use. Sometimes a weak algorithm might be sufficient to cover those needs. Other times, the risk of something like a hash collision could make an algorithm a severely bad choice. We can't hope to explain all of these in the draft itself, so have chosen to highlight the considerations and provide a clear status to implementations that need to pick an algorithm for their needs. We also need to accept that most implementers will lack the domain knowledge to make the correct selection. It seems remiss to present a set of algorithms in a way that appears to give them a level playing field, when the community does not think that. In this light, hoping that a selection was "likely good enough" makes me uncomfortable. Nudging people towards stronger algorithms seems to have little downside.
To put things in a different context, some other integrity mechanisms used regularly on the Internet, such as Subresource Integrity [1], provide stronger normative language on the choice of algorithm. Given the legacy of this draft, the WG consensus is that this document strikes the correct balance between highlighting security considerations and allowing applications to make their own decisions specific to their needs.
I did not thoroughly read through the appendices nor attempt to validate the
examples they contain. I do appreciate the inclusion of Python code to make it
easier to calculate the hash values needed to validate the examples.
Thanks!
The rest of this review is minor comments and nits.
General: change all "use-cases" to "use cases" for consistent with other usage
in the document.
General: append a comma after all occurrences of "e.g.".
Page 4, 1st partial paragraph, 1st whole sentence: change the comma after
"connection" to a period. Capitalize the following "in".
Page 5, section 1.2, 1st paragraph, 2nd sentence: change the comma after
"message" to a period. Capitalize the following "the".
Page 5, section 1,.2, 1st paragraph, last sentence: append "registry" after the
quoted registry name. Also consider changing the pointer from section 5 to
section 7.2, where the registry really gets defined.
Page 5, section 1.2, 2nd paragraph, 1st sentence: insert "the" before "HTTP".
Page 6, 1st paragraph, 1st sentence: change "Authorization" to lowercase
"authorization" and append a comma after it.
Page 6, section 1.3, 1st paragraph, 2nd sentence: delete the comma after
"defined".
Page 6, last paragraph: append a comma after the quoted "user agent".
Page 12, 3rd to last paragraph, 3rd sentence: append a comma after "broken".
Yeah, I like Oxford/Harvard commas.
Page 13, section 6.2, 1st paragraph, 2nd sentence: insert "an" before "HTML".
Append a comma after "player".
Page 13, section 6.3, 2nd paragraph, 1st sentence: change "digitial" to
"digital".
Page 14, section 6.4, 3rd paragraph: change to the comma after "Section" to a
period. Make "Section" lowercase. Capitalize the following "some". Change
"pre-processing" to "preprocessing".
Page 14, section 6.5, 1st paragraph, 2nd sentence: change "mulitple" to
"multiple".
Page 14, section 6.5, 1st paragraph, 3rd sentence: change the comma after
"metadata" to a period. Capitalize the following "for". Append a comma after
"instance".
Page 21, Appendix A, 1st paragraph, 1st sentence: append a comma after
"Transformations". I'm not sure that word needs to be capitalized, but HTTP
terms is not an area with which I'm overly familiar.
Page 23, Appendix B, 2nd paragraph, 3rd sentence: change "spaced" to "spaces".
Page 36, Acknowledgements: append a comma after "Yaskin".
Thank you for these. I've applied almost all of them to the editor's copy and they improve the readability of the document.
Cheers,
Lucas
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
