Enable client certificates in SecureSocket and SecureServerSocket

Enable client certificates in SecureSocket and SecureServerSocket


BUG=

Committed: https://code.google.com/p/dart/source/detail?r=15957

[Søren Gjesse, 2012-12-07 13:00:23]

I think it will be beneficial to combine some of the booleans used into "enums"
or "flags" and use ints.

https://codereview.chromium.org/11467004/diff/3001/runtime/bin/secure_socket.cc
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/11467004/diff/3001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:409: bool send_client_certificate) {
Maybe add some assets on valid combinations of is_server,
request_client_certificate, require_client_certificate and
send_client_certificate.

https://codereview.chromium.org/11467004/diff/3001/runtime/bin/secure_socket.h
File runtime/bin/secure_socket.h (right):

https://codereview.chromium.org/11467004/diff/3001/runtime/bin/secure_socket....
runtime/bin/secure_socket.h:77: const char* certificate_name,
Now about making an int with flags here instead of a number of boolean
arguments. is_server could be a flag as well.

https://codereview.chromium.org/11467004/diff/3001/runtime/bin/secure_socket_...
File runtime/bin/secure_socket_patch.dart (right):

https://codereview.chromium.org/11467004/diff/3001/runtime/bin/secure_socket_...
runtime/bin/secure_socket_patch.dart:37: void connect(String hostName,
With all these bool consider using an int with flags as suggested for the C++
code. Maybe creating two mentods createServer and createClient could also help.

https://codereview.chromium.org/11467004/diff/3001/sdk/lib/io/secure_server_s...
File sdk/lib/io/secure_server_socket.dart (right):

https://codereview.chromium.org/11467004/diff/3001/sdk/lib/io/secure_server_s...
sdk/lib/io/secure_server_socket.dart:28: {bool requestClientCertificate: false,
How about using an "enum" here with the following values:

SecureServerSocket.NO_CLIENT_CERTIFICATE
SecureServerSocket.REQUEST_CLIENT_CERTIFICATE
SecureServerSocket.REQUIRE_CLIENT_CERTIFICATE

I think it will be easier to understand.

https://codereview.chromium.org/11467004/diff/3001/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/11467004/diff/3001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:19: * a certificate in the certificate database.
How about rephrasing this additional comment to 

"If clientCertificate is set to the nickname" ... "otherwise an appropriate" ...

The "unless" way is harder to decode.

https://codereview.chromium.org/11467004/diff/3001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:24: String certificateName}) {
Please indent one char more here.

https://codereview.chromium.org/11467004/diff/3001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:42: * client certificate was requested or
supplied.  For a client socket, this
Maybe remove "requested or".

[Bill Hesse, 2012-12-10 15:15:48]

Addressed comments:
I gave my reasons for using 3 boolean variables in the API, rather than flags
combined into an int.  We could also use flags for the Dart - C++ interface, but
I'm not convinced that this makes things clearer.  There are two bools for
server, and one for client.

Changed most members of the private implementation class to public, so that the
constructor is simplified.  Simplified the constructors of the _SecureSocket
class from 3 (.server, .client, and .internal) to 1 (default).

https://codereview.chromium.org/11467004/diff/3001/runtime/bin/secure_socket.cc
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/11467004/diff/3001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:409: bool send_client_certificate) {
On 2012/12/07 13:00:23, Søren Gjesse wrote:
> Maybe add some assets on valid combinations of is_server,
> request_client_certificate, require_client_certificate and
> send_client_certificate.

No combinations are invalid per se.  The most "invalid" would be an explicit
setting of request to false and require to true, but that combination requests
and requires certificates, and is in fact the expected combination that users of
the API should send, by only explicitly setting require_client_certificate.

The other interactions are just that request.. and require.. are only meaningful
on server connections, and send.. is only meaningful on a client connection. 
They are just ignored in cases where they are not applicable.

https://codereview.chromium.org/11467004/diff/3001/runtime/bin/secure_socket.h
File runtime/bin/secure_socket.h (right):

https://codereview.chromium.org/11467004/diff/3001/runtime/bin/secure_socket....
runtime/bin/secure_socket.h:77: const char* certificate_name,
On 2012/12/07 13:00:23, Søren Gjesse wrote:
> Now about making an int with flags here instead of a number of boolean
> arguments. is_server could be a flag as well.

Because we would just pack them all up before calling the function, and unpack
them all in the called function, using flag definitions on the bits that need to
be added to the Dart and C++ files, and kept in sync between them, I think it is
clearer, and not that much less efficient, to just pass them explicitly.  We can
always change this later, especially if we add more flags.

https://codereview.chromium.org/11467004/diff/3001/runtime/bin/secure_socket_...
File runtime/bin/secure_socket_patch.dart (right):

https://codereview.chromium.org/11467004/diff/3001/runtime/bin/secure_socket_...
runtime/bin/secure_socket_patch.dart:37: void connect(String hostName,
On 2012/12/07 13:00:23, Søren Gjesse wrote:
> With all these bool consider using an int with flags as suggested for the C++
> code. Maybe creating two mentods createServer and createClient could also
help.

This is the call to the C++ method in the other comment, so I have discussed
there why I think this is clearer.

I have actually remove the .client and .server constructor at a higher level
(the _SecureSocket class), and I think it makes it clearer.

https://codereview.chromium.org/11467004/diff/3001/sdk/lib/io/secure_server_s...
File sdk/lib/io/secure_server_socket.dart (right):

https://codereview.chromium.org/11467004/diff/3001/sdk/lib/io/secure_server_s...
sdk/lib/io/secure_server_socket.dart:28: {bool requestClientCertificate: false,
On 2012/12/07 13:00:23, Søren Gjesse wrote:
> How about using an "enum" here with the following values:
> 
> SecureServerSocket.NO_CLIENT_CERTIFICATE
> SecureServerSocket.REQUEST_CLIENT_CERTIFICATE
> SecureServerSocket.REQUIRE_CLIENT_CERTIFICATE
> 
> I think it will be easier to understand.


I strongly feel that for the user of the API, choosing between
new SecureServerSocket(...., requestClientCertificate: true)
new SecureServerSocket(...., requireClientCertificate: true)
and
new SecureServerSocket(....)

will be easier than writing
new SecureServerSocket(...., SecureServerSocket.REQUEST_CLIENT_CERTIFICATE)
or 
new SecureServerSocket(...., SecureServerSocket.REQUEST_CLIENT_CERTIFICATE),

especially since there should be editor support for the first ones.

I can see combining the flags at the native implementation level, because
parameter passing is such a pain, but here this seems better to me.

https://codereview.chromium.org/11467004/diff/3001/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/11467004/diff/3001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:19: * a certificate in the certificate database.
On 2012/12/07 13:00:23, Søren Gjesse wrote:
> How about rephrasing this additional comment to 
> 
> "If clientCertificate is set to the nickname" ... "otherwise an appropriate"
...
> 
> The "unless" way is harder to decode.

Done.

https://codereview.chromium.org/11467004/diff/3001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:24: String certificateName}) {
On 2012/12/07 13:00:23, Søren Gjesse wrote:
> Please indent one char more here.

Done.

https://codereview.chromium.org/11467004/diff/3001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:42: * client certificate was requested or
supplied.  For a client socket, this
On 2012/12/07 13:00:23, Søren Gjesse wrote:
> Maybe remove "requested or".

Done.

[Søren Gjesse, 2012-12-10 15:28:01]

LGTM

[Mads Ager (google), 2012-12-11 07:45:26]

LGTM

Now, we need to take this up to the HTTP level where an HTTP server should be
able to request client certificates and be able to filter out requests based on
whether it got a certificate (and maybe on the fields of the certificate as
well).

https://codereview.chromium.org/11467004/diff/9001/runtime/bin/secure_socket.cc
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/11467004/diff/9001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:92: ASSERT(false);
Can we use FATAL here with a string explaining what went wrong?

https://codereview.chromium.org/11467004/diff/9001/sdk/lib/io/secure_server_s...
File sdk/lib/io/secure_server_socket.dart (right):

https://codereview.chromium.org/11467004/diff/9001/sdk/lib/io/secure_server_s...
sdk/lib/io/secure_server_socket.dart:98: final bool requireClientCertificate;}
Move the '}' where it belongs.

https://codereview.chromium.org/11467004/diff/9001/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/11467004/diff/9001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:151: if (requestClientCertificate is! bool) throw
new ArgumentError(
Please use curly braces when using multiple lines for an if statement:

if (requestClientCertificate is! bool) {
  throw new ArgumentError(
      "SecureSocket constructor: ...");
}

Please update all three of these.

[Bill Hesse, 2012-12-11 09:20:53]

https://codereview.chromium.org/11467004/diff/9001/runtime/bin/secure_socket.cc
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/11467004/diff/9001/runtime/bin/secure_socket....
runtime/bin/secure_socket.cc:92: ASSERT(false);
On 2012/12/11 07:45:27, Mads Ager wrote:
> Can we use FATAL here with a string explaining what went wrong?

Done.

https://codereview.chromium.org/11467004/diff/9001/sdk/lib/io/secure_server_s...
File sdk/lib/io/secure_server_socket.dart (right):

https://codereview.chromium.org/11467004/diff/9001/sdk/lib/io/secure_server_s...
sdk/lib/io/secure_server_socket.dart:98: final bool requireClientCertificate;}
On 2012/12/11 07:45:27, Mads Ager wrote:
> Move the '}' where it belongs.

Done.

https://codereview.chromium.org/11467004/diff/9001/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/11467004/diff/9001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:151: if (requestClientCertificate is! bool) throw
new ArgumentError(
On 2012/12/11 07:45:27, Mads Ager wrote:
> Please use curly braces when using multiple lines for an if statement:
> 
> if (requestClientCertificate is! bool) {
>   throw new ArgumentError(
>       "SecureSocket constructor: ...");
> }
> 
> Please update all three of these.

Done.