Skip to content

MDEV-35254 Make iterations count configurable in PARSEC plugin #4504

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

Open
deo002 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

MDEV-35254 Make iterations count configurable in PARSEC plugin #4504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions mysql-test/suite/plugins/r/parsec.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ ERROR HY000: Wrong ext-salt format
create user test1@'%' identified via parsec using PASSWORD('pwd');
show grants for test1@'%';
Grants for test1@%
GRANT USAGE ON *.* TO `test1`@`%` IDENTIFIED VIA parsec USING 'P0:salt:password'
GRANT USAGE ON *.* TO `test1`@`%` IDENTIFIED VIA parsec USING 'P8:salt:password'
connect con1, localhost, test1, pwd;
select 1, USER(), CURRENT_USER();
1 USER() CURRENT_USER()
Expand Down Expand Up @@ -32,7 +32,7 @@ drop user test1@'%';
create user test2@'%' identified via parsec using PASSWORD('');
show grants for test2@'%';
Grants for test2@%
GRANT USAGE ON *.* TO `test2`@`%` IDENTIFIED VIA parsec USING 'P0:salt:password'
GRANT USAGE ON *.* TO `test2`@`%` IDENTIFIED VIA parsec USING 'P8:salt:password'
Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

I doubt this should have been changed.

Copy link
Contributor Author

@deo002 deo002 Jan 7, 2026

Choose a reason for hiding this comment

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

I had changed the default value to 2^18, thus the 8.

connect con4, localhost, test2,;
select 4, USER(), CURRENT_USER();
4 USER() CURRENT_USER()
Expand Down
70 changes: 70 additions & 0 deletions mysql-test/suite/plugins/r/parsec_iterations.result
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
#
# Basic visibility and default value
#
select @@global.parsec_iterations;
#
# Valid power-of-two values should be accepted
#
set global parsec_iterations = 1024;
select @@global.parsec_iterations;
@@global.parsec_iterations
1024
set global parsec_iterations = 262144;
select @@global.parsec_iterations;
@@global.parsec_iterations
262144
#
# Values should be rounded UP to next power of two
#
# 5000 -> 8192
set global parsec_iterations = 5000;
Warnings:
Warning 1231 parsec: parsec_iterations rounded up to 8192 (next supported value)
select @@global.parsec_iterations;
@@global.parsec_iterations
8192
# 131073 -> 262144
set global parsec_iterations = 131073;
Warnings:
Warning 1231 parsec: parsec_iterations rounded up to 262144 (next supported value)
select @@global.parsec_iterations;
@@global.parsec_iterations
262144
#
# Lower bound enforcement
#
set global parsec_iterations = 512;
Warnings:
Warning 1292 Truncated incorrect parsec_iterations value: '512'
select @@global.parsec_iterations;
@@global.parsec_iterations
1024
#
# Upper bound enforcement
#
set global parsec_iterations = 2097152;
Warnings:
Warning 1292 Truncated incorrect parsec_iterations value: '2097152'
select @@global.parsec_iterations;
@@global.parsec_iterations
524288
#
# Session variable must not exist
#
select @@session.parsec_iterations;
ERROR HY000: Variable 'parsec_iterations' is a GLOBAL variable
set session parsec_iterations = 1024;
ERROR HY000: Variable 'parsec_iterations' is a GLOBAL variable and should be set with SET GLOBAL
#
# 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');
select @@global.parsec_iterations = @iter_before;
@@global.parsec_iterations = @iter_before
1
drop user t_iter@'%';
#
# Preserve the state that existed before the test case was executed
#
set global parsec_iterations = 262144;
1 change: 1 addition & 0 deletions mysql-test/suite/plugins/t/parsec_iterations.opt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
--plugin-load-add=$AUTH_PARSEC_SO
86 changes: 86 additions & 0 deletions mysql-test/suite/plugins/t/parsec_iterations.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
#
# Test for MDEV-35254: PARSEC plugin should allow DBAs to specify number of iterations
Copy link
Member

@grooverdan grooverdan Jan 7, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

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.

#

source include/platform.inc;
source include/not_embedded.inc;

if (`select count(*) = 0 from information_schema.plugins where plugin_name = 'parsec'`)
{
--skip Needs parsec plugin
}

if (!$PARSEC_SO) {
skip No auth_parsec plugin;
}

--echo #
--echo # Basic visibility and default value
--echo #

--disable_result_log
select @@global.parsec_iterations;
--enable_result_log

--echo #
--echo # Valid power-of-two values should be accepted
--echo #

set global parsec_iterations = 1024;
select @@global.parsec_iterations;

set global parsec_iterations = 262144;
select @@global.parsec_iterations;

--echo #
--echo # Values should be rounded UP to next power of two
--echo #

--echo # 5000 -> 8192
set global parsec_iterations = 5000;
select @@global.parsec_iterations;

--echo # 131073 -> 262144
set global parsec_iterations = 131073;
select @@global.parsec_iterations;

--echo #
--echo # Lower bound enforcement
--echo #

set global parsec_iterations = 512;
select @@global.parsec_iterations;

--echo #
--echo # Upper bound enforcement
--echo #

set global parsec_iterations = 2097152;
select @@global.parsec_iterations;

--echo #
--echo # Session variable must not exist
--echo #

--error ER_INCORRECT_GLOBAL_LOCAL_VAR
select @@session.parsec_iterations;
Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

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.


--error ER_GLOBAL_VARIABLE
set session parsec_iterations = 1024;

--echo #
--echo # parsec_iterations should should not get accidently mutated during user creation
--echo #

set @iter_before := @@global.parsec_iterations;
create user t_iter@'%' identified via parsec using password('pwd');
select @@global.parsec_iterations = @iter_before;
drop user t_iter@'%';

-- echo #
-- echo # Preserve the state that existed before the test case was executed
Copy link
Member

@grooverdan grooverdan Jan 7, 2026

Choose a reason for hiding this comment

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

no need to echo non-test information.

-- echo #

set global parsec_iterations = 262144;
Copy link
Member

@grooverdan grooverdan Jan 7, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Member

@grooverdan grooverdan Jan 7, 2026

Choose a reason for hiding this comment

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

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.



55 changes: 51 additions & 4 deletions plugin/auth_parsec/server_parsec.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,52 @@ constexpr size_t PBKDF2_HASH_LENGTH= ED25519_KEY_LENGTH;
constexpr size_t CLIENT_RESPONSE_LENGTH= CHALLENGE_SCRAMBLE_LENGTH
+ ED25519_SIG_LENGTH;

constexpr unsigned int PARSEC_ITERATIONS_DEFAULT= 1u << 18;
Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

Make a parsec_iterations_t type and alias it to uint32

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

Default should be the old default value. 1024, IIRC? that is, 1u<<ITER_FACTOR_BASE_VAL

Copy link
Contributor Author

@deo002 deo002 Jan 7, 2026

Choose a reason for hiding this comment

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

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.

constexpr unsigned int PARSEC_ITERATIONS_MIN= 1u << 10;
constexpr unsigned int PARSEC_ITERATIONS_MAX= 1u << 19;
Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@deo002 deo002 Jan 7, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@deo002 deo002 Jan 7, 2026

Choose a reason for hiding this comment

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

We will require a little change. After '9', we have some symbols like ':', ';'...etc after which we have 'A', 'B', ...

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

After '9', we have some symbols like ':', ';'

exactly.

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@deo002 deo002 Jan 7, 2026

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

@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 unsigned int PARSEC_ITERATIONS_STEP= 1u;
constexpr unsigned int ITER_FACTOR_BASE_VAL= 10u;

static unsigned int iterations= PARSEC_ITERATIONS_DEFAULT;

static inline unsigned int round_to_power_of_two(const unsigned int x) {
unsigned int p = 1;
while (p < x)
p <<= 1;
return p;
}


static inline unsigned int log_2_of_power_of_2(const unsigned int x) {
// function will not work correctly for non power of 2 unsigned integers
unsigned int p = 0;
while ((1u << p) != x)
Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

This code proves it was written manually, without AI, which I like:) However there are at least 2 ways to do it O(1) machine instructions.

We have my_bits.h that does what you need, pick the right function from there.

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

Though it's O(loglog), not O(1) yet. C++20 has std::bit_ceil, but we only use c++17 yet.

p++;
return p;
}


static void update_parsec_iterations(MYSQL_THD thd,
struct st_mysql_sys_var *var __attribute__((unused)),
Copy link
Member

@vaintroub vaintroub Jan 7, 2026

Choose a reason for hiding this comment

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

you do not need var __attribute__((unused)), , it can be just struct st_mysql_sys_var *,. It is C++, after all

Copy link

@kulebelden-wq kulebelden-wq Jan 7, 2026

Choose a reason for hiding this comment

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

even me that's what i thought in the first place

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

So we need this gcc-ism

void *var_ptr __attribute__((unused)), const void *save)
{
unsigned int iterations_user_input= *(unsigned int *) save;
iterations= round_to_power_of_two(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)",
ME_WARNING, iterations);
}

static MYSQL_SYSVAR_UINT(iterations, iterations, PLUGIN_VAR_NOCMDOPT,
Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

Why NOCMDOPT? I think it can be perfectly set with a command line.

Copy link
Contributor Author

@deo002 deo002 Jan 7, 2026

Choose a reason for hiding this comment

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

I had done it with PLUGIN_VAR_OPCMDARG before, but the setting of value via command line didn't go through the usual pipeline of check and update functions. So, this allowed values like 5000 to be accepted and stored even though the variable is defined to operate on powers of two.

I'll try to round off value set in the command line in the init function. It seems a bit off as it's not the usual pipeline, but could be done. What do you suggest?

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

Yes, that's pretty surprising for me as well. Let me check..

"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);
Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

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


static struct st_mysql_sys_var* system_vars[] = {
MYSQL_SYSVAR(iterations),
NULL
};

constexpr size_t base64_length(size_t input_length)
{
return ((input_length + 2) / 3) * 4; // with padding
Expand Down Expand Up @@ -95,7 +141,7 @@ int compute_derived_key(const char* password, size_t pass_len,
{
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,
Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

(1<<ITER_BASE_VAL)

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

Or maybe PARSEC_ITERATIONS_MIN, if that'll be an alias

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

EVP_sha512(), PBKDF2_HASH_LENGTH, derived_key);
if(ret == 0)
return print_ssl_error();
Expand Down Expand Up @@ -176,7 +222,7 @@ int hash_password(const char *password, size_t password_length,

Passwd_in_memory memory;
memory.algorithm= 'P';
memory.iterations= 0;
memory.iterations= log_2_of_power_of_2(iterations) - ITER_FACTOR_BASE_VAL;
my_random_bytes(memory.salt, sizeof(memory.salt));

uchar derived_key[PBKDF2_HASH_LENGTH];
Expand Down Expand Up @@ -204,10 +250,11 @@ int digest_to_binary(const char *hash, size_t hash_length,
{
auto stored= (Passwd_as_stored*)hash;
auto memory= (Passwd_in_memory*)out;
const uchar ITER_MAX_VAL= '0' + (log_2_of_power_of_2(PARSEC_ITERATIONS_MAX) - ITER_FACTOR_BASE_VAL);

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 ||
Copy link
Member

@grooverdan grooverdan Jan 7, 2026

Choose a reason for hiding this comment

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

so 0 -> 3 was somehow allowed before, but didn't have a meaning? Its its meaning now compatible?

Copy link
Contributor Author

@deo002 deo002 Jan 7, 2026

Choose a reason for hiding this comment

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

Yes, although the iteration factor was defined to support values from 0 to 3, it was hard-coded to 0—probably to allow for future changes. It now supports values from 0 to 9(with this PR).

Copy link
Contributor

@FooBarrior FooBarrior Jan 7, 2026

Choose a reason for hiding this comment

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

This is a question to me rather. And I don't remember the reason:) Perhaps a dumb sanity check.

stored->colon != ':' || stored->colon2 != ':')
{
my_printf_error(ER_PASSWD_LENGTH, "Wrong ext-salt format", 0);
Expand Down Expand Up @@ -313,7 +360,7 @@ maria_declare_plugin(auth_parsec)
NULL,
0x0100,
NULL,
NULL,
system_vars,
"1.0",
MariaDB_PLUGIN_MATURITY_GAMMA
}
Expand Down