MDEV-35254 Make iterations count configurable in PARSEC plugin#4504
MDEV-35254 Make iterations count configurable in PARSEC plugin#4504deo002 wants to merge 5 commits into
Conversation
910d32e to
c425564
Compare
|
Related PR: mariadb-corporation/mariadb-connector-c#297 |
c425564 to
087c1fa
Compare
gkodinov
left a comment
There was a problem hiding this comment.
Please make sure the tests are passing. There are some parsec related failures as a result of your changes in buildbot.
|
Hi @gkodinov , |
087c1fa to
0d033e0
Compare
gkodinov
left a comment
There was a problem hiding this comment.
OK, I'll leave that to the final reviewer. Still learning how CI works. OK as far as the preliminary review goes.
Apparently this is an architectural issue with buildbot. Leaving it to the final reviewer.
| @@ -0,0 +1,86 @@ | |||
| # | |||
| # Test for MDEV-35254: PARSEC plugin should allow DBAs to specify number of iterations | |||
There was a problem hiding this comment.
note this could have been included in the existing test case parsec.test.
Tests would normally echo this line rather than it being a comment.
There was a problem hiding this comment.
Right you are @grooverdan.
@deo002 please start the line with --echo, without "Testg for":
--echo MDEV-35254: PARSEC plugin should allow....
Include this test into parsec.test.
| drop user t_iter@'%'; | ||
|
|
||
| -- echo # | ||
| -- echo # Preserve the state that existed before the test case was executed |
There was a problem hiding this comment.
no need to echo non-test information.
| -- echo # Preserve the state that existed before the test case was executed | ||
| -- echo # | ||
|
|
||
| set global parsec_iterations = 262144; |
There was a problem hiding this comment.
we'd normally save the default global value as user variable at the beginning of the test and set it back at the end. This enables a change in default without changing the test case.
There was a problem hiding this comment.
An end of tests would normally include a --echo # End of 12.4 tests statement to make the test case easier to merge if in future tests are added in later versions.
|
|
||
|
|
||
| static void update_parsec_iterations(MYSQL_THD thd, | ||
| struct st_mysql_sys_var *var __attribute__((unused)), |
There was a problem hiding this comment.
you do not need var __attribute__((unused)), , it can be just struct st_mysql_sys_var *,. It is C++, after all
There was a problem hiding this comment.
even me that's what i thought in the first place
There was a problem hiding this comment.
I'm afraid it cannot -- we have buld issues with unused variables and, sometimes, functions. Most likely because we have the waning switch turned on manually in cmake. I don't like it either, but that's what we have.
There was a problem hiding this comment.
do we? I thought it's standard C++ and a common pattern we already use. What build issues?
FooBarrior
left a comment
There was a problem hiding this comment.
Good start!
Lots of comments, enough for now, then we'll do another iteration
| @@ -0,0 +1,86 @@ | |||
| # | |||
| # Test for MDEV-35254: PARSEC plugin should allow DBAs to specify number of iterations | |||
There was a problem hiding this comment.
Right you are @grooverdan.
@deo002 please start the line with --echo, without "Testg for":
--echo MDEV-35254: PARSEC plugin should allow....
Include this test into parsec.test.
| --echo # | ||
|
|
||
| --error ER_INCORRECT_GLOBAL_LOCAL_VAR | ||
| select @@session.parsec_iterations; |
There was a problem hiding this comment.
We agreed with @vuvova that setting session value should be made possible: there can be special users that require more hardened security, like admin roles.
|
|
||
|
|
||
| static void update_parsec_iterations(MYSQL_THD thd, | ||
| struct st_mysql_sys_var *var __attribute__((unused)), |
There was a problem hiding this comment.
I'm afraid it cannot -- we have buld issues with unused variables and, sometimes, functions. Most likely because we have the waning switch turned on manually in cmake. I don't like it either, but that's what we have.
|
|
||
|
|
||
| static void update_parsec_iterations(MYSQL_THD thd, | ||
| struct st_mysql_sys_var *var __attribute__((unused)), |
| static MYSQL_SYSVAR_UINT(iterations, iterations, PLUGIN_VAR_NOCMDOPT, | ||
| "Number of iterations to be used when generating the key corresponding to the password", | ||
| NULL, update_parsec_iterations, PARSEC_ITERATIONS_DEFAULT, PARSEC_ITERATIONS_MIN, | ||
| PARSEC_ITERATIONS_MAX, PARSEC_ITERATIONS_STEP); |
There was a problem hiding this comment.
Can PARSEC_ITERATIONS_STEP be anything but 1? If not, then the existence of this variable confuses more, than if it was just "1" here
|
|
||
| constexpr unsigned int PARSEC_ITERATIONS_DEFAULT= 1u << 18; | ||
| constexpr unsigned int PARSEC_ITERATIONS_MIN= 1u << 10; | ||
| constexpr unsigned int PARSEC_ITERATIONS_MAX= 1u << 19; |
There was a problem hiding this comment.
this isn't a correct max value. To stay on the safe side, make it 1<<31, and that'll be our technical limitation of 32-bit int (because of my_bit.h function)
There was a problem hiding this comment.
There's a slight problem for showing values above 9 in the auth string. I can do it via a hex style encoding:
0–9 → 0..9
A–V → 10..31
Thoughts?
There was a problem hiding this comment.
All the symbols from ascii'0' to ascii'z' are printable, so what we have will already work fine.
I.e. ascii'0' + iterations - base.
@vuvova agree?
There was a problem hiding this comment.
We will require a little change. After '9', we have some symbols like ':', ';'...etc after which we have 'A', 'B', ...
There was a problem hiding this comment.
After '9', we have some symbols like ':', ';'
exactly.
There was a problem hiding this comment.
Oh, my bad! Our format
XY:salt:hash
Allows storing an arbitrary number in y. That is, for X=P, that is PBKDF2, and unadjusted iterations power Y=27, it'll be P27:salt:hash
There was a problem hiding this comment.
We have no other algorithm than P right now, but in allows an arbitrary config line in the first part. For P=PBKDF2, it's only an iterations count
There was a problem hiding this comment.
Oh, my bad! Our format XY:salt:hash
Allows storing an arbitrary number in y. That is, for X=P, that is PBKDF2, and unadjusted iterations power Y=27, it'll be P27:salt:hash
I think 27 might be a slip - shouldn't it be 21(31 - 10)?
There was a problem hiding this comment.
@deo002, We had a talk with @vuvova
MDEV-32618 declares the format
concat('P', conv(log2(iterations)-10, 10, 62), ':', base64(salt), ':', base64(hash))
that is, the format is conv(log2(iterations)-10, 10, 62)
Which is base62 and is 0..9,A..Z,a..z as you suggested
Feel free to use _dig_vec_base62 which is declared in m_string.h and is defined in int2str.cc. And str2int for the backward conversion.
| constexpr size_t CLIENT_RESPONSE_LENGTH= CHALLENGE_SCRAMBLE_LENGTH | ||
| + ED25519_SIG_LENGTH; | ||
|
|
||
| constexpr unsigned int PARSEC_ITERATIONS_DEFAULT= 1u << 18; |
There was a problem hiding this comment.
Make a parsec_iterations_t type and alias it to uint32
| constexpr size_t CLIENT_RESPONSE_LENGTH= CHALLENGE_SCRAMBLE_LENGTH | ||
| + ED25519_SIG_LENGTH; | ||
|
|
||
| constexpr unsigned int PARSEC_ITERATIONS_DEFAULT= 1u << 18; |
There was a problem hiding this comment.
Default should be the old default value. 1024, IIRC? that is, 1u<<ITER_FACTOR_BASE_VAL
There was a problem hiding this comment.
Yes, it was 1024. I thought of keeping it close to the OWASP's recommended value for PBKDF2-HMAC-SHA512(210,000) as stated in the Jira ticket.
| assert(params->algorithm == 'P'); | ||
| int ret = PKCS5_PBKDF2_HMAC(password, (int)pass_len, params->salt, | ||
| sizeof(params->salt), 1024 << params->iterations, | ||
| sizeof(params->salt), 1024u << params->iterations, |
There was a problem hiding this comment.
Or maybe PARSEC_ITERATIONS_MIN, if that'll be an alias
There was a problem hiding this comment.
I would maybe suggest renaming PARSEC_ITERATIONS_MIN -> ITER_BASE_VAL. So we'll have this one, and FACTOR one: ITER_FACTOR_BASE_VAL. And use ITER_BASE_VAL as min value in system variable declaration.
FooBarrior
left a comment
There was a problem hiding this comment.
Forgot to press "request changes"
2be048a to
eaaa106
Compare
gkodinov
left a comment
There was a problem hiding this comment.
For the sake of process, I'd like to mark the preliminary review as done. I see that the final review is in full swing. Please keep working with the other reviewers.
|
|
||
|
|
||
| static void update_parsec_iterations(MYSQL_THD thd, | ||
| struct st_mysql_sys_var *var __attribute__((unused)), |
There was a problem hiding this comment.
do we? I thought it's standard C++ and a common pattern we already use. What build issues?
| /* | ||
| copied from my_bit.h and m_string.h because making a service | ||
| will be an overkill | ||
| */ |
There was a problem hiding this comment.
why not include my_bit.h? it looks like a clean header, no dependencies
There was a problem hiding this comment.
ABI may break if the my_bit.h's definitions change between versions. Although, it is extremely unlikely that they will ever change. Will add it as a header.
There was a problem hiding this comment.
My bad, ABI is not a concern here. All are inline functions, compiled into the caller.
There was a problem hiding this comment.
Tried including my_bit.h directly (requires my_global.h as a prerequisite). This caused a Windows build failure. Any suggestions on how should I proceed? @vuvova
There was a problem hiding this comment.
no, don't include my_global.h, if you cannot use my_bit.h without it then better revert to local functions.
But why my_bit.h needs my_global.h ? if it's only because of uchar/uint/ulonglong then you can either
- typedef them in the plugin, or
- change
my_bit.hto useunsigned char,unigned int, etc., making it independent frommy_global.h
There was a problem hiding this comment.
I made my_bit.h standalone. I changed to unsigned char, unsigned int etc, but also had to change some macros like C_MODE_START, C_MODE_END, CONSTEXPR. Please let me know if this change is ok.
There was a problem hiding this comment.
Parsec already uses ma_global.h from libmariadb. Perhaps it's fine to use it here either?
my_global.h lies in the same directory as my_bit.h: in root/include. If my_bit.h is fine to include, then why my_global.h is not?
| auto stored= (Passwd_as_stored*)hash; | ||
| auto memory= (Passwd_in_memory*)out; | ||
| const uchar ITER_MAX_VAL= parsec_dig_vec_base62[parsec_bit_log2_uint32(PARSEC_ITERATIONS_MAX) - ITER_FACTOR_BASE_VAL]; | ||
|
|
There was a problem hiding this comment.
I'd add
static_assert(ITER_MAX_VAL == 'z');
just in case
| } | ||
|
|
||
| static MYSQL_THDVAR_UINT(iterations, PLUGIN_VAR_NOCMDOPT, | ||
| "Number of iterations to be used when generating the key corresponding to the password", |
There was a problem hiding this comment.
perhaps, add ". Must be a power of two". Or ". Will be rounded up to a power of two"
| parsec_iterations_t iterations_user_input= *static_cast<const parsec_iterations_t *>(save); | ||
| parsec_iterations_t iterations= parsec_round_up_to_next_power(iterations_user_input); | ||
| if (iterations != iterations_user_input) | ||
| my_printf_error(ER_WRONG_VALUE_FOR_VAR, "parsec: parsec_iterations rounded up to %d (next supported value)", |
There was a problem hiding this comment.
remove "parsec: " prefix, it's redundant. And may be " (next supported value)" too, because the variable help text should say that, not a warning message after the fact
| source include/wait_until_count_sessions.inc; | ||
| drop user test2@'%'; | ||
|
|
||
| --echo # Test for MDEV-35254: PARSEC plugin should allow DBAs to specify number of iterations |
There was a problem hiding this comment.
three-line header:
--echo #
--echo # MDEV-35254 PARSEC plugin should allow DBAs to specify number of iterations
--echo #
|
|
||
| --echo # parsec_iterations should should not get accidently mutated during user creation | ||
| set @iter_before := @@global.parsec_iterations; | ||
| create user t_iter@'%' identified via parsec using password('pwd'); |
There was a problem hiding this comment.
print the password here, that is,
select left(json_value(...)) from mysql.global.priv where user='t_iter'to verify that the password indeed has a correct iteration character. Perhaps even repeat it for a couple of different iteration values.
|
Working on it! |
4922a96 to
90fba23
Compare
FooBarrior
left a comment
There was a problem hiding this comment.
Expressed my concerns, basically directed to @vuvova.
| */ | ||
|
|
||
| C_MODE_START | ||
| MY_BIT_C_MODE_START |
There was a problem hiding this comment.
MY_BIT_C_MODE_START is weird with this MY_BIT prefix. There is nothing specific to MY_BIT in this macro
If you want to avoid duplicate macro definition warning, then either:
#undefmacro before#define-- undeffing an undefined macro doesn't produce a new warning- Wrap the definition in
#ifndef/#endif
The first solution is one line shorter, but if the definition would change in my_global someday, it would likely not be updated here, and those who are using my_bit.h would end up in stale definition (hiding the one from my_global.h).
So I'd go with #1, however, see my next comment.
There was a problem hiding this comment.
I'd do
#ifdef __cplusplus
extern "C" {
#else
#define constexpr
#endif
// all my_bit.h content
#ifdef __cplusplus
}
#else
#undef constexpr
#endifThere was a problem hiding this comment.
And also uint, uchar,...
And do you suggest to change CONSTEXPR to constexpr?
There was a problem hiding this comment.
no uint, no uchar, this is already replaced by unsigned int, unsigned char, etc. only c/c++ changes to make a universal c/c++ header.
| #ifndef MY_BIT_INCLUDED | ||
| #define MY_BIT_INCLUDED | ||
|
|
||
| /* C vs C++ */ |
There was a problem hiding this comment.
It would have to be a separate commit
6270e83 to
eb5d7a8
Compare
|
I have pushed the requested changes. Some tests are failing, they don't seem to be related to this PR. |
|
@deo002 Also put MDEV-35254 in the refactor commit header, that'll make it easier for us to cherry-pick changes if we'll want to. |
eb5d7a8 to
d61b6ba
Compare
|
Done! |
There was a problem hiding this comment.
Pull request overview
Adds a configurable iteration count for the PARSEC authentication plugin by introducing a new parsec_iterations plugin system variable, plus a new thd_service to let plugins access the current THD and thus session/global sysvars.
Changes:
- Add
parsec_iterations(power-of-two, rounded up) and encode iteration exponent in the stored auth string using base62. - Introduce a new plugin service
thd_servicewithget_current_thd()and wire it through the service registry. - Update plugin interface version and adjust affected mysql-test expected outputs; refactor
my_bit.hhelpers for broader (C/C++) usability.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sql/sql_plugin_services.inl | Registers the new thd_service in the server’s exported plugin services list. |
| sql/sql_class.cc | Implements get_current_thd() for the thd_service. |
| plugin/auth_parsec/server_parsec.cc | Adds parsec_iterations sysvar, updates hash format iteration encoding/decoding, and uses thd_service to read THDVAR. |
| mysql-test/suite/plugins/t/parsec.test | Extends PARSEC test coverage to validate sysvar behavior, rounding, and encoding in auth string. |
| mysql-test/suite/plugins/r/parsec.result | Updates expected output for the new PARSEC sysvar tests and warnings. |
| mysql-test/suite/plugins/r/simple_password_check.result | Bumps expected plugin library version due to interface change. |
| mysql-test/suite/plugins/r/show_all_plugins.result | Updates expected plugin library versions. |
| mysql-test/suite/plugins/r/cracklib_password_check.result | Bumps expected plugin library version due to interface change. |
| mysql-test/suite/plugins/r/auth_ed25519.result | Bumps expected plugin library version due to interface change. |
| mysql-test/main/plugin.result | Updates expected plugin library versions. |
| mysql-test/main/handlersocket.result | Updates expected plugin library version. |
| libservices/thd_service.c | Adds SERVICE_VERSION thd_service entry for the new service. |
| libservices/CMakeLists.txt | Builds the new thd_service.c into libservices. |
| include/service_versions.h | Introduces VERSION_thd for service versioning. |
| include/mysql/services.h | Exposes the new service_thd.h via umbrella services header. |
| include/mysql/service_thd.h | Defines the thd_service interface (dynamic macro + static prototype). |
| include/mysql/plugin.h | Bumps MARIA_PLUGIN_INTERFACE_VERSION. |
| include/mysql/plugin_password_validation.h.pp | Regenerates preprocessed plugin header output to include thd_service symbols. |
| include/mysql/plugin_function.h.pp | Same as above. |
| include/mysql/plugin_ftparser.h.pp | Same as above. |
| include/mysql/plugin_encryption.h.pp | Same as above. |
| include/mysql/plugin_data_type.h.pp | Same as above. |
| include/mysql/plugin_auth.h.pp | Same as above. |
| include/mysql/plugin_audit.h.pp | Same as above. |
| include/my_bit.h | Refactors bit utility helpers/types to be usable cleanly from C++ plugin code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const uchar ITER_MAX_VAL= parsec_dig_vec_base62[my_bit_log2_uint32(PARSEC_ITERATIONS_MAX) - ITER_FACTOR_BASE_VAL]; | ||
| assert(ITER_MAX_VAL == 'L'); | ||
|
|
||
| if (hash_length != sizeof (*stored) || *out_length < sizeof(*memory) || | ||
| stored->algorithm != 'P' || | ||
| stored->iterations < '0' || stored->iterations > '3' || | ||
| stored->iterations < '0' || stored->iterations > ITER_MAX_VAL || | ||
| stored->colon != ':' || stored->colon2 != ':') |
| static void update_parsec_iterations(MYSQL_THD thd, | ||
| struct st_mysql_sys_var *var, void *var_ptr, const void *save) | ||
| { | ||
| parsec_iterations_t iterations_user_input= *static_cast<const parsec_iterations_t *>(save); | ||
| parsec_iterations_t iterations= my_round_up_to_next_power(iterations_user_input); | ||
| if (iterations != iterations_user_input) | ||
| my_printf_error(ER_WRONG_VALUE_FOR_VAR, "parsec_iterations rounded up to %d", | ||
| ME_WARNING, iterations); | ||
| *static_cast<parsec_iterations_t*>(var_ptr)= iterations; | ||
| } |
There was a problem hiding this comment.
nitpicking. signed/unsigned point is moot if we use a lower MAX value per previous comment
| constexpr parsec_iterations_t ITER_FACTOR_BASE_VAL= 10u; | ||
| constexpr parsec_iterations_t ITER_BASE_VAL= 1u << ITER_FACTOR_BASE_VAL; | ||
| constexpr parsec_iterations_t PARSEC_ITERATIONS_MAX= 1u << 31; | ||
|
|
| set session parsec_iterations = 1024; | ||
| select @@session.parsec_iterations; | ||
|
|
||
| --echo # parsec_iterations should should not get accidently mutated during user creation |
Removed type aliases and macros defined in my_global.h from my_bit.h, making it possible to include it in plugins or external code without pulling in my_global.h
This patch adds a global and session level plugin variable parsec_iterations to define the number of iterations to be used when generating the key corresponding to the password. It has a default value, lower and upper bounds.
* reduced PARSEC_ITERATIONS_MAX because PKCS5_PBKDF2_HMAC() takes a signed int * instead of checking for '0'..'L' range (which includes punctuation, check for the valid result of base62_to_uchar() * add a connection test with non-default parsec_iterations
vuvova
left a comment
There was a problem hiding this comment.
Ok, this looks good.
The service should be in a separate commit. Copilot had a couple of good comments.
And an test with an actual connection attempt was missing.
I've rebased on top of main and fixed the above, now it's good for 13.1 and testing.
| constexpr parsec_iterations_t ITER_FACTOR_BASE_VAL= 10u; | ||
| constexpr parsec_iterations_t ITER_BASE_VAL= 1u << ITER_FACTOR_BASE_VAL; | ||
| constexpr parsec_iterations_t PARSEC_ITERATIONS_MAX= 1u << 31; | ||
|
|
| static void update_parsec_iterations(MYSQL_THD thd, | ||
| struct st_mysql_sys_var *var, void *var_ptr, const void *save) | ||
| { | ||
| parsec_iterations_t iterations_user_input= *static_cast<const parsec_iterations_t *>(save); | ||
| parsec_iterations_t iterations= my_round_up_to_next_power(iterations_user_input); | ||
| if (iterations != iterations_user_input) | ||
| my_printf_error(ER_WRONG_VALUE_FOR_VAR, "parsec_iterations rounded up to %d", | ||
| ME_WARNING, iterations); | ||
| *static_cast<parsec_iterations_t*>(var_ptr)= iterations; | ||
| } |
There was a problem hiding this comment.
nitpicking. signed/unsigned point is moot if we use a lower MAX value per previous comment
| const uchar ITER_MAX_VAL= parsec_dig_vec_base62[my_bit_log2_uint32(PARSEC_ITERATIONS_MAX) - ITER_FACTOR_BASE_VAL]; | ||
| assert(ITER_MAX_VAL == 'L'); | ||
|
|
||
| if (hash_length != sizeof (*stored) || *out_length < sizeof(*memory) || | ||
| stored->algorithm != 'P' || | ||
| stored->iterations < '0' || stored->iterations > '3' || | ||
| stored->iterations < '0' || stored->iterations > ITER_MAX_VAL || | ||
| stored->colon != ':' || stored->colon2 != ':') |
d61b6ba to
9800b34
Compare
|
|
|
On it! Will push the requested changes soon! |
Description
Currently it is not possible with the PARSEC plugin to define the number of iterations to be used when generating the key corresponding to the password.
To ensure proper security for users long term, the PARSEC plugin should allow specifying the number of iterations. Additionally, the default should be something more secure.
This patch adds a global plugin variable parsec_iterations to define define the number of iterations to be used when generating the key corresponding to the password. It has a default value, lower and upper bounds.
Basing the PR against the correct MariaDB version
mainbranch.PR quality check