timoninmaxim commented on a change in pull request #8892:
URL: https://github.com/apache/ignite/pull/8892#discussion_r614775648
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/authentication/IgniteAuthenticationProcessor.java
##########
@@ -314,15 +314,14 @@ public AuthorizationContext authenticate(String login,
String passwd) throws Ign
}
fut.get();
Review comment:
Looks like, we can wait here forever. I see that AuthenticateFuture has
retry param that triggers on NodeLeft event. But it is still a bad practice to
skip timeout attribute. As there could be any bugs that will lead to silently
hanging in this place.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/authentication/IgniteAuthenticationProcessor.java
##########
@@ -136,8 +160,8 @@
/** Node activate future. */
private final GridFutureAdapter<Void> activateFut = new
GridFutureAdapter<>();
- /** Validate error. */
- private String validateErr;
+ /** User management operations. */
+ EnumSet<SecurityPermission> userOps = EnumSet.of(CREATE_USER, DROP_USER,
ALTER_USER);
Review comment:
Make it private final
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/authentication/IgniteAuthenticationProcessor.java
##########
@@ -1335,4 +1332,99 @@ private RefreshUsersStorageWorker(ArrayList<User> usrs) {
}
}
}
+
+ /** Represents {@link SecuritySubject} implementation. */
+ private static class SecuritySubjectImpl implements SecuritySubject {
+ /** */
+ private static final long serialVersionUID = 0L;
+
+ /** Security subject identifier. */
+ private final UUID id;
+
+ /** Security subject login. */
+ private final Object login;
+
+ /** Security subject type. */
+ private final SecuritySubjectType type;
+
+ /** Security subject address. */
+ private final InetSocketAddress addr;
+
+ /** */
+ public SecuritySubjectImpl(UUID id, Object login, SecuritySubjectType
type, InetSocketAddress addr) {
+ this.id = id;
+ this.login = login;
+ this.type = type;
+ this.addr = addr;
+ }
+
+ /** {@inheritDoc} */
+ @Override public UUID id() {
+ return id;
+ }
+
+ /** {@inheritDoc} */
+ @Override public Object login() {
+ return login;
+ }
+
+ /** {@inheritDoc} */
+ @Override public SecuritySubjectType type() {
+ return type;
+ }
+
+ /** {@inheritDoc} */
+ @Override public InetSocketAddress address() {
+ return addr;
+ }
+
+ /** {@inheritDoc} */
+ @Override public SecurityPermissionSet permissions() {
+ return null;
Review comment:
There is an inconsistency. SecuritySubjectImpl#permissions is null, but
SecurityContextImpl#operationAllowed is true. If all operations are allowed
that we need to fill permissions with all possible values.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/authentication/IgniteAuthenticationProcessor.java
##########
@@ -836,14 +792,11 @@ private void
onAuthenticateResponseMessage(UserAuthenticateResponseMessage msg)
* Local node joined to topology. Discovery cache is available but no
discovery custom message are received.
* Initial user set and initial user operation (received on join) are
processed here.
*/
- public void onLocalJoin() {
- if (coordinator() == null)
+ private void onLocalJoin() {
+ if (ctx.isDaemon() || ctx.clientDisconnected() || coordinator() ==
null)
Review comment:
Can we replace clientDisconnected with ctx.clientNode(), and then remove
a check below. Client disconnected looks weird, as there no actions on
reconnected, and it's misleading how a node should fill initial user state.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/authentication/IgniteAuthenticationProcessor.java
##########
@@ -971,6 +893,81 @@ private static boolean isNodeHoldsUsers(ClusterNode n) {
return !n.isClient() && !n.isDaemon();
}
+ /** {@inheritDoc} */
+ @Override public SecurityContext authenticateNode(ClusterNode node,
SecurityCredentials cred) throws IgniteCheckedException {
+ return new SecurityContextImpl(
+ node.id(),
+ node.attribute(ATTR_IGNITE_INSTANCE_NAME),
+ REMOTE_NODE,
+ new InetSocketAddress(F.first(node.addresses()), 0));
+ }
+
+ /** {@inheritDoc} */
+ @Override public SecuritySubject authenticatedSubject(UUID subjId) throws
IgniteCheckedException {
+ throw new UnsupportedOperationException();
+ }
+
+ /** {@inheritDoc} */
+ @Override public Collection<SecuritySubject> authenticatedSubjects()
throws IgniteCheckedException {
+ throw new UnsupportedOperationException();
+ }
+
+ /** {@inheritDoc} */
+ @Override public boolean isGlobalNodeAuthentication() {
+ return false;
+ }
+
+ /** {@inheritDoc} */
+ @Override public void authorize(String name, SecurityPermission perm,
SecurityContext securityCtx) throws SecurityException {
+ if (!userOps.contains(perm))
+ return;
+
+ SecuritySubject subj = ctx.security().securityContext().subject();
+
+ Object login = subj.login();
+
+ if (subj.type() == REMOTE_NODE) {
+ throw new SecurityException("User management operations initiated
on behalf of the Ignite node" +
+ " are not supported [igniteInstanceName=" + login + ']');
+ }
+
+ if (!DEFAULT_USER_NAME.equals(login) && !(ALTER_USER == perm &&
Objects.equals(login, name)))
+ throw new SecurityException("User management operations are not
allowed for user [curUser=" + login + ']');
+
+ if (DROP_USER == perm && DEFAULT_USER_NAME.equals(name))
+ throw new SecurityException("Default user cannot be removed.");
+ }
+
+ /** {@inheritDoc} */
+ @Override public void onSessionExpired(UUID subjId) {
+ // No-op.
+ }
+
+ /** {@inheritDoc} */
+ @Override public SecurityContext securityContext(UUID subjId) {
+ User user = users.get(subjId);
+
+ return user == null ? null : new SecurityContextImpl(subjId,
user.name(), REMOTE_CLIENT, null);
Review comment:
Why does it always return REMOTE_CLIENT? Docs say it applies for nodes
too: "Gets security context for authenticated nodes and thin clients". Looks
like we can support a cache for contexts, also it will fix @dgarus comment (I
agree with him).
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/authentication/IgniteAuthenticationProcessor.java
##########
@@ -971,6 +893,81 @@ private static boolean isNodeHoldsUsers(ClusterNode n) {
return !n.isClient() && !n.isDaemon();
}
+ /** {@inheritDoc} */
+ @Override public SecurityContext authenticateNode(ClusterNode node,
SecurityCredentials cred) throws IgniteCheckedException {
+ return new SecurityContextImpl(
+ node.id(),
+ node.attribute(ATTR_IGNITE_INSTANCE_NAME),
+ REMOTE_NODE,
+ new InetSocketAddress(F.first(node.addresses()), 0));
+ }
+
+ /** {@inheritDoc} */
+ @Override public SecuritySubject authenticatedSubject(UUID subjId) throws
IgniteCheckedException {
+ throw new UnsupportedOperationException();
+ }
+
+ /** {@inheritDoc} */
+ @Override public Collection<SecuritySubject> authenticatedSubjects()
throws IgniteCheckedException {
+ throw new UnsupportedOperationException();
+ }
+
+ /** {@inheritDoc} */
+ @Override public boolean isGlobalNodeAuthentication() {
+ return false;
+ }
+
+ /** {@inheritDoc} */
+ @Override public void authorize(String name, SecurityPermission perm,
SecurityContext securityCtx) throws SecurityException {
+ if (!userOps.contains(perm))
+ return;
+
+ SecuritySubject subj = ctx.security().securityContext().subject();
+
+ Object login = subj.login();
+
+ if (subj.type() == REMOTE_NODE) {
+ throw new SecurityException("User management operations initiated
on behalf of the Ignite node" +
+ " are not supported [igniteInstanceName=" + login + ']');
+ }
+
+ if (!DEFAULT_USER_NAME.equals(login) && !(ALTER_USER == perm &&
Objects.equals(login, name)))
+ throw new SecurityException("User management operations are not
allowed for user [curUser=" + login + ']');
+
+ if (DROP_USER == perm && DEFAULT_USER_NAME.equals(name))
+ throw new SecurityException("Default user cannot be removed.");
+ }
+
+ /** {@inheritDoc} */
+ @Override public void onSessionExpired(UUID subjId) {
+ // No-op.
+ }
+
+ /** {@inheritDoc} */
+ @Override public SecurityContext securityContext(UUID subjId) {
+ User user = users.get(subjId);
+
+ return user == null ? null : new SecurityContextImpl(subjId,
user.name(), REMOTE_CLIENT, null);
+ }
+
+ /**
+ * Gets the user with the specified ID and login. It is necessary to check
the login to make sure that there was
+ * no collision when calculating the user ID.
+ */
+ private User findUser(UUID subjId, String login) {
+ User user = users.get(subjId);
Review comment:
The users variable guarded with the mux lock in other methods. Actually
this lock is tricky, I'm not fully understand what is a purpose of that. Could
you please should we use this lock there too?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/ClientListenerAbstractConnectionContext.java
##########
@@ -104,33 +95,12 @@ public GridKernalContext kernalContext() {
/**
* Perform authentication.
*
- * @return Auth context.
* @throws IgniteCheckedException If failed.
*/
- protected AuthorizationContext authenticate(GridNioSession ses, String
user, String pwd)
- throws IgniteCheckedException {
- if (ctx.security().enabled())
- authCtx = authenticateExternal(ses, user,
pwd).authorizationContext();
- else if (ctx.authentication().enabled()) {
- if (F.isEmpty(user))
- throw new IgniteAccessControlException("Unauthenticated
sessions are prohibited.");
-
- authCtx = ctx.authentication().authenticate(user, pwd);
+ protected void authenticate(GridNioSession ses, String user, String pwd)
throws IgniteCheckedException {
Review comment:
There is a weird behavior on last line of this method. We do a check of
secCtx for NULL, if NULL that throws an exception. But actually AuthProcessor
never returns NULL, it just throws exception on auth failure. Also docs for
GridSecurityProcessor are misleading:
```
* @return {@code True} if succeeded, {@code false} otherwise.
public SecurityContext authenticate(AuthenticationContext ctx) throws
IgniteCheckedException;
```
Let's fix declaration of return value in the docs (true / false ->
SecurityContext), also there should be a description for auth fail behavior -
should it return NULL or throw an exception.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/authentication/IgniteAuthenticationProcessor.java
##########
@@ -314,15 +314,14 @@ public AuthorizationContext authenticate(String login,
String passwd) throws Ign
}
fut.get();
Review comment:
Below there are also some fut.get(). Let's fix them too.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
