Subject: Re: [Cellar] AD Evaluation of draft-ietf-cellar-ebml-08.

From: Ben Campbell <[email-masked]>
Date: Fri, 1 Mar 2019 15:39:05 -0600

Hi, apologies for the delay. Catching up on the comments below. I’m skipping the comments with “Done in github” responses until I can review the draft update, which I will start in a few minutes.

Thanks!

Ben.

> On Jan 19, 2019, at 3:30 AM, Steve Lhomme <[email-masked]> wrote:
> 
> Hi Ben,
> 
> On 07/01/2019 22:14, Ben Campbell wrote:
>> Hi,
>> 
>> This is my AD Evaluation of draft-ietf-cellar-ebml-08.
>> 
>> Thanks for this work. It's generally on the right track, but I have a number of comments that I would like to discuss prior progressing.
>> 
>> Thanks!
>> 
>> Ben.
>> 
>> *** Substantive Comments ***
>> 
>> - The Shepherd Report says "There are minor nits to be resolved (adding one RFC 2119 keyword and some <element>schema  attributes which are mentioned but not defined."
>> 
>> Do that comment still apply? To be clear, those are more than nits. Adding a 2119 keyword is substantive change that needs to be agreed to but the working group.
> 
> I don't understand this. RFC2119 has been mentioned in the header and a link is generated at the bottom of the document since 2016. I don't think we should include the whole document, so what is missing ?

The document shepherd will need to answer this, since it is a shepherd review comment. But I assumed that by “2119 keyword” he meant that there was some normative language in the form of a a MUST, SHOULD, MAY, etc was needed somewhere in the document. That is what I mean by it being a substantive change.

Also, I don’t know if the comment is still applicable; again the shepherd would need to answer.

> 
>>  Undefined attributes suggest that the draft is incomplete. If the comment is still accurate, then these need to be fixed prior to progressing the draft. If it is not accurate, then I ask the shepherd to please update the shepherd report.
> 
> Which attributes are mentioned but not defined ? All the "attribute" mentions I see in the text are all attributes described further in their own section.

Again, the draft shepherd will need to answer that. It’s possible these were comments on an earlier revision that may have been resolved. The point of my questions was to make sure that was the case before moving forward.

> 
>> 
>> - I'm a little surprised to see the draft describe EBML as a general purpose markup language format, as opposed to one designed to be used by Matroska and similar technologies. I suspect others will also be surprised during the IETF LC and IESG review. The review bar is higher for the IETF to recommend EBML as a general purpose markup language than it is to recommend it for specific purposes. I also note that the CELLAR charter talks about EBML in terms of being used by Matroska and FFV1.
> 
> It's actually not used in FFV1, only Matroska. But we are aware of other people using EBML for other purposes/formats and some considering to use it.
> 
>> Would it be acceptable to add some scoping language to the effect of "EBML is used by Matroska[citation] and FFV1[citation]. It MAY be used for use cases similar to those. The applicability of EBML for other use cases is beyond the scope of this document".
> 
> Yes, (apart from the FFV1 part).
> Done in this PR https://github.com/Matroska-Org/ebml-specification/pull/199 <https://github.com/Matroska-Org/ebml-specification/pull/199>

> 
>> 
>> - There are several defined elements/attributes related to versioning, but I don't find an explanation of how they all work together. Please add a section describing versioning in general.
> 
> Done in PR https://github.com/Matroska-Org/ebml-specification/pull/200
> 
>> 
>> §2: Please use the newer boilerplate from RFC 8174.
> 
> Done in PR https://github.com/Matroska-Org/ebml-specification/pull/201
> 
>> 
>> §3: Can you offer guidance on the specific impacts of the mentioned attacks, and how to mitigate them? (If there is no way to mitigate an attack, it's okay to say that.)
> 
> Partially covered by https://github.com/Matroska-Org/ebml-specification/pull/202
> I'm not sure how to mitigate side channels errors, or rather if we have to go in deep details for each (it could go very deep on all kinds of security things).
>> 
>> §5: Please elaborate on how this is similar to UTF8. I assume this refers to using prefix bits to indicate the field length. UTF8 does that to maintain backwards compatibility with Ascii. What is the design motivation for using that approach for EBML, which I presume doesn't have such a requirement?
> 
> Done in https://github.com/Matroska-Org/ebml-specification/pull/203
> 
> 
>> 
>> §5.3: "If the number of bits required for "VINT_DATA"
>> are less than the bit size of "VINT_DATA", then "VINT_DATA" SHOULD be
>> zero-padded to the left to a size that fits."
>> 
>> Why not MUST? What happens if this is violated?
> 
> Done in https://github.com/Matroska-Org/ebml-specification/pull/204
> 
> I'll try to continue with the rest ASAP
> 
> Thanks a lot for all your comments.
> 
> _______________________________________________
> Cellar mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/cellar

---

From: Ben Campbell <[email-masked]>
Date: Fri, 1 Mar 2019 16:02:07 -0600

> On Jan 27, 2019, at 9:31 AM, Steve Lhomme <[email-masked]> wrote:
> 
> Hi,Some more comments on the comments:
> 
> Le lun. 7 janv. 2019 à 22:15, Ben Campbell <[email-masked]> a écrit :
>>>> Hi,
>>> §6:
>> - "The "VINT_DATA" component of the "Element ID" MUST
>> NOT be either defined or written as either all zero values or all one
>> values."
>> Why?
> 
> There is no technical reason why these values can't be used. For
> example VINT_DATA all set to 0 is valid for the Element Size (and all
> ones has a special meaning). These are rather reserved values that may
> help expand EBML one day if necessary. Should we call them reserved
> rather than invalid ?

I think “reserved” would make more sense.

> 
>> §8.7: "In this case, the "EBML Reader" should skip data
>> until a valid..."
>> Should that be a normative statement? If so, why should and not MUST?
> 
> It can't be a MUST if we tolerate values to be there, someone might
> want to make use of these data. It could also be data that have been
> damaged and don't appear as EBML Elements anymore. In that case the
> reader should do its best to skip over these data. In any case the
> behaviour of the reader when such data is found not normative.

Thanks, I think some text to that effect in the document would be helpful.

> 
>> §8.8: If the EBML reader does not interpret Binary Elements, why add them at all? What does interpret them? is the point that the EBML reader treats Binary Elements as opaque, and just hands them to the application?
> 
> Yes, it's exactly that. Binary is a blob that is unknown to the EBML
> Reader. Only an app that knows the particular Element in the DocType
> can interpret what is in this blob.

Okay. It might be helpful to mention they are handed to the application.

> 
>> §10.1.3: "NOT RECOMMENDED" seems overly strong, especially in light of the MAY in the first paragraph. Is the point to say "MUST NOT... except when the implementation needs to update an element without rewriting the entire document"?
> 
> The MAY means zeros can be added after a string, but one shouldn't do
> that.
> The NOT RECOMMENDED also says that it shouldn't be used unless
> you know what you're doing (the definition of NOT RECOMMENDED).Should
> we rephrase the paragraph ? Saying that use Null terminated should not
> be used in general but must be handled by a Reader because it's a
> possibility ?

I think the problem is in the normative MAY, which effectively says it’s okay, which conflicts with the NOT RECOMMENDED, which says that it’s not okay unless you fully understand the consequences and have a really good reason.

Perhaps the MAY was intended as a statement of fact? That is, doing this is NOT RECOMMENDED, but since some implementations might do it anyway a reader SHOULD/MUST be able to handle it?

[…]

> 
>> §13.1: "The "DocType"
>> value for an "EBML Document Type" SHOULD be unique and persistent."
>>> What is the scope of uniqueness? How should it be achieved? Is there an expectation to register Document Types?  (Also, why not MUST)?
> 
> There is a IANA registry and it MUST be unique (see 15.2 CELLAR EBML
> DocType Registry), although if people use the same separately string
> separately without going through IANA registration, it's possible they
> won't be unique. That being said I think it should be a MUST.

If there is a registry, then that would normally avoid collision problems without needing a separate normative statement about uniqueness. Is there a requirement that all DocTypes be registered?

> 
> See Pull Request #213https://github.com/Matroska-Org/ebml-specification/pull/213
> 
>> §13.1.4.1: Are there uniqueness requirements for name?
> 
> No, it's a human readable name that's only there for readability.
> never ends up in the actual EBML data. The same name could be found in
> different path for example.

Okay.

> 
>> §13.1.4.2:
>> - The idea behind "path" needs elaboration. I'd like to see some high-level discussion of how you indicate where an element is allowed, and how you encode the structure.
> 
> I don't understand what you mean.

I am deferring this one until I can look at the update. The example might be sufficient to resolve my concern.

> 
>> An example (local to the section) would be helpful.
> 
> Added in Pull Request #214
> https://github.com/Matroska-Org/ebml-specification/pull/214
> 
>> §13.1.4.4:
>> - "The "minOccurs" value MUST be
>> equal to the "EBMLMinOccurrence" value of the "path"."
>>> Why have both if they have to be the same? (Same question for §13.1.4.5)
> 
> EBMLMinOccurrence is just a coded name in the path definition. It's
> not found in the path itself. Whereas minOccurs is found in the XML
> Schema describing the format.The former has a long name to make it
> clearer what it does in this context. The latter has a short name to
> be more readable and is always found in its context anyway.

This goes back to my comment about elaborating the use of Path. I will check the example.

[…]

> 
> 
>> §13.1.10:
>> - Please state (and cite) which XML schema format is used here.
>>> - Has the schema been mechanically verified?
> 
> I'm not competent on that one (I suppose Dave or Jerome have used it ?)

This depends on which schema format is being used.

> 
>> §13.3.1:
>> - "The CRC value MUST be computed on a little
>> endian bitstream and MUST use little endian storage."
>>> Please elaborate. Most elements have been big-endian so far. Do there have to be converted to calculate the CRC?
> 
> The EBML headers and integers are in Big Endian. But the Binary data
> can be anything. For convenience with modern CPUs the CRC-32 is done
> on little endian reading of the EBML header+data and stored as such.I
> can't access the ISO spec, but the ITU one doesn't mention anything
> about endianess.
> 

“Convenience with modern CPUs” explains why the CRC needs to be little-endian. But it would be useful to have an explanation why byte order is not consistent for all aspects. (“Historical reasons” is a perfectly good answer, but if that is the answer it would be good to say so.)

>> §15.1:
>> - "The numbers 0x3FFF and 0x4000 are RESERVED.", "The numbers 0x1FFFFF and 0x200000 are RESERVED.", and "The numbers 0xFFFFFFF and 0x1000000 are RESERVED."
>> What is the purpose of reserving them?
> 
> The answer can be found here
> https://github.com/Matroska-Org/ebml-specification/pull/179#discussion_r196481366
> There are invalid values, but that's not a word used for IANA
> registration. Should we mention that they are invalid anyway ?

So they can never be valid? Is there a potential that a revision to EMBL could use them?

> 
>> §15.2:
>> - "The strings may be allocated according to
>> First Come First Served"
>> 
>> Is there a requirement to register them, or is this optional?
> 
> We can't force people to do it, but it's encouraged.

We can’t force people to do anything. But saying “may be allocated…” makes it look completely optional. And it’s pretty common to see language along the lines of “MUST be registered” in other RFCs, to strongly discourage code-point squatting.

---

From: Ben Campbell <[email-masked]>
Date: Fri, 1 Mar 2019 16:11:45 -0600

> On Jan 27, 2019, at 9:48 AM, Steve Lhomme <[email-masked]> wrote:
> 
> More:
>> *** Editorial Comments and Nits ***
>> 
>> - General: The draft puts double quotes around every mention of terms that it defines. That is unconventional for IETF documents. I personally find that it makes the draft harder to read. I suggest quoting them on the first mention, then just capitalizing them in subsequent mentions.
> 
> I suppose you don't mean full capital letters ? The idea is that if
> you read a particular section and not the whole document you can still
> understand what is internal terms or generic names.

No, I meant writing in the form of proper names.

But to be clear, I find the use of double-quotes everywhere to create visual noise that makes the document hard to read.

> 
> 
>> §11.1:
>> - Is the concept of a file concrete (in the sense of a unit of persistent storage) or abstract (as in any stream of data)? I had assumed the latter, but with the different rules about data outside of EBML elements for "files" and "streaming applications", I am not so sure. If you mean "file" concretely, does there need to be additional text in this section describing the structure for streaming applications?
> 
> It's more of an abstract thing. When we mention streaming it's some
> kind of reading where you can't seek forward (and only backward is
> there's a cache). But in general the storage itself doesn't matter.

Okay

> 
>> §13.4.1.2:
>> - "The "EBMLElementOccurrence" part is interpreted as an ABNF Variable
>> Repetition.
>> 
>> I don't understand what that means. There won't be ABNF in an actual schema, will there? (Same for "VariableParentOccurrence")
> 
> The path is in the Schema and is interpreted as ABNF

So if someone were to use a schema-aware encoder to create EBML data, the interpreter would need to understand ABNF?
[…]

> 
>> §13.1.4.7
>> - The attribute is called "size" but defined to be "length". Why not call it length? (Or describe it as "size")?
> 
> That must be for historical reasons. I'm fine with renaming it to
> length since that's the term we use everywhere else.
> See Pull Request #217
> https://github.com/Matroska-Org/ebml-specification/pull/217at <https://github.com/Matroska-Org/ebml-specification/pull/217at>

I’m okay with calling it size for historical reasons if people prefer. If so, it would be good to make a comment to that effect in the text.

---

From: Steve Lhomme <[email-masked]>
Date: Tue, 5 Mar 2019 16:18:06 +0100

On 3/1/2019 11:11 PM, Ben Campbell wrote:
>
>
>> On Jan 27, 2019, at 9:48 AM, Steve Lhomme <[email-masked] 
>> <mailto:[email-masked]>> wrote:
>>
>> More:
>> Le lun. 7 janv. 2019 à 22:15, Ben Campbell <[email-masked] 
>> <mailto:[email-masked]>> a écrit :
>>
>>> - General: The draft puts double quotes around every mention of 
>>> terms that it defines. That is unconventional for IETF documents. I 
>>> personally find that it makes the draft harder to read. I suggest 
>>> quoting them on the first mention, then just capitalizing them in 
>>> subsequent mentions.
>>
> But to be clear, I find the use of double-quotes everywhere to create 
> visual noise that makes the document hard to read.

This is gone in the new/future version.

>>> I don't understand what that means. There won't be ABNF in an actual 
>>> schema, will there? (Same for "VariableParentOccurrence")
>>
> So if someone were to use a schema-aware encoder to create EBML data, 
> the interpreter would need to understand ABNF?
> […]

Yes

>
>>
>>> §13.1.4.7
>>> - The attribute is called "size" but defined to be "length". Why not 
>>> call it length? (Or describe it as "size")?
>>
>> https://github.com/Matroska-Org/ebml-specification/pull/217at
>
> I’m okay with calling it size for historical reasons if people prefer. 
> If so, it would be good to make a comment to that effect in the text.

We use "length" now.