Implement SSL_MODE_AUTO_RETRY (aws#1333)

# Notes

This change implements `SSL_MODE_AUTO_RETRY`, which was previously ignored. When this option (off by default) is set, `SSL_get_error`s behavior hews a little more closely to OpenSSL when processing a return code of 0 (i.e. empty I/O). With this option set, we allow error processing for empty reads if we're in a retryable state. With this option unset, we treat (most) 0-valued return codes as `SSL_ERROR_SYSCALL`, using it as a sort-of alias for EOF in the underlying transport. Internal calls to `SSL_get_error` (such as [those in `bio_ssl.cc`](https://github.com/aws/aws-lc/blob/main/ssl/bio_ssl.cc#L29)) then observe the  `SSL_ERROR_SYSCALL` and fail to process potential retries.

OpenSSL implements this mode differently, only checking its value in its [main read loop](https://github.com/openssl/openssl/blob/398011848468c7e8e481b295f7904afc30934217/ssl/record/rec_layer_s3.c#L992). Neither OpenSSL [1.1.1][6] nor [OpenSSL 3.x][10] return `SSL_ERROR_SYSCALL` early for empty reads, instead allowing them to be processed for retryable state. BoringSSL [appears to have diverged][7] from this behavior in 2015, but that commit was just a revert of a regression committed ~1 month prior. Their [previous implementation][18] closely resembles [that of OpenSSL 1.0.2][13], guarding retryable error processing with `i < 0` checks. `SSL_get_error` in [OpenSSL 1.1.1][12] and [OpenSSL 3.0][11], however, have no such guards. This change resembles the latter two. 

Why is it important that we process 0-valued return codes more fully? The recommended SSL IO functions (`SSL_read`, `SSL_write`, etc.) are, by contract, allowed to return negative numbers to indicate an error or non-success condition. However, their `size_t`-clean counterparts (`SSL_read_ex`, `SSL_write_ex`, etc.) must, by contract, only return 0 or 1. This means that passing the return code from the various `_ex` functions to `SSL_get_error` loses error reporting information, resulting in inconsistent handling of otherwise identical error cases between the two function families.

Another solution to the `_ex` functions' loss of error reporting information would be to change the contract of those functions to allow negative return values, directly returning the return code from internal calls to `SSL_read` or `SSL_write`. I decided not to take this approach as it would break callers who do not account for negative return values in their error handling.

# Relevant Links

- [OpenSSL 1.0.2 documentation for `SSL_get_error`][3]
- [OpenSSL 1.1.1 documentation for `SSL_get_error`][4]
- [OpenSSL 3.0 documentation for `SSL_get_error`][5]
- [OpenSSL 1.0.2 implementation for `SSL_get_error`][13]
- [OpenSSL 1.1.1 implementation for `SSL_get_error`][12]
- [OpenSSL 3.0 implementation for `SSL_get_error`][11]
- [OpenSSL master (3.2) implementation for `SSL_get_error`][10]
- [OpenSSL 1.1.1 commit dropping `ret < 0` guards for hint processing][6]
- [Unresolved OpenSSL issue for distinguishing EOF from failure in `BIO_read_ex`][1]
- [BoringSSL commit introducing `SSL_ERROR_SYSCALL` for `ret == 0` case][7]
- [BoringSSL Issue 503: Stop preserving arbitrary BIO error return values][2]
- [BoringSSL Issue 24: Sanitize SSL and BIO return codes.][15]
- [OpenSSL 3.0 commit returning `SSL_ERROR_SSL`, add reason to err stack on EOF][14]
- [OpenSSL 3.0 commit introducing `SSL_OP_IGNORE_UNEXPECTED_EOF`  option][8]
- [CPython commit from @davidben removing CPython's use of `SSL_OP_IGNORE_UNEXPECTED_EOF`][9]
- [CPython commit switching from `SSL_read` and `SSL_write` to `SSL_read_ex` and `SSL_write_ex`][16]
- [OpenSSL 1.1.1 documentation for `SSL_MODE_AUTO_RETRY`][17]



[1]: openssl/openssl#8208
[2]: https://bugs.chromium.org/p/boringssl/issues/detail?id=503
[3]: https://www.openssl.org/docs/man1.0.2/man3/SSL_get_error.html
[4]: https://www.openssl.org/docs/man1.1.1/man3/SSL_get_error.html
[5]: https://www.openssl.org/docs/man3.0/man3/SSL_get_error.html
[6]: openssl/openssl@8051ab2
[7]: google/boringssl@9a38e92
[8]: openssl/openssl@09b90e0
[9]: python/cpython#95495
[10]: https://github.com/openssl/openssl/blob/master/ssl/ssl_lib.c#L4601
[11]: https://github.com/openssl/openssl/blob/openssl-3.0/ssl/ssl_lib.c#L3839
[12]: https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/ssl/ssl_lib.c#L3617
[13]: https://github.com/openssl/openssl/blob/OpenSSL_1_0_2-stable/ssl/ssl_lib.c#L2709
[14]: openssl/openssl@d924dbf
[15]: https://bugs.chromium.org/p/boringssl/issues/detail?id=24
[16]: python/cpython@89d1550
[17]: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_mode.html
[18]: google/boringssl@fcf2583#diff-38e1eecb6013c2fe8bf194e5020ce1afd6e31c371d7849e041464ad822fe745bL2271

Author: WillChilds-Klein

include/openssl/ssl.h

@@ -834,6 +834,10 @@ OPENSSL_EXPORT uint32_t SSL_get_options(const SSL *ssl);
 // |write|. In DTLS, it does nothing.
 #define SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER 0x00000002L
 
+// SSL_MODE_AUTO_RETRY suppresses terminal errors on empty reads if the
+// underlying connection state is retryable, allowing for automatic retries.
+#define SSL_MODE_AUTO_RETRY 0x00000004L
+
 // SSL_MODE_NO_AUTO_CHAIN disables automatically building a certificate chain
 // before sending certificates to the peer. This flag is set (and the feature
 // disabled) by default.
@@ -5283,7 +5287,6 @@ DEFINE_STACK_OF(SSL_COMP)
 
 // The following flags do nothing and are included only to make it easier to
 // compile code with BoringSSL.
-#define SSL_MODE_AUTO_RETRY 0
 #define SSL_MODE_RELEASE_BUFFERS 0
 #define SSL_MODE_SEND_CLIENTHELLO_TIME 0
 #define SSL_MODE_SEND_SERVERHELLO_TIME 0

ssl/ssl_lib.cc

@@ -1387,9 +1387,16 @@ int SSL_get_error(const SSL *ssl, int ret_code) {
       return SSL_ERROR_ZERO_RETURN;
     }
     // An EOF was observed which violates the protocol, and the underlying
-    // transport does not participate in the error queue. Bubble up to the
-    // caller.
-    return SSL_ERROR_SYSCALL;
+    // transport does not participate in the error queue. If
+    // |SSL_MODE_AUTO_RETRY| is unset, bubble up to the caller.
+    if ((ssl->ctx->mode & SSL_MODE_AUTO_RETRY) == 0) {
+      return SSL_ERROR_SYSCALL;
+    }
+    // If |SSL_MODE_AUTO_RETRY| is set, proceed if in a retryable state.
+    if (ssl->s3->rwstate != SSL_ERROR_WANT_READ
+            && ssl->s3->rwstate != SSL_ERROR_WANT_WRITE) {
+      return SSL_ERROR_SYSCALL;
+    }
   }
 
   switch (ssl->s3->rwstate) {

ssl/ssl_test.cc

@@ -10458,6 +10458,83 @@ TEST(SSLTest, ErrorSyscallAfterCloseNotify) {
   write_failed = false;
 }
 
+static void TestIntermittentEmptyRead(bool auto_retry) {
+  bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
+  bssl::UniquePtr<SSL_CTX> server_ctx =
+      CreateContextWithTestCertificate(TLS_method());
+  ASSERT_TRUE(client_ctx);
+  ASSERT_TRUE(server_ctx);
+  bssl::UniquePtr<SSL> client, server;
+  ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
+                                     server_ctx.get()));
+
+  // Create a fake read BIO that returns 0 on read to simulate empty read
+  bssl::UniquePtr<BIO_METHOD> method(BIO_meth_new(0, nullptr));
+  ASSERT_TRUE(method);
+  ASSERT_TRUE(BIO_meth_set_create(method.get(), [](BIO *b) -> int {
+    BIO_set_init(b, 1);
+    return 1;
+  }));
+  ASSERT_TRUE(BIO_meth_set_read(method.get(), [](BIO *, char *, int) -> int {
+    return 0;
+  }));
+  bssl::UniquePtr<BIO> rbio_empty(BIO_new(method.get()));
+  ASSERT_TRUE(rbio_empty);
+  BIO_set_flags(rbio_empty.get(), BIO_FLAGS_READ);
+
+  // Save off client rbio and use empty read BIO
+  bssl::UniquePtr<BIO> client_rbio(SSL_get_rbio(client.get()));
+  ASSERT_TRUE(client_rbio);
+  // Up-ref |client_rbio| as SSL_CTX dtor will also attempt to free it
+  ASSERT_TRUE(BIO_up_ref(client_rbio.get()));
+  SSL_set0_rbio(client.get(), rbio_empty.release());
+
+  if (auto_retry) {
+      // Set flag under test
+      ASSERT_TRUE(SSL_CTX_set_mode(client_ctx.get(), SSL_MODE_AUTO_RETRY));
+      ASSERT_TRUE(SSL_CTX_get_mode(client_ctx.get()) & SSL_MODE_AUTO_RETRY);
+  } else {
+      // |SSL_MODE_AUTO_RETRY| is off by default
+      ASSERT_FALSE(SSL_CTX_get_mode(client_ctx.get()) & SSL_MODE_AUTO_RETRY);
+  }
+
+  // Server writes some data to the client
+  const uint8_t write_data[] = {1, 2, 3};
+  int ret = SSL_write(server.get(), write_data, (int) sizeof(write_data));
+  EXPECT_EQ(ret, (int) sizeof(write_data));
+  EXPECT_EQ(SSL_get_error(server.get(), ret), SSL_ERROR_NONE);
+
+  uint8_t read_data[] = {0, 0, 0};
+  ret = SSL_read(client.get(), read_data, sizeof(read_data));
+  EXPECT_EQ(ret, 0);
+  if (auto_retry) {
+      // On empty read, client should still want a read so caller will retry
+      EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_WANT_READ);
+  } else {
+      // On empty read, client should error out signaling EOF
+      EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_SYSCALL);
+  }
+
+  // Reset client rbio, read should succeed
+  SSL_set0_rbio(client.get(), client_rbio.release());
+  ret = SSL_read(client.get(), read_data, sizeof(read_data));
+  EXPECT_EQ(ret, (int) sizeof(write_data));
+  EXPECT_EQ(OPENSSL_memcmp(read_data, write_data, sizeof(write_data)), 0);
+  EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_NONE);
+
+  // Subsequent attempts to read should fail
+  ret = SSL_read(client.get(), read_data, sizeof(read_data));
+  EXPECT_LT(ret, 0);
+  EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_WANT_READ);
+}
+
+// Test that |SSL_MODE_AUTO_RETRY| suppresses failure on (potentially)
+// transient empty reads.
+TEST(SSLTest, IntermittentEmptyRead) {
+    TestIntermittentEmptyRead(false);
+    TestIntermittentEmptyRead(true);
+}
+
 // Test that |SSL_shutdown|, when quiet shutdown is enabled, simulates receiving
 // a close_notify, down to |SSL_read| reporting |SSL_ERROR_ZERO_RETURN|.
 TEST(SSLTest, QuietShutdown) {