[UTA feedback] - General feedback from Ned

[jjones425]

-----Original Message----- From: Uta [mailto:uta-bounces@ietf.org] On Behalf Of ned+uta@mrochek.com Sent: Tuesday, September 13, 2016 9:44 AM To: Daniel Margolis dmargolis@google.com Cc: uta@ietf.org Subject: Re: [Uta] MTA STS and HTTPS fetch timeouts

I just took a look at this draft with an eye to implementing it and I think it suffers from several more fundamental problems than a failure to recommend https timeout values.

Apologies is some/most of this has already been pointed out.

I'm going to start in an unusual place: The validation pseudocode in Appendix 1. From there I'll go on to other stuff. But I think the pseudocode, done right, will be enormously helpful to implementors, so that's going to be my starting point.

That pseudocode isn't even close to suffciently detailed, making it much more difficult than it should be to go from specification to implementation. It also overlaps, and in one place, conflicts, with the steps listed in section 3.5, which also suffer from being insufficiently detailed.

The pseudocode doesn't properly account for the case where a cached policy says reject, the mx or tls checks fail, and the policy is reacquired but now says report.

If I understand the proposal correctly, the pseudocode should look something like this:

policy_domain = recipient_address_domain() [a] policy = policy_from_cache(policy_domain) if not policy or is_expired(policy): txt_record_name = "_mta_sts." . policy_domain [3.3] txt_record = dns_lookup_txt(txt_record_name) if txt_record and txt_record_valid(txt_record): [3.1.1] https_url = "https://na01.safelinks.protection.outlook.com/?url=https%3a%2f%2fpolicy.mta-sts&data=02%7c01%7cJanet.Jones%40microsoft.com%7c09f2173d05b547f49d0e08d3dc1f09cd%7c72f988bf86f141af91ab2d7cd011db47%7c1%7c0%7c636093998283563674&sdata=NEKeLr9rbS4VCzNYIhqsE%2bsQdT%2bgFmblIkqB9LeNY8Y%3d." . policy_domain . "/.well-known/mta-sts/current" [3.3] policy = policy_from_https_endpoint(https_url) update_cache = true policy_rechecked = false; :policy_recheck: if policy and policy_valid(policy) and [3.1.2] ids_match(policy, txt_record): [3.3] if invalid_mx_or_tls(policy): // check MX and TLS cert [3.4, 4] if mode = report: [b] generate_report() if mode = enforce: if not policy_rechecked: policy = policy_from_https_endpoint(https_url) policy_rechecked = true update_cache = true goto :policy_recheck: reject_message() [4] update_cache = false [c] if update_cache: update_cache(policy)

In regards to the missing references marked with letters:

[a] This document needs to be expicit about how to turn a recipient address into a policy domain, even if that process is simply "everything after the last @". (And on a side note - are we 100% sure this is the process we want?)

[b] rua isn't defined in this document any more so I removed it. Various references to draft-ietf-uta-smtp-tlsrpt need to be inserted as noted in section 4.

[c] There's nothing in section 4 (or anywhere else AFAICT) to justify turning off caching at this point. And even if there was some justification, I nevertheless object because disabling caching at this point in the process makes it impossible to cleanly tease the policy acquisition mechanism away from the SMTP client, since you don't know if you're going to update the cache until you've fetched the MX records and made the SMTP connection.

In fact I see no point in reacquiring the policy unconditionally in the reject case - you should only do so if the policy came from the cache. Why look something up you've looked up only a fraction of a second before?

And if you are reacquiring policy, I think you should also redo the TXT record lookup.

One thing this pseudocode now makes clear that you have to choose between reporting and enforcement. What if someone wants both?

Another thing the psuedocode makes clear is how the sections in the document don't really follow the actual steps used to implement this protocol. I think some consideration should be given to reordering the document to more closely match how an implementation actually works, and there probably should be more discussion of caching in various places.

Next up: The ABNF specification of the JSON policy format is incorrect in several places. For exmaple:

sts-record = WSP %x7B WSP ; { left curly bracket sts-element ; comma-separated [ ; list WSP %x2c WSP ; of sts-element ; sts-elements ] WSP %x7d WSP ; } right curly bracket

Keeping in mind that RFC 5234 defines WSP as:

WSP = SP / HTAB

What this says is that a policy statement must consist of a single space/tab followed by a { followed by a single space/tab followed by a list of elements separated by space/tab delimited commas followed by ...

I think you get the idea - the white space as specified lacks sufficient flexibility and fails to allow for line breaks. It certainly doesn't match the example in section 9.1, which uses normal JSON formatting.

One way to fix this would be to use the convention that's used in in the JSON specification itself (RFC 7159). Define ws as follows:

  ws = *(
          %x20 /              ; Space
          %x09 /              ; Horizontal tab
          %x0A /              ; Line feed or New line
          %x0D )              ; Carriage return

And then use if in all the places where you're currently using either WSP or *WSP. This will fix most, if not all of the ABNF problems.

But I really cannot recommend creating a syntax for a particular JSON instance that overlaps the more general JSON ABNF. We have a lot of experience with this approach, including RFC 822 and RFC 1341/RFC 1342, and have found it almost never works out well. (Which is why this approach is no longer used in the later versions of those specifications.)

The alternative is to specify your format using higher level specification tools than ABNF. With XML this is a no-brainer: Use XML Schema or Relax NG.

JSON has a schema language and IMO it's easy to read and use. The problem is it's still at draft status and hasn't come out as an RFC. (I don't follow the JSON work so I don't know why this is the case, but since the latest draft is three years old I'm going to assume it's ratholed for whatever reason.)

It would be one thing if the JSON being used here was complicated - it is was I'd say use JSON schema and ask for an exception to reference the draft definitions.

But there's nothing remotely complicated about policy objects, so IMO the way to go is a prose description along the lines of what was used in RFC 7033, RFC 7266, RFC 7483, and RFC 7484.

You probably want to register a media type for the policy format. Something like application/mta-sts-policy+json.

This text:

A size limitation in a sts-uri, if provided, is interpreted as a count of units followed by an OPTIONAL unit size ("k" for kilobytes, "m" for megabytes, "g" for gigabytes, "t" for terabytes). Without a unit, the number is presumed to be a basic byte count. Note that the units are considered to be powers of two; a kilobyte is 2^10, a megabyte is 2^20, etc.

appears to be a discussion of something that's no longer in this draft or in the tlsrcpt draft. As such, it should be removed.

Finally, the material comparing MTA-STS with DANE, while useful, has no value to someone implementing this specification. As such, it has no business being the first thing in the document. I therefore strongly recommend relegating it to the final appendix.

That's it for now. There are undoubtedly more issues but I think a new draft that cleans up some of this stuff will make it much easier to proceed.

            Ned

[jjones425]

@danmarg can you confirm Ned's feedback was incorporated in your last pull

[danmarg]

I rewrote the pseudocode, and (in b14f13e) removed the ABNF for the json.