Copilot commented on code in PR #4477:
URL: https://github.com/apache/solr/pull/4477#discussion_r3327103761
##########
solr/core/src/java/org/apache/solr/security/Sha256AuthenticationProvider.java:
##########
@@ -93,6 +93,7 @@ public void init(Map<String, Object> pluginConfig) {
@Override
public boolean authenticate(String username, String password) {
+ if (username.equals(password)) return false;
String cred = credentials.get(username);
if (cred == null || cred.isEmpty()) return false;
Review Comment:
`authenticate()` now calls `username.equals(password)` which can throw a
NullPointerException if `username` is null. The previous logic handled null
usernames safely (`credentials.get(null)` is allowed), so this is a behavioral
regression.
Use a null-safe equality check (or guard for null) to avoid potential NPEs.
##########
solr/core/src/java/org/apache/solr/security/Sha256AuthenticationProvider.java:
##########
@@ -165,6 +166,10 @@ public Map<String, Object> edit(Map<String, Object>
latestConf, List<CommandOper
cmd.addError("name and password must be non-null");
return null;
}
+ if (e.getKey().equals(String.valueOf(e.getValue()))) {
+ cmd.addError("Password must not be the same as the username");
+ return null;
+ }
Review Comment:
The new username==password rule is enforced in `authenticate()` and for
`set-user`, but there are no tests covering either behavior. Since this is
security-sensitive and can disable existing well-known accounts, please add
unit tests asserting:
- `authenticate(user,user)` returns false even when the stored hash matches
- `edit(..., set-user)` rejects entries where username==password
##########
solr/core/src/java/org/apache/solr/cli/AuthTool.java:
##########
@@ -286,6 +286,14 @@ private void handleBasicAuth(CommandLine cli) throws
Exception {
String.format(
Locale.ROOT, "Successfully enabled basic auth with username
[%s].", username);
echo(successMessage);
+ CLIO.out(
+ "\nIMPORTANT: The following template users have been created
with NO password set"
+ + " and cannot log in until passwords are assigned:");
+ CLIO.out(" - admin (roles: admin, index, search)");
+ CLIO.out(" - index (roles: index, search)");
Review Comment:
`bin/solr auth enable` currently allows enabling BasicAuth with
username==password (e.g. `admin:admin`). With this PR’s new policy, that
credential will never authenticate, and since `--block-unknown` defaults to
true this can immediately lock an operator out after enabling auth.
Validate and reject username==password before generating/uploading
`security.json` and before writing `basicAuth.conf`.
##########
solr/core/src/java/org/apache/solr/cli/AuthTool.java:
##########
@@ -286,6 +286,14 @@ private void handleBasicAuth(CommandLine cli) throws
Exception {
String.format(
Locale.ROOT, "Successfully enabled basic auth with username
[%s].", username);
echo(successMessage);
+ CLIO.out(
+ "\nIMPORTANT: The following template users have been created
with NO password set"
+ + " and cannot log in until passwords are assigned:");
+ CLIO.out(" - admin (roles: admin, index, search)");
+ CLIO.out(" - index (roles: index, search)");
+ CLIO.out(" - search (roles: search)");
+ CLIO.out(
+ "Set their passwords using the Admin UI Security page or the
authentication API.");
Review Comment:
PR description mentions that `bin/solr auth enable` will prompt the operator
to set passwords for template users at enable-time, but the current
implementation only prints a post-enable notice. Either add the prompting
behavior or update the PR description to match the implemented behavior.
##########
solr/core/src/java/org/apache/solr/cli/AuthTool.java:
##########
@@ -286,6 +286,14 @@ private void handleBasicAuth(CommandLine cli) throws
Exception {
String.format(
Locale.ROOT, "Successfully enabled basic auth with username
[%s].", username);
echo(successMessage);
+ CLIO.out(
+ "\nIMPORTANT: The following template users have been created
with NO password set"
+ + " and cannot log in until passwords are assigned:");
+ CLIO.out(" - admin (roles: admin, index, search)");
+ CLIO.out(" - index (roles: index, search)");
Review Comment:
`--block-unknown` is parsed (defaulting to true), but the generated
`security.json` is updated with a *top-level* `blockUnknown` field. Solr reads
`blockUnknown` from the `authentication` plugin config (i.e.,
`security.json.authentication.blockUnknown`), so the CLI option currently won’t
take effect and the template’s value will be used instead.
This is security-relevant because operators may believe they enabled
mandatory authentication when they did not.
##########
solr/core/src/java/org/apache/solr/security/Sha256AuthenticationProvider.java:
##########
@@ -165,6 +166,10 @@ public Map<String, Object> edit(Map<String, Object>
latestConf, List<CommandOper
cmd.addError("name and password must be non-null");
return null;
}
+ if (e.getKey().equals(String.valueOf(e.getValue()))) {
+ cmd.addError("Password must not be the same as the username");
+ return null;
+ }
Review Comment:
The new username==password rule is enforced in `authenticate()` and for
`set-user`, but there are no tests covering either behavior. Since this is
security-sensitive and can disable existing well-known accounts, please add
unit tests asserting:
- `authenticate(user,user)` returns false even when the stored hash matches
- `edit(..., set-user)` rejects entries where username==password
##########
solr/core/src/java/org/apache/solr/cli/AuthTool.java:
##########
@@ -286,6 +286,14 @@ private void handleBasicAuth(CommandLine cli) throws
Exception {
String.format(
Locale.ROOT, "Successfully enabled basic auth with username
[%s].", username);
echo(successMessage);
+ CLIO.out(
+ "\nIMPORTANT: The following template users have been created
with NO password set"
+ + " and cannot log in until passwords are assigned:");
+ CLIO.out(" - admin (roles: admin, index, search)");
+ CLIO.out(" - index (roles: index, search)");
Review Comment:
`bin/solr auth enable` currently allows enabling BasicAuth with
username==password (e.g. `admin:admin`). With this PR’s new policy, that
credential will never authenticate, which can leave operators unable to use
`basicAuth.conf` credentials (and can cause a full lockout if `blockUnknown` is
enabled).
Validate and reject username==password before generating/uploading
`security.json` and before writing `basicAuth.conf`.
##########
solr/core/src/java/org/apache/solr/cli/AuthTool.java:
##########
@@ -286,6 +286,14 @@ private void handleBasicAuth(CommandLine cli) throws
Exception {
String.format(
Locale.ROOT, "Successfully enabled basic auth with username
[%s].", username);
echo(successMessage);
+ CLIO.out(
+ "\nIMPORTANT: The following template users have been created
with NO password set"
+ + " and cannot log in until passwords are assigned:");
+ CLIO.out(" - admin (roles: admin, index, search)");
+ CLIO.out(" - index (roles: index, search)");
+ CLIO.out(" - search (roles: search)");
+ CLIO.out(
+ "Set their passwords using the Admin UI Security page or the
authentication API.");
Review Comment:
PR description mentions that `bin/solr auth enable` will prompt the operator
to set passwords for template users at enable-time, but the current
implementation only prints a post-enable notice. Either add the prompting
behavior or update the PR description to match the implemented behavior.
##########
solr/core/src/java/org/apache/solr/security/Sha256AuthenticationProvider.java:
##########
@@ -93,6 +93,7 @@ public void init(Map<String, Object> pluginConfig) {
@Override
public boolean authenticate(String username, String password) {
+ if (username.equals(password)) return false;
String cred = credentials.get(username);
if (cred == null || cred.isEmpty()) return false;
Review Comment:
`authenticate()` now calls `username.equals(password)` which can throw a
NullPointerException if `username` is null. The previous logic handled null
usernames safely (`credentials.get(null)` is allowed), so this is a behavioral
regression.
Use a null-safe equality check (or guard for null) to avoid potential NPEs.
##########
solr/solr-ref-guide/modules/deployment-guide/pages/solr-control-script-reference.adoc:
##########
@@ -881,7 +881,7 @@ The `bin/solr` script allows enabling or disabling
Authentication, allowing you
Currently this command is only available when using SolrCloud mode and must be
run on the machine hosting Solr.
-For Basic Authentication the script provides
https://github.com/apache/solr/blob/main/solr/core/src/resources/security.json[user
roles and permission mappings], and maps the created user to the `superadmin`
role.
+For Basic Authentication the script provides
https://github.com/apache/solr/blob/main/solr/core/src/resources/security.json[user
roles and permission mappings], and maps the created user to all roles
(`superadmin`, `admin`, `index`, `search`).
Review Comment:
The link added here correctly points to
`solr/core/src/resources/security.json`, but a few lines later in this same
section the bullet "Take the base ..." still points at
`solr/core/resources/security.json` (missing `src`). That later link will be
broken/outdated; please update it to keep the documentation consistent and
accurate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
