Subject: [sipcore] Alexey Melnikov's Discuss on draft-ietf-sipcore-sip-push-21: (with DISCUSS and COMMENT)

From: Alexey Melnikov <[email-masked]>
Date: Wed, 09 Jan 2019 04:00:13 -0800

Alexey Melnikov has entered the following ballot position for
draft-ietf-sipcore-sip-push-21: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-sipcore-sip-push/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

I am generally excited about addition of push notifications to SIP.
I have a couple of comments (and a few less serious ones) that I would like to
discuss before recommending approval of this document:

10.  pn-provider, pn-param and pn-prid URI Parameters for Apple Push
     Notification service

   When the Apple Push Notification service (APNs) is used, the PNS-
   related SIP URI parameters are set as described below.

   The value of the pn-provider URI parameter is "apns".

   Example: pn-provider = apns

Spaces are not allowed in URIs unencoded, so your example is misleading. I
suggest you change it to "pn-provider=apns" (i.e. delete space before and after
"=").

Similar comment about 2 other parameter examples defined in this section.

10.  pn-provider, pn-param and pn-prid URI Parameters for Apple Push
     Notification service

   The value of the pn-param URI parameter is a string that is composed
   by two values, separated by a period (.): Team ID and Topic.  The
   Team ID is provided by Apple and is unique to a development team.

I assume it doesn't contain any periods?

   The Topic consists of the Bundle ID, which uniquelly identifies an
   appliciation, and a service value that identifies a service
   associated with the application, separated by a period (.).  For VoIP
   applications the service value is "voip".

How many periods are used in the value? If your example below is correct, can
you clarify that Bundle ID itself contains periods?

   Example: pn-param = DEF123GHIJ.com.yourcompany.yourexampleapp.voip

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I am agreeing with Adam's DISCUSS.

In 8.2:

     pns-list        = pns *(COMMA pns)

My understanding is that COMMA (defined in RFC 3261) allows folding whitespace,
in particular CRLF. Do you really want this in values? It is Ok if you do, I
just wanted to double check.

8.7.  SIP URI Parameters

     COLON = <COLON defined in [RFC3261]>

COLON is not used in this section or anywhere else in the document.

14.4.1.  sip.pnsreg

      Security considerations: This media feature tag does not introduce
         new security considerations, as it simply indicates support for
         a basic SIP feature. If an attacker manages to remove the media
         feature tag, push notifications towards the client will be

Is "not" missing before "be"?

         requested.

---

From: Christer Holmberg <[email-masked]>
Date: Wed, 9 Jan 2019 14:09:21 +0000

Hi Alexey,

Thank You for the review! Please see inline.

>I have a couple of comments (and a few less serious ones) that I would like to discuss before recommending approval of this document:
>
> Spaces are not allowed in URIs unencoded, so your example is misleading. I suggest you change it to "pn-provider=apns" (i.e. delete space before and after "=").
>

Good catch. I will fix it (by deleting the spaces).

---

> How many periods are used in the value? If your example below is correct, can you clarify that Bundle ID itself contains periods?
>

The Bundle ID part of the Topic typically contains periods. The Team ID, and the service part of the Topic, do not contain periods.

Note that the Topic is actually created by Apple, and I don't even know whether the Apple PNS needs to separate the Bundle ID part from the service part. And, as far as I know, we don't need to separate the parts in our proxy implementation. We do separate the Team ID from the Topic, but we know the first period is the separator (as the Team Id does not contain periods).

> My understanding is that COMMA (defined in RFC 3261) allows folding whitespace, in particular CRLF. Do you really want this in values? It is Ok if you do, I just wanted to double check.

In the SIP syntax we are less strict than in some other protocols when it comes to folding whitespaces etc. The syntax above is very common in SIP.

---

> COLON is not used in this section or anywhere else in the document.

I have no idea how it got there, because as far as I can remember we haven't used COLON in any version of the draft.

I will remove it.

---

> Is "not" missing before "be"?

Correct. I will add "not".

Regards,

Christer

---

From: Alexey Melnikov <[email-masked]>
Date: Wed, 09 Jan 2019 14:58:32 +0000

Hi Christer,
I omitted the parts where we are in agreement.

> Thank You for the review! Please see inline.

 [snip]

> The Bundle ID part of the Topic typically contains periods. The Team ID, 
> and the service part of the Topic, do not contain periods.
> 
> Note that the Topic is actually created by Apple, and I don't even know 
> whether the Apple PNS needs to separate the Bundle ID part from the 
> service part. And, as far as I know, we don't need to separate the parts 
> in our proxy implementation. We do separate the Team ID from the Topic, 
> but we know the first period is the separator (as the Team Id does not 
> contain periods).

I think you either need to clarify that the Bundle ID can contain periods (so that if somebody wants to parse a particular pn-param value, they know to search for the last "." (*)) or you should avoid talking about internal structure altogether. Your text is sort of in between.

(*) - it might not be sensible to need to parse internal structure of the value. But I can't decide one way or another without investigating Apple API for this, which I haven't done.

> In the SIP syntax we are less strict than in some other protocols when 
> it comes to folding whitespaces etc. The syntax above is very common in 
> SIP.

I was just surprised to see this inside a quoted strings. In email quoted strings typically either disallow spaces inside quoted strings or only allow SP, but not CRLF. But anyway, no change is needed if this was intended.

Best Regards,
Alexey

---

From: Christer Holmberg <[email-masked]>
Date: Wed, 9 Jan 2019 15:49:36 +0000

Hi,

>> The Bundle ID part of the Topic typically contains periods. The Team 
>> ID, and the service part of the Topic, do not contain periods.
>> 
>> Note that the Topic is actually created by Apple, and I don't even 
>> know whether the Apple PNS needs to separate the Bundle ID part from 
>> the service part. And, as far as I know, we don't need to separate the 
>> parts in our proxy implementation. We do separate the Team ID from the 
>> Topic, but we know the first period is the separator (as the Team Id 
>> does not contain periods).
>
> I think you either need to clarify that the Bundle ID can contain periods (so that if somebody wants to parse a particular 
> pn-param value, they know to search for the last "." (*)) or you should avoid talking about internal structure altogether. 
> Your text is sort of in between.

I could add a note, saying that the Bundle ID can contain periods.

>> it comes to folding whitespaces etc. The syntax above is very common 
>> in SIP.
>
> I was just surprised to see this inside a quoted strings. In email quoted strings typically either disallow spaces inside quoted strings or only allow SP, but not CRLF. 
> But anyway, no change is needed if this was intended.

In SIP, the definitions of RDQUOT and LDQUOTE themselves also allow SP with CRLF :)

LWS  =  [*WSP CRLF] 1*WSP ; linear whitespace
SWS  =  [LWS] ; sep whitespace
LDQUOT  =  SWS DQUOTE; open double quotation mark
RDQUOT  =  DQUOTE SWS ; close double quotation mark

Regards,

Christer

---

From: Ben Campbell <[email-masked]>
Date: Wed, 9 Jan 2019 13:32:33 -0600

> On Jan 9, 2019, at 8:58 AM, Alexey Melnikov <[email-masked]> wrote:
> 
> Hi Christer,

Given that we don’t have change control over the parameters for Apple (or any other proprietary PNS), I’d be very careful about expectations to parse internal structure. These structures could change at any time.

---

From: Christer Holmberg <[email-masked]>
Date: Wed, 9 Jan 2019 19:55:02 +0000

Hi,

>>>>  The value of the pn-param URI parameter is a string that is 
>>>> composed  by two values, separated by a period (.): Team ID and 
>>>> Topic.  The  Team ID is provided by Apple and is unique to a development team.
>>>> 
>>>> associated with the application, separated by a period (.).  For 
>>>> VoIP  applications the service value is "voip".
>>>> 
>>> the service part. And, as far as I know, we don't need to separate 
>>> the parts in our proxy implementation. We do separate the Team ID 
>>> from the Topic, but we know the first period is the separator (as the 
>>> Team Id does not contain periods).
>> 
>> I think you either need to clarify that the Bundle ID can contain periods (so that if somebody wants to parse a particular pn-param 
>> value, they know to search for the last "." (*)) or you should avoid talking about internal structure altogether. Your text is sort of in between.
>> 
> Given that we don’t have change control over the parameters for Apple (or any other proprietary PNS), I’d be very careful about expectations to parse 
> internal structure. These structures could change at any time.

Sure. But, if it changes in a way that is not compatible with the current text/syntax, then one would need to register a new pn-provider value.

Regards,

Christer

---

From: Christer Holmberg <[email-masked]>
Date: Thu, 10 Jan 2019 17:25:52 +0000

Hi,

Based on Alexey's DISCUSS and COMMENTs, I have updated the pull request with the review changes.

The commit can be found at:

https://github.com/cdh4u/draft-sip-push/pull/31/commits/efcb6f0dea222c5c9dccaf0381216c3cf34be766

Regards,

Christer