Add runtime detection for CA Certs on Linux #1393
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Shelnutt2
force-pushed
the
ss/add-runtime-curl-ca-cert-handling
branch
from
ecabd36 to
0f925f9
Compare
tdenniston
suggested changes
Oct 2, 2019
tiledb/sm/misc/utils.cc
Outdated
| namespace https { | ||
| std::string find_ca_certs_linux(const tiledb::sm::VFS& vfs) { | ||
| // Check ever cert file location to see if the certificate exists | ||
| for (std::string cert : constants::cert_files_linux) { |
There was a problem hiding this comment.
Do for (const auto& cert : ...) instead
ihnorton
reviewed
Oct 2, 2019
Shelnutt2
force-pushed
the
ss/add-runtime-curl-ca-cert-handling
branch
from
0f925f9 to
b278909
Compare
Shelnutt2
force-pushed
the
ss/add-runtime-curl-ca-cert-handling
branch
2 times, most recently
from
7ab3be5 to
c60d268
Compare
ihnorton
reviewed
Oct 3, 2019
| // for each s3/rest call as appropriate | ||
| VFS vfs; | ||
| vfs.init(config_.vfs_params()); | ||
| cert_file_ = utils::https::find_ca_certs_linux(vfs); |
There was a problem hiding this comment.
don't let it slow the PR down, but this seems like something to log if/when that is feasible.
There was a problem hiding this comment.
@tdenniston do we have a way to log debug messages? I checked and only saw LOG_ERROR and LOG_STATUS.
There was a problem hiding this comment.
No we don't -- in general our error reporting and logging needs an overhaul.
ihnorton
approved these changes
Oct 3, 2019
Shelnutt2
force-pushed
the
ss/add-runtime-curl-ca-cert-handling
branch
3 times, most recently
from
907a781 to
a5dcecc
Compare
ihnorton
force-pushed
the
ss/add-runtime-curl-ca-cert-handling
branch
from
bd78af9 to
6492456
Compare
This solves a major issue when we build libcurl as part of a super build. Curl at configuration/compile time detects the systems CA bundle path, and then hard codes this into the compiled lib. This causes issues when libtiledb has a statically linked libcurl and is copied between linux systems (i.e pip or maven). Often then the hard coded CA bundle path is no longer valid and curl fails to handle SSL. The solution is to at runtime try to detect the location of the CA cert bundle for the host os. Currently the CA cert issues only exists on linux, so we only do runtime detection for linux. [ch814]
In the S3 case, vfs->init calls GetGlobalState, so it must be initialized first.
- After the previous patch to change initialization order, if GlobalState::init fails then the StorageManager may be left in a partially initialized state. If the StorageManager destructor is then called to clean up, we must avoid calling VFS operations. Bug description: In the TBB tests, we try initializing TBB with thread_count = -3. This properly fails, in the GlobalState::init call, leading to destruction of the Ctx object, here: - https://github.com/TileDB-Inc/TileDB/blob/1ff92bda90921df0ab6e1a44dec88dd5d26bf998/tiledb/sm/c_api/tiledb.cc#L1113-L1114 https://github.com/TileDB-Inc/TileDB/blob/dev/tiledb/sm/c_api/tiledb.cc#L1114 which then calls the StorageManager destructor, which makes several calls on the `vfs_` object: - https://github.com/TileDB-Inc/TileDB/blob/b175d59bcea9ca243e060445bbd14d308681a155/tiledb/sm/storage_manager/storage_manager.cc#L77-L98 These calls fail, because with the patched initialization order in the previous commit, VFS has not been assigned or initialized and is nullptr.
ihnorton
force-pushed
the
ss/add-runtime-curl-ca-cert-handling
branch
from
6492456 to
bf9aa0c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This solves a major issue when we build libcurl as part of a super build on Linux. Curl at configuration/compile time detects the systems CA bundle path, and then hard codes this into the compiled lib. This causes issues when libtiledb has a statically linked libcurl and is copied between
linux systems (i.e pip or maven). Often then the hard coded CA bundle path is no longer valid and curl fails to handle SSL.
The solution is to at runtime try to detect the location of the CA cert bundle for the host os.
Currently the CA cert issues only exists on linux, so we only do runtime detection for linux.
I have left adding an config override for the REST server to another PR as #1392 is reworking on the config is handled internally.
[ch814]