Subject: Re: [sipcore] Draft new version: SIP Push-19 - Robert's issues 1 and 2 and nits - The Pull Request

From: Christer Holmberg <[email-masked]>
Date: Wed, 17 Oct 2018 12:55:16 +0000

https://github.com/cdh4u/draft-sip-push/pull/28

Regards,

Christer

On 17/10/2018, 14.01, "sipcore on behalf of Christer Holmberg" <[email-masked] on behalf of [email-masked]> wrote:

    Hi,
    
    I addressed issue #3 in another reply, so I will address issues #1 and #2, and the nits,  in this reply. 
    
    > I've reviewed -19, and still have a few comments and questions.
    >
    > I'm ok if dealing with these is deferred to IETF LC, but having an answer before then will likely 
    > improve the quality of any LC reviews the document receives.
    I’d prefer to have everything in the draft before we move it forward.
    > 1) This document is putting tokens representing the _current_ apple and google push services into a registry. 
    > What happens if one of those companies changes the way their push service works significantly? The point of carrying 
    > around the pns-provider is to let the consumer of it know what not-specified-here thing to do with the pn-prid and pn-param 
    > (if it's not, the document needs more clarity). If that not-specified-here thing changes for a given value, the consumer is going 
    > to behave incorrectly. Should these names we're putting in for the apple and google services be versioned in some fashion? 
    
    If they change things in a way that is not backward compatible they need to define a new provider value. They can define a value “XXXv2” if they want, but I don’t think we need to define a built in versioning mechanism.
    
    --
    
    > 2) The privacy treatment of 556 is still incomplete. At a minimum, the security considerations section needs to point back to the 
    >
    > "Note" in 5.3.2 that says the "proxy operator needs to determine whether it could have privacy implications". I'm personally not convinced that we 
    > need a specific "Push Notification Failed" response. What would something _do_ with it differently than they would do with the 480 alternative already 
    > in the draft? My recommendation is to just remove 556 from the draft completely and always send the 480. If we leave 556 in, a thorough description 
    > of "could have privacy implications" needs to be added.
    
    Maybe something like:
    
    "proxy operator needs to determine whether it could have privacy implications, as it will inform the calling network (and possibly the calling party) that the called party is using the SIP push mechanism.".
    
    However, I am also ok to remove 556. As I said earlier, I think 556 would be useful mostly from a logging/trouble shooting perspective - functionally I don't think it would be treated differently from 480.
    
    --
    
    Nits
    
    > "upstream" and "downstream" in section 5.3.1 are undefined and a likely source of confusion. I recommend saying "a proxy between this proxy 
    > and the UA" when you say "upstream", and "a proxy that is not between the push proxy and the UA" when you say downstream.
    
    I will fix as suggested.
    
    > Consider pointing to the 'reg' event package for one way a proxy would be "able to retrieve" registration information in the first paragraph of section 5.2.
    
    I suggest the following modified text:
    
       "The proxy needs to be able to retrieve the information from the registrar using some mechanism.  Such mechanisms are outside the
       scope of this document, but could be implemented e.g., using the SIP registration event package [ref-to-RFC3680]."
    
    > Why is the sentence "A UA MUST NOT create bindings for other UAS (using a different PRID)." in the document? It feels like a "don't do this obviously 
    > stupid thing the protocol doesn't tell you to do." requirement, and those attract trouble. I don't see anything in the document that would mislead a UA 
    > into acting that way. Adding this might have rough edges in the future if a way to delegate the creation of bindings is found that makes sense. I recommend 
    > removing the sentence.
    
    I added the sentence based on Paul's WGLC review. But, I think Paul later indicated that we don't need to add anything, so I am happy to remove the text.
    
    --
    
    Micro-Nits:
    
    >At the end of section 6, "is wake" -> "is awake"
    
    Will fix.
    
    Thanks!
    
    Regards,
    
    Christer

    On 10/12/18 4:55 AM, Christer Holmberg wrote:
    Hi,
     
    Based on Paul’s WGLC comments I have submitted a new version (-19) of draft-sip-push.
     
    The following changes were done:
     
    - ABNF 
    - UA text regarding binding expiration (in case of multiple bindings) and usage of PRIDs associated with other UAs 
     
    The following pull request was merged for the new version:
     
    https://github.com/cdh4u/draft-sip-push/pull/27
     
    A big Thank You to Paul for his review!
     
    Regards,
     
    Christer

    _______________________________________________
    sipcore mailing list
    mailto:[email-masked]
    https://www.ietf.org/mailman/listinfo/sipcore

    _______________________________________________
    sipcore mailing list
    [email-masked]
    https://www.ietf.org/mailman/listinfo/sipcore