Closed Bug 1279655 Opened 4 years ago Closed 4 years ago

Properly test that the handshake can't complete without AuthCertificateComplete

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mt, Assigned: mt)

Details

Attachments

(1 file, 2 obsolete files)

Bug 1274718 corrected a problem where we would always wait for the AuthComplete callback before sending the client's second flight.  However, this exposed that the client doesn't wait for AuthComplete before reporting the handshake as complete.

The following modification to the test confirms this (though it asserts when calling SSL_AuthCertificateComplete() because that checks that we aren't in the idle_handshake wait state):

TEST_F(TlsConnectDatagram13, AuthCompleteAfterFinished) {
  client_->SetAuthCertificateCallback(
      [this](TlsAgent&, PRBool, PRBool) -> SECStatus {
        return SECWouldBlock;
      });
  Connect();
  EXPECT_EQ(SECSuccess, SSL_AuthCertificateComplete(client_->ssl_fd(), 0));
}
Attached patch bug1279655-1.patch (obsolete) — Splinter Review
Assignee: nobody → martin.thomson
Attachment #8762251 - Flags: review?(ekr)
It looks to me like bug 1274718 introduced this defect, because previously we would hold all of ssl3_ClientSendSecondRound(). And that patch was backed out for bustage.

Assuming I do understand correctly, I propose just WONTFIXING 1274718 and marking this bug WORKSFORME. My reasoning is:

1. It's simpler code.
2. HTTP servers don't typically want to send first.
3. If the server cares about being able to send immediately, we can instead add support for 0.5RTT data, which will actually be faster for the server.

Martin?
Flags: needinfo?(martin.thomson)
Comment on attachment 8762251 [details] [diff] [review]
bug1279655-1.patch

Review of attachment 8762251 [details] [diff] [review]:
-----------------------------------------------------------------

I think you should land some test here, but I suggest a simpler one if you follow my advice not to land this libssl patch.

The server sends its entire flight at once, so why not just validate that
after the server has sent its ServerHello and you have re-entered the
event loop, both sides are still in CONNECTING?

::: external_tests/ssl_gtest/libssl_internals.c
@@ +148,5 @@
> +  if (!ss) {
> +    /* No socket: can't get more idle than that. */
> +    return PR_TRUE;
> +  }
> +  return ss->ssl3.hs.ws == idle_handshake;

Do you need to check this state? Can't you just check for the agent state?

I.e., client_->state()
Yes, let's update the testing but leave the code.  I think that the agent state will do.
Flags: needinfo?(martin.thomson)
Summary: TLS 1.3 will complete the handshake prior to the async certificate verification completing → Properly test that the handshake can't complete without AuthCertificateComplete
Attached patch bug1279655-1.patch (obsolete) — Splinter Review
Attachment #8762251 - Attachment is obsolete: true
Attachment #8762251 - Flags: review?(ekr)
Attachment #8762383 - Flags: review?(ekr)
Comment on attachment 8762383 [details] [diff] [review]
bug1279655-1.patch

Review of attachment 8762383 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: external_tests/ssl_gtest/ssl_loopback_unittest.cc
@@ +934,5 @@
> +  client_->StartConnect();
> +  do {
> +    client_->Handshake();
> +    server_->Handshake();
> +  } while (server_->state() == TlsAgent::STATE_CONNECTING);

!= STATE_CONNECTED.

@@ +937,5 @@
> +    server_->Handshake();
> +  } while (server_->state() == TlsAgent::STATE_CONNECTING);
> +  client_->Handshake();
> +  client_->Handshake();
> +  EXPECT_EQ(TlsAgent::STATE_CONNECTING, client_->state());

You should actually call the callback and verify that the handshake completes.
Attachment #8762383 - Flags: review?(ekr) → review+
> != STATE_CONNECTED.

I don't think that's ideal.  We could end up in a situation where the test hangs if there is an error.  CONNECTING is fine I think.
Attachment #8762383 - Attachment is obsolete: true
Attachment #8762617 - Flags: review?(ekr)
Updated patch on rietveld: https://codereview.appspot.com/302040043/
Flags: needinfo?(ekr)
Updated patch here: https://nss-dev.phacility.com/D68
https://hg.mozilla.org/projects/nss/rev/66d0a9ca356480db53561a3d3f75c1cb4a671b0b
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(ekr)
Resolution: --- → FIXED
Target Milestone: --- → 3.28
Comment on attachment 8762617 [details] [diff] [review]
bug1279655-1.patch

OBE
Attachment #8762617 - Flags: review?(ekr)
You need to log in before you can comment on or make changes to this bug.