Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow Clients with Optional Client Auth to not negotiate Client Auth #816

Merged
merged 3 commits into from
Aug 2, 2018
Merged

Allow Clients with Optional Client Auth to not negotiate Client Auth #816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c75c18e
Allow Clients with Optional Client Auth to not negotiate Client Auth
alexw91 Jul 25, 2018
a008305
Proof fix and small README fix for SAW
jldodds Jul 30, 2018
5ff94c1
Merge pull request #2 from GaloisInc/optionalClientAuth
alexw91 Jul 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions tests/saw/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
#SAW tests for s2n
# SAW tests for s2n

This repository contains specifications of the various parts of the HMAC
algorithm used in TLS along with SAW scripts to prove the s2n implementation of
this algorithm equivalent to the spec.


##The tests
## The tests

Currently this directory houses a test that compares the s2n
implementation of HMAC with a cryptol spec of the same. There are 3
Expand Down Expand Up @@ -37,7 +37,7 @@ instructions for how automated solvers can be used to prove this
equivalence. When run with the command `saw HMAC.saw`, this file loads
in the other two and proves the HMAC files equivalent.

##The build
## The build

Running the saw tests will require a SAW executable, which must be
able to find the Z3 prover on the path. Future examples might require
Expand Down
8 changes: 5 additions & 3 deletions tests/saw/s2n_handshake_io.saw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ let setup_connection = do {
ocsp_flag <- crucible_fresh_var "ocsp_flag" (llvm_int 32);
crucible_points_to (conn_status_type pconn) (crucible_term ocsp_flag);


let client_cert_auth_type = {{ if cca_ov != 0 then cca else config_cca }};

return (pconn, {{ {corked_io = corked_io
,mode = mode
,handshake = {message_number = message_number
Expand All @@ -123,8 +124,9 @@ let setup_connection = do {
((ocsp_flag == 1) && (status_size > 0)) ||
((mode == 1) && (ocsp_flag == 1))
,resume_from_cache = False
,client_auth_flag = (if cca_ov != 0 then cca != 0 else config_cca != 0)
}
,client_auth_flag = if mode == S2N_CLIENT then client_cert_auth_type == 1 else
if mode == S2N_SERVER then client_cert_auth_type != 0 else False
}
}});
};

Expand Down
71 changes: 71 additions & 0 deletions tests/unit/s2n_optional_client_auth_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,77 @@ int main(int argc, char **argv)

EXPECT_SUCCESS(s2n_config_free(client_config));

/*
* Test optional client auth using **s2n_config_set_client_auth_type** with S2N_CERT_AUTH_NONE for Server
*/

EXPECT_NOT_NULL(client_config = s2n_config_new());
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key(client_config, cert_chain_pem, private_key_pem));
EXPECT_SUCCESS(s2n_config_disable_x509_verification(client_config));
EXPECT_SUCCESS(s2n_config_set_client_auth_type(client_config, S2N_CERT_AUTH_OPTIONAL));

/* Server does not request a Client Cert. */
EXPECT_SUCCESS(s2n_config_set_client_auth_type(server_config, S2N_CERT_AUTH_NONE));
EXPECT_SUCCESS(s2n_config_disable_x509_verification(server_config));

/* Verify that a handshake succeeds for every cipher in the default list. */
for (int cipher_idx = 0; cipher_idx < default_cipher_preferences->count; cipher_idx++) {
struct s2n_cipher_preferences server_cipher_preferences;
struct s2n_connection *client_conn;
struct s2n_connection *server_conn;
int client_to_server[2];
int server_to_client[2];

/* Craft a cipher preference with a cipher_idx cipher. */
memcpy(&server_cipher_preferences, default_cipher_preferences, sizeof(server_cipher_preferences));
server_cipher_preferences.count = 1;
struct s2n_cipher_suite *cur_cipher = default_cipher_preferences->suites[cipher_idx];

if (!cur_cipher->available) {
/* Skip ciphers that aren't supported with the linked libcrypto. */
continue;
}

server_cipher_preferences.suites = &cur_cipher;
client_config->cipher_preferences = &server_cipher_preferences;
server_config->cipher_preferences = &server_cipher_preferences;

/* Create nonblocking pipes. */
EXPECT_SUCCESS(pipe(client_to_server));
EXPECT_SUCCESS(pipe(server_to_client));
for (int i = 0; i < 2; i++) {
EXPECT_NOT_EQUAL(fcntl(client_to_server[i], F_SETFL, fcntl(client_to_server[i], F_GETFL) | O_NONBLOCK), -1);
EXPECT_NOT_EQUAL(fcntl(server_to_client[i], F_SETFL, fcntl(server_to_client[i], F_GETFL) | O_NONBLOCK), -1);
}

EXPECT_NOT_NULL(client_conn = s2n_connection_new(S2N_CLIENT));
EXPECT_SUCCESS(s2n_connection_set_config(client_conn, client_config));
EXPECT_SUCCESS(s2n_connection_set_read_fd(client_conn, server_to_client[0]));
EXPECT_SUCCESS(s2n_connection_set_write_fd(client_conn, client_to_server[1]));

EXPECT_NOT_NULL(server_conn = s2n_connection_new(S2N_SERVER));
EXPECT_SUCCESS(s2n_connection_set_config(server_conn, server_config));
EXPECT_SUCCESS(s2n_connection_set_read_fd(server_conn, client_to_server[0]));
EXPECT_SUCCESS(s2n_connection_set_write_fd(server_conn, server_to_client[1]));

/* Verify the handshake was successful. */
EXPECT_SUCCESS(try_handshake(server_conn, client_conn));

/* Verify that neither connections negotiated mutual auth. */
EXPECT_FALSE(s2n_connection_client_cert_used(server_conn));
EXPECT_FALSE(s2n_connection_client_cert_used(client_conn));

EXPECT_SUCCESS(s2n_connection_free(client_conn));
EXPECT_SUCCESS(s2n_connection_free(server_conn));

for (int i = 0; i < 2; i++) {
EXPECT_SUCCESS(close(server_to_client[i]));
EXPECT_SUCCESS(close(client_to_server[i]));
}
}

EXPECT_SUCCESS(s2n_config_free(client_config));


/*
* Test optional client auth using **s2n_config_set_client_auth_type** with no client cert provided.
Expand Down
27 changes: 23 additions & 4 deletions tls/s2n_handshake_io.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,8 @@ static message_type_t handshakes[128][16] = {
#define ACTIVE_STATE( conn ) state_machine[ ACTIVE_MESSAGE( (conn) ) ]
#define PREVIOUS_STATE( conn ) state_machine[ PREVIOUS_MESSAGE( (conn) ) ]

#define EXPECTED_MESSAGE_TYPE( conn ) ACTIVE_STATE( conn ).message_type

/* Used in our test cases */
message_type_t s2n_conn_get_current_message_type(struct s2n_connection *conn)
{
Expand Down Expand Up @@ -323,7 +325,12 @@ int s2n_conn_set_handshake_type(struct s2n_connection *conn)

s2n_cert_auth_type client_cert_auth_type;
GUARD(s2n_connection_get_client_auth_type(conn, &client_cert_auth_type));
if(client_cert_auth_type != S2N_CERT_AUTH_NONE) {

if (conn->mode == S2N_CLIENT && client_cert_auth_type == S2N_CERT_AUTH_REQUIRED) {
/* If we're a client, and Client Auth is REQUIRED, then the Client must expect the CLIENT_CERT_REQ Message */
conn->handshake.handshake_type |= CLIENT_AUTH;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the client is dynamically reacting to the presence of a CertificateRequest now, do we still need to have the client explicitly setting a client auth mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're still honoring the client_cert_auth_type value.

If it's NONE we'll abort the handshake rather than enter the ClientCertRequest state, if OPTIONAL we'll enter that state only if the Server asks and potentially send an empty response if no ClientCert is configured, and if REQUIRED we'll always expect the ClientCertRequest from the server and abort the handshake if no ClientCert is configured.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I think my comment was a little confusing. I meant to ask if we could relax the constraint of REQUIRED altogether. Could the client just dynamically react to the CertificateRequest in all cases (REQUIRED, OPTIONAL, and NONE)? If it has a client cert configured and it receives a CertificateRequest, send the cert. If it does not have a client cert configured and it receives a CertificateRequest, send an empty response. If no CertificateRequest is sent, carry on.

If the client is configured to REQUIRED and the server does not send the CertificateRequest, why fail the handshake? Why not just let the server decide the requirements of client authentication? Maybe I'm missing a use case or complexity here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are clients out there that would expect to fail if the server doesnt support mutual auth. by allowing it to succeed and having the server decide what to do, you are potentially allowing your client to start communicating with an untrusted or unexpected server.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, wouldn't the client still have authenticated the server to ensure it was communicating with a trusted endpoint? The server would have still sent it's own certificate. The client could enforce server auth without having to enforce mutual auth.

Copy link
Contributor Author

@alexw91 alexw91 Jul 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an API design discussion. If a developer configures their s2n client with CERT_AUTH_NONE, what are they trying to say?

I see two plausible options:

  1. "I don't want to connect to any servers that request Client Auth, abort the handshake if a Client Cert is requested"
  2. "I never want to send any Client Certs, just send an empty ClientCert message if necessary, and let the server decide to abort the handshake if necessary"

The 2nd option is better for compatibility reasons, since the client can connect to more Servers (those that always request a ClientCert but don't require it), but the 2nd option has exactly the same behavior as an s2n Client with no ClientCert configured with CERT_AUTH_OPTIONAL (and it seems weird for someone to add a ClientCert but set ClientCertAuth to NONE).

If we adopt the 2nd option, the behavior of the 1st option can also be replicated manually by calling s2n_connection_client_cert_used() after the handshake is completed, and aborting the handshake if necessary, but that requires more work on the developer's part.

I'm okay going either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of optional client certs doesn't map to the client very well. The RFC says:

If no suitable certificate is available, the client MUST send a certificate message containing no certificates.
...
If the client does not send any certificates, the server MAY at its discretion either continue the handshake without client authentication, or respond with a fatal handshake_failure alert.

I interpret that as basically saying the RFC doesn't even really allow for clients deciding to end the handshake based on the existence (or lack of) a certificate request. The rules for either party of a handshake sending an error are a little fuzzy in that you could interpret it to mean that if an implementation decides something is an error then it can bail-out, but I interpret it more that if an error as defined by the RFC is encountered then bail out. Since the client certificate portion doesn't allow the client to determine that there's been an error, it wouldn't be valid for the client to bail-out here.

All that is to say that per the RFC, I think setting any of the REQUIRED/OPTIONAL/NONE in a client context is invalid. If the consumer, at the application level, wants to require the use of a client cert then s2n_connection_client_cert_used() is the mechanism for allowing that (after the handshake is complete).

If you want a feature that allows the client to bail out of connections based on the server's client certificate behavior, I think there's probably a few use-cases for that and it wouldn't be a big deal in practice, but I don't think it would be compliant with the RFC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #819 to continue this discussion.

} else if (conn->mode == S2N_SERVER && client_cert_auth_type != S2N_CERT_AUTH_NONE) {
/* If we're a server, and Client Auth is REQUIRED or OPTIONAL, then the server must send the CLIENT_CERT_REQ Message*/
conn->handshake.handshake_type |= CLIENT_AUTH;
}

Expand Down Expand Up @@ -592,8 +599,8 @@ static int handshake_read_io(struct s2n_connection *conn)
/* Record is a handshake message */
while (s2n_stuffer_data_available(&conn->in)) {
int r;
uint8_t handshake_message_type;
GUARD((r = read_full_handshake_message(conn, &handshake_message_type)));
uint8_t actual_handshake_message_type;
GUARD((r = read_full_handshake_message(conn, &actual_handshake_message_type)));

/* Do we need more data? */
if (r == 1) {
Expand All @@ -606,7 +613,19 @@ static int handshake_read_io(struct s2n_connection *conn)
return 0;
}

S2N_ERROR_IF(handshake_message_type != ACTIVE_STATE(conn).message_type, S2N_ERR_BAD_MESSAGE);
s2n_cert_auth_type client_cert_auth_type;
GUARD(s2n_connection_get_client_auth_type(conn, &client_cert_auth_type));

/* If we're a Client, and received a ClientCertRequest message instead of a ServerHelloDone, and ClientAuth
* is set to optional, then switch the State Machine that we're using to expect the ClientCertRequest. */
if(conn->mode == S2N_CLIENT
&& client_cert_auth_type == S2N_CERT_AUTH_OPTIONAL
&& actual_handshake_message_type == TLS_CLIENT_CERT_REQ
&& EXPECTED_MESSAGE_TYPE(conn) == TLS_SERVER_HELLO_DONE) {
conn->handshake.handshake_type |= CLIENT_AUTH;
}

S2N_ERROR_IF(actual_handshake_message_type != EXPECTED_MESSAGE_TYPE(conn), S2N_ERR_BAD_MESSAGE);

/* Call the relevant handler */
r = ACTIVE_STATE(conn).handler[conn->mode] (conn);
Expand Down