PS-9823 fix: mysql_migrate_keyring won't work with PS's components (8.4)#5635
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,27 @@ | |||
| # ==== Purpose ==== | |||
There was a problem hiding this comment.
❌ clang-diagnostic-error ❌
invalid preprocessing directive
| @@ -0,0 +1,27 @@ | |||
| # ==== Purpose ==== | |||
| # | |||
| # Check if the provided library ('.so') can be successfully loaded with 'dlopen(..., RTLD_NOW)' | |||
There was a problem hiding this comment.
❌ clang-diagnostic-error ❌
invalid preprocessing directive
| # | ||
| # Check if the provided library ('.so') can be successfully loaded with 'dlopen(..., RTLD_NOW)' | ||
| # | ||
| # ==== Usage ==== |
There was a problem hiding this comment.
❌ clang-diagnostic-error ❌
invalid preprocessing directive
| # | ||
| # ==== Usage ==== | ||
| # | ||
| # --let $DLOPEN_CHECKER_LIBRARY_PATH = <path_to_the_library> |
There was a problem hiding this comment.
❌ clang-diagnostic-error ❌
invalid preprocessing directive
| # ==== Usage ==== | ||
| # | ||
| # --let $DLOPEN_CHECKER_LIBRARY_PATH = <path_to_the_library> | ||
| # --source include/keyring_tests/mats/dynamic_loading.inc |
There was a problem hiding this comment.
❌ clang-diagnostic-error ❌
invalid preprocessing directive
| # --let $DLOPEN_CHECKER_LIBRARY_PATH = <path_to_the_library> | ||
| # --source include/keyring_tests/mats/dynamic_loading.inc | ||
| # | ||
| # ==== Parameters ==== |
There was a problem hiding this comment.
❌ clang-diagnostic-error ❌
invalid preprocessing directive
| # | ||
| # ==== Parameters ==== | ||
| # | ||
| # DLOPEN_CHECKER_LIBRARY_PATH |
There was a problem hiding this comment.
❌ clang-diagnostic-error ❌
invalid preprocessing directive
| # ==== Parameters ==== | ||
| # | ||
| # DLOPEN_CHECKER_LIBRARY_PATH | ||
| # Full path to the library that needs to be checked for unresolved symbols ('.so') |
There was a problem hiding this comment.
❌ clang-diagnostic-error ❌
invalid preprocessing directive
| # Full path to the library that needs to be checked for unresolved symbols ('.so') | ||
| # | ||
|
|
||
| --let $dlopen_checker_source = $MYSQL_TEST_DIR/std_data/dlopen_checker.cpp |
There was a problem hiding this comment.
❌ clang-diagnostic-error ❌
expected unqualified-id
| free(mh); | ||
| } | ||
|
|
||
| extern "C" void my_memset_s(void *dest, size_t dest_max, int c, size_t n) { |
There was a problem hiding this comment.
parameter name c is too short, expected at least 2 characters
| #if defined(WIN32) | ||
| SecureZeroMemory(dest, n); | ||
| #else | ||
| volatile unsigned char *p = static_cast<unsigned char *>(dest); |
There was a problem hiding this comment.
variable name p is too short, expected at least 2 characters
| SecureZeroMemory(dest, n); | ||
| #else | ||
| volatile unsigned char *p = static_cast<unsigned char *>(dest); | ||
| while (dest_max-- && n--) { |
There was a problem hiding this comment.
implicit conversion size_t (aka unsigned long) -> bool
| while (dest_max-- && n--) { | |
| while (((dest_max--) != 0u) && n--) { |
| SecureZeroMemory(dest, n); | ||
| #else | ||
| volatile unsigned char *p = static_cast<unsigned char *>(dest); | ||
| while (dest_max-- && n--) { |
There was a problem hiding this comment.
implicit conversion size_t (aka unsigned long) -> bool
| while (dest_max-- && n--) { | |
| while (dest_max-- && ((n--) != 0u)) { |
| */ | ||
| extern "C" void my_free(void *ptr); | ||
|
|
||
| extern "C" void my_memset_s(void *dest, size_t dest_max, int c, size_t n); |
There was a problem hiding this comment.
parameter name c is too short, expected at least 2 characters
| } | ||
|
|
||
| const char *lib_path = argv[1]; | ||
| using handle_guard = std::unique_ptr<void, decltype([](void *h) { |
There was a problem hiding this comment.
parameter name h is too short, expected at least 2 characters
|
|
||
| class IKeyring_vault_curl : public Comp_keyring_alloc, | ||
| private boost::noncopyable { | ||
| class IKeyring_vault_curl : private boost::noncopyable { |
There was a problem hiding this comment.
class IKeyring_vault_curl defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator
percona-ysorokin
force-pushed
the
dev/PS-9823-8.4-mysql_migrate_keyring_unusable
branch
3 times, most recently
from
78706b4 to
3adf10b
Compare
percona-ysorokin
changed the title
percona-ysorokin
force-pushed
the
dev/PS-9823-8.4-mysql_migrate_keyring_unusable
branch
from
3adf10b to
27b4ffa
Compare
dlenev
left a comment
dlenev
left a comment
There was a problem hiding this comment.
Hello Yura!
The updated version of file looks almost OK to me.
I have only a couple of comments for your consideration.
| } | ||
|
|
||
| } catch (const std::exception &e) { | ||
| LogComponentErr(ERROR_LEVEL, ER_STD_UNKNOWN_EXCEPTION, e.what(), __func__); |
There was a problem hiding this comment.
AFAIU ER_STD_UNKNOWN_EXCEPTION and ER_UNKNOWN_ERROR are errors which are supposed to be sent to clients and not to the error log. I think it is better to replace them with one of errors from share/messages_to_error_log.txt (perhaps use ER_LOG_PRINTF_MSG in other places in this file).
What do you think?
| m_valid = true; | ||
|
|
||
| return false; | ||
| } catch (const std::exception &e) { |
There was a problem hiding this comment.
Same comment as above here.
percona-ysorokin
force-pushed
the
dev/PS-9823-8.4-mysql_migrate_keyring_unusable
branch
2 times, most recently
from
60511bf to
9007094
Compare
https://perconadev.atlassian.net/browse/PS-9823 Reworked keyring components to make sure their corresponding '.so' objects do not have unresolved symbols (from the 'dlopen(..., RTLD_NOW)' point of view). This change is needed to ensure that keyring components can be loaded not only from the 'mysqld' executable but from utilities like 'mysql_migrate_keyring' as well. Keyring components' 'CMakeLists.txt' files fortified with aditional linking option '${LINK_FLAG_NO_UNDEFINED}' (-Wl,--no-undefined) which prevents building '.so' shared objects with unresolved sumbols. Removed custom allocator from the 'components/keyrings/common/data/pfs_string.h' header to eliminate divergence from upstream code. 'pfs_string' kept as an alias to 'std::string' to minimize Percona code changes. Removed 'DBUG_TRACE' calls from the 'component_keyring_kmip' code to get rid of 'mysys' library dependency. Calls to 'mysql_components_handle_std_exception()' inside 'component_keyring_kmip' replaced with 'LogComponentErr()' to avoid dependency on 'minchassis'. 'memset_s()' Percona's extension function renamed to 'my_memset_s()'. Added a series of 'component_keyring_xxx.dynamic_loading' MTR test cases (one for each keyring component: 'file', 'kmip', 'kms') that checks if the component's '.so' file does not have unresolved symbols in order to make sure that it can be loaded from auxiliary utilities (like 'mysql_migrate_keyring'). These MTR test cases internally build a helper utility from the '.cpp' file ('mysql-test/std_data/dlopen_checker.cpp') that simply performs an attempt to call 'dlopen(..., RTLD_NOW)' for the provided '.so' object. Added 'keyring_vault.migrate_keyring' MTR test case that tests for keyring data migration from 'keyring_vault' plugin to 'keyring_file' plugin and back. Internally, it uses 'mysqld' executable in keyring data migration mode to perform key stansfer from one plugin to another. Added 'have_keyring_file_plugin.inc' MTR include file that helps to identify if the server was build with 'keyring_file' plugin. Removed 'have_keyring_kmip_plugin.inc' MTR include file added previously by mistake.
…v/PS-9823-8.4-mysql_migrate_keyring_unusable https://perconadev.atlassian.net/browse/PS-9823 Reworked keyring components to make sure their corresponding '.so' objects do not have unresolved symbols (from the 'dlopen(..., RTLD_NOW)' point of view). This change is needed to ensure that keyring components can be loaded not only from the 'mysqld' executable but from utilities like 'mysql_migrate_keyring' as well. Keyring components' 'CMakeLists.txt' files fortified with aditional linking option '${LINK_FLAG_NO_UNDEFINED}' (-Wl,--no-undefined) which prevents building '.so' shared objects with unresolved sumbols. Removed custom allocator from the 'components/keyrings/common/data/pfs_string.h' header to eliminate divergence from upstream code. 'pfs_string' kept as an alias to 'std::string' to minimize Percona code changes. Removed 'DBUG_TRACE' calls from the 'component_keyring_kmip' code to get rid of 'mysys' library dependency. Calls to 'mysql_components_handle_std_exception()' inside both 'component_keyring_kmip' and 'component_keyring_vault' replaced with 'LogComponentErr()' to avoid dependency on 'minchassis'. Added explicit dependency on 'OpenSSL::Crypto' for the component_keyring_vault' (needed for AES functions). 'memset_s()' Percona's extension function moved from 'mysys' to 'library_mysys' and renamed to 'my_memset_s()'. Removed unused 'components/keyrings/common/data/keyring_alloc.h'. Removed unused 'plugin/keyring/common/secure_string.h'. Removed unused 'Secure_allocator' class template from the 'plugin/keyring/common/keyring_memory.h'. Added a series of 'component_keyring_xxx.dynamic_loading' MTR test cases (one for each keyring component: 'file', 'vault', 'kmip', 'kms') that checks if the component's '.so' file does not have unresolved symbols in order to make sure that it can be loaded from auxiliary utilities (like 'mysql_migrate_keyring'). These MTR test cases internally build a helper utility from the '.cpp' file ('mysql-test/std_data/dlopen_checker.cpp') that simply performs an attempt to call 'dlopen(..., RTLD_NOW)' for the provided '.so' object. Added 'component_keyring_vault.migrate_keyring' MTR test case that tests for keyring data migration from 'component_keyring_vault' to 'component_keyring_file' and back.
percona-ysorokin
force-pushed
the
dev/PS-9823-8.4-mysql_migrate_keyring_unusable
branch
from
9007094 to
5d83f73
Compare
2 participants
https://perconadev.atlassian.net/browse/PS-9823
Reworked keyring components to make sure their corresponding '.so' objects do
not have unresolved symbols (from the 'dlopen(..., RTLD_NOW)' point of view).
This change is needed to ensure that keyring components can be loaded not only
from the 'mysqld' executable but from utilities like 'mysql_migrate_keyring' as
well.
Keyring components' 'CMakeLists.txt' files fortified with aditional linking option
'${LINK_FLAG_NO_UNDEFINED}' (-Wl,--no-undefined) which prevents building
'.so' shared objects with unresolved sumbols.
Removed custom allocator from the 'components/keyrings/common/data/pfs_string.h' header
to eliminate divergence from upstream code. 'pfs_string' kept as an alias to 'std::string' to
minimize Percona code changes.
Removed 'DBUG_TRACE' calls from the 'component_keyring_kmip' code to get
rid of 'mysys' library dependency.
Calls to 'mysql_components_handle_std_exception()' inside both
'component_keyring_kmip' and 'component_keyring_vault' replaced with
'LogComponentErr()' to avoid dependency on 'minchassis'.
Added explicit dependency on 'OpenSSL::Crypto' for the
component_keyring_vault' (needed for AES functions).
'memset_s()' Percona's extension function moved from 'mysys' to 'library_mysys'
and renamed to 'my_memset_s()'.
Removed unused 'components/keyrings/common/data/keyring_alloc.h'.
Removed unused 'plugin/keyring/common/secure_string.h'.
Removed unused 'Secure_allocator' class template from the
'plugin/keyring/common/keyring_memory.h'.
Added a series of 'component_keyring_xxx.dynamic_loading' MTR test cases (one
for each keyring component: 'file', 'vault', 'kmip', 'kms') that checks if the
component's '.so' file does not have unresolved symbols in order to make sure
that it can be loaded from auxiliary utilities (like 'mysql_migrate_keyring'). These
MTR test cases internally build a helper utility from the '.cpp' file
('mysql-test/std_data/dlopen_checker.cpp') that simply performs an attempt to
call 'dlopen(..., RTLD_NOW)' for the provided '.so' object.
Added 'component_keyring_vault.migrate_keyring' MTR test case that tests for
keyring data migration from 'component_keyring_vault' to
'component_keyring_file' and back.