Subject: [secdir] Secdir last call review of draft-ietf-httpbis-proxy-status-05

From: Rich Salz via Datatracker <[email-masked]>
Date: Thu, 22 Jul 2021 11:29:03 -0700

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

---

From: Mark Nottingham <[email-masked]>
Date: Fri, 23 Jul 2021 11:58:47 +1000

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 <[email-masked]> wrote:
> 
> Reviewer: Rich Salz
> Review result: Has Nits
> 

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