Re: Secdir last call review of draft-ietf-httpbis-proxy-status-05

Thanks for the review, Rich. I've created:
  https://github.com/httpwg/http-extensions/issues/1580


> On 23 Jul 2021, at 4:29 am, Rich Salz via Datatracker <noreply@ietf.org> wrote:
> 
> Reviewer: Rich Salz
> Review result: Has Nits
> 
> 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
> 
> 
> 

--
Mark Nottingham   https://www.mnot.net/

Received on Friday, 23 July 2021 01:59:05 UTC