Skip to main content

Completely Encrypting RTP Header Extensions and Contributing Sources
draft-ietf-avtcore-cryptex-08

Yes


No Objection

Erik Kline
John Scudder
(Andrew Alston)
(Martin Duke)

Note: This ballot was opened for revision 06 and is now closed.

Murray Kucherawy
Yes
Comment (2022-06-06 for -06) Not sent
I have requested an SDP Directorate review, and will make sure that feedback gets addressed.
Zaheduzzaman Sarker
Yes
Comment (2022-06-15 for -06) Sent
Thanks for working on this important specification. I just wish if we could provide more stronger specification and don't allow the mix of cryptex with other not so secure exiting solutions.

I have some comments/suggestions/questions -

- the shepherd's write-up says there is not IPR declarations but https://datatracker.ietf.org/ipr/search/?submit=draft&id=draft-ietf-avtcore-cryptex return two hits on IPR declarations. The write up should be updated, was the WG not aware of these IPRs? 

- section 1.1 : it says -
                 
        "Accordingly, these identifiers can be considered a fingerprinting issue." 

     is there any analysis of this claim that can be referred to?"

- section 1.3 : it says -

        "While we considered possible solutions that would have encrypted more of the RTP header (e.g., the number of CSRCs), we felt the inability to parse the resultant packets with current tools, as well as additional complexity incurred, outweighed the slight improvement in confidentiality"

     if I have understood it correctly, I would suggest to rewrite this to something like -

        While considering the possible solutions that would have encrypted more of the RTP header (e.g., the number of CSRCs), lack of support on current tools was inevitable and the additional complexity outweighed the slight improvement in confidentiality by fixing previous solutions. Hence, new approach was needed to solve the described problem in section 1.1.
Erik Kline
No Objection
Francesca Palombini
No Objection
Comment (2022-06-16 for -06) Not sent
Thank you for the work on this document.

Many thanks to Henry Thompson for his ART ART review: https://mailarchive.ietf.org/arch/msg/art/xWXqmu5iN2hSwZMs4ZKHpvUg-QE/.

Francesca
John Scudder
No Objection
Paul Wouters
(was Discuss) No Objection
Comment (2022-08-19) Sent
Thanks for addressing my DISCUSS in -08. I have changed my ballot to No Objection.

Old DISCUSS:


Thanks for this document. It is clear, and I appreciate reading the rationale of the proposed solution. Just one discuss item:

   Peers MAY negotiate both Cryptex and the header extension mechanism
   defined in [RFC6904] via signaling, and if both mechanisms are
   supported, either one can be used for any given packet.  However, if
   a packet is encrypted with Cryptex, it MUST NOT also use [RFC6904]
   header extension encryption, and vice versa.

Why this complexity? Based on the Section 1, Cryptex is much more preferred.
Why allow "either one can be used for any given packet" instead of saying
if both are negotiated, Cryptex SHOULD be used? Or why not stronger, if
both peers support Cryptex, RFC6904 SHOULD NOT (MUST NOT?) be used?

COMMENTS:



The Shepherds write-up with respect to IPR shows two authors confirming they
have filed "all required disclosures" but does not list whether this
means "there are non", "there are some, compatible with IETF" or that
there is "disclosed, incompatible with IETF". The Shepherds write up
does state no disclosures are filed. I find the two authors' response a
bit odd.

Section 1.2 describes a problem of "using a few more bytes", but how is that
a real problem? Does it really matter? The other reasons stated seem
real issues to me but the "few more bytes" seems fairly harmless?

        we felt

Who or what is "we"? Probably rewrite to "It was considered" or something?

   Alternatively, if the implementation considers the use of this
   specification mandatory and the "defined by profile" field does not
   match one of the values defined above, it SHOULD stop the processing
   of the RTP packet and report an error for the RTP stream.

Why is this not a MUST stop ? If it is mandatory, what is an example
where it can continue processing an RTP packet without the mandatory requirement?

It seems Section 3 could be just a last paragraph of the Introduction?

Nits:

Section 1.1 first line misses "(SRTP)" acronym

CSRC not expanded before first use.

Section 1.2  "RFC6904" is not properly linked

Section 4's title could be improved, eg "Signaling of Cryptex Support"

Section 4's opening line could be simplified eg:

   Cryptex support is indicated via the new "a=cryptex" Session
   Description Protocol (SDP) attribute.

SDP not expanded on first use

BUNDLE is not expanded (or linked to a reference) on first use.

[RFC8285] in Section 5 is not a proper link

I personally dislike "+-+-+-+-+-+-+-+-+-+-+-+-+-+-+" and prefer the
less christmas tree "+------+----------+" style diagram :)
Roman Danyliw
(was Discuss) No Objection
Comment (2022-07-25 for -07) Sent for earlier
Thank you to Rifaat Shekh-Yusef for the SECDIR review. 

Thank you for addressing my COMMENTS and DISCUSS feedback.
Warren Kumari
No Objection
Comment (2022-06-13 for -06) Sent
Thank you for writing this document; like Rob I'm far from an RTP expert, but I also found it readable and useful.



I do have a question, as well as some nits to try and make if even better.

Question:
"[RFC8285] defines two values for the "defined by profile" field for carrying one-byte and two-byte header extensions." - RFC8285 defines 0XBEDE, and has a cute justification. This document defines 0xCODE and 0XC2DE... but it was quite hard to figure out what the *other* value that RFC8285 defined -- and that's my issue. Should this document create an IANA registry to track these? Is there already a registry? E.g: If I magically become an RTP expert and write draft-ietf-avtcode-even-more-awesome-protocol how do I know not to use any of these existing numbers? 



1: "If BUNDLE is in use and the a=cryptex attribute" - in all of the other instances where you have 'a=cryptex', you have it enclosed in double-quotes. I suggest doing so here too for consistency. 

2: Section 5.1: "When the mechanism defined by this specification has been negotiated, sending a RTP packet that has any CSRCs or contains any {RFC8285}} header extensions follows the steps below." - something went kablooie in your formatting / markdown / kramdown / whatever and you have an extra '}' breaking the reference...

3: I got confused when reading "Note that the 4 bytes at the start of the extension block are not encrypted, as required by [RFC8285]." - it seemed ambiguous to me. I'd suggest "Note that, as required by [RFC8285], the 4 bytes at the start of the extension block are not encrypted." 

I did already mention that these are nits -- this comes from 'nitpick', which dictionary.com says is "to be excessively concerned with or critical of inconsequential details." -- feel free to ignore them :-)
Alvaro Retana Former IESG member
No Objection
No Objection (2022-06-15 for -06) Sent
I support Paul's DISCUSS.

I believe that this document should formally Update rfc3711.  The mechanism described here is a replacement for what rfc6904 defined.  That original mechanism was tagged as formally Updating rfc3711 -- this work should also be tagged that way.
Andrew Alston Former IESG member
No Objection
No Objection (for -06) Not sent

                            
Lars Eggert Former IESG member
(was Discuss) No Objection
No Objection (2022-06-16) Sent for earlier
## Comments

### Section 1.2, paragraph 1
```
     [RFC6904] was proposed in 2013 as a solution to the problem of
     unprotected header extension values.  However, it has not seen
     significant adoption, and has a few technical shortcomings.
```
Would it be time to deprecate 6904? (Also see Paul's DISCUSS and Alavaro's
comment.)

### Inclusive language

Found terminology that should be reviewed for inclusivity; see
https://www.rfc-editor.org/part2/#inclusive_language for background and more
guidance:

 * Term `master`; alternatives might be `active`, `central`, `initiator`,
   `leader`, `main`, `orchestrator`, `parent`, `primary`, `server`

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Section 5.1, paragraph 1
```
     When the mechanism defined by this specification has been negotiated,
     sending a RTP packet that has any CSRCs or contains any {RFC8285}}
     header extensions follows the steps below.  This mechanism MUST NOT
     be used with header extensions other than the [RFC8285] variety.
```
Likely Markdown nit: {RFC8285}}

### Outdated references

Reference `[RFC4566]` to `RFC4566`, which was obsoleted by `RFC8866` (this may
be on purpose).

### Grammar/style

#### Section 5, paragraph 0
```
fication has been negotiated, sending a RTP packet that has any CSRCs or cont
                                      ^
```
Use "an" instead of "a" if the following word starts with a vowel sound, e.g.
"an article", "an hour".

#### Section 5.1, paragraph 4
```
ecryption Procedure, and passed to the the next layer to process the packet a
                                   ^^^^^^^
```
Possible typo: you repeated a word.

#### Section 6.2, paragraph 9
```
ence number and SSRC identifier. Accordingly these values are also not encryp
                                 ^^^^^^^^^^^
```
A comma may be missing after the conjunctive/linking adverb "Accordingly".

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool
Martin Duke Former IESG member
No Objection
No Objection (for -06) Not sent

                            
Robert Wilton Former IESG member
No Objection
No Objection (2022-06-13 for -06) Sent
Hi,

I don't know RTP at all, but found that document very clear and easy to read, so thank you for that.

Only one trivial comment:

   If the packet contains solely one-byte extension ids, the 16-bit RTP
   header extension tag MUST be set to 0xC0DE to indicate that the
   encryption has been applied, and the one-byte framing is being used.
   If the packet contains only two-byte extension ids, the header
   extension tag MUST be set to 0xC2DE to indicate encryption has been
   applied, and the two-byte framing is being used.

This text doesn't say what to do if the packet contained a mix of 1 and 2 byte extension-ids.  My presumption is that this isn't allowed.  I will leave it to the authors to decide whether to clarify this, but acknowledge that it may be obvious to everyone familiar with RTP ...

Regards,
Rob