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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call




[Index of Archives]     [IETF Annoucements]     [IETF]     [IP Storage]     [Yosemite News]     [Linux SCTP]     [Linux Newbies]     [Mhonarc]     [Fedora Users]

  Powered by Linux