Hi Ekr,
I did a quick review of draft-rescorla-tcpinc-tls-option-04, as an individual
(not as chair).
Thanks for putting all the explanational text in there; at least for me that
was very helpful. I have a few comments and (potentially stupid) questions:
1) Section 2 say that if there is a decryption failure/finished failure,
endpoint SHOULD signal a connection failure. Does it mean that endpoint
SHOULD/MUST also send a TCP RST? And do we really, really need to close the
connection here, or is there maybe a way to think about falling back to TCP
only…?
2) The current proposal has the possibility that both endpoints might not
support the same chipper suite and therefore tcpinc will not be used. Should we
maybe think about picking a default one (or set of default ones that can be
ordered by preference) that must be supported. I know that's hard, but for
tcpinc, as the intention is anyway ‚just‘ opportunistic encryption, that might
make more sense. Maybe we can specify something while at the same time saying
that the server should not expect that a specific chiper suite will be
available such that we still have a change to deprecate this specification
later…
3) Section 3.1.2.3 says
"Unless the implementation is specifically configured
otherwise, it SHOULD NOT attempt to validate the certificate, even
if it is of type X.509 but merely extract the key.“
Isn’t the intention here that TCP could pass the cert to the app and the app
could validate if it wants…?
4) Figure 4 (in section 3.1.4 which is wrongly labeled Figure 2) indicated that
the server cannot send data with the ServerHello. That means to actually access
data form a server, you’ll always have an additional RTT even in 0-RTT mode. Is
that true?
5) Section 3.2. says „… MUST respond to renegotiation with fatal
„no_renegotiation“ alert“. What does this mean for the TCP connection?
That’s also a general comment that the integration with TCP is probably in some
cases not well enough specified yet. For my point of view that’s where a
(kernel) implementation would be helpful!
6) I’d recommend to specify the ENO sub option at the beginning of the doc. And
you further need to specify this more detailed, maybe with an image, and give
all the specfic bits such as the application-aware bits, variable-length bit,
and the sub option type.
7) Similar as in point 1 & 5: section 5 says „If [..] TLS integrity fails, the
connection MUST be torn down". Should the endpoint send a TCP RST here?
Further, as TLS integrity checking should effectively also be done in the
kernel and data should not be delivered to the app if the integrity check
fails, there is the option to simply drop the TCP packet and wait for a
retransmit. This does have security implications but maybe we can come up with
some additional logic that e.g. makes sure that if that happens ‚too often‘ we
will close the connection…
8) Further the draft says in section 6, that there are two options for
implementation. And I disagree there. tcpinc should always be implemented in
the kernel and only if there is potentially an app that also implements TLS
there might be a config option to allow the app to do the TLS handling if
successfully negotiated in TCP. It is not an option to have a tcpinc
implementation that relies on the possibility that the app might have TLS
implemented because then we are end up at a the state where we already are.
9) Reading section 9, we must be careful that we don’t create a new attack here
where broken data causes the connection to close, e.g. if packet seems to be in
a valid window but does not pass the the integrity check. Don’t think that’s
actually a case but maybe worth think about this.
And some nits/comments:
- in Figure 1) the packet from the server is 'SYN/ACK + TCP-ENO [ENO]‘ and
should be 'SYN/ACK + TCP-ENO [TLS]‘…?
- second point in 3.1.1 says ‚(see Figure 2‘ which probably should be ‚(see
Figure 3)‘ and Figure 3 is actually called Figure 1…
- Can you maybe try and expend (and refer) all acronyms (ECDSA, TOFO, ECDHE,
ALPN, HKDF, AEAD, …)
- In 3.1.2.1.2 text says ‚(see {{server-first-flight)‘. As this reference is
broken (missing closing brackets), the ref also doesn’t show up in the ref list
at the end…
- Missing reference in 3.3 (on session resumption; can you also provide more
reasoning here) and in 3.4.
- Another broken reference in 7: {{RFC5246}
- section 3.1.6 is not really clear: what’s N_MIN, what’s the record seq
number, is ContentType only one byte?
Hope that’s helpful!
Mirja
_______________________________________________
Tcpinc mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/tcpinc
