Subject: [Last-Call] Artart last call review of draft-ietf-jmap-smime-08

From: Kirsty Paine via Datatracker <[email-masked]>
Date: Wed, 22 Sep 2021 09:58:38 -0700

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 :)

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? •       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]" •       Missing
words: "assumes that *the/a* JMAP client trusts *the/a* JMAP server…" •      
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.

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).

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. •       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.

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 :) •       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. It
could be a good idea to include something like "The smimeStatus value can only
be one of the following four messages". 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…" •       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. •       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. •       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. •  
    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. •      
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." 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. •       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. •       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. •   
   "(but see below about result caching)" – do you mean in the Security
Considerations or later in this 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. •       Missing word(?): In
all cases "smimeVerifiedAt" is set to *the* time when "smimeStatus" and
"smimeErrors" were last updated. •       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"…)

Section 5.1
•       should this be "smimeverify" and not "smime", 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. •       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. •       I'm not sure why
10 minutes specifically is recommended – it seems to be landed there without
rationale and without conveying why it's not longer/shorter.

I hope this review is helpful to the authors and, finally, thanks for your work
and I wish you all the best for this draft as it continues its IETF journey.

---

From: Alexey Melnikov <[email-masked]>
Date: Fri, 24 Sep 2021 12:39:47 +0100

Kirsty,
Thank you for your thorough review.
Quickly replying to a couple of your points and I will reply in more details in a separate email.

> On 22 Sep 2021, at 18:00, Kirsty Paine via Datatracker <[email-masked]> wrote:
 [snip]
>  •  
> says "Servers MAY return other values not defined below."
smimeStatus use tokens, not free form human readable text, so use of language tags is not appropriate for it. But I will check if this can be clarified.

 [snip]

> •       I'm not sure why

It seemed like a reasonable default and making it configurable seemed like overkill (and a way to introduce unnecessary complexity into the protocol).
To be honest the proposed default hasn’t been discussed in the WG, so if you have an alternative proposal, it would be great to hear.

Best Regards,
Alexey

---

From: Alexey Melnikov <[email-masked]>
Date: Thu, 30 Sep 2021 17:44:09 +0100

Hi Kirsty,

Thank you for your review.

> extended here, or expanding the acronym "JMAP". S/MIME is probably well-known
I changed "JMAP" to "JMAP for Mail (RFC 8621)".
> [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
> 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.
> that the client trusts the server's configuration and signature verification
Yes, good point.
> editorial conventions (e.g. for "server-set" in Section 4).
Ok, I will review.
> defined in Section 2 of RFC8620.
Ok.
> •       Is this a hidden requirement: "The
It is the same thing without using RFC 2119 language.
> 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,
> 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
> S/MIME message…"
Good point, added.
> •       For unknown, you use the present tense "is" but for
> same for them all.
Changed.
> •       You probably don't need to say "compliant with this
> the FilterCondition object too perhaps.
Ok, I will try to improve this.
> •       I find it odd that the bullet
> orders it smimeStatus, smimeStatusAtDelivery, smimeErrors, smimeVerifiedAt.
Good point, reordered.
> 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.

> says "Servers MAY return other values not defined below."
See above, this doesn't apply to smimeStatus.
> For details, see RFC
> JMAP server might be unable to retrieve a CRL for the certificate.
Clarified.
> •       The
> particular definition of 'server-set' in RFC8621.
'server-set' is defined in Section 1.1 of RFC 8620.
>   •       Re: "UTCDate", please
> various options, such as use of non-UTC time zones and fractional seconds.
This is fully defined in Section 1.4 of RFC 8620.
> Considerations or later in this paragraph?
Later in the same paragraph.
> I'd suggest just removing this
> 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
> "smimeErrors" were last updated.
Yes.
> •       Should the IDs match? You have
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
> "server *signature* verification code" as it is in the Introduction.
Yes, good point.
> •       I
> all signed body parts or to download and decode CMS.

Ok. I will try to wordsmith this.

Best Regards,

Alexey