Subject: [siprec] AD evaluation: draft-ietf-siprec-metadata-18

From: Alissa Cooper <[email-masked]>
Date: Tue, 17 Nov 2015 16:17:32 -0800

I have reviewed this document in preparation for IETF LC. I believe it needs some additional work before going to IETF LC. The document is not particularly readable and there are many editorial inconsistencies that give the impression that it has not been thoroughly reviewed.

My substantive comments are below, along with some nits.

Comments:

== Section 6 ==

Please do a thorough edit pass of this section to ensure that each Attributes sub-section is consistent with its corresponding XML element subsection. I found a lot of inconsistencies and I wasn’t sure if they were purposeful or by-products of sloppy drafting.

I made a bunch of comments below about unique element IDs. I think a lot of the confusion could be resolved by having a section up front that explains how the IDs are expected to be generated for each element and what the naming convention for the IDs is.

== Section 6.1 ==

For avoidance of doubt, I think it would be better to list the two time fields separately (unless the slash is some UML notation I’m not aware of):

start-time
end-time

And end-time would be optional. They should then also be explained separately in 6.1.1 and 6.1.3. Using a slash implies they are alternatives, but you can’t have an end without a start, right?

== Section 6.2.1 ==

I don’t understand why there is a Unique-ID attribute listed here, rather than just calling this “group_id.” If these are both meant to refer to the same thing, why give the UML attribute a different name from the XML attribute?

== Section 6.2.3 ==

I think “group-id” should be “group_id.”

== Section 6.3.1 ==

- This section needs to explain how the CS identifier is generated, how its uniqueness is guaranteed, and why both the CS identifier and the SIP session ID are needed.

- "The SRS is assumed to behave in accordance with policy.” What policy? Whose policy? This is pretty obtuse as written.

- "However if there are implementations where SRC has enough information, this could be sent as Extension Data attached to the CS.” I assume the main barrier here isn’t whether the SRC “has enough information,” but rather whether the entity operating the SRC has a desire to specify its own policy preferences and whether any entities operating SRSes have any interest in supporting them in an automated fashion. So I would suggest that this needs to be re-framed to explain the circumstance better.

== Section 6.3.3 ==

- As with the group_id, I find it quite confusing that both a “CS Identifier” and a “session_id” are specified. The term “session_id” should be used everywhere to denote it.

- “Each session element has unique 'session_id’ attribute which helps to uniquely identify the CS.” It helps? Shouldn’t it either uniquely  identify the CS or not? And what are the circumstances under which it doesn’t?

== Section 6.5 ==

For groups and sessions, the unique ID for the element was listed as a UML attribute. For participants, it is not. Why not?

== Section 6.5.1 ==

- "An AoR MAY be a SIP/SIPS/TEL URI.” Why is this a MAY? Why not specify precisely what is expected here?

- Please list which SIP header(s) the Names and AoRs will be drawn from.

== Section 6.5.3 ==

- Why is the AoR element specified as a "SIP/SIPS URI FQDN or IP address” but the UML attribute corresponding to it is specified as "SIP/SIPS/TEL URI”?

== Section 6.6 ==

Why are the capabilities of the participant being recorded? It’s not obvious from the text.

== Section 6.6.3 ==

Could you be more specific about how “RFC 4235 encoding” will be used to express a list of capabilities in XML? I’m missing where that is specified in RFC 4235, and the XML schema in section 9 of this draft doesn’t specify it.

== Section 6.7.1 ==

Why is the attribute here called “Content” but in 6.7 and 6.7.3 called “Content-type”?

== Section 6.7.3 ==

Same comment above regarding “helps to uniquely identify.”

== Section 6.8.1 ==

Again here, there is a reference to a “Unique_id” that is not defined anywhere. I think what you mean is stream_id. Please be consistent in naming the attributes and elements.

== Section 9 ==

Why does the schema specify the ability to include elements from other namespaces in the requestsnapshot element? I thought the expectation was that there might be one requestreason given and that’s all. 

== Section 10 ==

The normative language in the first sentence is not appropriate. I would suggest taking these two sentences from below and making them the first two sentences of the section:

"This document describes an extensive set of metadata that may be recorded by the SRS. Most of the metadata could be considered private data.”	

== Section 10.1 ==

- "The mechanism used for conveying the metadata information MUST ensure integrity and SHOULD ensure confidentially of the information.” Why is there a difference in normative requirements for integrity and confidentiality?

- I think the last paragraph should echo the language from draft-ietf-siprec-protocol about storage and drop the notion of “retention policy” (since that often refers to the duration of retention). Here is a suggested edit:

Some implementations may have the SRC choose parts of metadata that can be sent to the SRS.  In other cases, SRCs may send metadata that is not appropriate for the SRS to record. Which metadata is actually recorded by the SRS must be carefully considered to balance privacy concerns with usability. Implementations MUST control what metadata is recorded, and MUST NOT save metadata sent by the SRC that does not conform to the recording policy of the SRS. Metadata in storage needs to be provided with a level of security that is comparable to that of the recording session.

Nits:

There is a lot of inconsistent capitalization in this document that could lead to misinterpretation. The word “metadata” is inconsistently capitalized — please change to lowercase throughout. The names of UML and XML attributes and elements are also inconsistently capitalized (and in some cases hyphenated) all over the document. They should either start with a lowercase letter or an uppercase letter but they should all be capitalized and hyphenated the same way throughout the document.

The use of acronyms is also inconsistent. There is no need to continually spell something out and also provide its acronym — do one or the other. E.g., spell out “communication session (CS)” on first use and then use the acronym thereafter, or spell it out every time, but don’t do both sometimes and use one or the other at other times.

== Section 3 ==

Might be useful to include a pointer to RFC 6341 for siprec-specific terms.

== Section 4 ==

Expand “CS” on first use.

s/structure of a metadata/structure of metadata/

s/linkages between the classes represents/linkages between the classes represent/

s/associations or Composition/associations or composition/

== Section 5.1 ==

s/a XML document/an XML document/

== Section 6.1.3 ==

s/Start and End time value/Start and End time values/

== Section 6.2.2 ==

OLD
Each Communication Session Group object has to be associated with
      one or more RS [Here each RS can be setup by the potentially
      different SRCs]

NEW
Each Communication Session Group object has to be associated with
      one or more RS. Here each RS can potentially be setup by a
      different SRC.

s/per CS Group [e.g. Consult Transfer]/per CS Group (for example, in the case where the call is transferred)./

== Section 6.2.3 ==

OLD
The <group> element is an optional element provides the information
   about the communication session group

NEW
The <group> element is an optional element that provides the information
   about the communication session group.

s/(CSG)object/(CSG) object/

== Section 6.3.1 ==

s/as seen by SRC/as seen by SRC./

== Section 6.3.2 ==

Expand 3pcc on first use.

s/Stream in persistent/A stream in a persistent/

s/CS is created/the CS is created/

OLD
Each CS has to be associated with one more RS [ Here each RS can
      be potentially setup by different SRCs]

NEW
Each CS has to be associated with one or more RSes. Each RS can
      be potentially setup by a different SRC.

== Section 6.3.3 ==

s/SIP dialog’s/SIP dialogs/

s/sipSessionID/sipSession-ID/

OLD
For e.g.,
   Three participants A, B and C are in conference that has a Focus(F)
   acting as SRC.  The metadata sent from SRC to SRS will likely have
   three sipSession-Id elements that correspond to the SIP dialog's the
   Focus has with each of the three participants.

NEW
For example, if three participants A, B and C are in a conference that has a focus
   acting as SRC, the metadata sent from the SRC to the SRS will likely have
   three sipSession-Id elements that correspond to the SIP dialogs the
   focus has with each of the three participants.

== Section 6.5.2 ==

Whisper calls should be explained.

== Section 6.5.3 ==

OLD
Participant MUST have a NameID complex element which contains AoR as
   attribute and Name as element.  AOR element is SIP/SIPS URI FQDN or
   IP address which represents the user. name is an optional element to
   represent display name.

NEW
Participant MUST have a NameID complex element which contains an AoR as
   an attribute and Name as an element. The AoR element is a SIP/SIPS URI FQDN or
   IP address which represents the user. name is an optional element to
   represent display name.

== Section 6.10 ==

Great example here of inconsistent usage: “Unique id,” “UUID,” and “unique-id” all used to refer to exactly the same thing. Please fix.

== Section 10 ==

- I don’t understand why this is split into two subsections or why the subsection is titled “Connection Security.”

- This section should reference the security considerations in draft-ietf-siprec-protocol.

---

From: "Hutton, Andrew" <[email-masked]>
Date: Thu, 19 Nov 2015 09:38:07 +0000

I think it is fair to say that during the WGLC process we found it difficult to get fresh eyeballs on this draft and the subject matter makes it a difficult document to review if you’re not deep in to the implementation.

Please can the editors respond to Alissa’s comments and then we can decide how to move this forward.

Regards
Andy (SIPREC Co-Chair).

From: siprec [mailto:[email-masked]] On Behalf Of Alissa Cooper
Sent: 18 November 2015 00:18
To: [email-masked]
Subject: [siprec] AD evaluation: draft-ietf-siprec-metadata-18

Comments:

== Section 6 ==

== Section 6.1 ==

start-time
end-time

== Section 6.2.1 ==

== Section 6.2.3 ==

== Section 6.3.1 ==

== Section 6.3.3 ==

== Section 6.5 ==

== Section 6.5.1 ==

== Section 6.5.3 ==

== Section 6.6 ==

== Section 6.6.3 ==

== Section 6.7.1 ==

== Section 6.7.3 ==

== Section 6.8.1 ==

== Section 9 ==

== Section 10 ==

== Section 10.1 ==

Nits:

== Section 3 ==

== Section 4 ==

== Section 5.1 ==

== Section 6.1.3 ==

== Section 6.2.2 ==

OLD

      different SRCs]

NEW

      different SRC.

== Section 6.2.3 ==

OLD

NEW

== Section 6.3.1 ==

== Section 6.3.2 ==

OLD

NEW

== Section 6.3.3 ==

OLD

For e.g.,

NEW

== Section 6.5.2 ==

== Section 6.5.3 ==

OLD

   represent display name.

NEW

   represent display name.

== Section 6.10 ==

== Section 10 ==

---

From: "Ram Mohan R (rmohanr)" <[email-masked]>
Date: Thu, 19 Nov 2015 17:32:34 +0000

Hi Alissa, Thanks for your feedback. Please see inline

>
>
>From:  siprec <[email-masked]> on behalf of Alissa Cooper
><[email-masked]>
>Date:  Wednesday, 18 November 2015 at 5:47 AM
>To:  "[email-masked]" <[email-masked]>
>I have reviewed this document in preparation for IETF LC. I believe it
>needs some additional work before going to IETF LC. The document is not
>particularly readable and there are many editorial inconsistencies that
>give the impression that it
> has not been thoroughly reviewed.
>
>Please do a thorough edit pass of this section to ensure that each
>Attributes sub-section is consistent with its corresponding XML element
>subsection. I found a lot of inconsistencies and I wasn¹t sure if they
>were purposeful or by-products of
> sloppy drafting.

One of the reasons  for the inconsistency in attribute names is because -
we started with UML
diagram and defining attributes for each element and then started writing
XML
elements for the attributes.  I think it make sense to use attribute name
in XML (as that is the one used by all implementations) for the UML
attribute names as well.
Note that some of the UML attributes don’t have XML element at all since
those attributes will be carried in SDP itself (for e.g; content-type
attribute )

>
>I made a bunch of comments below about unique element IDs. I think a lot
>of the confusion could be resolved by having a section up front that
>explains how the IDs are expected to be generated for each element and
>what the naming convention for
> the IDs is.

With the above change, I believe this may not be needed.
>
>== Section 6.1 ==
>
>For avoidance of doubt, I think it would be better to list the two time
>fields separately (unless the slash is some UML notation I¹m not aware
>of):
>
>start-time
>end-time

Will fix this.
>
>And end-time would be optional. They should then also be explained
>separately in 6.1.1 and 6.1.3. Using a slash implies they are
>alternatives, but you can¹t have an end without a start, right?

Right. Will fix

>
>== Section 6.2.1 ==
>
>I don¹t understand why there is a Unique-ID attribute listed here, rather
>than just calling this ³group_id.² If these are both meant to refer to the
>same thing, why give the UML attribute a different name from the XML
>attribute?

I prefer to use group_id in all places as that is what is used in XML.

>
>== Section 6.2.3 ==
>
>I think ³group-id² should be ³group_id.²

Will fix this.

>
>== Section 6.3.1 ==
>
>- This section needs to explain how the CS identifier is generated, how
>its uniqueness is guaranteed, and why both the CS identifier and the SIP
>session ID are needed.

I tried to look at the history as to why CS Identifier was added. It looks
this was added as placeholder for a unique CS session identifier. Will sip
session id that is achieved.
And sip session id is listed as another attribute in this block. I don’t
see a need for this CS identifier. Also the XML schema does not have any
element for this, so this seems redundant.
I will let some one in WG comment on this on whether there was any other
reason (that I don’t seem to find) on why this attribute is present.

IMO CS identifier should be removed from the UML.

>
>- "The SRS is assumed to behave in accordance with policy.² What policy?
>Whose policy? This is pretty obtuse as written.

SRS policy. The policy is for actions like how long an SRS can retain the
recording which is specified in the previous sentence.
 
>
>- "However if there are implementations where SRC has enough
> information, this could be sent as Extension Data attached to the CS.² I
>assume the main barrier here isn¹t whether the SRC ³has enough
>information,² but rather whether the entity operating the SRC has a desire
>to specify its
> own policy preferences and whether any entities operating SRSes have any
>interest in supporting them in an automated fashion.

Yes that’s right. Will re-word the sentence to some thing like this. Does
this sound better and takes care of this and the previous comment ?

EXISTING:
This document does not specify attributes relating to what should
   happen to a recording of a CS after it has been delivered to the SRS.
   (E.g., how long to retain the recording, what access controls to
   apply.)  The SRS is assumed to behave in accordance with policy.  The
   ability for the SRC to influence this policy is outside the scope of
   this document.  However if there are implementations where SRC has
   enough information, this could be sent as Extension Data attached to
   the CS.

NEW:
This document does not specify attributes relating to what should
   happen to a recording of a CS after it has been delivered to the SRS.
   (E.g., how long to retain the recording, what access controls to
   apply.)  The SRS is assumed to behave in accordance with its policy.
The
   ability for the SRC to influence this policy is outside the scope of
   this document.  However if there are implementations where SRC has
has desire to specify its own policy preferences, this could be sent as
Extension Data attached to the CS.

>So I would suggest
>that this needs to be re-framed to explain the circumstance better.
>
>== Section 6.3.3 ==
>
>- As with the group_id, I find it quite confusing that both a ³CS
>Identifier² and a ³session_id² are specified. The term ³session_id² should
>be used everywhere to denote it.

The session_id here is the XML id generated for the <session> XML element.
This id follows the UUID semantics as defined in section 6.10 and then
does a base64 encoding of the same.

>
>- ³Each session element has unique 'session_id¹ attribute which helps to
>uniquely identify the
> CS.² It helps? Shouldn¹t it either uniquely  identify the CS or not?

The SRC is expected to use a different session_id for each session XML
element it sends in the metadata snapshot.
So this will be unique for a given session XML element.

>And
>what are the circumstances under which it doesn¹t?
>
>== Section 6.5 ==
>
>For groups and sessions, the unique ID for the element was listed as a UML
>attribute. For participants, it is not. Why not?

The UML lists only the XML elements inside a complex XML element
represented by the UML block. Participant does not have any XML element
like unique id.

>
>== Section 6.5.1 ==
>
>- "An AoR MAY be a SIP/SIPS/TEL URI.² Why is this a MAY? Why not specify
>precisely what is expected here?

Will fix this AoR can be one of the SIP/SIPS/TEL URI

>- Please list which SIP header(s) the Names and AoRs will be drawn from.

Ok will add some text for this as well.

>
>== Section 6.5.3 ==
>
>- Why is the AoR element specified as a "SIP/SIPS URI FQDN or IP address²
>but
> the UML attribute corresponding to it is specified as "SIP/SIPS/TEL URI²?

I am not sure why this was like this. Will check this and get back.
 
>
>== Section 6.6 ==
>
>Why are the capabilities of the participant being recorded? It¹s not
>obvious from the text.

There was a requirement to indicate the capability of the participant (for
e.g; whether the participant is “automata”, “focus”, “msg-taker” and so
on) 
I believe SRS vendors would use this information along with other metadata.

>
>== Section 6.6.3 ==
>
>Could you be more specific about how ³RFC 4235 encoding² will be used to
>express a list of capabilities in XML? I¹m missing where that is specified
>in RFC 4235, and the XML schema in section 9 of this
> draft doesn¹t specify it.

This was discussed in the WG and the decision was to only include two XML
elements that were defined in RFC 4235. In stead of importing the entire
XML schema
We decided to duplicate the XML elements needed in this schema.
<xs:complexType>
                  <xs:attribute name="pname" type="xs:string"
use="required"/>
                  <xs:attribute name="pval" type="xs:string"
use="required"/>
                </xs:complexType>

There is some inconsistency here. In the XML schema these XML elements are
defined inside <participant> complex element where as in the UML they
are in participantsession UML block. That needs to be fixed.
>
>== Section 6.7.1 ==
>
>Why is the attribute here called ³Content² but in 6.7 and 6.7.3 called
>³Content-type²?

It should be Content-type.
>
>== Section 6.7.3 ==
>
>Same comment above regarding ³helps to uniquely identify.²
>
>== Section 6.8.1 ==
>
>Again here, there is a reference to a ³Unique_id² that is not defined
>anywhere. I think what you mean is stream_id.

Yes. This should be stream_id.

> Please be consistent in
>naming the attributes and elements.
>
>== Section 9 ==
>
>Why does the schema specify the ability to include elements from other
>namespaces in the requestsnapshot element? I thought the expectation was
>that there might be one requestreason given and that¹s all.

Yes. This was from the discussions in the WG. We want to have a
flexibility to allow a SRS include any extension data in the request
(additional information about why it is requesting for snapshot).

<xs:element name="requestsnapshot" type="tns:requestsnapshot"/>
     <xs:complexType name="requestsnapshot">
       <xs:sequence>
          <xs:element name="requestreason" type="tns:name"  minOccurs="0"/>
          <xs:any namespace='##other'
            minOccurs='0'
            maxOccurs='unbounded'
            processContents='lax'/>
       </xs:sequence>
     </xs:complexType>

>
>== Section 10 ==
>
>The normative language in the first sentence is not appropriate. I would
>suggest taking these two sentences from below and making them the first
>two sentences of the section:
>
>"This document describes an extensive set of metadata that may be recorded
>by the SRS. Most of the metadata
> could be considered private data.²
>
>== Section 10.1 ==
>
>- "The mechanism used for conveying the metadata information MUST ensure
>integrity and SHOULD ensure confidentially
> of the information.² Why is there a difference in normative requirements
>for integrity and confidentiality?
>
>- I think the last paragraph should echo the language from
>draft-ietf-siprec-protocol about storage and drop the notion of ³retention
>policy² (since that often refers to the duration of retention). Here is a
>suggested edit:
>
>Some implementations may have the SRC choose parts of metadata that can be
>sent to the SRS.  In other cases, SRCs may send metadata that is not
>appropriate for the SRS to record. Which metadata is actually recorded by
>the SRS
> must be carefully considered to balance privacy concerns with usability.
>Implementations MUST control what metadata is recorded, and MUST NOT save
>metadata sent by the SRC that does not conform to the recording policy of
>the SRS. Metadata in storage needs
> to be provided with a level of security that is comparable to that of the
>recording session.

I am fine with including the above text.

Will take care of below nits as well. Thanks.

Regards,
Ram

>
>
>Nits:
>
>There is a lot of inconsistent capitalization in this document that could
>lead to misinterpretation. The word ³metadata² is inconsistently
>capitalized ‹ please change to lowercase throughout. The names of UML and
>XML attributes and elements are
> also inconsistently capitalized (and in some cases hyphenated) all over
>the document. They should either start with a lowercase letter or an
>uppercase letter but they should all be capitalized and hyphenated the
>same way throughout the document.
>
>The use of acronyms is also inconsistent. There is no need to continually
>spell something out and also provide its acronym ‹ do one or the other.
>E.g., spell out ³communication session (CS)² on first use and then use the
>acronym thereafter, or
> spell it out every time, but don¹t do both sometimes and use one or the
>other at other times.
>
>== Section 3 ==
>
>Expand ³CS² on first use.
>
>s/per CS Group [e.g. Consult Transfer]/per CS
> Group (for example, in the case where the call is transferred)./
>
>== Section 6.2.3 ==
>
>OLD
>s/SIP dialog¹s/SIP dialogs/
>
>Great example here of inconsistent usage: ³Unique id,² ³UUID,² and
>³unique-id² all used to refer to exactly the same thing. Please fix.
>
>== Section 10 ==
>
>- I don¹t understand why this is split into two subsections or why the
>subsection is titled ³Connection Security.²
>

---

From: Paul Kyzivat <[email-masked]>
Date: Thu, 19 Nov 2015 13:01:18 -0500

Alissa,

It is very valuable to have a review from a fresh set of eyes!

Ram has covered most of your points. I have one small comment:

> - Please list which SIP header(s) the Names and AoRs will be drawn from.

The SRC needs to have some discretion here, because deployments don't 
all use the same thing.

For instance there is From or P-Asserted-Identity. I don't think we can 
assert a rule for which to use if both are present. It has to be policy. 
Profiles for particular environments (e.g. 3gpp) can nail this down.

Also, it is possible to use siprec to record sessions that weren't set 
up with SIP. (E.g., could be a WebRTC session.) Again the SRC would have 
to use policy to decide what to use here.

	Thanks,
	Paul

---

From: Alissa Cooper <[email-masked]>
Date: Thu, 19 Nov 2015 12:21:52 -0800

Hi Ram,

Thanks. A few comments below.

> On Nov 19, 2015, at 9:32 AM, Ram Mohan R (rmohanr) <[email-masked]> wrote:
>> 

This is better, thanks. I would suggest s/its policy/its local policy/

> I believe SRS vendors would use this information along with other metadata.

It would help to include a motivating example in the text here.

>          <xs:any namespace='##other'

Ah right, I recall that now.

Will await a rev with the changes.

Alissa

---

From: Alissa Cooper <[email-masked]>
Date: Thu, 19 Nov 2015 12:25:55 -0800

Paul,

> On Nov 19, 2015, at 10:01 AM, Paul Kyzivat <[email-masked]> wrote:
> 
> Alissa,
> 
> The SRC needs to have some discretion here, because deployments don't all use the same thing.
> 
> For instance there is From or P-Asserted-Identity. I don't think we can assert a rule for which to use if both are present. It has to be policy. Profiles for particular environments (e.g. 3gpp) can nail this down.

That’s fine, I should have said “may be drawn from.” 

Alissa

> 
> Also, it is possible to use siprec to record sessions that weren't set up with SIP. (E.g., could be a WebRTC session.) Again the SRC would have to use policy to decide what to use here.
> 
> 	Thanks,
> 	Paul
> 
> _______________________________________________
> siprec mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/siprec