[stir] AD Review: draft-ietf-stir-passport-divert

[Adam Roach <[e-mail]>]

This is my AD review for draft-ietf-stir-passport-divert.

Thanks to Jon and everyone else who worked on this document. The mechanism
itself looks to be in very good shape, and I'm glad to see this work
drawing to a conclusion.  There are a number of issues I identify below that
are significant enough that I would like to see a new version of the
document prior to IETF last call.

---------------------------------------------------------------------------

Abstract:

 >  This document extends PASSporT, which is specified in RFC 8225 to
 >  convey cryptographically-signed information about the people involved
 >  in personal communications, to include an indication that a call has
 >  been diverted from its original destination to a new one.

This sentence is a bit long and confusing. Consider splitting along the 
lines
of:

    PASSporT is specified in RFC 8225 to convey cryptographically-signed
    information about the people involved in personal communications. This
    document extends PASSporT to include an indication that a call has been
    diverted from its original destination to a new one.

---------------------------------------------------------------------------

§1:

 >  The address in the To header field value of SIP requests is
 >  not supposed to change, according to baseline [RFC3261], as it is the
 >  Request-URI that is supposed to be updated when a call is retargeted,
 >  but practically speaking many operational environments do alter the
 >  To header field.

This is also a bit overly complex. Consider:

    The address in the To header field value of SIP requests is not 
supposed to
    change, according to baseline SIP behavior [RFC3261]; instead, the
    Request-URI is supposed to be updated when a call is retargeted.
    Practically speaking, however, many operational environments do 
alter the
    To header field.

---------------------------------------------------------------------------

§3:

 >  A "div" PASSporT claims set is populated with elements drawn from the
 >  PASSporT(s) received for a call by the retargeting entity: at a high
 >  level, the original identifier for the called party in the "dest"
 >  array will become the "div" claim in the new PASSporT.  If the "dest"
 >  array of the original PASSporT contains multiple identifiers, the
 >  retargeting entity MUST select only one them to occupy the "div"
 >  field in the new PASSporT...

This is confusing, for a couple of reasons. While the language in RFC 8225
gets objects and arrays mixed up (and I need to file an errata on this), the
intention is clear enough to implement. However, the preceding text is 
actually
ambiguous due to the confusion between "array" and "object".

To be clear:
  - The value of "dest" is an *object*.
  - That object contains name/value pairs
  - The value portion of those name/value pairs is an *array* of strings.

The ambiguity in the preceding text arises because it's not clear
whether the intention is to select only one name/value pair and put it 
in the
"div" field, or to select only one string from one of the arrays that
constitutes the value portion of the name/value pair. For example; if the
original "dest" name/value pair were:

   "dest":{"tn":["12155551213","19995551234"],
           "uri":["sips:[e-mail]","sips:[e-mail]"]},

Then it is not clear whether the resulting "div" field must be limited to
exactly one identity:

   "div":{"tn":["12155551213"]}

...or if it is to be limited to only one name/value pair from the original
"dest" value:

   "div":{"tn":["12155551213","19995551234"]}

I suspect it's the former, but the paragraph can be read either way due 
to its
confusion around the names of JSON things. Please reformulate the 
paragraph to
be clearer about the intended behavior, and ideally make the example more
complex -- similar to the example I give above -- so that it 
demonstrates the
proper behavior unambiguiously.

---------------------------------------------------------------------------

§3:

       { "orig":{"tn":"12155551212"},
         "dest":{"tn":"12155551214"},
         "iat":1443208345,
         "div":{"tn":["121555551213"]} }


The "dest" value here isn't formatted in the way that PASSporTs format
"dest". It needs to be:

         "dest":{"tn":["12155551214"]},

It's not clear, given the description above, whether the "div" parameter is
correct (i.e., whether it should contain an array or a string).

It's worth noting that the example PASSporT gets "dest" right, and uses a
string for the "dest":

{"dest":{"tn":["12155551214"]},"div":{"tn":"121555551213"},
  "iat":1443208345,"orig":{"tn":"12155551212"}}

---------------------------------------------------------------------------

§3:

 >  Note that the "div" element may contain other elements than just a
 >  destination, including a History-Info indicator (see Section 8).

Let's be precise with terminology here:

    Note that the "div" object may contain other name/value pairs than 
just a
    destination, including a History-Info indicator (see Section 8).

---------------------------------------------------------------------------

§4.1:

 > Identity:eyJhbGciOiJFUzI1NiIsInBwdCI6ImRpdiIsInR5cCI6InBhc3Nwb3J \
 > 0IiwieDV1IjoiaHR0cHM6Ly93d3cuZXhhbXBsZS5jb20vY2VydC5wa3gifQ.eyJk \
 > ZXN0Ijp7InRuIjpbIjEyMTU1NTUxMjE0Il19LCJkaXYiOnsidG4iOiIxMjE1NTU1 \
 > NTEyMTMifSwiaWF0IjoxNDQzMjA4MzQ1LCJvcmlnIjp7InRuIjoiMTIxNTU1NTEy \
 > MTIifX0.YZX3UGjaXsAYpYEjWAVBcQxNFOFEqIVuhVPPUv-7yhyeKRazMQLjn9cH \
 >     maq0Mof2N-bfvRXPXuchtDJm8VbrbQ; \
 > info=<https://biloxi.example.org/biloxi.cer>;ppt="div"

This PASSporT contains an "x5u" value of "https://www.example.com/cert.pkx"
while the Identity header field info parameter has a value of
"https://biloxi.example.org/biloxi.cer". Per RFC 8224 section 4.1,
third bullet, these values must be the same (modulo string comparison
rules).

---------------------------------------------------------------------------

§4.1:

 >  Furthermore note that a request may also be retargeted a
 >  second time, at which point the subsequent retargeting entity SHOULD
 >  generate one "div" PASSporT for each previous "div" PASSporT in the
 >  request.  This can create multiple chains of "div" PASSporTs in a
 >  single request, which complicates the procedures that need to be
 >  performed at verification services.

Read literally, this doesn't just create multiple chains; it creates
a exponential explosion of Identity header fields. Consider a request
that contains two non-"div" Identity header fields that gets redirected
four times.  Per the text above:

  - The first redirection will create two "div" passports (one for each
    non-"div" passport). The message now contains two "div" passports,
    and two non-"div" passports.

  - The second redirection will create two more "div" passports (one for
    each existing "div" passport, per the cited text above). The message now
    contains four "div" passports, and two non-"div" passports.

  - The third redirection will create four more "div" passports (one for
    each existing "div" passport). The message now contains eight "div"
    passports, and two non-"div" passports.

  - The fourth redirection will create eight more "div" passports (one for
    each existing "div" passport). The message now contains 16 "div"
    passports, and two non-"div" passports.

While I concede that four redirections isn't all that common in normal use,
the fact that a malicious user could set up a relatively small number of
redirections on purpose and produce a message that increases in both
size and computational complexity at an exponential rate is problematic.

The assertion that what you want here is actually two parallel chains of
"div" passports is almost certainly correct, but it is not what happens
when the text is applied as written.

---------------------------------------------------------------------------

§4.2:

 >  of the verification service is to extract all PASSporTs from the two
 >  or more Identity headers in a request, identify which are "div"

Nit: "...Identity header fields..."

 >  header field values associated with a request, as an Identity header
 >  field value containing "div" necessary refers to an earlier PASSporT

Nit: "...necessarily..."

---------------------------------------------------------------------------

§5:

     { "orig":{"tn":"12155551212"},
       "dest":{"tn":["12155551214"]},
       "iat":1443208345,
       "div":{"tn":"121555551213"},
"opt":"4F7jsZv0mJ5bjg4Xik6Mfah3IO8K6FIsUIgvt0dE7Qm3KZr5UF_UpCrz7 \
c0_0eQi4e9FVX-WmvX3uETtlVjAtgeyJhbGciOiJFUzI1NiIsInR5cCI6InBhc3N \
wb3J0IiwieDV1IjoiaHR0cHM6Ly93d3cuZXhhbXBsZS5jb20vY2VydC5wa3gifQ. \
eyJkZXN0Ijp7InRuIjpbIjEyMTU1NTUxMjEzIl19LCJpYXQiOjE0NDMyMDgzNDUs \
Im9yaWciOnsidG4iOiIxMjE1NTU1MTIxMiJ9fQ.4F7jsZv0mJ5bjg4Xik6Mfah3I \
O8K6FIsUIgvt0dE7Qm3KZr5UF_UpCrz7c0_0eQi4e9FVX-WmvX3uETtlVjAtg"} }

The header of the PASSporT in the "opt" value does not decode into a
valid JSON object. If I remove the first 86 characters (4F7jsZv0mJ5bjg
4Xik6Mfah3IO8K6FIsUIgvt0dE7Qm3KZr5UF_UpCrz7c0_0eQi4e9FVX-WmvX3uETtlVjAtg),
it does appear to be correct. (This extra prepended text appears to be
a copy of the JWT signature.)

Also, this JSON object appears to have an extra closing bracket at the end
of the last line.

---------------------------------------------------------------------------

§5:

 > Identity:eyJhbGciOiJFUzI1NiIsInBwdCI6ImRpdi1vIiwidHlwIjoicGFzc3Bvc \
 > nQiLCJ4NXUiOiJodHRwczovL3d3dy5leGFtcGxlLmNvbS9jZXJ0LnBreCJ9.eyJkZX \
 > N0Ijp7InRuIjoiMTIxNTU1NTEyMTQifSwiZGl2Ijp7InRuIjoiMTIxNTU1NTUxMjEz \
 > In0sImlhdCI6MTQ0MzIwODM0NSwib3B0IjoiZXlKaGJHY2lPaUpGVXpJMU5pSXNJbl \
 > I1Y0NJNkluQmhjM053YjNKMElpd2llRFYxSWpvaWFIUjBjSE02THk5M2QzY3VaWGho \
 > YlhCc1pTNWpiMjB2WTJWeWRDNXdhM2dpZlEuZXlKa1pYTjBJanA3SW5SdUlqcGJJak \
 > V5TVRVMU5UVXhNakV6SWwxOUxDSnBZWFFpT2pFME5ETXlNRGd6TkRVc0ltOXlhV2Np \
 > T25zaWRHNGlPaUl4TWpFMU5UVTFNVEl4TWlKOWZRLjRGN2pzWnYwbUo1YmpnNFhpaz \
 > ZNZmFoM0lPOEs2RklzVUlndnQwZEU3UW0zS1pyNVVGX1VwQ3J6N2MwXzBlUWk0ZTlG \
 > VlgtV212WDN1RVR0bFZqQXRnIiwib3JpZyI6eyJ0biI6IjEyMTU1NTUxMjEyIn19.M \
 > CYorw_3FaH78VuERURlJp1hD6qh2eIct4RIebVtYp3es9HTsvCz1qXRWq3j0E9Pb2h \
 >   YrMUXSQbBYQSviW5cCA; \
 > info=<https://biloxi.example.org/biloxi.cer>;ppt="div-o"

There are two issues with this example.

The "info" parameter does not match the "x5u" value in the PASSporT header.

The body of the (outer) PASSporT decodes to:

{
    "dest":{ "tn":"12155551214" },
    "div":{ "tn":"121555551213" },
    "iat":1443208345,
"opt":"eyJhbGciOiJFUzI1NiIsInR5cCI6InBhc3Nwb3J0IiwieDV1IjoiaHR0cHM6Ly93d3cuZXhhbXBsZS5jb20vY2VydC5wa3gifQ.eyJkZXN0Ijp7InRuIjpbIjEyMTU1NTUxMjEzIl19LCJpYXQiOjE0NDMyMDgzNDUsIm9yaWciOnsidG4iOiIxMjE1NTU1MTIxMiJ9fQ.4F7jsZv0mJ5bjg4Xik6Mfah3IO8K6FIsUIgvt0dE7Qm3KZr5UF_UpCrz7c0_0eQi4e9FVX-WmvX3uETtlVjAtg",
    "orig":{ "tn":"12155551212" }
}

The "dest" value "tn" value is (incorrectly) a string instead of an
array containing a string.

---------------------------------------------------------------------------

§7:

 >  document consequently updates [RFC8224] to permit carrying Identity
 >  headers in SIP 300-class responses.  It is left to the originating

Nit: "...Identity header fields..."

 >  user agent to determine which Identity headers should be copied from

Nit: "...Identity header fields..."


 >  the 3xx into any new requests resulting from the redirection, if any:
 >  use of these Identity headers by entities receiving a 3xx response is

Nit: "...Identity header fields..."

---------------------------------------------------------------------------

§11:

 >  to the called party, many times the called party should likely be
 >  entitled to information about why they receiving these calls.

Nit: "...why they are receiving..."