Add a callback to SecureSocket for certificates that fail to be authenticated.

Add a callback to SecureSocket for certificates that fail to be authenticated.


BUG=

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

[Bill Hesse, 2012-12-04 19:02:09]

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

https://codereview.chromium.org/11415290/diff/1/runtime/bin/secure_socket.cc#...
runtime/bin/secure_socket.cc:452: PRBool as_server = is_server ? PR_TRUE :
PR_FALSE;
Whoops - I'll move this down below the new stuff.

[Mads Ager (google), 2012-12-05 07:30:53]

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

https://codereview.chromium.org/11415290/diff/1/runtime/bin/secure_socket.cc#...
runtime/bin/secure_socket.cc:370: 
Inconsistent spacing. Two new lines between other methods.

https://codereview.chromium.org/11415290/diff/1/runtime/bin/secure_socket.cc#...
runtime/bin/secure_socket.cc:377: // PRInt32 error_code = PR_GetError();
Remove.

https://codereview.chromium.org/11415290/diff/1/runtime/bin/secure_socket.cc#...
runtime/bin/secure_socket.cc:388: static_cast<int64_t>(start_validity) /
PR_USEC_PER_MSEC;
Do you get warnings if you do not have the static cast? It seems that you should
be able to do:

start_validity / PR_USEC_PER_MSEC

since they are both defined by NSS and they are both 64-bit integers. Do you
need the cast? Can you cast the result of the operation instead of one of the
operands?

https://codereview.chromium.org/11415290/diff/1/runtime/bin/secure_socket.cc#...
runtime/bin/secure_socket.cc:392: return
CallBadCertificateCallback(bad_certificate_callback_,
I would find it easier to read if this was:

bool continue = CallBadCertificateCallback(...,
                                           ...);
return continue ? SECSuccess : SECFailure;

https://codereview.chromium.org/11415290/diff/1/runtime/bin/secure_socket.cc#...
runtime/bin/secure_socket.cc:457: static_cast<void *>(this));
void * -> void*

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

https://codereview.chromium.org/11415290/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:17: /**
Please add a blank line between members.

https://codereview.chromium.org/11415290/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:24: /**
Ditto.

https://codereview.chromium.org/11415290/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:62: abstract class X509Certificate {
I think we should just remove the abstract class. That would allow users to
create X509Certificate objects, but I don't see any harm in that.

https://codereview.chromium.org/11415290/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:192: if (callback is! Function  && callback !=
null) {
Remove extra space before '&&'

https://codereview.chromium.org/11415290/diff/1/tests/standalone/io/secure_so...
File tests/standalone/io/secure_socket_bad_certificate_test.dart (right):

https://codereview.chromium.org/11415290/diff/1/tests/standalone/io/secure_so...
tests/standalone/io/secure_socket_bad_certificate_test.dart:32: then: (){
Add space: () {

testCertificateCallback should return a future so you can actually use .then().

https://codereview.chromium.org/11415290/diff/1/tests/standalone/io/secure_so...
tests/standalone/io/secure_socket_bad_certificate_test.dart:35: then: (){
Ditto

https://codereview.chromium.org/11415290/diff/1/tests/standalone/io/secure_so...
tests/standalone/io/secure_socket_bad_certificate_test.dart:36: // TODO(whesse):
Open a receive port, and close it when we get here.
This is bad. Please file a high-priority bug report on this.

https://codereview.chromium.org/11415290/diff/1/tests/standalone/io/secure_so...
tests/standalone/io/secure_socket_bad_certificate_test.dart:58: 
Remove empty line?

[Bill Hesse, 2012-12-05 12:35:38]

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

https://codereview.chromium.org/11415290/diff/1/runtime/bin/secure_socket.cc#...
runtime/bin/secure_socket.cc:370: 
On 2012/12/05 07:30:53, Mads Ager wrote:
> Inconsistent spacing. Two new lines between other methods.

Done.

https://codereview.chromium.org/11415290/diff/1/runtime/bin/secure_socket.cc#...
runtime/bin/secure_socket.cc:377: // PRInt32 error_code = PR_GetError();
On 2012/12/05 07:30:53, Mads Ager wrote:
> Remove.

Done.

https://codereview.chromium.org/11415290/diff/1/runtime/bin/secure_socket.cc#...
runtime/bin/secure_socket.cc:392: return
CallBadCertificateCallback(bad_certificate_callback_,
On 2012/12/05 07:30:53, Mads Ager wrote:
> I would find it easier to read if this was:
> 
> bool continue = CallBadCertificateCallback(...,
>                                            ...);
> return continue ? SECSuccess : SECFailure;

Done.

https://codereview.chromium.org/11415290/diff/1/runtime/bin/secure_socket.cc#...
runtime/bin/secure_socket.cc:457: static_cast<void *>(this));
On 2012/12/05 07:30:53, Mads Ager wrote:
> void * -> void*

Done.

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

https://codereview.chromium.org/11415290/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:17: /**
On 2012/12/05 07:30:53, Mads Ager wrote:
> Please add a blank line between members.

Done.

https://codereview.chromium.org/11415290/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:62: abstract class X509Certificate {
On 2012/12/05 07:30:53, Mads Ager wrote:
> I think we should just remove the abstract class. That would allow users to
> create X509Certificate objects, but I don't see any harm in that.

Done.

https://codereview.chromium.org/11415290/diff/1/tests/standalone/io/secure_so...
File tests/standalone/io/secure_socket_bad_certificate_test.dart (right):

https://codereview.chromium.org/11415290/diff/1/tests/standalone/io/secure_so...
tests/standalone/io/secure_socket_bad_certificate_test.dart:36: // TODO(whesse):
Open a receive port, and close it when we get here.
On 2012/12/05 07:30:53, Mads Ager wrote:
> This is bad. Please file a high-priority bug report on this.

Done.

https://codereview.chromium.org/11415290/diff/1/tests/standalone/io/secure_so...
tests/standalone/io/secure_socket_bad_certificate_test.dart:58: 
On 2012/12/05 07:30:53, Mads Ager wrote:
> Remove empty line?

Done.

[Mads Ager (google), 2012-12-05 12:40:53]

DBC

https://codereview.chromium.org/11415290/diff/5002/tests/standalone/io/secure...
File tests/standalone/io/secure_socket_bad_certificate_test.dart (right):

https://codereview.chromium.org/11415290/diff/5002/tests/standalone/io/secure...
tests/standalone/io/secure_socket_bad_certificate_test.dart:43: void
testCertificateCallback({String host,
Please return a Future from here and use the 'then' of the future in main.

[Bill Hesse, 2012-12-05 12:58:25]

https://codereview.chromium.org/11415290/diff/5002/tests/standalone/io/secure...
File tests/standalone/io/secure_socket_bad_certificate_test.dart (right):

https://codereview.chromium.org/11415290/diff/5002/tests/standalone/io/secure...
tests/standalone/io/secure_socket_bad_certificate_test.dart:43: void
testCertificateCallback({String host,
On 2012/12/05 12:40:53, Mads Ager wrote:
> Please return a Future from here and use the 'then' of the future in main.

Done.

[Mads Ager (google), 2012-12-05 13:08:03]

LGTM once the persistent handle leak is fixed.

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

https://codereview.chromium.org/11415290/diff/11001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:311: bad_certificate_callback_ =
ThrowIfError(Dart_NewPersistentHandle(complete));
We need to get rid of the last persistent handle somewhere as well to not leak a
closure per filter.

https://codereview.chromium.org/11415290/diff/11001/tests/standalone/io/secur...
File tests/standalone/io/secure_socket_bad_certificate_test.dart (right):

https://codereview.chromium.org/11415290/diff/11001/tests/standalone/io/secur...
tests/standalone/io/secure_socket_bad_certificate_test.dart:43: Function then})
{
Delete the 'then' parameter now that you return a future. :)

https://codereview.chromium.org/11415290/diff/11001/tests/standalone/io/secur...
tests/standalone/io/secure_socket_bad_certificate_test.dart:61: secure.onData =
(){
Add a space before the {

[Bill Hesse, 2012-12-05 14:31:06]

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

https://codereview.chromium.org/11415290/diff/11001/runtime/bin/secure_socket...
runtime/bin/secure_socket.cc:311: bad_certificate_callback_ =
ThrowIfError(Dart_NewPersistentHandle(complete));
On 2012/12/05 13:08:03, Mads Ager wrote:
> We need to get rid of the last persistent handle somewhere as well to not leak
a
> closure per filter.
Done - It is freed in the destructor, if it exists.
Done.

https://codereview.chromium.org/11415290/diff/11001/tests/standalone/io/secur...
File tests/standalone/io/secure_socket_bad_certificate_test.dart (right):

https://codereview.chromium.org/11415290/diff/11001/tests/standalone/io/secur...
tests/standalone/io/secure_socket_bad_certificate_test.dart:43: Function then})
{
On 2012/12/05 13:08:03, Mads Ager wrote:
> Delete the 'then' parameter now that you return a future. :)

Done.

https://codereview.chromium.org/11415290/diff/11001/tests/standalone/io/secur...
tests/standalone/io/secure_socket_bad_certificate_test.dart:61: secure.onData =
(){
On 2012/12/05 13:08:03, Mads Ager wrote:
> Add a space before the {

Done.