From: Alexey Melnikov <alexey.melnikov@xxxxxxxxx>
Date: Thursday, 30 September 2021 at 18:44
To: Kirsty Paine <kirsty.p@xxxxxxxxxxx>, art@xxxxxxxx <art@xxxxxxxx>
Cc: jmap@xxxxxxxx <jmap@xxxxxxxx>, draft-ietf-jmap-smime.all@xxxxxxxx <draft-ietf-jmap-smime.all@xxxxxxxx>, last-call@xxxxxxxx <last-call@xxxxxxxx>
Subject: Re: [Jmap] Artart last call review of draft-ietf-jmap-smime-08
Hi Kirsty,
Thank you for your review.
On 22/09/2021 17:58, Kirsty Paine via Datatracker wrote:
> Reviewer: Kirsty Paine
> Review result: Ready with Issues
>
> I'm the assigned reviewer for the ART area. Apologies for the delay in my
> review - I was on holiday.
>
> In summary, I think the draft could do with being clearer in a few places and
> having a more logical ordering in Section 4. That said, most of my comments are
> nits or common ART review issues, so I think the draft is essentially Ready.
>
> Abstract
> • it could be worth including the RFC reference to the protocol being
> extended here, or expanding the acronym "JMAP". S/MIME is probably well-known
> enough by now though :)
I changed "JMAP" to "JMAP for Mail (RFC 8621)".
> Introduction
> • RFC8621 specifies "JMAP for email", JMAP is specified by RFC8620. So
> should this be "JMAP for Email [RFC8621]" or "JMAP [RFC8620]" rather than "JMAP
> [RFC8620]" as it currently is?
Sure. I used "JMAP for Mail [RFC8621]", to match the title of RFC 8621.
> • The first sentence in the second
> paragraph in the introduction could benefit from more specific references too.
> As an example, you could rephrase like "when the multipart/signed media type is
> used [RFC1847], the client is not required to download… and when the
> application/pkcs7-mime media type is used [RFC8551 Sec. 3.2]"
Added, thanks.
> • Missing
> words: "assumes that *the/a* JMAP client trusts *the/a* JMAP server…"
Fixed, thanks.
> •
> I'm not sure that "this JMAP extension assumes" makes much sense – is it really
> the extension that assumes this trust relationship? Perhaps more accurately,
> the use of the extension implies the client trusts the server's configuration
> and code… Or that when this extension is in use, the threat model must assume
> that the client trusts the server's configuration and signature verification
> code.
Yes, good point.
> Section 2:
> • I see this document follows the conventions in Section 1.1 of RFC8621.
> However, if there are particular meanings for words and asterisks later in the
> document, it might be helpful to reference Section 1.1 at the point of use too,
> so that the specific normative meaning isn't lost on those unfamiliar with the
> editorial conventions (e.g. for "server-set" in Section 4).
Ok, I will review.
> Section 3:
> • It might be worth highlighting where exactly in RFC8620 one can find
> details about the capabilities object, since it's a 90-page RFC and it's not a
> section header. The capabilities object and Session object referenced are
> defined in Section 2 of RFC8620.
Ok.
> • Is this a hidden requirement: "The
> value of this property is an empty object"? For example, you could reword to
> say "the value of the urn:ietf:params:jmap:smimeverify property MUST be an
> empty object" or "must be set to" or similar.
It is the same thing without using RFC 2119 language.
> Section 4:
> • In the second sentence of Section 4, "This document" refers to the
> current document you're writing in, not RFC8621, if I understand correctly. Try
> to make that clearer :)
Yes. Any suggestions how to make this clearer? I've seen "this document"
used frequently in RFCs.
> • There's no sentence introducing the unknown,
> signed, signed/verified and signed/failed messages, and they don't form a
> bullet point list as you have above, so I found this difficult to parse.
They should be a bullet list. This might be a rendering issue.
They are introduced by "Possible string values of the property are
listed below."
> It could be a good idea to include something like "The smimeStatus value can only
> be one of the following four messages".
This is actually not true, as there is a sentence saying
"Servers MAY return other values not defined below". In particular my
implementation also emits "encrypted+signed/verified".
> This would help readability, and it
> might be worth adding a colon after these messages or indenting more, because
> it's not very clear, e.g. "unknown: S/MIME message" read to me as "unknown
> S/MIME message…"
Good point, added.
> • For unknown, you use the present tense "is" but for
> the other smimeStatus responses, you use "was". I would suggest making it the
> same for them all.
Changed.
> • You probably don't need to say "compliant with this
> document", just say "JMAP servers SHOULD…" • Structurally for Section 4,
> it might be helpful to have sub-headings for the four different property values
> and then group all the text about it in one place. And then another section for
> the FilterCondition object too perhaps.
Ok, I will try to improve this.
> • I find it odd that the bullet
> points are in a different order to the text that follows, e.g. the bullets are
> smimeStatus, smimeErrors, smimeVerifiedAt, smimeStatusAtDelivery; the text
> orders it smimeStatus, smimeStatusAtDelivery, smimeErrors, smimeVerifiedAt
