Subject: [calsify] Genart last call review of draft-ietf-calext-jscalendar-25

From: Robert Sparks via Datatracker <[email-masked]>
Date: Tue, 03 Mar 2020 13:10:01 -0800

Reviewer: Robert Sparks
Review result: Ready with Issues

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-calext-jscalendar-25
Reviewer: Robert Sparks
Review Date: 2020-03-03
IETF LC End Date: 2020-03-09
IESG Telechat date: Not scheduled for a telechat

Summary: This document has minor issues to address before being published as a
Proposed Standard RFC.

Caveats: I did not carefully verify that the initial registry values (of which
there are many) matched the document text.

Minor issues:

ABNF is used, but there is no reference to RFC 5234. The document shepherd
report implies that the ABNF has not been verified (see item 19 in that report).

The first registry in section 8.4.3 should be explicit that it is a registry
for values of the "@type" property. Right now the reader has to infer that.

It's not stated clearly whether a patch should succeed or fail if the resulting
object doesn't meet this specification's requirements. Side question: if a
patch changes 'updated' (4.1.6) in an object that didn't already have a
'created' (4.1.5), should it fail if it doesn't also result in an object that
has a 'created'? Or is it ok to lose the original creation time information?

The security consideration section only points to section 7 of RFC 3986 for
potential issues related to the URIs that can be carried in this
representation. I don't think that's sufficient. There should be some
discussion (or a pointer to discussions) about the potential for malicious
construction of jscalendar objects containing potentially very large numbers of
URIs in, say, as Link objects (4.2.7). Is there an opportunity for amplified
attacks here? (Especially if these URLs might be automatically referenced by
any client, and even more so if the object is sent from a calendar with a large
number of subscribers.)

Have you considered any tighter integrity checking for (4.2.7) links? Maybe a
checksum property?

It doesn't look like application/jscalendar+json has had a media type review.

Nits:

Introduction, second sentence: "It" is ambiguous. As written, it could point to
"This document" or "a data model". You sort of mean the later, but I think you
really mean the JSON representation (especially when you say "process"), but
you don't talk about that here.

Introduction, design considerations list, last bullet: This bullet is currently
written as description of the end result, not as a design consideration. Please
restate it to match the rest of the elements in the list.

Last full paragraph on page 2: "unlike most common JSON data representations"
is asserted without data. The document doesn't really need it. I suggest simply
deleting the phrase from the sentence.

I don't expect anything to change at this point, but I do have to point to the
dissonance in the conventions A[] and A[B]. It would have been far less
confusing for A have the same semantic in both cases (preferably value), than
the current situation where it means value for A[], but key for A[B].

1.4.3 last paragraph: You don't need to use MUST in this example - it's not a
requirement on the protocol. I suggest replacing "MUST be" with "is correctly".

In 4.2.5 at 'locationTypes', it would be better to point to the actual registry
(https://www.iana.org/assignments/location-type-registry/location-type-registry.xhtml)
than only to RFC4589.

Do you want to say anything about what should happen if the size of a resource
(as represented by 'size' in 4.2.7) doesn't match the actual fully decoded
size? I think you mean for this property to be an informational estimate, and
you should explicitly say the actual size could be quite different.

In 4.2.10, 'american-football""' should be 'american-football"' (one fewer ")

In 4.3.2 at 'byDay', why do you have * symbols around 'An *NDay* object'?

In 4.4.5 at 'scheduleUpdated', do you really want to couple this so tightly to
iTIP? Perhaps you should say "the last time this participant provided an
update" and point to iTIP as how these are typically provided?

Major issues:

Minor issues:

Nits/editorial comments:

---

From: Barry Leiba <[email-masked]>
Date: Tue, 3 Mar 2020 17:30:50 -0500

Robert, thanks for the careful (as always) review.

> It doesn't look like application/jscalendar+json has had a media type review.

There aren't formal media-type reviews for types registered in
IETF-consensus documents: see Section 3.1 of RFC 6838.

That said, Section 5.1 of 6838 says this:

   Notice of a potential media type registration in the standards tree
   SHOULD be sent to the [email-masked] mailing list for review.
   This mailing list has been established for the purpose of reviewing
   proposed media and access types.

I strongly encourage the authors to follow the advice in that section
now: it's not too late.  And I'm sorry that I didn't catch that
myself; thanks for picking it up.

Barry

---

From: "Neil Jenkins" <[email-masked]>
Date: Sun, 08 Mar 2020 13:58:13 +1100

Thanks for the review Robert.

> ABNF is used, but there is no reference to RFC 5234.

I have added a normative reference to this RFC. 

> The first registry in section 8.4.3 should be explicit that it is a registry for values of the "@type" property.

That's not quite correct; the registry is for the names of types. Not all types have an "@type" property (for example, primitive types like `UnsignedInt`). The "@type" property is primarily to help where the context allows multiple types (e.g. the "entries" property in a JSGroup object is a list of JSEvent and/or JSTask objects, so you need the @type property on these to know which one you are getting).

> object doesn't meet this specification's requirements. 

Good spot. I have added a requirement for the PatchObject to be valid that:

The value for the patch MUST be valid for the property being set (of the correct type and obeying any other applicable restrictions), or if null the property MUST be optional.

(Thus if this is not true the whole PatchObject should be ignored, as per any other patch failure.)

> Side question: if a patch changes 'updated' (4.1.6) in an object that didn't already have a 'created' (4.1.5), should it fail if it doesn't also result in an object that has a 'created'? Or is it ok to lose the original creation time information?

No, this doesn't fail. The "created" property is optional, so you can have an object that doesn't record its creation date.

> any client, and even more so if the object is sent from a calendar with a large

I have added:

*A maliciously constructed JSCalendar object may contain a very large number of URIs. In the case of published calendars with a large number of subscribers, such objects could be widely distributed. Implementations should be careful to limit the automatic fetching of linked resources to reduce the risk of this being an amplification vector for a denial-of-service attack.*

> Have you considered any tighter integrity checking for (4.2.7) links? Maybe a

No. The integrity of data is left to the transmission protocol and not part of the data format being described in this document. I don't see any good reason why this property should be subject to extra integrity checks beyond the whole object.

> It doesn't look like application/jscalendar+json has had a media type review.

Thanks, I have now sent this for review.

> the current situation where it means value for A[], but key for A[B].

I see your point, but we have already used the same syntax in JMAP (RFC8620/8621) and I think it is better to stay consistent.

I have published v26 <https://tools.ietf.org/html/draft-ietf-calext-jscalendar-26> with all of the above updates.

Cheers,
Neil.