Re: [rtcweb] SDP directorate review of SDP Identity attribute (draft-ietf-rtcweb-security-arch-14)

[Martin Thomson <[e-mail]>]

Hi Christer,

I went through this review, and there are some changes that I think
are useful.  I'm not sure if there isn't some misunderstanding on some
others.

I have PRs up on the repo for the simple ones.  The suggestion to
include the usual section headers might take some more time to sort
out.

On Thu, May 24, 2018 at 6:20 PM Christer Holmberg
<[e-mail]> wrote:
> As the SDP attribute definition should be able to stand on its own legs, there should be a description/reference to the IdP solution that it is used for.

I believe that context is sufficient for that purpose.

> Also, if the usage of the attribute is scoped to devices implementing the WebRTC specification, that should be indicated.

The mechanism isn't necessarily scoped to this particular usage
domain.  I've proposed some text, but I don't think that it's
necessary.  The use of JSON allows for many things, some of which
might be extending this to other uses.

> Q1 (Structure):
>
> The document lacks the offer/answer procedure structure used for SDP attributes.

This is a fair criticism.  That needs work.  Not because the headings
are valuable, but because there is missing information that those
headings cause you to think about (which is why they exist, of
course).  For instance, we don't really say anything here about
whether the value can change (it can, but the identity can't).

> Q2. The mux category is missing.

I couldn't find this in the draft or on the list, but I found a branch
that adds this, so it's good.

> Q3 (Grammar):
>     identity-attribute  = "identity:" identity-assertion
>                                         *( SP identity-extension )

Sure.  I'm going with this because anything else would break existing
deployments (such as they are).  ';' is now allowed in the extensions.

> Q4 (Attribute definition):
> Historically there have been problems with the definitions of new SDP attributes. Especially, the manner of defining the syntax was inconsistent. RFC4566 gave insufficient guidance on how to do this. draft-ietf-mmusic-rfc4566bis (especially section 8.2.4.1) has provided more guidance on how to do this. Please do your definitions in that style. For instance:

How is the definition in the IANA considerations section insufficient?


> Q5:
> The draft says that, at minimum, the fingerprint needs to be bound to the identity.
>
> First, I assume this means that, for each assertion, the associated SDP fingerprint attribute must be included in the offer/answer. If so, please include text about that..

The text says:

> The identity attribute attests to all "a=fingerprint" attributes in the session description. It is therefore a session-level attribute.
>
> Multiple "a=fingerprint" values can be used to offer alternative certificates for a peer.  The
"a=identity" attribute MUST include all fingerprint values that are included in
"a=fingerprint" lines.

That would seem to be sufficient for this purpose.

> Second, it would be good to indicate that, in the SDP, there is no link between a given assertion and the associated fingerprint.

I don't know what you are looking for here.  If you are saying that
the SDP doesn't include a pointer from a=identity to a=fingerprint, or
vice versa, that's right.  But - as defined - a=identity includes ALL
a=fingerprint instances, so any pointer would be redundant.

> Q6:
> Section 5.6.4.1 says:
>    "The "a=identity" attribute MUST include all
>    fingerprint values that are included in "a=fingerprint" lines."
> It is unclear what “attribute MUST include all fingerprint values” means.

The previous section describes the construction of the value that is
used for a=identity.  I don't understand how that could be clearer.
I've tried to clarify this in a PR.

> It should also be clarified that, if the attribute is used on media-level in an m- section, it contains the assertion for each fingerprint associated with that m- section.

It's session-level only.

> Q7:
> Section 5.6.4.2 says:
>   “The semantics of multiple identity attributes are undefined.”
> First, I don’t see the difference in having a single attribute with multiple assertions, or multiple attributes with single assertions. Having said that, if we only want to allow one way, I would suggest to simply forbid multiple attributes.

That's OK, but what is the usual policy for handling malformed
attributes if you know the syntax of the attribute?  What handling
should we specify here?

> Second, it needs to be clarified that the rule apply to a given m- section.

It's session-level only.

> Q8:
> In the example in Section 5.6.4.1 the SDP fingerprint attribute is included as a session-level attribute. However, it is currently only defined as a media-level attribute.

I can't find any evidence of this being defined as media-level.  It
says session-level (only) in several places.