Ignore host certificate in remoting::V2Authenticator on the client side.

Ignore host certificate in remoting::V2Authenticator on the client side.

Previously V2Authenticator was passing host certificate from the host
to the client. It wasn't possible to implement CertVerifier that worked
in the renderer because X509Certificate doesn't work there. It's no
longer an issue because the code now runs under PNaCl. Cleaned up the
code to ignore the certificate that the host sends to the client.

[Sergey Ulanov, 2016-02-25 02:03:52]

sergeyu@chromium.org changed reviewers:
+ davidben@chromium.org

[Sergey Ulanov, 2016-02-25 02:03:53]

David, sending this to you because you looked at this code in the past.
I think it should be safe now given that we have require_ecdhe flag implemented
correctly.

[davidben, 2016-02-25 02:53:28]

On 2016/02/25 02:03:53, Sergey Ulanov wrote:
> David, sending this to you because you looked at this code in the past.
> I think it should be safe now given that we have require_ecdhe flag
implemented
> correctly.

Are we at the point that we can assume extended master secret support on all
peers that we'd talk to? Looking back at https://crbug.com/481163, I think so?
And if you have code still using SSLClientSocketNSS, even that supports it now.
Before we remove this, I think we should first finish the job and make these
connections require EMS, since it seems the migration to QUIC hasn't happened
yet.

What's the oldest client and server implementation on each platform that you'd
still need to be able to talk to?

[Sergey Ulanov, 2016-02-25 19:27:25]

On 2016/02/25 02:53:28, davidben wrote:
> On 2016/02/25 02:03:53, Sergey Ulanov wrote:
> > David, sending this to you because you looked at this code in the past.
> > I think it should be safe now given that we have require_ecdhe flag
> implemented
> > correctly.
> 
> Are we at the point that we can assume extended master secret support on all
> peers that we'd talk to? Looking back at https://crbug.com/481163, I think so?
> And if you have code still using SSLClientSocketNSS, even that supports it
now.
> Before we remove this, I think we should first finish the job and make these
> connections require EMS, since it seems the migration to QUIC hasn't happened
> yet.

We dropped QUIC and now working on a new protocol based on WebRTC. I still don't
think it's worth spending time on getting EMS enabled.
How is it related to this change?

I'm planning to implement new version of SPAKE for authentication see
crbug.com/589698. With this change I just wanted to clean up old cruft from the
auth code. If you think the certs are still useful there we can keep this logic
for now.

> 
> What's the oldest client and server implementation on each platform that you'd
> still need to be able to talk to?

The current release on iOS is M44. AFAICT it doesn't have EMS. Everything else
is on M49 now.

[davidben, 2016-02-25 19:33:01]

On 2016/02/25 19:27:25, Sergey Ulanov wrote:
> On 2016/02/25 02:53:28, davidben wrote:
> > On 2016/02/25 02:03:53, Sergey Ulanov wrote:
> > > David, sending this to you because you looked at this code in the past.
> > > I think it should be safe now given that we have require_ecdhe flag
> > implemented
> > > correctly.
> > 
> > Are we at the point that we can assume extended master secret support on all
> > peers that we'd talk to? Looking back at https://crbug.com/481163, I think
so?
> > And if you have code still using SSLClientSocketNSS, even that supports it
> now.
> > Before we remove this, I think we should first finish the job and make these
> > connections require EMS, since it seems the migration to QUIC hasn't
happened
> > yet.
> 
> We dropped QUIC and now working on a new protocol based on WebRTC. I still
don't
> think it's worth spending time on getting EMS enabled.
> How is it related to this change?

EMS is already enabled. BoringSSL doesn't let you disable it. It's just not
enforced.

For this kind of thing to be work, you need to ensure contributory behavior in
the key exchange. It turns out that ECDH is prrrooobbbbably okay, but this is
not the real fix. You really need to be requiring EMS for this kind of thing to
work. On the bug, we talked about it and decided to hold on figuring out how to
force it on because you were planning on migrating to QUIC anyway. If we'll be
removing this check in advance of that, you need to be using EMS.

> I'm planning to implement new version of SPAKE for authentication see
> crbug.com/589698. With this change I just wanted to clean up old cruft from
the
> auth code. If you think the certs are still useful there we can keep this
logic
> for now.
> 
> > 
> > What's the oldest client and server implementation on each platform that
you'd
> > still need to be able to talk to?
> 
> The current release on iOS is M44. AFAICT it doesn't have EMS. Everything else
> is on M49 now.

Yeah, that's too old. How long before iOS is updated to M49 as well? iOS is only
a client, right, not a server? So we can at least require EMS on the client half
today.

[davidben, 2016-02-25 19:34:14]

On 2016/02/25 19:33:01, davidben wrote:
> > I'm planning to implement new version of SPAKE for authentication see
> > crbug.com/589698. With this change I just wanted to clean up old cruft from
> the
> > auth code. If you think the certs are still useful there we can keep this
> logic
> > for now.

This SPAKE exchange is going to be bound to the TLS connection in some way,
right? That also requires EMS for the channel binding to work.

[Sergey Ulanov, 2016-02-25 20:06:21]

On 2016/02/25 19:33:01, davidben wrote:
> EMS is already enabled. BoringSSL doesn't let you disable it. It's just not
> enforced.
> 
> For this kind of thing to be work, you need to ensure contributory behavior in
> the key exchange. It turns out that ECDH is prrrooobbbbably okay, but this is
> not the real fix. You really need to be requiring EMS for this kind of thing
to
> work. On the bug, we talked about it and decided to hold on figuring out how
to
> force it on because you were planning on migrating to QUIC anyway. If we'll be
> removing this check in advance of that, you need to be using EMS.
> 
> > I'm planning to implement new version of SPAKE for authentication see
> > crbug.com/589698. With this change I just wanted to clean up old cruft from
> the
> > auth code. If you think the certs are still useful there we can keep this
> logic
> > for now.
> > 
> > > 
> > > What's the oldest client and server implementation on each platform that
> you'd
> > > still need to be able to talk to?
> > 
> > The current release on iOS is M44. AFAICT it doesn't have EMS. Everything
else
> > is on M49 now.
> 
> Yeah, that's too old. How long before iOS is updated to M49 as well?

No very soon. Probably when we release M50 or M51 we will update iOS too.


> iOS is only
> a client, right, not a server? So we can at least require EMS on the client
half
> today.

Yes. What's the right way make the client require EMS?

[Sergey Ulanov, 2016-02-25 20:17:21]

On Thu, Feb 25, 2016 at 11:34 AM, <davidben@chromium.org> wrote:

> On 2016/02/25 19:33:01, davidben wrote:
> > > I'm planning to implement new version of SPAKE for authentication see
> > > crbug.com/589698. With this change I just wanted to clean up old
> cruft from
> > the
> > > auth code. If you think the certs are still useful there we can keep
> this
> > logic
> > > for now.
>
> This SPAKE exchange is going to be bound to the TLS connection in some way,
> right?
>

It will work the same way the current  P224+SPAKE2 works in V2Authenticator.


> That also requires EMS for the channel binding to work.
>
> https://codereview.chromium.org/1739503003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[davidben, 2016-02-25 21:15:22]

On 2016/02/25 20:06:21, Sergey Ulanov wrote:
> On 2016/02/25 19:33:01, davidben wrote:
> > EMS is already enabled. BoringSSL doesn't let you disable it. It's just not
> > enforced.
> > 
> > For this kind of thing to be work, you need to ensure contributory behavior
in
> > the key exchange. It turns out that ECDH is prrrooobbbbably okay, but this
is
> > not the real fix. You really need to be requiring EMS for this kind of thing
> to
> > work. On the bug, we talked about it and decided to hold on figuring out how
> to
> > force it on because you were planning on migrating to QUIC anyway. If we'll
be
> > removing this check in advance of that, you need to be using EMS.
> > 
> > > I'm planning to implement new version of SPAKE for authentication see
> > > crbug.com/589698. With this change I just wanted to clean up old cruft
from
> > the
> > > auth code. If you think the certs are still useful there we can keep this
> > logic
> > > for now.
> > > 
> > > > 
> > > > What's the oldest client and server implementation on each platform that
> > you'd
> > > > still need to be able to talk to?
> > > 
> > > The current release on iOS is M44. AFAICT it doesn't have EMS. Everything
> else
> > > is on M49 now.
> > 
> > Yeah, that's too old. How long before iOS is updated to M49 as well?
> 
> No very soon. Probably when we release M50 or M51 we will update iOS too.
> 
> 
> > iOS is only
> > a client, right, not a server? So we can at least require EMS on the client
> half
> > today.
> 
> Yes. What's the right way make the client require EMS?

Just adding a bool require_ems or something and failing the connection is fine.
BoringSSL's API is this thing:
https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#S...

I'm not sure off-hand what NSS's is. SpawnedTestServer should support EMS now
and be able to disable it, so we should be able to write tests against that.