frankgh commented on code in PR #3085:
URL: https://github.com/apache/cassandra/pull/3085#discussion_r1480889114
##########
src/java/org/apache/cassandra/auth/AuthenticatedUser.java:
##########
@@ -74,6 +95,22 @@ public RoleResource getPrimaryRole()
return role;
}
+ /**
+ * The mode of authentication used to authenticate this user.
+ */
+ public AuthenticationMode getAuthenticationMode()
+ {
+ return authenticationMode;
+ }
+
+ /**
+ * {@link IAuthenticator}-contextual metadata about how the user was
authenticated.
Review Comment:
```suggestion
* @returns {@link IAuthenticator}-contextual metadata about how the
user was authenticated
```
##########
src/java/org/apache/cassandra/auth/AuthenticatedUser.java:
##########
@@ -74,6 +95,22 @@ public RoleResource getPrimaryRole()
return role;
}
+ /**
+ * The mode of authentication used to authenticate this user.
Review Comment:
NIT:
```suggestion
* @returns the mode of authentication used to authenticate this user
```
##########
src/java/org/apache/cassandra/tools/nodetool/ClientStats.java:
##########
@@ -86,54 +92,51 @@ public void execute(NodeProbe probe)
return;
}
- if (listConnections)
+ // Note: for compatbility with existing implementation if someone
passes --all (listConnections),
+ // --client-options, and --metadata all three will be printed.
+ List<Map<String, String>> clients = (List<Map<String, String>>)
probe.getClientMetric("connections");
+ if (!clients.isEmpty() && (listConnections || clientOptions ||
verbose))
{
- List<Map<String, String>> clients = (List<Map<String, String>>)
probe.getClientMetric("connections");
- if (!clients.isEmpty())
+ List<String> tableHeaderBase = Lists.newArrayList("Address",
"SSL", "Cipher", "Protocol", "Version",
+ "User", "Keyspace", "Requests", "Driver-Name",
+ "Driver-Version");
+ List<String> tableFieldsBase =
Lists.newArrayList(ConnectedClient.ADDRESS, ConnectedClient.SSL,
Review Comment:
```suggestion
List<String> tableFieldsBase = List.of(ConnectedClient.ADDRESS,
ConnectedClient.SSL,
```
##########
src/java/org/apache/cassandra/tools/nodetool/ClientStats.java:
##########
@@ -86,54 +92,51 @@ public void execute(NodeProbe probe)
return;
}
- if (listConnections)
+ // Note: for compatbility with existing implementation if someone
passes --all (listConnections),
+ // --client-options, and --metadata all three will be printed.
+ List<Map<String, String>> clients = (List<Map<String, String>>)
probe.getClientMetric("connections");
+ if (!clients.isEmpty() && (listConnections || clientOptions ||
verbose))
{
- List<Map<String, String>> clients = (List<Map<String, String>>)
probe.getClientMetric("connections");
- if (!clients.isEmpty())
+ List<String> tableHeaderBase = Lists.newArrayList("Address",
"SSL", "Cipher", "Protocol", "Version",
Review Comment:
NIT: prefer JDK APIs. Since trunk and 5.0 are on Java 11+
```suggestion
List<String> tableHeaderBase = List.of("Address", "SSL",
"Cipher", "Protocol", "Version",
```
##########
src/java/org/apache/cassandra/auth/IAuthenticator.java:
##########
@@ -179,5 +195,64 @@ default boolean shouldSendAuthenticateMessage()
{
return true;
}
+
+ /**
+ * @return The assumed mode of authentication attempted using this
negotiator, this will usually be some value
+ * of {@link AuthenticationMode#toString()}} unless an implementor
provides their own custom authentication
+ * scheme.
+ */
+ default AuthenticationMode getAuthenticationMode()
+ {
+ return AuthenticationMode.UNAUTHENTICATED;
+ }
+ }
+
+ /**
+ * Known modes of authentication supported by Cassandra's provided {@link
IAuthenticator} implementations.
+ */
+ abstract class AuthenticationMode
+ {
+ private final String displayName;
+
+ public AuthenticationMode(@Nonnull String displayName)
+ {
+ this.displayName = displayName;
+ }
+
+ /**
+ * User was not authenticated in any particular way.
+ */
+ public static final AuthenticationMode UNAUTHENTICATED = new
AuthenticationMode("Unauthenticated") {};
+
+ /**
+ * User authenticated using a password.
+ */
+ public static final AuthenticationMode PASSWORD = new
AuthenticationMode("Password") {};
+
+ /**
+ * User authenticated using a trusted identity in their client
certificate.
+ */
+ public static final AuthenticationMode MTLS = new
AuthenticationMode("Mtls") {};
Review Comment:
Should we spell out the label?
```suggestion
public static final AuthenticationMode MTLS = new
AuthenticationMode("Mutual TLS") {};
```
##########
src/java/org/apache/cassandra/auth/AllowAllAuthenticator.java:
##########
@@ -22,13 +22,17 @@
import java.util.Map;
import java.util.Set;
+import com.google.common.collect.Sets;
+
import org.apache.cassandra.exceptions.AuthenticationException;
import org.apache.cassandra.exceptions.ConfigurationException;
public class AllowAllAuthenticator implements IAuthenticator
{
private static final SaslNegotiator AUTHENTICATOR_INSTANCE = new
Negotiator();
+ private static final Set<AuthenticationMode> AUTHENTICATION_MODES =
Sets.newHashSet(AuthenticationMode.UNAUTHENTICATED);
Review Comment:
NIT: prefer JDK APIs when possible:
```suggestion
private static final Set<AuthenticationMode> AUTHENTICATION_MODES =
Collections.singleton(AuthenticationMode.UNAUTHENTICATED);
```
##########
src/java/org/apache/cassandra/metrics/ClientMetrics.java:
##########
@@ -51,20 +93,50 @@ public final class ClientMetrics
private Meter protocolException;
private Meter unknownException;
+ private static final String AUTH_SUCCESS = "AuthSuccess";
+
+ private static final String AUTH_FAILURE = "AuthFailure";
+
+ private static final String CONNECTED_NATIVE_CLIENTS =
"ConnectedNativeClients";
+
private ClientMetrics()
{
}
+ /**
+ * @deprecated by {@link #markAuthSuccess(String)}
+ */
+ @Deprecated(since="5.1", forRemoval = true)
public void markAuthSuccess()
+ {
+ markAuthSuccess(null);
+ }
+
+ public void markAuthSuccess(String mode)
{
authSuccess.mark();
+ Optional.ofNullable(mode)
+ .flatMap((m) -> Optional.ofNullable(authSuccessByMode.get(m)))
+ .ifPresent(Meter::mark);
Review Comment:
a bit hard to read, but it is the equivalent of:
```
Meter meterByMode;
if (mode != null && (meterByMode = authSuccessByMode.get(mode)) !=
null) {
meterByMode.mark();
}
```
##########
src/java/org/apache/cassandra/metrics/ClientMetrics.java:
##########
@@ -51,20 +93,50 @@ public final class ClientMetrics
private Meter protocolException;
private Meter unknownException;
+ private static final String AUTH_SUCCESS = "AuthSuccess";
+
+ private static final String AUTH_FAILURE = "AuthFailure";
+
+ private static final String CONNECTED_NATIVE_CLIENTS =
"ConnectedNativeClients";
+
private ClientMetrics()
{
}
+ /**
+ * @deprecated by {@link #markAuthSuccess(String)}
+ */
+ @Deprecated(since="5.1", forRemoval = true)
public void markAuthSuccess()
+ {
+ markAuthSuccess(null);
+ }
+
+ public void markAuthSuccess(String mode)
{
authSuccess.mark();
+ Optional.ofNullable(mode)
+ .flatMap((m) -> Optional.ofNullable(authSuccessByMode.get(m)))
+ .ifPresent(Meter::mark);
}
+ /**
+ * @deprecated by {@link #markAuthFailure(String)}
+ */
+ @Deprecated(since="5.1", forRemoval = true)
public void markAuthFailure()
{
authFailure.mark();
Review Comment:
should this call `markAuthFailure(null)` just like we do for the success
case?
##########
src/java/org/apache/cassandra/auth/IAuthenticator.java:
##########
@@ -104,12 +110,23 @@ default SaslNegotiator newSaslNegotiator(InetAddress
clientAddress, Certificate[
return newSaslNegotiator(clientAddress);
}
+ /**
+ * @return The supported authentication 'modes' of this authenticator.
+ * scheme.
Review Comment:
```suggestion
* @return The supported authentication 'modes' of this authenticator
scheme
```
##########
src/java/org/apache/cassandra/transport/messages/AuthUtil.java:
##########
@@ -58,6 +58,7 @@ public final class AuthUtil
static Response handleLogin(Connection connection, QueryState queryState,
byte[] token,
BiFunction<Boolean, byte[], Response>
messageToSendBasedOnNegotiation)
{
+ IAuthenticator.SaslNegotiator negotiator = ((ServerConnection)
connection).getSaslNegotiator(queryState);
Review Comment:
it seems we only need the negotiator in the scope of the catch below, should
we move it there instead?
--
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]
