Re: [Jmap] Artart last call review of draft-ietf-jmap-smime-08

[Alexey Melnikov <[e-mail]>]

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.
Good point, reordered.
> •
>      The "string" values that are allowed for smimeStatus and smimeErrors do not
> explicitly mention the allowable charsets (standard ART area guidance). In IETF
> protocols, UTF-8 is preferred - see RFC 2277 (BCP 18), Sections 3.1 and 3.2.
> Please reference this and make it explicit what characters are allowed.

This is defined by reference: "String" is defined as JSON string, which 
means UTF-8.

smimeStatus is not subject to internationalization (its values are 
tokens), but I clarified that they should be in ASCII.

> •
> Similarly, this draft has protocol elements that contain human-readable text,
> so you need to specify language tags for those fields, or justify why language
> tagging is not needed. There is text that alludes to this in smimeErrors,
> saying "in the language specified in the Content-Language header field, if any"
> – but there's no language stipulation for others, e.g. smimeStatus where it
> says "Servers MAY return other values not defined below."
See above, this doesn't apply to smimeStatus.
> For details, see RFC
> 2277 (BCP 18), Section 4.2. •       Suggest adding some words for readability:
> *In one* example, the signing certificate might be expired and the message From
> email address might not correspond to any of the email addresses in the signing
> certificate. *In another example*, the certificate might be expired and the
> JMAP server might be unable to retrieve a CRL for the certificate.
Clarified.
> •       The
> smimeVerifiedAt value is described as "server-set". I was unsure if all of
> these four are server-set, on reflection this could be because I missed the
> particular definition of 'server-set' in RFC8621.
'server-set' is defined in Section 1.1 of RFC 8620.
>   •       Re: "UTCDate", please
> make sure you define the details of its syntax (standard ART area guidance).
> Even if you reference something like RFC 3339 (which is generally recommended
> for IETF specifications), make sure that you document all the details about
> various options, such as use of non-UTC time zones and fractional seconds.
This is fully defined in Section 1.4 of RFC 8620.
> •
>     "(but see below about result caching)" – do you mean in the Security
> Considerations or later in this paragraph?
Later in the same paragraph.
> I'd suggest just removing this
> bracket, it doesn't help with understanding nor does it contradict that the
> value is calculated when the request was processed.
I was trying to say that "it is calculated at the time of the request, 
unless it has been less than 10 minutes since the last request to the 
same values". I am open to suggestions about the best way of saying this.
> •       Missing word(?): In
> all cases "smimeVerifiedAt" is set to *the* time when "smimeStatus" and
> "smimeErrors" were last updated.
Yes.
> •       Should the IDs match? You have
> f123u987 in the request but f123u457 in the response (even if they don't need
> to match, I'm still twitching that "f123u457" isn't "f123u456"…)
Well spotted. Yes, they need to match.
> Section 5.1
> •       should this be "smimeverify" and not "smime",
Yes. Fixed.
> i.e. instead of "IANA is
> requested to register the "smime" JMAP Capability as follows", it should be
> "IANA is requested to register the "smimeverify" JMAP Capability as follows"
>
> Section 6
> •       I think you're missing a word - "server verification code" ought to be
> "server *signature* verification code" as it is in the Introduction.
Yes, good point.
> •       I
> suggest it's more of a trade-off than described currently, it's more subtle
> than "the client trusts the server's configuration". The client trusts the
> server's configuration and signature verification code *more than* the
> inefficiency/work/bandwidth required to download the signature body part and
> all signed body parts or to download and decode CMS.

Ok. I will try to wordsmith this.

Best Regards,

Alexey