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

CDRIVER-3668 support OCSP back to OpenSSL 1.0.1 #623

Merged
merged 3 commits into from
Jun 5, 2020
Merged

CDRIVER-3668 support OCSP back to OpenSSL 1.0.1 #623

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8de890
CDRIVER-3668 support OCSP back to OpenSSL 1.0.1
kevinAlbs May 29, 2020
ed27348
regenerate tests
kevinAlbs May 29, 2020
54df966
fix and test DER parsing
kevinAlbs May 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1,832 changes: 1,731 additions & 101 deletions .evergreen/config.yml
View file
Open in desktop

Large diffs are not rendered by default.

79 changes: 40 additions & 39 deletions build/evergreen_config_lib/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,15 @@
class CompileTask(NamedTask):
def __init__(self, task_name, tags=None, config='debug',
compression='default', continue_on_err=False,
extra_commands=None, depends_on=None,
extra_script=None, **kwargs):
suffix_commands=None, depends_on=None,
extra_script=None, prefix_commands=None, **kwargs):
super(CompileTask, self).__init__(task_name=task_name,
depends_on=depends_on,
tags=tags,
**kwargs)

self.extra_commands = extra_commands or []
self.suffix_commands = suffix_commands or []
self.prefix_commands = prefix_commands or []
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added "prefix_commands" here, so I could create a debug-compile task that used OpenSSL 1.0.1 specifically. That needs to call the "install ssl" evergreen function first.

if extra_script:
self.extra_script = "\n" + extra_script
else:
Expand All @@ -65,6 +66,8 @@ def __init__(self, task_name, tags=None, config='debug',
def to_dict(self):
task = super(CompileTask, self).to_dict()

task['commands'].extend(self.prefix_commands)

script = ''
for opt, value in sorted(self.compile_sh_opt.items()):
script += 'export %s="%s"\n' % (opt, value)
Expand All @@ -73,7 +76,7 @@ def to_dict(self):
self.extra_script
task['commands'].append(shell_mongoc(script))
task['commands'].append(func('upload build'))
task['commands'].extend(self.extra_commands)
task['commands'].extend(self.suffix_commands)
return task


Expand Down Expand Up @@ -135,7 +138,7 @@ def __init__(self, *args, **kwargs):


class LinkTask(NamedTask):
def __init__(self, task_name, extra_commands, orchestration=True, **kwargs):
def __init__(self, task_name, suffix_commands, orchestration=True, **kwargs):
if orchestration == 'ssl':
bootstrap_commands = [bootstrap(SSL=1)]
elif orchestration:
Expand All @@ -147,7 +150,7 @@ def __init__(self, task_name, extra_commands, orchestration=True, **kwargs):
task_name=task_name,
depends_on=OD([('name', 'make-release-archive'),
('variant', 'releng')]),
commands=bootstrap_commands + extra_commands,
commands=bootstrap_commands + suffix_commands,
**kwargs)


Expand Down Expand Up @@ -201,7 +204,7 @@ def __init__(self, task_name, extra_commands, orchestration=True, **kwargs):
SpecialTask('debug-compile-coverage',
tags=['debug-compile', 'coverage'],
COVERAGE='ON',
extra_commands=[func('upload coverage')]),
suffix_commands=[func('upload coverage')]),
CompileTask('debug-compile-no-counters',
tags=['debug-compile', 'no-counters'],
ENABLE_SHM_COUNTERS='OFF'),
Expand Down Expand Up @@ -244,7 +247,7 @@ def __init__(self, task_name, extra_commands, orchestration=True, **kwargs):
continue_on_err=True,
ANALYZE='ON',
CC='clang',
extra_commands=[
suffix_commands=[
func('upload scan artifacts'),
shell_mongoc('''
if find scan -name \*.html | grep -q html; then
Expand Down Expand Up @@ -300,70 +303,70 @@ def __init__(self, task_name, extra_commands, orchestration=True, **kwargs):
tags=['debug-compile'],
SRV='OFF'),
LinkTask('link-with-cmake',
extra_commands=[
suffix_commands=[
func('link sample program', BUILD_SAMPLE_WITH_CMAKE=1)]),
LinkTask('link-with-cmake-ssl',
extra_commands=[
suffix_commands=[
func('link sample program',
BUILD_SAMPLE_WITH_CMAKE=1,
ENABLE_SSL=1)]),
LinkTask('link-with-cmake-snappy',
extra_commands=[
suffix_commands=[
func('link sample program',
BUILD_SAMPLE_WITH_CMAKE=1,
ENABLE_SNAPPY=1)]),
LinkTask('link-with-cmake-mac',
extra_commands=[
suffix_commands=[
func('link sample program', BUILD_SAMPLE_WITH_CMAKE=1)]),
LinkTask('link-with-cmake-deprecated',
extra_commands=[
suffix_commands=[
func('link sample program',
BUILD_SAMPLE_WITH_CMAKE=1,
BUILD_SAMPLE_WITH_CMAKE_DEPRECATED=1)]),
LinkTask('link-with-cmake-ssl-deprecated',
extra_commands=[
suffix_commands=[
func('link sample program',
BUILD_SAMPLE_WITH_CMAKE=1,
BUILD_SAMPLE_WITH_CMAKE_DEPRECATED=1,
ENABLE_SSL=1)]),
LinkTask('link-with-cmake-snappy-deprecated',
extra_commands=[
suffix_commands=[
func('link sample program',
BUILD_SAMPLE_WITH_CMAKE=1,
BUILD_SAMPLE_WITH_CMAKE_DEPRECATED=1,
ENABLE_SNAPPY=1)]),
LinkTask('link-with-cmake-mac-deprecated',
extra_commands=[
suffix_commands=[
func('link sample program',
BUILD_SAMPLE_WITH_CMAKE=1,
BUILD_SAMPLE_WITH_CMAKE_DEPRECATED=1)]),
LinkTask('link-with-cmake-windows',
extra_commands=[func('link sample program MSVC')]),
suffix_commands=[func('link sample program MSVC')]),
LinkTask('link-with-cmake-windows-ssl',
extra_commands=[func('link sample program MSVC', ENABLE_SSL=1)],
suffix_commands=[func('link sample program MSVC', ENABLE_SSL=1)],
orchestration='ssl'),
LinkTask('link-with-cmake-windows-snappy',
extra_commands=[
suffix_commands=[
func('link sample program MSVC', ENABLE_SNAPPY=1)]),
LinkTask('link-with-cmake-mingw',
extra_commands=[func('link sample program mingw')]),
suffix_commands=[func('link sample program mingw')]),
LinkTask('link-with-pkg-config',
extra_commands=[func('link sample program')]),
suffix_commands=[func('link sample program')]),
LinkTask('link-with-pkg-config-mac',
extra_commands=[func('link sample program')]),
suffix_commands=[func('link sample program')]),
LinkTask('link-with-pkg-config-ssl',
extra_commands=[func('link sample program', ENABLE_SSL=1)]),
suffix_commands=[func('link sample program', ENABLE_SSL=1)]),
LinkTask('link-with-bson',
extra_commands=[func('link sample program bson')],
suffix_commands=[func('link sample program bson')],
orchestration=False),
LinkTask('link-with-bson-mac',
extra_commands=[func('link sample program bson')],
suffix_commands=[func('link sample program bson')],
orchestration=False),
LinkTask('link-with-bson-windows',
extra_commands=[func('link sample program MSVC bson')],
suffix_commands=[func('link sample program MSVC bson')],
orchestration=False),
LinkTask('link-with-bson-mingw',
extra_commands=[func('link sample program mingw bson')],
suffix_commands=[func('link sample program mingw bson')],
orchestration=False),
NamedTask('debian-package-build',
commands=[
Expand Down Expand Up @@ -408,7 +411,10 @@ def __init__(self, task_name, extra_commands, orchestration=True, **kwargs):
CompileWithClientSideEncryption('debug-compile-sasl-winssl-cse', tags=[
'debug-compile', 'sasl', 'winssl'], SASL="AUTO", SSL="WINDOWS"),
CompileWithClientSideEncryptionAsan('debug-compile-asan-openssl-cse', tags=[
'debug-compile', 'asan-clang'], SSL="OPENSSL")
'debug-compile', 'asan-clang'], SSL="OPENSSL"),
CompileTask ('debug-compile-nosasl-openssl-1.0.1',
prefix_commands=[func("install ssl", SSL="openssl-1.0.1u")],
CFLAGS="-Wno-redundant-decls", SSL="OPENSSL", SASL="OFF")
]


Expand Down Expand Up @@ -667,11 +673,11 @@ def _compressor_list(self):

class SpecialIntegrationTask(NamedTask):
def __init__(self, task_name, depends_on='debug-compile-sasl-openssl',
extra_commands=None, uri=None,
suffix_commands=None, uri=None,
tags=None, version='latest', topology='server'):
commands = [func('fetch build', BUILD_NAME=depends_on),
bootstrap(VERSION=version, TOPOLOGY=topology),
run_tests(uri)] + (extra_commands or [])
run_tests(uri)] + (suffix_commands or [])
super(SpecialIntegrationTask, self).__init__(task_name,
commands=commands,
depends_on=depends_on,
Expand Down Expand Up @@ -824,7 +830,7 @@ def name(self):
SSLTask('openssl-1.0.2', 'l'),
SSLTask('openssl-1.1.0', 'f'),
SSLTask('libressl-2.5', '.2', require_tls12=True),
SSLTask('libressl-3.0', '.2', require_tls12=True, enable_ssl="AUTO"),
SSLTask('libressl-3.0', '.2', require_tls12=True, enable_ssl="AUTO", cflags="-Wno-redundant-decls"),
SSLTask('libressl-3.0', '.2', require_tls12=True),
])

Expand Down Expand Up @@ -900,7 +906,7 @@ class OCSPTask(MatrixTask):
axes = OD([('test', ['test_1', 'test_2', 'test_3', 'test_4', 'soft_fail_test', 'malicious_server_test_1', 'malicious_server_test_2', 'cache']),
('delegate', ['delegate', 'nodelegate']),
('cert', ['rsa', 'ecdsa']),
('ssl', ['openssl', 'darwinssl', 'winssl'])])
('ssl', ['openssl', 'openssl-1.0.1', 'darwinssl', 'winssl'])])

name_prefix = 'test-ocsp'

Expand Down Expand Up @@ -942,11 +948,9 @@ def to_dict(self):

# Testing in OCSP has a lot of exceptions.
def _check_allowed(self):
# Current latest macOS does not support the disableStapling failpoint.
# There are no tests that can run on macOS in current evergreen configuration.
if self.ssl == 'darwinssl':
# TODO: remove this when macOS latest download is updated
prohibit (True)
# Secure Transport quietly ignores a must-staple certificate with no stapled response.
prohibit (self.test == 'malicious_server_test_2')

# ECDSA certs can't be loaded (in the PEM format they're stored) on Windows/macOS. Skip them.
if self.ssl == 'darwinssl' or self.ssl == 'winssl':
Expand All @@ -959,9 +963,6 @@ def _check_allowed(self):
if self.test == 'soft_fail_test' or self.test == 'malicious_server_test_2' or self.test == 'cache':
prohibit(self.delegate == 'delegate')

# Until OCSP is supported in OpenSSL, skip tests that expect to be revoked.
if self.ssl == 'openssl':
prohibit (self.test in ['test_2', 'test_4', 'malicious_server_test_1', 'malicious_server_test_2'])


all_tasks = chain(all_tasks, OCSPTask.matrix())
Expand Down
8 changes: 5 additions & 3 deletions build/evergreen_config_lib/variants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,11 +573,13 @@ def days(n):
batchtime=days(1)),
Variant ('ocsp', 'OCSP tests', 'ubuntu1804-test', [
OD([('name', 'debug-compile-nosasl-openssl'), ('distros', ['ubuntu1804-test'])]),
#OD([('name', 'debug-compile-nosasl-darwinssl'), ('distros', ['macos-1014'])]),
OD([('name', 'debug-compile-nosasl-darwinssl'), ('distros', ['macos-1014'])]),
OD([('name', 'debug-compile-nosasl-winssl'), ('distros', ['windows-64-vs2017-test'])]),
OD([('name', '.ocsp-openssl'), ('distros', ['ubuntu1804-test'])]),
#OD([('name', '.ocsp-darwinssl'), ('distros', ['macos-1014'])]),
OD([('name', '.ocsp-winssl'), ('distros', ['windows-64-vs2017-test'])])
OD([('name', '.ocsp-darwinssl'), ('distros', ['macos-1014'])]),
OD([('name', '.ocsp-winssl'), ('distros', ['windows-64-vs2017-test'])]),
OD([('name', 'debug-compile-nosasl-openssl-1.0.1'), ('distros', ['ubuntu1804-test'])]),
OD([('name', '.ocsp-openssl-1.0.1'), ('distros', ['ubuntu1804-test'])])
], {}, batchtime=days(14)),
Variant ('packaging', 'Linux Distro Packaging', 'ubuntu1604-test', [
'debian-package-build',
Expand Down
8 changes: 2 additions & 6 deletions src/libmongoc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,10 +628,8 @@ if (NOT ENABLE_SSL STREQUAL OFF)
${PROJECT_SOURCE_DIR}/src/mongoc/mongoc-stream-tls-openssl.c
${PROJECT_SOURCE_DIR}/src/mongoc/mongoc-stream-tls-openssl-bio.c
${PROJECT_SOURCE_DIR}/src/mongoc/mongoc-openssl.c
${PROJECT_SOURCE_DIR}/src/mongoc/mongoc-ocsp-cache.c
)
if (OPENSSL_VERSION GREATER 1.1.1 OR OPENSSL_VERSION EQUAL 1.1.1)
set (SOURCES ${SOURCES} ${PROJECT_SOURCE_DIR}/src/mongoc/mongoc-ocsp-cache.c)
endif()
set (SSL_LIBRARIES ${OPENSSL_LIBRARIES})
include_directories (${OPENSSL_INCLUDE_DIR})
if (WIN32)
Expand Down Expand Up @@ -948,10 +946,8 @@ if (MONGOC_ENABLE_SSL)
${PROJECT_SOURCE_DIR}/tests/test-mongoc-stream-tls-error.c
${PROJECT_SOURCE_DIR}/tests/test-mongoc-stream-tls.c
${PROJECT_SOURCE_DIR}/tests/test-mongoc-x509.c
${PROJECT_SOURCE_DIR}/tests/test-mongoc-ocsp-cache.c
)
if (OPENSSL_VERSION GREATER 1.1.1 OR OPENSSL_VERSION EQUAL 1.1.1)
set (test-libmongoc-sources ${test-libmongoc-sources} ${PROJECT_SOURCE_DIR}/tests/test-mongoc-ocsp-cache.c)
endif()
endif ()

if (MONGOC_ENABLE_SASL_CYRUS)
Expand Down
5 changes: 3 additions & 2 deletions src/libmongoc/doc/configuring_tls.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,10 @@ Ensure your system's OpenSSL is a recent version (at least 1.0.1), or install a

When compiled against OpenSSL, the driver will attempt to load the system default certificate store, as configured by the distribution. That can be overridden by setting the ``tlsCAFile`` URI option or with the fields ``ca_file`` and ``ca_dir`` in the :symbol:`mongoc_ssl_opt_t`.

Setting ``tlsDisableOCSPEndpointCheck`` and ``tlsDisableCertificateRevocationCheck`` has no effect.
Setting ``tlsDisableCertificateRevocationCheck`` disables OCSP revocation checking.
Setting ``tlsDisableOCSPEndpointCheck`` disables OCSP responders from being contacted when OCSP revocation checking is enabled, and a server presents a certificate without stapled OCSP response.

The Online Certificate Status Protocol (OCSP) is partially supported (see `RFC 6960 <https://tools.ietf.org/html/rfc6960>`_). Support requires OpenSSL 1.1.0 and has the following behavior:
The Online Certificate Status Protocol (OCSP) is partially supported (see `RFC 6960 <https://tools.ietf.org/html/rfc6960>`_). Support requires OpenSSL 1.0.1 and has the following behavior:

- Stapled OCSP responses are validated on certificates presented by the server.
- Server certificates with a Must-Staple extension (see `RFC 7633 <https://tools.ietf.org/html/rfc7633>`_) are required to have stapled responses.
Expand Down
23 changes: 21 additions & 2 deletions src/libmongoc/src/mongoc/mongoc-ocsp-cache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,26 @@ update_entry (cache_entry_list_t *entry,
entry->cert_status = cert_status;
entry->reason = reason;
}

#if OPENSSL_VERSION_NUMBER >= 0x10101000L
static int
_cmp_time (ASN1_TIME *a, ASN1_TIME *b)
{
return ASN1_TIME_compare (a, b);
}
#else
static int
_cmp_time (ASN1_TIME *a, ASN1_TIME *b)
{
/* For older OpenSSL, always report that "a" is before "b". I.e. do not
* replace the entry.
* If a driver would accept a stapled OCSP response and that response has a
* later nextUpdate than the response already in the cache, drivers SHOULD
* replace the older entry in the cache with the fresher response. */
return -1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll admit, this is not ideal. After searching a bit, and checking with Shreyas to see how the server polyfilled the time comparison, it seems like we'd need to parse the string representation of the time to something we can easily compare:
https://github.com/mongodb/mongo/blob/f627ff829ba96487fccb6945d0c2807b1bb51d52/src/mongo/util/net/ssl_manager_openssl.cpp#L375-L412

It doesn't look like it's required by the spec to check nextUpdate when updating the cache. By always comparing "a" to be before "b", we won't overwrite cache entries. They'll still get removed when they're detected as being expired by OCSP_check_validity.

If this seems ok with you, I'll create a ticket to do this as post-4.4 follow-up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created https://jira.mongodb.org/browse/CDRIVER-3707

}
#endif

void
_mongoc_ocsp_cache_set_resp (OCSP_CERTID *id,
int cert_status,
Expand All @@ -96,8 +116,7 @@ _mongoc_ocsp_cache_set_resp (OCSP_CERTID *id,
entry->id = OCSP_CERTID_dup (id);
LL_APPEND (cache, entry);
update_entry (entry, cert_status, reason, this_update, next_update);
} else if (next_update &&
ASN1_TIME_compare (next_update, entry->next_update) == 1) {
} else if (next_update && _cmp_time (next_update, entry->next_update) == 1) {
update_entry (entry, cert_status, reason, this_update, next_update);
} else {
/* Do nothing; our next_update is at a later date */
Expand Down
11 changes: 6 additions & 5 deletions src/libmongoc/src/mongoc/mongoc-openssl-private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@
#include <openssl/err.h>

#include "mongoc-ssl.h"
#include "mongoc-stream-tls-openssl-private.h"

#if (OPENSSL_VERSION_NUMBER >= 0x10101000L) && !defined(OPENSSL_NO_OCSP) && \
#if (OPENSSL_VERSION_NUMBER >= 0x10001000L) && !defined(OPENSSL_NO_OCSP) && \
!defined(LIBRESSL_VERSION_NUMBER)
#define MONGOC_ENABLE_OCSP_OPENSSL
#endif
Expand All @@ -35,9 +36,9 @@
BSON_BEGIN_DECLS

bool
_mongoc_openssl_check_cert (SSL *ssl,
const char *host,
bool allow_invalid_hostname);
_mongoc_openssl_check_peer_hostname (SSL *ssl,
const char *host,
bool allow_invalid_hostname);
SSL_CTX *
_mongoc_openssl_ctx_new (mongoc_ssl_opt_t *opt);
char *
Expand All @@ -49,7 +50,7 @@ _mongoc_openssl_cleanup (void);

#ifdef MONGOC_ENABLE_OCSP_OPENSSL
int
_mongoc_ocsp_tlsext_status_cb (SSL *ssl, void *arg);
_mongoc_ocsp_tlsext_status (SSL *ssl, mongoc_openssl_ocsp_opt_t *opts);
#endif


Expand Down