24.8.14: ALTER USER bypasses allow_no_password/allow_plaintext_password and formatter drops comma in multi-method auth

[Selfeer]

ALTER USER bypasses allow_no_password/allow_plaintext_password; formatter drops comma in multi-method auth (PR #1371)

Source: Altinity/ClickHouse PR #1371 – audit review (comment by @Selfeer, 2026-03-17)
Context: 24.8.14 Backport of ClickHouse/ClickHouse #65277 (Multi auth methods by @arthurpassos)
AI audit note: The original review was generated by AI (gpt-5.3-codex).

---

Summary

Code audit of the multi-auth-methods backport identified 2 confirmed defects: one Medium (policy enforcement bypass on ALTER USER) and one Low (SQL formatter bug when highlighting is enabled). Both should be fixed and covered by tests.

---

Confirmed defects

1. Medium: ALTER USER bypasses allow_no_password / allow_plaintext_password policy checks

• Impact: ALTER USER ... IDENTIFIED ... can persist auth methods disallowed by server policy, creating policy-violating users and (for some configs) users that then cannot authenticate.
• Anchor: src/Interpreters/Access/InterpreterCreateUserQuery.cpp / updateUserFromQueryImpl()
• Trigger: Set allow_no_password=0 or allow_plaintext_password=0, then run ALTER USER with the corresponding disallowed auth method.
• Affected transition: ALTER USER AST → updateUserFromQueryImpl() method merge/replace → user entity write.
• Why defect: The policy gate is only executed under if (!query.alter), so all alter flows skip auth-type policy enforcement.
• Smallest logical reproduction:
  1. Disable one of these auth types in server config.
  2. Create a user with an allowed method.
  3. Run ALTER USER ... IDENTIFIED WITH no_password (or plaintext_password).
  4. Alter succeeds despite policy.
• Fix direction: Apply the allow/disallow auth-type validation when auth methods are explicitly changed on ALTER USER too.
• Regression test direction: Add integration test asserting ALTER USER ... IDENTIFIED ... rejects disallowed auth types under both config flags.
• Affected subsystem / blast radius: Access control DDL interpreter; all clusters relying on auth-type hardening via those config flags.

Code reference:

for (const auto & authentication_method : authentication_methods)
{
    user.authentication_methods.emplace_back(authentication_method);
}
...
if (!query.alter)
{
    for (const auto & authentication_method : user.authentication_methods)
    {
        auto auth_type = authentication_method.getType();
        if (((auth_type == AuthenticationType::NO_PASSWORD) && !allow_no_password) ||
            ((auth_type == AuthenticationType::PLAINTEXT_PASSWORD) && !allow_plaintext_password))
        {
            throw Exception(ErrorCodes::BAD_ARGUMENTS, ...);
        }
    }
}

---

2. Low: SQL formatter drops comma between auth methods when highlighting is enabled

• Impact: Formatted CREATE/ALTER USER ... IDENTIFIED WITH ... output can be malformed in highlighted formatting mode (missing comma separator between methods), affecting readability and copy/paste reliability.
• Anchor: src/Parsers/Access/ASTCreateUserQuery.cpp / formatAuthenticationData()
• Trigger: Render query AST with settings.hilite=true and multiple authentication methods.
• Affected transition: AST serialization → SQL string emission for multi-method auth list.
• Why defect: The separator branch emits IAST::hilite_keyword instead of emitting the comma token.
• Smallest logical reproduction: Parse a multi-method CREATE USER ... IDENTIFIED WITH ... , ...; serialize with hilite enabled; separator is not emitted as ,.
• Fix direction: Emit comma regardless of highlight mode and wrap highlight markers separately.
• Regression test direction: Add parser/formatter unit test with hilite-enabled formatting for multi-method auth list and assert comma-preserving output.
• Affected subsystem / blast radius: SQL AST formatting path (SHOW CREATE / formatted query output), user-facing diagnostics.

Code reference:

for (std::size_t i = 0; i < authentication_methods.size(); i++)
{
    authentication_methods[i]->format(settings);

    bool is_last = i < authentication_methods.size() - 1;
    if (is_last)
    {
        settings.ostr << (settings.hilite ? IAST::hilite_keyword : ",");
    }
}