Move SSL layer initialization into ChannelAuthenticator implementations.

Move SSL layer initialization into ChannelAuthenticator implementations.

Also separate client and host authenticators into separate files.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111311

[Ryan Sleevi, 2011-11-19 19:45:26]

drive by

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
File remoting/protocol/simple_host_channel_authenticator.cc (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.cc:141: result |=
received_auth_bytes[i] ^ auth_bytes_[i];
nit: See crypto/secure_util.h -> crypto::SecureMemEqual

[Sergey Ulanov, 2011-11-22 02:49:18]

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
File remoting/protocol/simple_host_channel_authenticator.cc (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.cc:141: result |=
received_auth_bytes[i] ^ auth_bytes_[i];
On 2011/11/19 19:45:26, Ryan Sleevi wrote:
> nit: See crypto/secure_util.h -> crypto::SecureMemEqual

Done.

[Wez, 2011-11-22 22:29:46]

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.cc
File remoting/protocol/auth_util.cc (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.cc...
remoting/protocol/auth_util.cc:10: #include "crypto/sha2.h"
This should come after crypto/hmac.h

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.cc...
remoting/protocol/auth_util.cc:26: LOG(FATAL) << "Failed to encode auth token";
Out of interest, how on earth can that ever fail??

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.cc...
remoting/protocol/auth_util.cc:46: NOTREACHED() << "HMAC::Init failed";
Would it be cleaner to get rid of the return code and CHECK() these?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.h
File remoting/protocol/auth_util.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.h#...
remoting/protocol/auth_util.h:6: #define REMOTING_PROTOCOL_AUTH_UTIL_H_
This code defines constants and functions for the IT2Me authentication protocol
version 1, so consider either naming this file accordingly, or putting the
definitions & functions in a namespace in this header?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.h#...
remoting/protocol/auth_util.h:29: const std::string& auth_token);
Should these be base::StringPiece, not std::string?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
File remoting/protocol/channel_authenticator.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
remoting/protocol/channel_authenticator.h:21: class ChannelAuthenticator :
public base::NonThreadSafe {
Should this interface really derive from NonThreadSafe?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
remoting/protocol/channel_authenticator.h:21: class ChannelAuthenticator :
public base::NonThreadSafe {
Give a short description of what this interface is for?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
remoting/protocol/channel_authenticator.h:26: ChannelAuthenticator() {}
Make this protected, since it doesn't make sense to instantiate one of these
directly?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
remoting/protocol/channel_authenticator.h:29: // Starts SSL on the |socket| and
authenticates it. Takes ownership
Indicate that the caller must not attempt to use |socket| after calling
SecureAndAuthenticate; they can only use a newly-returned StreamSocket, if the
call succeeds.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
remoting/protocol/channel_authenticator.h:30: // of |socket|. |done_callback| is
called when authentication is
Is this always the case?  Surely whether SSL is started is dependent on the
implementation?  The interface just promises that encryption will be set up and
the authentication protocol executed?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
remoting/protocol/channel_authenticator.h:31: // finished. Callback handler must
take ownership of the result.
Indicate that the callback may be invoked before SecureAndAuthenticate returns.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_chann...
File remoting/protocol/jingle_channel_connector.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_chann...
remoting/protocol/jingle_channel_connector.h:31: // Starts connection process
for the channel. Takes ownership of
nit: Starts _the_ connection process ...

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_datag...
File remoting/protocol/jingle_datagram_connector.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_datag...
remoting/protocol/jingle_datagram_connector.h:27: virtual void
Connect(ChannelAuthenticator* authenticator,
Does this make sense?  ChannelAuthenticator takes a StreamSocket, whereas this
is the datagram connector, which will create a datagram socket?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_sessi...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_sessi...
remoting/protocol/jingle_session.cc:439: }
Is the intent for this code to be replaced with calls to
Authenticator::CreateChannelAuthenticator() in a follow-up CL?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_strea...
File remoting/protocol/jingle_stream_connector.cc (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_strea...
remoting/protocol/jingle_stream_connector.cc:112: if (error != net::OK) {
nit: Make this an if ... else ... block?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/pepper_chann...
File remoting/protocol/pepper_channel.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/pepper_chann...
remoting/protocol/pepper_channel.h:34: // |authenticator|.
nit: How is |pp_instance| used, and owned?  How is |authenticator| used?  Is
|authenticator| torn down once authentication is complete, for example, or kept
around and torn down with the channel?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
File remoting/protocol/simple_client_channel_authenticator.cc (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:22:
net::SSLClientSocket* CreateSSLClientSocket(
Is it really worth having this a separate method from SecureAndAuthenticate?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:27: // Certificate
provided by the host doesn't need authority.
Isn't this also to do with being self-signed, as below?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:34: // certs. Disable
it so that SSL layer doesn't try to initialize
nit: that _the_ SSL layer

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:35: // OCSP (OCSP works
only on IO thread).
nit: on _the_ IO thread

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:38: // SSLClientSocket
takes ownership of the adapter.
nit: What adapter?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:51: const std::string&
remote_cert,
nit: |remote_cert| -> |peer_cert|?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:71:
cert_verifier_.reset(new net::CertVerifier());
Why do we pass in a standard CertVerifier if we never verify certificates?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:75: int result =
socket_->Connect(&connect_callback_);
It seems strange for connecting the SteramSocket to be done here; it's neither
securing nor authenticating?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:84: LOG(ERROR) <<
"Failed to establish SSL connection";
nit: I don't think this needs logging here; calling code can decide what to log?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:92: LOG(ERROR) <<
"Error fetching keying material: " << export_result;
nit: See above.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:99:
std::string(key_material, key_material + kAuthDigestLength),
Why not just std::string(key_material, kAuthDigestLength)?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:105: // Allocate a
buffer to write the authentication digest.
nit: This function doesn't have comments on earlier code blocks, which arguably
would benefit more from explanation than this one!

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:111: void
SimpleClientChannelAuthenticator::DoAuthWrite() {
nit: DoAuthWrite -> WriteAuthenticationBytes() or similar?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
File remoting/protocol/simple_client_channel_authenticator.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.h:27: class
SimpleClientChannelAuthenticator : public ChannelAuthenticator {
I don't think "simple" is sufficiently descriptive.  How about
It2MeV1ClientChannelAuthenticator or simply V1ClientChannelAuthenticator?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.h:35: net::StreamSocket*
socket, const DoneCallback& done_callback);
OVERRIDE?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
File remoting/protocol/simple_host_channel_authenticator.cc (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.cc:11: #include
"net/socket/ssl_server_socket.h"
Move this below x509_certificate.h

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.cc:51: // Create server SSL
socket.
nit: This comment's a bit redundant...

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.cc:112: LOG(ERROR) << "Error
reading authentication: " << read_result;
nit: Don't really need this logging, or the preceding instances

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.cc:124: LOG(ERROR) <<
"Mismatched authentication";
nit: Make this a warning, or remove it?  Is there a better error-code we can
return?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
File remoting/protocol/simple_host_channel_authenticator.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.h:26: class
SimpleHostChannelAuthenticator : public ChannelAuthenticator {
See previous comment about the naming of this.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.h:30: crypto::RSAPrivateKey*
local_private_key,
Can this parameter be const&?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.h:34: //
ChannelAuthenticator implementation.
nit: implementation -> interface?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.h:48: std::string
auth_bytes_;
Why store |auth_bytes_|, rather than deriving it from |shared_secret| and the
SSL keying material when verifying?

[Sergey Ulanov, 2011-11-23 01:23:41]

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.cc
File remoting/protocol/auth_util.cc (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.cc...
remoting/protocol/auth_util.cc:10: #include "crypto/sha2.h"
On 2011/11/22 22:29:48, Wez wrote:
> This should come after crypto/hmac.h

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.cc...
remoting/protocol/auth_util.cc:26: LOG(FATAL) << "Failed to encode auth token";
On 2011/11/22 22:29:48, Wez wrote:
> Out of interest, how on earth can that ever fail??
It should not, that's why we have LOG(FATAL) here, but there may be cases when
we get error from the library (e.g. out of memory). Ignoring possible error is
not a good idea.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.cc...
remoting/protocol/auth_util.cc:46: NOTREACHED() << "HMAC::Init failed";
On 2011/11/22 22:29:48, Wez wrote:
> Would it be cleaner to get rid of the return code and CHECK() these?
This method may fail, for example, if key_material has invalid length. Also
current code yields smaller binary, so I think it's better to keep it as it is.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.h
File remoting/protocol/auth_util.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.h#...
remoting/protocol/auth_util.h:6: #define REMOTING_PROTOCOL_AUTH_UTIL_H_
On 2011/11/22 22:29:48, Wez wrote:
> This code defines constants and functions for the IT2Me authentication
protocol
> version 1, so consider either naming this file accordingly, or putting the
> definitions & functions in a namespace in this header?

Some of the code defined in this file will still be useful for EKE-based auth,
e.g. kSslFakeHostName, GetAuthBytes, etc. 

GenerateSupportAuthToken(), VerifySupportAuthToken() will not be used in the
future, but their names are unambiguous already. I agree that it would be better
to rename them, but I would prefer to do it in a separate CL.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/auth_util.h#...
remoting/protocol/auth_util.h:29: const std::string& auth_token);
On 2011/11/22 22:29:48, Wez wrote:
> Should these be base::StringPiece, not std::string?

Yes, but I just moving this code in this CL, so I'd rather to keep it as is.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
File remoting/protocol/channel_authenticator.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
remoting/protocol/channel_authenticator.h:21: class ChannelAuthenticator :
public base::NonThreadSafe {
On 2011/11/22 22:29:48, Wez wrote:
> Give a short description of what this interface is for?

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
remoting/protocol/channel_authenticator.h:21: class ChannelAuthenticator :
public base::NonThreadSafe {
On 2011/11/22 22:29:48, Wez wrote:
> Should this interface really derive from NonThreadSafe?

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
remoting/protocol/channel_authenticator.h:26: ChannelAuthenticator() {}
On 2011/11/22 22:29:48, Wez wrote:
> Make this protected, since it doesn't make sense to instantiate one of these
> directly?

Just removed this constructor.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
remoting/protocol/channel_authenticator.h:29: // Starts SSL on the |socket| and
authenticates it. Takes ownership
On 2011/11/22 22:29:48, Wez wrote:
> Indicate that the caller must not attempt to use |socket| after calling
> SecureAndAuthenticate; they can only use a newly-returned StreamSocket, if the
> call succeeds.

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
remoting/protocol/channel_authenticator.h:30: // of |socket|. |done_callback| is
called when authentication is
On 2011/11/22 22:29:48, Wez wrote:
> Is this always the case?  Surely whether SSL is started is dependent on the
> implementation?  The interface just promises that encryption will be set up
and
> the authentication protocol executed?

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/channel_auth...
remoting/protocol/channel_authenticator.h:31: // finished. Callback handler must
take ownership of the result.
On 2011/11/22 22:29:48, Wez wrote:
> Indicate that the callback may be invoked before SecureAndAuthenticate
returns.

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_chann...
File remoting/protocol/jingle_channel_connector.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_chann...
remoting/protocol/jingle_channel_connector.h:31: // Starts connection process
for the channel. Takes ownership of
On 2011/11/22 22:29:48, Wez wrote:
> nit: Starts _the_ connection process ...

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_datag...
File remoting/protocol/jingle_datagram_connector.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_datag...
remoting/protocol/jingle_datagram_connector.h:27: virtual void
Connect(ChannelAuthenticator* authenticator,
On 2011/11/22 22:29:48, Wez wrote:
> Does this make sense?  ChannelAuthenticator takes a StreamSocket, whereas this
> is the datagram connector, which will create a datagram socket?

Currently JingleDatagramConnector doesn't use SSL anyway. When we get to
implementing it can be done in two ways:
 (1) Modify ChannelAuthenticator so that it knows how to authenticate datagram
channels.
 (2) Add DatagramChannelAuthenticator interface for datagram channels and add
CreateDatagramChannelAuthenticator() in Authenticator interface.

Because we are not using datagram channels right now, I don't think it is
necessary to solve this problem right now. Added TODO.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_sessi...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_sessi...
remoting/protocol/jingle_session.cc:439: }
On 2011/11/22 22:29:48, Wez wrote:
> Is the intent for this code to be replaced with calls to
> Authenticator::CreateChannelAuthenticator() in a follow-up CL?
Yes, and I have it cooking: http://codereview.chromium.org/8619011/

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_strea...
File remoting/protocol/jingle_stream_connector.cc (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/jingle_strea...
remoting/protocol/jingle_stream_connector.cc:112: if (error != net::OK) {
On 2011/11/22 22:29:48, Wez wrote:
> nit: Make this an if ... else ... block?

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/pepper_chann...
File remoting/protocol/pepper_channel.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/pepper_chann...
remoting/protocol/pepper_channel.h:34: // |authenticator|.
On 2011/11/22 22:29:48, Wez wrote:
> nit: How is |pp_instance| used, and owned?
There is always only one pp_instance per plugin, so it is not owned, and I don't
think it is necessary to specify that it is not owned everywhere where we pass
it around because it is obvious.

> How is |authenticator| used? Is
> |authenticator| torn down once authentication is complete, for example, or
kept
> around and torn down with the channel?
In the current implementation it is kept around while channel exists, but it may
be as well destroyed after authentication is finished - it doesn't make any
difference. I don't think it is important to specify it here, as it is a detail
of correct ChannelAuthenticator usage. The only aspect a caller may need to care
about is that the authenticator is destroyed before or when PepperChannel
instance is destroyed - but I don't think it is necessary to explicitly specify
it here.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
File remoting/protocol/simple_client_channel_authenticator.cc (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:22:
net::SSLClientSocket* CreateSSLClientSocket(
On 2011/11/22 22:29:48, Wez wrote:
> Is it really worth having this a separate method from SecureAndAuthenticate?

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:27: // Certificate
provided by the host doesn't need authority.
On 2011/11/22 22:29:48, Wez wrote:
> Isn't this also to do with being self-signed, as below?

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:34: // certs. Disable
it so that SSL layer doesn't try to initialize
On 2011/11/22 22:29:48, Wez wrote:
> nit: that _the_ SSL layer

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:35: // OCSP (OCSP works
only on IO thread).
On 2011/11/22 22:29:48, Wez wrote:
> nit: on _the_ IO thread

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:38: // SSLClientSocket
takes ownership of the adapter.
On 2011/11/22 22:29:48, Wez wrote:
> nit: What adapter?

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:51: const std::string&
remote_cert,
On 2011/11/22 22:29:48, Wez wrote:
> nit: |remote_cert| -> |peer_cert|?
Changed it to host_cert as it is more specific.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:71:
cert_verifier_.reset(new net::CertVerifier());
On 2011/11/22 22:29:48, Wez wrote:
> Why do we pass in a standard CertVerifier if we never verify certificates?
This is because SSLClientSocket still expects to get CertVerifier instance :(
Certificate verification will fail even if we get CA-signed cert because this
code runs in sandbox and X509Certificate work in sandbox. 

Ideally CertVerifier should be an interface, and we would have our own
implementation that accepts only one cert - hopefully I'll fix this some day.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:75: int result =
socket_->Connect(&connect_callback_);
On 2011/11/22 22:29:48, Wez wrote:
> It seems strange for connecting the SteramSocket to be done here; it's neither
> securing nor authenticating?

This connects SSL layer, not the socket itself. This method is expected to be
called with a connected socket - added DCHECK for this.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:84: LOG(ERROR) <<
"Failed to establish SSL connection";
On 2011/11/22 22:29:48, Wez wrote:
> nit: I don't think this needs logging here; calling code can decide what to
log?
This log message should be very rare, and I think it's better to log it here so
we can distinguish between connection and read errors.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:92: LOG(ERROR) <<
"Error fetching keying material: " << export_result;
On 2011/11/22 22:29:48, Wez wrote:
> nit: See above.
same here

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:99:
std::string(key_material, key_material + kAuthDigestLength),
On 2011/11/22 22:29:48, Wez wrote:
> Why not just std::string(key_material, kAuthDigestLength)?

key_material is unsigned int, so std::string(key_material, kAuthDigestLength)
would be ambiguous. Changed it to
std::string(reinterpret_cast<char*>(key_material), kAuthDigestLength).

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:105: // Allocate a
buffer to write the authentication digest.
On 2011/11/22 22:29:48, Wez wrote:
> nit: This function doesn't have comments on earlier code blocks, which
arguably
> would benefit more from explanation than this one!

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.cc:111: void
SimpleClientChannelAuthenticator::DoAuthWrite() {
On 2011/11/22 22:29:48, Wez wrote:
> nit: DoAuthWrite -> WriteAuthenticationBytes() or similar?

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
File remoting/protocol/simple_client_channel_authenticator.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.h:27: class
SimpleClientChannelAuthenticator : public ChannelAuthenticator {
On 2011/11/22 22:29:48, Wez wrote:
> I don't think "simple" is sufficiently descriptive.  How about
> It2MeV1ClientChannelAuthenticator or simply V1ClientChannelAuthenticator?

How about Legacy?

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_clien...
remoting/protocol/simple_client_channel_authenticator.h:35: net::StreamSocket*
socket, const DoneCallback& done_callback);
On 2011/11/22 22:29:48, Wez wrote:
> OVERRIDE?

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
File remoting/protocol/simple_host_channel_authenticator.cc (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.cc:11: #include
"net/socket/ssl_server_socket.h"
On 2011/11/22 22:29:48, Wez wrote:
> Move this below x509_certificate.h

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.cc:51: // Create server SSL
socket.
On 2011/11/22 22:29:48, Wez wrote:
> nit: This comment's a bit redundant...

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.cc:112: LOG(ERROR) << "Error
reading authentication: " << read_result;
On 2011/11/22 22:29:48, Wez wrote:
> nit: Don't really need this logging, or the preceding instances

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.cc:124: LOG(ERROR) <<
"Mismatched authentication";
On 2011/11/22 22:29:48, Wez wrote:
> nit: Make this a warning, or remove it?  Is there a better error-code we can
> return?

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
File remoting/protocol/simple_host_channel_authenticator.h (right):

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.h:30: crypto::RSAPrivateKey*
local_private_key,
On 2011/11/22 22:29:48, Wez wrote:
> Can this parameter be const&?

Currently net::CreateSSLServerSocket() expects non-const pointer and we would
have to copy the key which currently is not very easy to do :(

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.h:34: //
ChannelAuthenticator implementation.
On 2011/11/22 22:29:48, Wez wrote:
> nit: implementation -> interface?

Done.

http://codereview.chromium.org/8604001/diff/25/remoting/protocol/simple_host_...
remoting/protocol/simple_host_channel_authenticator.h:48: std::string
auth_bytes_;
On 2011/11/22 22:29:48, Wez wrote:
> Why store |auth_bytes_|, rather than deriving it from |shared_secret| and the
> SSL keying material when verifying?

Done.

[Wez, 2011-11-23 01:43:16]

LGTM with the rename Simple -> V1 as discussed.

[commit-bot: I haz the power, 2011-11-23 02:07:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/8604001/14002

[commit-bot: I haz the power, 2011-11-23 03:35:33]

Try job failure for 8604001-14002 (retry) on mac_rel for step "media_unittests".
It's a second try, previously, step "media_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...