[sipcore] AD Evaluation of draft-ietf-sipcore-sip-push-20

[Ben Campbell <[e-mail]>]

Hi,

This is my AD Evaluation of draft-ietf-sipcore-sip-push-20. I reviewed a previous version, but since there have been substantial changes since that version, I treated this as a “from scratch” review.

This version is in much better shape than the previous. I think it’s almost ready for IETF LC, but I want to resolve at least the substantive comments first.

Thanks!

Ben.

*** Substantive Comments ***

§1, paragraph 5: "Note that this mechanism necessarily adds delay to
responding to non-INVITE requests requiring push notification."

Technically, it adds delay for any transaction. Non-INVITE transactions are just more delay sensitive.

§3,
- first paragraph:
Please state this in terms of a scope assumption, not a global statement. That is, limit the scope of this document is limited to PNSs that work this way, rather than state that all PNSs work this way.

- 2nd paragraph: I think it's safe to say "The format of the PRID varies..." rather than "... may vary...".

§4.1

- General: When we speak of multiple bindings, and doing something to all of those bindings (e.g. refreshing them on a push-notification), is it allowable for a UA to have some bindings that use a PNS and some that do not? Or perhaps some that use different PNSs? I realize that may not make sense for the mobile device use case that seems centric to this draft, but I can imagine a situation where a complex UA might have different PNS configurations for different network paths and/or different SIP services.

If you want to assume that the PNS configuration is the same for all contacts, please say so explicitly. If not, does it make sense to scope PNS-triggered refreshes to just those bindings that use the same PNS configuration?

-2nd paragraph:
I don't think we can assume that all UAs will always learn all the bindings they need to to setup at the same time. For example, a UA might have changes in network configuration that require them to add or remove bindings. This makes it impractical to register them at the same time and always use the same expirations, unless we ask them to refresh all existing bindings whenever they add a new one. (Or perhaps ask them to set the expiration of any new binding to match the remaining time for existing ones, but that seems like a more difficult approach.)

-6th paragraph: "REGISTER request will create NAT bindings..."

If we talk about creating NAT bindings as one of the purposes of the push-triggered REGISTER, we may also need to think about the lifetime of those nat bindings, and how that interacts with the REGISTER expires value. I'm not sure we want to go there; would it make sense to remove the mention?

- 7th paragraph:
--  The first sentence seems to still assume the REGISTER has as single contact. Should it say to insert the tag into each contact header field? (but see general §4.1 comment above.)

-- "if the response to the REGISTER request contains a ’sip.pnsreg’
feature-capability indicator with an indicator value, the UA MUST
send REGISTER requests prior to the registration expires."

This seems to duplicate a MUST from normal SIP. Would it make sense to just say "... the UA refreshes the binding according to normal SIP procedures".

§5.3.1:
- 4th paragraph: Section 4.1 talks about the UA paying attention to sip.pns in a 2XX response. It does not mention it for other result codes. If the UA needs to pay attention to it in 423 (or in any other response), it should be mentioned there.

-7th paragraph, "the proxy SHOULD insert a Feature-Caps header
field with a ’sip.pns’ feature-capability indicator": What if the response already contains it?

§5.3.2, 2nd paragraph: Please mention that the R-URI will only contain these parameters after it is retargeted. This mostly matters if the push-proxy is colocated with a retargeting proxy; retargeting must happen before this procedure.

Along those lines, what is the PUSH proxy expected to do with inbound SIP requests that do not contain the parameters in the R-URI? Route them normally? Reject them?

§6: Is support for push-notifications on mid-dialog requests optional? If so, please state that up front.

§6.1.1: Does the UA indicate support on a per-dialog basis? That is, it can support the mechanism for some dialogs but not others?

§8.1, 2nd paragraph: Should that say "only defined for ... responses", or "... and is not defined for use in request messages."?

§9, 1st paragraph: We are talking about a specification that defines the parameter usage for the given PNS, not the PNS in it's entirety, right?

§13, 3rd paragraph: The paragraph as written basically says "Operators must ensure SIP signaling is secured unless they are sure it's secured", which is circular logic. Would it make sense to say they must use TLS (or SIPS) unless they are sure that it is protected by other mechanisms?

§16.2: I think the reference to RFC 3891 should be normative.


*** Editorial Comments ***

- All instances of "binding refresh REGISTER" should be "binding-refresh REGISTER". (This is true anywhere binding-refresh is used as an adjective.)

- There is still a lot of incorrect comma usage, especially commas where they don't belong. I will call out the ones I noticed.

- The phrases "request for a new dialog" and  "standalone
SIP request" are used heavily and repeatedly, often in combination. The common terminology has historically been "dialog-initiating request" and "out-of-dialog request". These are, at least in combination, a bit shorter.

§1
- paragraph 5: Missing word in "awaken other means". I suggest "awakened by other means".
- Paragraph 6: "... SIP request for a new dialog, or a standalone
SIP request, addressed towards a UA...": Both commas should be removed.

- Figure 1: The "SIP 200 Okay" label is separated from the line it labels by a page break. (Other text changes may change the pagination, so this is probably one for the RFC editor if it's still an issue in the final version.)

§4.1, 7th paragraph ("Note"): The first sentence seems odd in the context of the previous paragraph, which talked extensively about UAs using non-push mechanisms.

§4.2,
- first paragraph:

-- first sentence: s/"network"/"SIP Network".
-- "... UA has received a response to the REGISTER request, with the
push notification information ...": improper comma.

- 3rd paragraph, "... early enough, in order for...": I suggest "... early enough for..."

§5.3.1
- 1st paragraph, "... towards the UA associated with the
REGISTER request, or perform any other procedures...": improper comma.

- 5th paragraph:
-- "... proxy that is not between the push proxy and the UA...". Doesn't this really mean a proxy that is between the push proxy and the registrar? If so, saying where the proxy _is_ would be clearer than saying where it is _not_.

-- "... (if the Contact header field URI of the REGISTER request
contains a pn-prid SIP URI parameter) ..." Could we have gotten this far if that were not true? The sentence would be easier to read without it. (I think this repeats a few times.)

- 6th paragraph: "MUST only" constructions can be ambiguous. It is better to state them in terms of "MUST NOT unless..."

- 2nd to last paragraph, "The proxy MUST be able..." It would be better to state this in terms of what the proxy _does_, not what it is _able_ to do. For example, "... MUST NOT ... unless it has determined that the PNS supports the VAPID mechanism."

§5.3.2
-  3rd paragraph: "This can happen if the
UA sends a binding refresh REGISTER request with a new contact...": This seems to ignore the potential of multiple contacts. I think the real condition is when the previous binding is _removed_.

-6th paragraph: I don't understand the meaning of this paragraph. (Also, both commas are incorrect.)

-11th paragraph: s/"losing race that results"/ "losing the race, which results"  ; or "... a race..."

§6.2.3: Parts of this section seem specific to SIP-Events dialogs. I assume that's not the intent?