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.

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

Add default password type #44674

Merged
merged 20 commits into from
Apr 27, 2023

Conversation

evillique
Copy link
Member

@evillique evillique commented Dec 28, 2022

Changelog category (leave one):

  • New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Password type in queries like CREATE USER u IDENTIFIED BY 'p' will be automatically set according to the setting default_password_type in the config.xml on the server. Closes #42915

Query parameters can now also be used to specify passwords, for example:
CREATE USER u IDENTIFIED BY {password:String}

@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-feature Pull request with new product feature label Dec 28, 2022
@vitlibar vitlibar self-assigned this Dec 28, 2022
@evillique evillique force-pushed the add_default_password_type branch 2 times, most recently from 6a8c63f to cc0f758 Compare January 4, 2023 15:04
src/Client/ClientBase.cpp Outdated Show resolved Hide resolved
src/Client/ClientBase.cpp Outdated Show resolved Hide resolved
@vitlibar
Copy link
Member

So let's keep the plaintext password in a CREATE USER query and do on the server both hash calculating and password complexity checking.

@vitlibar vitlibar marked this pull request as draft January 30, 2023 13:04
}
namespace
{
AuthenticationData makeAuthenticationData(const ASTAuthenticationData & query, ContextPtr context, bool check_password_rules)
Copy link
Member Author

Choose a reason for hiding this comment

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

Initially, I added functions makeAuthenticationData() and makeASTAuthenticationData() as methods to ASTAuthenticationData, but adding Context to clickhouse_parsers breaks the standalone keeper build, so I moved them to Interpreters.

Any suggestions on where to better put them are welcome

Copy link
Member

@vitlibar vitlibar Apr 21, 2023

Choose a reason for hiding this comment

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

Could we move that transformation function to Access/AuthenticationData.h/cpp?

We can make it a member function of AuthenticationData, kind of AuthenticationData::fromAST()

Copy link
Member Author

Choose a reason for hiding this comment

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

It sounds like a good idea, but I'm afraid that it will run into the same problem, clickhouse_common_access is linked to clickhouse_parsers.

Copy link
Member

Choose a reason for hiding this comment

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

AuthenticationData.cpp was in clickhouse_common_access only because it was used in ASTCreateUser. Now that's not the case anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

But AuthenticationData.h/cpp is used both in ASTAuthenticationData and in ParseCreateUserQuery because of the AuthenticationType. Maybe I'm missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do something like this:

static AuthenticationData::fromAST(
    ASTPtr query,
    const ASTs & prepared_arguments,
    std::optional<AuthenticationType> default_type
    //, std::optional<int> bcrypt_param - in the future
)

InterpreterCreateUserQuery::execute():

...
std::optional<AuthenticationData> auth_data;
if (query.auth_data)
{
    if (!query.attach)
    {
        auto password = auth_data->getPassword();
        if (password)
            global_context->getAccessControl().checkPasswordComplexityRules(*password);
    }

    AuthenticationType default_type = access_control.getDefaultPasswordType();
    ASTs args(args_size);
        for (size_t i = 0; i < args_size; ++i)
            args[i] = evaluateConstantExpressionAsLiteral(query.children[i], context);

    auth_data = AuthenticationData::fromAST(query, args, default_type);
}
...

Is it better?

Copy link
Member

Choose a reason for hiding this comment

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

But AuthenticationData.h/cpp is used both in ASTAuthenticationData and in ParseCreateUserQuery because of the AuthenticationType.

We can keep the AuthenticationType enum in the folder src/Access/Common and move AuthenticationData to src/Access.

@evillique evillique marked this pull request as ready for review April 10, 2023 22:05
return ptr;
auto clone = std::make_shared<ASTSubquery>(*this);
clone->cloneChildren();
return clone;
Copy link
Member

Choose a reason for hiding this comment

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

What happened here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not related, just a small improvement

}


AuthenticationData AuthenticationData::fromAST(const ASTAuthenticationData & query, ContextPtr context, bool check_password_rules)
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we check password complexity rules in the InterpreterCreateQuery or is here ok?

Copy link
Member

Choose a reason for hiding this comment

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

Both places seem ok.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to check them in the InterpreterCreateQuery to make the conversion function more simple, but that's not very important.

@evillique
Copy link
Member Author

Test failures are unrelated.

@evillique evillique merged commit ea55222 into ClickHouse:master Apr 27, 2023
@evillique evillique deleted the add_default_password_type branch April 27, 2023 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature Pull request with new product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CREATE USER ... IDENTIFIED WITH PASSWORD should use the preferred password hashing method
3 participants