diff --git a/CHANGES.md b/CHANGES.md index 8883f75f7..e4b23cf47 100644 --- 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 index 566e5f23d..26c454662 100644 --- 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 create(Collection> factories, S } } + /** + * Converts a list of factories to a list of transformed factories, optionally excluding unsupported factories. + * + * @param initial factory type + * @param 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 List setUpTransformedFactories( boolean ignoreUnsupported, Collection preferred, Function 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 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 List setUpBuiltinFactories( boolean ignoreUnsupported, Collection 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 new file mode 100644 index 000000000..43f94222c --- /dev/null +++ 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 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 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 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 filtered = NamedFactory.setUpBuiltinFactories(false, input); + assertIterableEquals(input, filtered); + } + + @Test + void testTransformedSupported() { + List input = new ArrayList<>(); + input.add(OptionalFeature.FALSE); + input.add(OptionalFeature.TRUE); + input.add(OptionalFeature.FALSE); + input.add(OptionalFeature.TRUE); + input.add(OptionalFeature.FALSE); + List 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 input = new ArrayList<>(); + input.add(OptionalFeature.FALSE); + input.add(OptionalFeature.TRUE); + input.add(OptionalFeature.FALSE); + input.add(OptionalFeature.TRUE); + input.add(OptionalFeature.FALSE); + List 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 index c7308bdea..b6a4353c4 100644 --- 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 index 0c2ecc950..6453420fb 100644 --- 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 index 5a7ed835c..c2c46ab16 100644 --- 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) {