Last Call Review of draft-ietf-httpbis-proxy-status-05
review-ietf-httpbis-proxy-status-05-secdir-lc-salz-2021-07-22-00

Request Review of draft-ietf-httpbis-proxy-status
Requested rev. no specific revision (document currently at 08)
Type Last Call Review
Team Security Area Directorate (secdir)
Deadline 2021-08-11
Requested 2021-07-21
Authors Mark Nottingham, Piotr Sikora
Draft last updated 2021-07-22
Completed reviews Secdir Last Call review of -05 by Rich Salz (diff)
Genart Last Call review of -05 by Thomas Fossati (diff)
Tsvart Last Call review of -05 by Magnus Westerlund (diff)
Artart Last Call review of -06 by Jim Fenton (diff)
Intdir Telechat review of -06 by Benno Overeinder (diff)
Assignment Reviewer Rich Salz 
State Completed
Review review-ietf-httpbis-proxy-status-05-secdir-lc-salz-2021-07-22
Posted at https://mailarchive.ietf.org/arch/msg/secdir/l-34g2pLVcj-ZHuEepPUKZEeQUQ
Reviewed rev. 05 (document currently at 08)
Review result Has Nits
Review completed: 2021-07-22

Review
review-ietf-httpbis-proxy-status-05-secdir-lc-salz-2021-07-22

This is the SECDIR review of the draft, primarily for the benefit of the Security AD's.  All others can treat this as any other last-call comments.

It's ready, there are some nits/confusions/inconsistencies that should be cleared up.

While the draft says it is both for CDN like things and outgoing reverse proxies, it doesn't quite seem that way.  For example, Sec 2 the Proxy-Status HTTP field says the order is first closest to the origin, and last closest to the user-agent. Shouldn't it always be in the direction of travel?  The example, shows "FooProxy, ExampleCDN", but the explanation doesn't fit if there is an outbound proxy. Or maybe it's "FooProxy" is a proxy at the origin side?  Either way, I think there's an ambiguity there that needs to be cleaned up.

Implied, but should be stated, as that when the request arrives at the origin, any proxy-status headers should be removed before responding.

Sec 2.1.3 should maybe refer to the ALPN registry directly. All current values are US-ASCII, and that seems unlikely to change.  Saying UTF-8 seems contradictory since sf-string is ASCII-only.

Sec 2.1.4 talks about received status, which is "in the opposite direction" from 2.1.3  Perhaps split this up into "common" "sender" "receiver" types of status fields?

Sec 2.1.5 uses sf-string which doesn't allow utf8 localization or markers thereof.  should it?

Sec 2.2 says "[a] specification document is appreciated, but not required."  Then the Reference bullet item needs editing.  I prefer to make it spec required.

Sec 2.3.3 says "destination not found"  It should have a forward link to 2.3.6  Consider shuffling some of those error cases around so that they fall into things like "proxy failures" "infrastructure failures" "next hop did something bad"  And consider making three subsections for each class.

I think having both "connection_limit_reached" and "destination_ip_prohibited" is too fine a grain, but not worth arguing about. (shades of 451 I guess)

The TLS errors should have "alert number" as extra info.  (Not the alert-message as mentioned in 2.3.15)

I am confused by 2.3.18 and 2.3.8 or even 2.3.10; maybe some guidance on when to use each?

I think, again, splitting this section up is worthwhile.  Maybe add "http errors" "tls errors" -- i.e., what layer in the IETF protocol model broke.  That, or the traffic direction suggest above, might really help implementors.

2.3.29 has no recommended http stauts code.

2.4 says a specification is appreciated, but the registration form doesn't list one.  Similar to the comments above about Sec 2.2