702322 - Create loopback test framework for libssl

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

We need a test framework that allows a single test to control in fine detail both sides of the connection, including the precise timing of messages sent/received.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I did some work on this that allows for writing tests like the one below. This bug is about the framework for the tests: DEFINE_TEST, setupClientServer, CHECK_*, the hooks in libssl that allow the test framework to define SSL_SetImmediatelyRenegotiate, etc.

The test below tests that asynchronous certificate validation (bug 542832) still allows us to see a hello_request that appears in the same packet as the Finished message. 

DEFINE_TEST(async_cert_restart_server_sends_hello_request_first_in_same_record)
{
    PRFileDesc * client;
    PRFileDesc * server;
    PRBool clientWaitingForServerAuth = PR_FALSE; // XXX: use
    PRBool clientDone = PR_FALSE;
    PRBool serverDone = PR_FALSE;
    SSL3HandshakeState *client_hs;
    SSL3HandshakeState *server_hs;

    setupClientServer(&client, clientHooks, 
                      &server, serverHooks,
                      ssl_kea_rsa);
    CHECK_NOT_NULL(client_hs = &ssl_FindSocket(client)->ssl3.hs);
    CHECK_NOT_NULL(server_hs = &ssl_FindSocket(server)->ssl3.hs);

    CHECK_SECSuccess(SSL_AuthCertificateHook(client, return_SECWouldBlock,
                                             &clientWaitingForServerAuth));
    CHECK_SECSuccess(SSL_HandshakeCallback(client, setHandshakeDone,
                                           &clientDone));
    CHECK_SECSuccess(SSL_HandshakeCallback(server, setHandshakeDone,
                                           &serverDone));
    /*************************************************************************/
    SSL_SetImmediatelyRenegotiate(serverHooks);
    /*************************************************************************/

    CHECK_TRUE(!client_hs->authCertificatePending);
    CHECK_NULL(client_hs->authCertificateRestartTarget);

    CHECK_SECFailure(SSL_ForceHandshake(client)); /* client hello */
    CHECK_TRUE(PORT_GetError() == PR_WOULD_BLOCK_ERROR);
    CHECK_WS(client, wait_server_hello);
    CHECK_TRUE(!client_hs->authCertificatePending);
    CHECK_NULL(client_hs->authCertificateRestartTarget);

    CHECK_WS(server, wait_client_hello);
    CHECK_TRUE(!server_hs->authCertificatePending);
    CHECK_NULL(server_hs->authCertificateRestartTarget);

    CHECK_SECFailure(SSL_ForceHandshake(server)); /* server hello...done */
    CHECK_TRUE(PORT_GetError() == PR_WOULD_BLOCK_ERROR);
    CHECK_WS(server, wait_client_key);
    CHECK_TRUE(!server_hs->authCertificatePending);
    CHECK_NULL(server_hs->authCertificateRestartTarget);

    CHECK_SECFailure(SSL_ForceHandshake(client)); /* client 2nd round */
    CHECK_TRUE(PORT_GetError() == PR_WOULD_BLOCK_ERROR);
    CHECK_WS(client, wait_change_cipher);
    CHECK_TRUE(client_hs->authCertificatePending);
    CHECK_NULL(client_hs->authCertificateRestartTarget);

    CHECK_SECSuccess(SSL_ForceHandshake(server)); /* server finished */
    CHECK_TRUE(serverDone);
    /*************************************************************************/
    CHECK_WS(server, wait_client_hello);
    /*************************************************************************/
    CHECK_TRUE(!server_hs->authCertificatePending);
    CHECK_NULL(server_hs->authCertificateRestartTarget);

    CHECK_SECSuccess(SSL_ForceHandshake(client)); /* consume server finished */
    CHECK_TRUE(!clientDone); /* client handshake callback not called yet */
    CHECK_WS(client, wait_finished);
    CHECK_TRUE(client_hs->authCertificatePending);
    CHECK_NOT_NULL(client_hs->authCertificateRestartTarget);

    CHECK_SECSuccess(SSL_RestartHandshakeAfterAuthCertificate(client));
    CHECK_TRUE(clientDone); /* client handshake callback not called yet */
    CHECK_WS(client, idle_handshake);
    CHECK_TRUE(!client_hs->authCertificatePending);
    CHECK_NULL(client_hs->authCertificateRestartTarget);

    CHECK_SECFailure(SSL_ForceHandshake(client));
    CHECK_TRUE(PORT_GetError() == PR_WOULD_BLOCK_ERROR);
    CHECK_WS(client, wait_server_hello);
}

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Improve SECU_Print* to include error message name and fix constness

Adding the error name to error messages make it *much* easier to investigate errors. The constness changes were done so that the loopback framework could use const in the correct places.

Note that I am posting Mercurial/git-format patches to this bug, mostly because they are much easier to manage when there are patches on top of other patches.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: The beginnings of a loopback testing framework for libssl

This simple framework makes it possible to test interactions between the client and the server during the handshake in a way that is under the test's control. For example, it allows us to test things like "what happens if the server sends application data before the client has validated the server's cert" in a way that isn't prone to any race conditions.

It creates a socket pair in the process, where one side gets configured as an SSL client and the other side gets configured as an SSL server.

It also includes the beginnings of a very simple hook mechanism. The idea is that, wherever we need testing code injected into libssl, we can create a new hook point. Then, libssl will call that hook at that point, if a hook is provided. For example, one of the tests that I will post uses such a hook to insert a hello_request record into the same record as the server's finished message. I am planning to expand this hooking mechanism so that it can let us to quickly simulate server-side features (e.g. the server side of OCSP stapling, server-side NPN support), without investing the time in production-quality server side code, and without littering libssl itself with non-production-quality code. I am also hoping that people will be able to use it to write self-contained test cases for bug reports.

See bug 542832 for examples of tests written using this.

Comment 4

[Robert Relyea]

Comment on attachment 574835 [details] [diff] [review]
Improve SECU_Print* to include error message name and fix constness

r+

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Improve SECU_Print* to include error message name and fix constness [v2]

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 584631 [details] [diff] [review]
Improve SECU_Print* to include error message name and fix constness [v2]

Carrying forward r+ and sr+. The only changes were to whitespace.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: The beginnings of a loopback testing framework for libssl [v2]

Work in progress, needed to run the tests for bug 542832.

I am going to update this based on feedback from rrelyea and add some documentation to it.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: The beginnings of a loopback testing framework for libssl [v3]

Comment 9

[Robert Relyea]

Comment on attachment 588356 [details] [diff] [review]
The beginnings of a loopback testing framework for libssl [v3]

r-, but only for some simple issues.

First, the private exports for testing should have _ symbols in their name (_ssl_SendHelloRequest()). Adding a comment in the def file that they are strictly for testing would also be a good idea.

Second. If you add the _ symbols, you can have both the _ssl_SendHelloRequest() and an ssl_SendHelloRequest(), the latter can include the flush and setting of ws state. That would simplify your patch to ssl3con.c

Third. You have this mechanism for creating tests with .h files, but I'm not exactly clear how it's supposed to work. In sslloopbacktests.h You have a #define X(x) in the header file, which is set based on some declaration... then you just #undefine it without using it. Is the goal here that you edit sslloopbacktests.h add add each test by hand? It might be more clear if it includes a 'definetests.h' which just has a bunch of entries:
   X(mytest1)
   X(mytest2)
   X(mytest3)
etc.

Forth, the frame work looks like it reaches right into the private ssl data structure. This should be avoided if at all possible (without introducing too much pain). We try pretty hard to keep a separation between the commands in cmd and the private NSS data. That way our old test commands will run on newer versions of NSS (something we use to actually test once in a while to make sure we didn't break binary compatibility). The SSL data structure is private, and we can and do change it between releases.

bob

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 584631 [details] [diff] [review]
Improve SECU_Print* to include error message name and fix constness [v2]

Part 1 was moved to bug 863888 and checked in.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I realized after I started this work that 75% of what I'd done was poorly re-implement GTest. It would be better to create a GTest-based framework for testing libssl, which should happen separately from this bug.