Reinforcing what David says here: It's natural to pick the cipher suite first and then look for the key_share extension, so if, for instance, you pick a PSK-only cipher suite, then you wouldn't look for the key_share.
Obviously, you could add a check that said that if an EC cipher suite was advertised, then you had to look for key shares even if you picked one, but it's not a check you otherwise need. -Ekr On Wed, Jul 13, 2016 at 8:58 AM, David Benjamin <david...@chromium.org> wrote: > On Wed, Jul 13, 2016 at 11:06 AM Hubert Kario <hka...@redhat.com> wrote: > >> On Wednesday 13 July 2016 14:43:58 David Benjamin wrote: >> > To be clear, I am not at all opposed to useful errors or strict >> policing of >> > what the peer sends. That's all great. Indeed the linked test below >> makes >> > use of more specific error codes than TLS provides. But I would like >> such >> > things to: >> > >> > 1. Be useful, either for debugging or because it rejects an invalid >> > handshake that would otherwise go unnoticed. >> > 2. Come out naturally out of what the implementation would do anyway, to >> > keep complexity down. >> > >> > I'm also quite okay with budging on #2 where #1 is strong. Complexity is >> > the currency we pay for adding things. But I do not think this >> particular >> > error code passes #1 (the special-case is not useful and such a peer >> would >> > not be able to handshake against correct implementations anyway), so I'm >> > not willing to sacrifice #2 for it. >> >> Then I fail to see how missing_extension does not flow out of correct >> implementation (and from general RFC recommendations). >> >> You need to parse the Client Hello, and then sanity check it (length of >> session ID, validity of ciphers, lack of duplicate extensions, etc.). >> Making sure that mandatory extensions are in place in the same code >> seems to me like a obvious place to do it. >> > > The extension is not mandatory. Whether it is mandatory or not depends on > the cipher suite selected. The cipher suite selected depends on the > contents (or lack of) the extension. This makes checking things somewhat > circular. > > If it were simply mandatory, by all means, mandate a missing_extension > alert. I still do not think it'd be very useful (I'm more interested in > non-syntax-error cases), but I have no objections. > > >> In other words, you shouldn't delay checking if a particular extension >> is present until the time you want to use it. If there is any chance >> (or possible configuration) in which you would end up using it, you >> should complain that the extension is missing or it is malformed. >> >> This way when you start to depend on it, things won't start breaking >> suddenly and you end up writing Yet Another Workaround for Broken >> Implementations™ >> > > I still do not see how returning handshake_failure (or whatever the "no > common cipher" error is) for this case instead of missing_extension will > cause broken implementations to come up. > > David > > >> > On Wed, Jul 13, 2016 at 10:02 AM David Benjamin <david...@chromium.org> >> > wrote: >> > >> > > On Wed, Jul 13, 2016 at 3:31 AM Dave Garrett <davemgarr...@gmail.com> >> > > wrote: >> > > >> > >> To be clear though, completely mandatory extensions, at least as >> we're >> > >> doing them here, are a new thing. TLS 1.2 relied on separate >> messages for >> > >> stuff, but we're front-loading everything into the ClientHello to get >> > >> reliable 1RTT (with the exception of HRR). >> > > >> > > >> > > On Wed, Jul 13, 2016 at 8:12 AM Hubert Kario <hka...@redhat.com> >> wrote: >> > > >> > >> On Wednesday 13 July 2016 05:23:53 David Benjamin wrote: >> > >> > I don't believe an implementation which fails to send >> supported_groups, >> > >> > etc., in 1.3 would ever leave a developer's workstation. It would >> not >> > >> > interoperate with anything. >> > >> >> > >> it would interoperate with itself, and for some deployments that's >> enough >> > >> of a passing grade... (Even if you do interoperatbility testing you >> > >> do not check all possible permutations of features and settings) >> > >> >> > >> I wholeheartedly agree with Dave here, error definitions should be >> strict >> > >> (both on the when and what to do). One, because it allows to better >> > >> diagnose (in general, maybe not in this specific situation) problems. >> > >> Two, because you can write a strict negative test case that actually >> > >> checks for it. >> > > >> > > >> > > >> > > >> https://boringssl.googlesource.com/boringssl/+/1c256544dda26e4042c1af082580a1b87c9a690f/ssl/test/runner/runner.go#5646 >> > > Also something I have plenty of experience with. We're obsessive about >> > > adding this kind of test in BoringSSL[0]. :-) >> > > >> > > One doesn't need this alert to write a test for curve negotiation. >> Just >> > > test that the handshake hits the usual codepath for there not being a >> > > common cipher. >> > > >> > > This semi-mandatory extension isn't new in TLS 1.3, depending on your >> 1.2 >> > > behavior. BoringSSL will already refuse to select an ECDHE cipher if >> > > supported_curves is missing. 1.2 does not require this, but we opted >> to do >> > > it for simplicity[1]. >> > > >> > > David >> > > >> > > [0] If anyone wants to try, I'm sure there is an >> implementation-agnostic >> > > TLS protocol test suite one could extract out of that. It would take >> some >> > > wrangling as we cheat and condition on BoringSSL error codes, but the >> nice >> > > thing is I've already built up a large battery of tests here. >> > > >> > > [1] Previously we always picking our most preferred curve if the >> extension >> > > was missing, like OpenSSL. But now our most preferred curve is >> X25519, not >> > > P-256. It did not seem worth kludging that when we could just decline >> the >> > > cipher. No one's noticed, so I would say that worked. >> > > >> >> -- >> Regards, >> Hubert Kario >> Senior Quality Engineer, QE BaseOS Security team >> Web: www.cz.redhat.com >> Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic > > > _______________________________________________ > TLS mailing list > TLS@ietf.org > https://www.ietf.org/mailman/listinfo/tls > >
_______________________________________________ TLS mailing list TLS@ietf.org https://www.ietf.org/mailman/listinfo/tls
