You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/08/19 09:46:53 UTC
[GitHub] [ignite] ololo3000 commented on a change in pull request #9290: IGNITE-12781 Fixes security context propagation for cache events.
ololo3000 commented on a change in pull request #9290:
URL: https://github.com/apache/ignite/pull/9290#discussion_r691957863
##########
File path: modules/clients/src/test/java/org/apache/ignite/common/ComputeTaskRemoteSecurityContextTest.java
##########
@@ -105,7 +105,7 @@
@RunWith(Parameterized.class)
public class ComputeTaskRemoteSecurityContextTest extends AbstractSecurityTest {
/** Port for REST client connection. */
- private static final String DFLT_REST_PORT = "11080";
+ static final String DFLT_REST_PORT = "11080";
Review comment:
Done. AbstractEventSecurityContextTest was created.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCachePartitionExchangeManager.java
##########
@@ -2980,6 +2983,13 @@ public BooleanMetricImpl clusterRebalancedMetric() {
return rebalanced;
}
+ /** Returns current thread security context if security is enabled, otherwise null. */
+ @Nullable private SecurityContext securityContext() {
+ IgniteSecurity security = cctx.kernalContext().security();
+
+ return security.enabled() ? security.securityContext() : null;
Review comment:
Done.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/discovery/SecurityAwareCustomMessageWrapper.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.managers.discovery;
+
+import java.util.UUID;
+import org.apache.ignite.spi.discovery.DiscoverySpiCustomMessage;
+import org.jetbrains.annotations.Nullable;
+
+/** Extends {@link CustomMessageWrapper} with ID of security subject that initiated the current message. */
+public class SecurityAwareCustomMessageWrapper extends CustomMessageWrapper {
+ /** */
+ private static final long serialVersionUID = 0L;
+
+ /** Security subject ID. */
+ private final UUID secSubjId;
+
+ /** */
+ public SecurityAwareCustomMessageWrapper(DiscoveryCustomMessage delegate, UUID secSubjId) {
+ super(delegate);
+
+ this.secSubjId = secSubjId;
+ }
+
+ /** Gets security Subject ID. */
+ public UUID securitySubjectId() {
+ return secSubjId;
+ }
+
+ /** {@inheritDoc} */
+ @Override public @Nullable DiscoverySpiCustomMessage ackMessage() {
+ DiscoveryCustomMessage ack = delegate().ackMessage();
+
+ return ack == null ? null : new SecurityAwareCustomMessageWrapper(ack, secSubjId);
+ }
+}
+
Review comment:
Done.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/discovery/GridDiscoveryManager.java
##########
@@ -917,13 +927,63 @@ else if (type == EVT_CLIENT_NODE_RECONNECTED) {
discoCache,
notification.getTopSnapshot(),
stateFinishMsg,
- notification.getSpanContainer()
+ notification.getSpanContainer(),
+ secCtx
)
);
if (type == EVT_CLIENT_NODE_DISCONNECTED)
discoWrk.awaitDisconnectEvent();
}
+
+ /** */
+ class SecurityAwareNotificationTask extends NotificationTask {
+ /** */
+ public SecurityAwareNotificationTask(DiscoveryNotification notification) {
+ super(notification);
+ }
+
+ /** */
+ @Override public void run() {
+ DiscoverySpiCustomMessage customMsg = notification.getCustomMsgData();
+
+ if (customMsg instanceof SecurityAwareCustomMessageWrapper) {
+ UUID secSubjId = ((SecurityAwareCustomMessageWrapper)customMsg).securitySubjectId();
+
+ try (OperationSecurityContext ignored = ctx.security().withContext(secSubjId)) {
+ super.run();
+ }
+ }
+ else {
+ SecurityContext initiatorNodeSecCtx = nodeSecurityContext(
+ marshaller,
+ U.resolveClassLoader(ctx.config()),
+ notification.getNode()
+ );
+
+ try (OperationSecurityContext ignored = ctx.security().withContext(initiatorNodeSecCtx)) {
+ super.run();
+ }
+ }
+ }
+ }
+
+ class NotificationTask implements Runnable {
Review comment:
Done.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/discovery/GridDiscoveryManager.java
##########
@@ -917,13 +927,63 @@ else if (type == EVT_CLIENT_NODE_RECONNECTED) {
discoCache,
notification.getTopSnapshot(),
stateFinishMsg,
- notification.getSpanContainer()
+ notification.getSpanContainer(),
+ secCtx
)
);
if (type == EVT_CLIENT_NODE_DISCONNECTED)
discoWrk.awaitDisconnectEvent();
}
+
+ /** */
+ class SecurityAwareNotificationTask extends NotificationTask {
+ /** */
+ public SecurityAwareNotificationTask(DiscoveryNotification notification) {
+ super(notification);
+ }
+
+ /** */
+ @Override public void run() {
+ DiscoverySpiCustomMessage customMsg = notification.getCustomMsgData();
+
+ if (customMsg instanceof SecurityAwareCustomMessageWrapper) {
+ UUID secSubjId = ((SecurityAwareCustomMessageWrapper)customMsg).securitySubjectId();
+
+ try (OperationSecurityContext ignored = ctx.security().withContext(secSubjId)) {
+ super.run();
+ }
+ }
+ else {
+ SecurityContext initiatorNodeSecCtx = nodeSecurityContext(
+ marshaller,
+ U.resolveClassLoader(ctx.config()),
+ notification.getNode()
+ );
+
+ try (OperationSecurityContext ignored = ctx.security().withContext(initiatorNodeSecCtx)) {
Review comment:
The main problem in this case is that IgniteSecurityProcessor#withContext(UUID) method uses GridDiscoveryManager#node(UUID) to obtain a node instance. But DiscoveryCache is updated DURING event handling (see `topSnap.set(new Snapshot(snapshot.topVer, discoCache));` calls in DiscoverySpiListener#onDiscovery0) but we must get node security context BEFORE event handling. So in case of NODE_JOINED event we will see NPE during IgniteSecurityProcessor#withContext(UUID) call as no node with specified ID will be found.
Since internal discovery events such as NODE_JOINED or NODE_LEFT does not occur so often, I think that we can sacrifice caching. WDYT?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org