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

From: Martin Thomson <[email-masked]>
Date: Mon, 9 Jul 2018 17:30:10 +1000

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
<[email-masked]> 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.

---

From: Christer Holmberg <[email-masked]>
Date: Wed, 11 Jul 2018 09:23:43 +0000

Hi,

Some things are addressed by Martin¹s pull request, so I will focus on the
things that are not - or are, but that I think needs to be discussed on
the list.

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

True. But we normally define the syntax what goes into an attribute.

The draft defines the JSON structure when the fingerprint is used.

If someone wants to use another JSON structure I think the draft should
say that the structure and associated procedures need to be specified.

>> 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:
>

In addition to the IANA considerations, there is a template for the
section defining the attribute.

See https://tools.ietf.org/html/draft-ietf-mmusic-mux-exclusive-12#page-3
for an example.

This has been driven very much by Paul K, so I assume he can give further
guidance.

>> 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.
>>

I think my comment was unclear, so I will try to re-phrase.

The identity attribute will obviously include fingerprints. My point was
that each of those fingerprints must also be placed in a fingerprint
attribute.

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

Correct, that¹s what I meant. One should e.g., not assume that the first
fingerprint attribute is linked to the first attribute listed in the
identity attribute, etc.

> But - as defined - a=identity includes ALL

Correct.

What if a fingerprint does exist in the identity attribute, but there is
no associated fingerprint attribute? Should it be discarded?

And, what if there is a fingerprint attribute, but the value does not
exist in the identity attribute. In this case I guess we could simply say
that the identity has not been asserted for that fingerprint.

>> 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.
>

One suggestion would be to simply discard all Identity attributes.

>> 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.
>

Note that in this comment I was talking about the *fingerprint* attribute
:)

Regards,

Christer