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

Alexey Melnikov <alexey.melnikov@isode.com> Thu, 30 September 2021 16:44 UTC

Return-Path: <alexey.melnikov@isode.com>
X-Original-To: jmap@ietfa.amsl.com
Delivered-To: jmap@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id F36223A0DB4; Thu, 30 Sep 2021 09:44:25 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.1
X-Spam-Level:
X-Spam-Status: No, score=-2.1 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, NICE_REPLY_A=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=isode.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id XZ02LmhAjzhH; Thu, 30 Sep 2021 09:44:21 -0700 (PDT)
Received: from waldorf.isode.com (waldorf.isode.com [62.232.206.188]) by ietfa.amsl.com (Postfix) with ESMTP id EEFC03A0DB3; Thu, 30 Sep 2021 09:44:20 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1633020256; d=isode.com; s=june2016; i=@isode.com; bh=E6PyZrPQOHq0KvQOp8zD/0lfjsIXi4TqSyKhNpSiJJQ=; h=From:Sender:Reply-To:Subject:Date:Message-ID:To:Cc:MIME-Version: In-Reply-To:References:Content-Type:Content-Transfer-Encoding: Content-ID:Content-Description; b=ODVFrbuukT4bSb6M1mEB4Tbe17/AIEAxv2ZJfU8p7mbsxyGPHQxe4AH92Zw755kX7P4qrk JIojGjjByUxcgfiPn4tCIUBVgUxo59SqMzYeGgPtQlSlCXT4aBdjnIPQ2/oT6F00PmeJVZ os8Cxt420p71iRiHvJ/n69fhteLxj8c=;
Received: from [192.168.0.5] ((unknown) [94.3.228.58]) by waldorf.isode.com (submission channel) via TCP with ESMTPSA id <YVXpXwABR339@waldorf.isode.com>; Thu, 30 Sep 2021 17:44:16 +0100
X-SMTP-Protocol-Errors: NORDNS
From: Alexey Melnikov <alexey.melnikov@isode.com>
To: Kirsty Paine <kirsty.p@ncsc.gov.uk>, art@ietf.org
Cc: jmap@ietf.org, draft-ietf-jmap-smime.all@ietf.org, last-call@ietf.org
References: <163232991808.32122.9195451729619651354@ietfa.amsl.com>
Message-ID: <003af00e-d7e4-1f50-9a7a-3d312f700df2@isode.com>
Date: Thu, 30 Sep 2021 17:44:09 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0
In-Reply-To: <163232991808.32122.9195451729619651354@ietfa.amsl.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-transfer-encoding: quoted-printable
Archived-At: <https://mailarchive.ietf.org/arch/msg/jmap/bXXE0coPfz-6qKy323-yJKo6v2I>
Subject: Re: [Jmap] Artart last call review of draft-ietf-jmap-smime-08
X-BeenThere: jmap@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: JSON Message Access Protocol <jmap.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/jmap>, <mailto:jmap-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/jmap/>
List-Post: <mailto:jmap@ietf.org>
List-Help: <mailto:jmap-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/jmap>, <mailto:jmap-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 30 Sep 2021 16:44:26 -0000

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