apacheGH-582: Fix filtering of unsupported algorithms in NamedFactory

Also give the methods some javadoc. Note that "ignoreUnsupported ? f.isSupported : true" is the same as "!ignoreUnsupported || f.isSupported()", but I find the former easier to read and understand. Bug: apache#582
tomaswolf · Aug 14, 2024 · 361da19 · 361da19
1 parent a9504ba
commit 361da19
Show file tree

Hide file tree

Showing 6 changed files with 165 additions and 10 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -40,6 +40,7 @@
 
 * [GH-524](https://github.com/apache/mina-sshd/issues/524) Performance improvements
 * [GH-533](https://github.com/apache/mina-sshd/issues/533) Fix multi-step authentication
+* [GH-582](https://github.com/apache/mina-sshd/issues/582) Fix filtering in `NamedFactory`
 
 ## New Features
 
@@ -48,6 +49,7 @@
 
 ## Potential compatibility issues
 
+### New security provider registrar
 There is a new `SecurityProviderRegistrar` that is registered by default
 if there is a `SunJCE` security provider and that uses the AES and
 HmacSHA* implementations from `SunJCE` even if Bouncy Castle is also
@@ -58,6 +60,33 @@ The new registrar has the name "SunJCEWrapper" and can be configured
 like any other registrar. It can be disabled via the system property
 `org.apache.sshd.security.provider.SunJCEWrapper.enabled=false`.
 
+### [GH-582](https://github.com/apache/mina-sshd/issues/582) Fix filtering in `NamedFactory`
+
+The methods `NamedFactory.setupBuiltinFactories(boolean ignoreUnsupported, ...)` and
+`NamedFactory.setupTransformedFactories(boolean ignoreUnsupported, ...)` had a bug that
+gave the "ignoreUnsupported" parameter actually the meaning of "include unsupported".
+
+This was fixed in this release, but existing code calling these or one of the following methods:
+
+* `BaseBuilder.setUpDefaultMacs(boolean ignoreUnsupported)`
+* `BaseBuilder.setUpDefaultCiphers(boolean ignoreUnsupported)`
+* `ClientBuilder.setUpDefaultCompressionFactories(boolean ignoreUnsupported)`
+* `ClientBuilder.setUpDefaultKeyExchanges(boolean ignoreUnsupported)`
+* `ClientBuilder.setUpDefaultSignatureFactories(boolean ignoreUnsupported)`
+* `ServerBuilder.setUpDefaultCompressionFactories(boolean ignoreUnsupported)`
+* `ServerBuilder.setUpDefaultKeyExchanges(boolean ignoreUnsupported)`
+* `ServerBuilder.setUpDefaultSignatureFactories(boolean ignoreUnsupported)`
+* any of the methods starting with `SshConfigFileReader.configure`
+* `SshClientConfigFileReader.configure(...)`
+* `SshServerConfigFileReader.configure(...)`
+
+should be reviewed:
+
+* if the method is called with parameter value `true`, the result will no longer include unsupported algorithms. Formerly it wrongly did.
+* if the method is called with parameter value `false`, the result may include unsupported algorithms. Formerly it did not.
+
+So if existing code used parameter value `false` to ensure it never got unsupported algorithms, change it to `true`.
+
 ## Major Code Re-factoring
 
 ### JDK requirements

diff --git a/sshd-common/src/main/java/org/apache/sshd/common/NamedFactory.java b/sshd-common/src/main/java/org/apache/sshd/common/NamedFactory.java
@@ -48,18 +48,36 @@ static <T> T create(Collection<? extends NamedFactory<? extends T>> factories, S
         }
     }
 
+    /**
+     * Converts a list of factories to a list of transformed factories, optionally excluding unsupported factories.
+     *
+     * @param  <S>               initial factory type
+     * @param  <E>               transformed factory type
+     * @param  ignoreUnsupported whether to filter out unsupported factories from {@code preferred}
+     * @param  preferred         initial list to filter
+     * @param  xform             the transformation to apply
+     * @return                   the filtered list of transformed factories
+     */
     static <S extends OptionalFeature, E extends NamedResource> List<E> setUpTransformedFactories(
             boolean ignoreUnsupported, Collection<? extends S> preferred, Function<? super S, ? extends E> xform) {
         return preferred.stream()
-                .filter(f -> ignoreUnsupported || f.isSupported())
+                .filter(f -> ignoreUnsupported ? f.isSupported() : true)
                 .map(xform)
                 .collect(Collectors.toList());
     }
 
+    /**
+     * Filters out unsupported factories from a given list if {@code ignoreUnsupported == true}.
+     *
+     * @param  <E>               factory type
+     * @param  ignoreUnsupported whether to filter out unsupported factories from {@code preferred}
+     * @param  preferred         initial list to filter
+     * @return                   the filtered list of factories
+     */
     static <E extends NamedResource & OptionalFeature> List<E> setUpBuiltinFactories(
             boolean ignoreUnsupported, Collection<? extends E> preferred) {
         return preferred.stream()
-                .filter(f -> ignoreUnsupported || f.isSupported())
+                .filter(f -> ignoreUnsupported ? f.isSupported() : true)
                 .collect(Collectors.toList());
     }
 }
diff --git a/sshd-common/src/test/java/org/apache/sshd/common/NamedFactoryTest.java b/sshd-common/src/test/java/org/apache/sshd/common/NamedFactoryTest.java
@@ -0,0 +1,108 @@
+/*
+ * 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.sshd.common;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.sshd.util.test.JUnitTestSupport;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+@Tag("NoIoTestCase")
+class NamedFactoryTest extends JUnitTestSupport {
+
+    @Test
+    void testBuiltinSupported() {
+        List<Factory> input = new ArrayList<>();
+        input.add(new Factory("A", false));
+        input.add(new Factory("B", true));
+        input.add(new Factory("C", false));
+        input.add(new Factory("D", true));
+        input.add(new Factory("E", false));
+        List<Factory> filtered = NamedFactory.setUpBuiltinFactories(true, input);
+        assertEquals(2, filtered.size());
+        assertEquals("B", filtered.get(0).getName());
+        assertEquals("D", filtered.get(1).getName());
+    }
+
+    @Test
+    void testBuiltinUnsupported() {
+        List<Factory> input = new ArrayList<>();
+        input.add(new Factory("A", false));
+        input.add(new Factory("B", true));
+        input.add(new Factory("C", false));
+        input.add(new Factory("D", true));
+        input.add(new Factory("E", false));
+        List<Factory> filtered = NamedFactory.setUpBuiltinFactories(false, input);
+        assertIterableEquals(input, filtered);
+    }
+
+    @Test
+    void testTransformedSupported() {
+        List<OptionalFeature> input = new ArrayList<>();
+        input.add(OptionalFeature.FALSE);
+        input.add(OptionalFeature.TRUE);
+        input.add(OptionalFeature.FALSE);
+        input.add(OptionalFeature.TRUE);
+        input.add(OptionalFeature.FALSE);
+        List<Factory> filtered = NamedFactory.setUpTransformedFactories(true, input,
+                o -> new Factory(o.toString(), o.isSupported()));
+        assertEquals(2, filtered.size());
+        assertTrue(filtered.stream().allMatch(Factory::isSupported));
+    }
+
+    @Test
+    void testTransformedUnsupported() {
+        List<OptionalFeature> input = new ArrayList<>();
+        input.add(OptionalFeature.FALSE);
+        input.add(OptionalFeature.TRUE);
+        input.add(OptionalFeature.FALSE);
+        input.add(OptionalFeature.TRUE);
+        input.add(OptionalFeature.FALSE);
+        List<Factory> filtered = NamedFactory.setUpTransformedFactories(false, input,
+                o -> new Factory(o.toString(), o.isSupported()));
+        assertEquals(input.size(), filtered.size());
+        for (int i = 0; i < input.size(); i++) {
+            assertEquals(input.get(i).isSupported(), filtered.get(i).isSupported());
+        }
+    }
+
+    private static class Factory implements NamedResource, OptionalFeature {
+
+        private final String name;
+
+        private final boolean supported;
+
+        Factory(String name, boolean supported) {
+            this.name = name;
+            this.supported = supported;
+        }
+
+        @Override
+        public String getName() {
+            return name;
+        }
+
+        @Override
+        public boolean isSupported() {
+            return supported;
+        }
+    }
+}
diff --git a/sshd-core/src/main/java/org/apache/sshd/client/ClientBuilder.java b/sshd-core/src/main/java/org/apache/sshd/client/ClientBuilder.java
@@ -114,15 +114,15 @@ protected ClientBuilder fillWithDefaultValues() {
         super.fillWithDefaultValues();
 
         if (signatureFactories == null) {
-            signatureFactories = setUpDefaultSignatureFactories(false);
+            signatureFactories = setUpDefaultSignatureFactories(true);
         }
 
         if (compressionFactories == null) {
-            compressionFactories = setUpDefaultCompressionFactories(false);
+            compressionFactories = setUpDefaultCompressionFactories(true);
         }
 
         if (keyExchangeFactories == null) {
-            keyExchangeFactories = setUpDefaultKeyExchanges(false);
+            keyExchangeFactories = setUpDefaultKeyExchanges(true);
         }
 
         if (kexExtensionHandler == null) {

diff --git a/sshd-core/src/main/java/org/apache/sshd/common/BaseBuilder.java b/sshd-core/src/main/java/org/apache/sshd/common/BaseBuilder.java
@@ -168,11 +168,11 @@ protected S fillWithDefaultValues() {
         }
 
         if (cipherFactories == null) {
-            cipherFactories = setUpDefaultCiphers(false);
+            cipherFactories = setUpDefaultCiphers(true);
         }
 
         if (macFactories == null) {
-            macFactories = setUpDefaultMacs(false);
+            macFactories = setUpDefaultMacs(true);
         }
 
         if (fileSystemFactory == null) {

diff --git a/sshd-core/src/main/java/org/apache/sshd/server/ServerBuilder.java b/sshd-core/src/main/java/org/apache/sshd/server/ServerBuilder.java
@@ -130,15 +130,15 @@ protected ServerBuilder fillWithDefaultValues() {
         super.fillWithDefaultValues();
 
         if (compressionFactories == null) {
-            compressionFactories = setUpDefaultCompressionFactories(false);
+            compressionFactories = setUpDefaultCompressionFactories(true);
         }
 
         if (signatureFactories == null) {
-            signatureFactories = setUpDefaultSignatureFactories(false);
+            signatureFactories = setUpDefaultSignatureFactories(true);
         }
 
         if (keyExchangeFactories == null) {
-            keyExchangeFactories = setUpDefaultKeyExchanges(false);
+            keyExchangeFactories = setUpDefaultKeyExchanges(true);
         }
 
         if (kexExtensionHandler == null) {