You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mina.apache.org by tw...@apache.org on 2022/11/09 12:18:10 UTC
[mina-sshd] branch master updated (bba481a9b -> 1ccde6cdf)
This is an automated email from the ASF dual-hosted git repository.
twolf pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
from bba481a9b MinaSession: avoid synthetic accesses
new 5a8fe830b Better file handling for host keys
new 1ccde6cdf Minor comment fix
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../sshd/common/util/buffer/BufferUtils.java | 2 +-
.../AbstractGeneratorHostKeyProvider.java | 88 ++++++++++++++---
.../SimpleGeneratorHostKeyProvider.java | 109 ++++++++++++++++++---
.../SimpleGeneratorHostKeyProviderTest.java | 26 ++++-
4 files changed, 195 insertions(+), 30 deletions(-)
[mina-sshd] 01/02: Better file handling for host keys
Posted by tw...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
twolf pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 5a8fe830b2a2308a2b24ac8115a391af477f64f5
Author: Thomas Wolf <tw...@apache.org>
AuthorDate: Sat Nov 5 21:00:40 2022 +0100
Better file handling for host keys
Store host keys in the OpenSSH format. This makes it possible to use
EdDSA host keys. Also set file permissions and read legacy files more
carefully.
---
.../AbstractGeneratorHostKeyProvider.java | 88 ++++++++++++++---
.../SimpleGeneratorHostKeyProvider.java | 109 ++++++++++++++++++---
.../SimpleGeneratorHostKeyProviderTest.java | 26 ++++-
3 files changed, 194 insertions(+), 29 deletions(-)
diff --git a/sshd-common/src/main/java/org/apache/sshd/server/keyprovider/AbstractGeneratorHostKeyProvider.java b/sshd-common/src/main/java/org/apache/sshd/server/keyprovider/AbstractGeneratorHostKeyProvider.java
index f8cb53334..29b6e3d80 100644
--- a/sshd-common/src/main/java/org/apache/sshd/server/keyprovider/AbstractGeneratorHostKeyProvider.java
+++ b/sshd-common/src/main/java/org/apache/sshd/server/keyprovider/AbstractGeneratorHostKeyProvider.java
@@ -18,6 +18,7 @@
*/
package org.apache.sshd.server.keyprovider;
+import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
@@ -25,6 +26,11 @@ import java.nio.file.Files;
import java.nio.file.LinkOption;
import java.nio.file.OpenOption;
import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+import java.nio.file.attribute.AclEntry;
+import java.nio.file.attribute.AclEntryType;
+import java.nio.file.attribute.AclFileAttributeView;
+import java.nio.file.attribute.UserPrincipal;
import java.security.GeneralSecurityException;
import java.security.InvalidKeyException;
import java.security.KeyPair;
@@ -46,6 +52,7 @@ import org.apache.sshd.common.keyprovider.AbstractKeyPairProvider;
import org.apache.sshd.common.keyprovider.KeySizeIndicator;
import org.apache.sshd.common.session.SessionContext;
import org.apache.sshd.common.util.GenericUtils;
+import org.apache.sshd.common.util.OsUtils;
import org.apache.sshd.common.util.io.IoUtils;
import org.apache.sshd.common.util.io.resource.PathResource;
import org.apache.sshd.common.util.security.SecurityUtils;
@@ -128,7 +135,7 @@ public abstract class AbstractGeneratorHostKeyProvider
}
}
- @Override // co-variant return
+ @Override
public synchronized List<KeyPair> loadKeys(SessionContext session) {
Path keyPath = getPath();
Iterable<KeyPair> ids;
@@ -140,7 +147,7 @@ public abstract class AbstractGeneratorHostKeyProvider
if (ids != null) {
keyPairHolder.set(ids);
}
- } catch (Throwable t) {
+ } catch (Exception t) {
warn("loadKeys({}) Failed ({}) to resolve: {}",
keyPath, t.getClass().getSimpleName(), t.getMessage(), t);
}
@@ -174,7 +181,7 @@ public abstract class AbstractGeneratorHostKeyProvider
if (kp != null) {
return ids;
}
- } catch (Throwable e) {
+ } catch (Exception e) {
warn("resolveKeyPair({}) Failed ({}) to load: {}",
keyPath, e.getClass().getSimpleName(), e.getMessage(), e);
}
@@ -193,7 +200,7 @@ public abstract class AbstractGeneratorHostKeyProvider
log.debug("resolveKeyPair({}) generated {} key={}-{}",
keyPath, alg, KeyUtils.getKeyType(key), KeyUtils.getFingerPrint(key));
}
- } catch (Throwable e) {
+ } catch (Exception e) {
warn("resolveKeyPair({})[{}] Failed ({}) to generate {} key-pair: {}",
keyPath, alg, e.getClass().getSimpleName(), alg, e.getMessage(), e);
return null;
@@ -202,7 +209,7 @@ public abstract class AbstractGeneratorHostKeyProvider
if (keyPath != null) {
try {
writeKeyPair(kp, keyPath);
- } catch (Throwable e) {
+ } catch (Exception e) {
warn("resolveKeyPair({})[{}] Failed ({}) to write {} key: {}",
alg, keyPath, e.getClass().getSimpleName(), alg, e.getMessage(), e);
}
@@ -263,22 +270,71 @@ public abstract class AbstractGeneratorHostKeyProvider
return SecurityUtils.loadKeyPairIdentities(session, resourceKey, inputStream, null);
}
- protected void writeKeyPair(KeyPair kp, Path keyPath, OpenOption... options)
+ protected void writeKeyPair(KeyPair kp, Path keyPath)
throws IOException, GeneralSecurityException {
- if ((!Files.exists(keyPath)) || isOverwriteAllowed()) {
- PathResource location = new PathResource(keyPath); // The options are for write (!!)
- try (OutputStream os = Files.newOutputStream(keyPath, options)) {
- doWriteKeyPair(location, kp, os);
- } catch (Throwable e) {
- warn("writeKeyPair({}) failed ({}) to write key {}: {}",
- keyPath, e.getClass().getSimpleName(), kp, e.getMessage(), e);
+ Objects.requireNonNull(kp, "No host key");
+ if (!Files.exists(keyPath) || isOverwriteAllowed()) {
+ // Create an empty file or truncate an existing file
+ Files.newOutputStream(keyPath).close();
+ setFilePermissions(keyPath);
+ try (OutputStream os = Files.newOutputStream(keyPath, StandardOpenOption.WRITE,
+ StandardOpenOption.TRUNCATE_EXISTING)) {
+ doWriteKeyPair(new PathResource(keyPath), kp, os);
+ } catch (Exception e) {
+ error("writeKeyPair({}) failed ({}) to write {} host key : {}",
+ keyPath, e.getClass().getSimpleName(),
+ KeyUtils.getKeyType(kp), e.getMessage(), e);
}
} else {
- log.error("Overwriting key ({}) is disabled: using throwaway {}: {}",
- keyPath, KeyUtils.getKeyType(kp), KeyUtils.getFingerPrint((kp == null) ? null : kp.getPublic()));
+ log.warn("Overwriting host key ({}) is disabled: using throwaway {} key: {}",
+ keyPath, KeyUtils.getKeyType(kp), KeyUtils.getFingerPrint(kp.getPublic()));
}
}
+ private void setFilePermissions(Path path) throws IOException {
+ Throwable t = null;
+ if (OsUtils.isWin32()) {
+ AclFileAttributeView view = Files.getFileAttributeView(path, AclFileAttributeView.class);
+ UserPrincipal owner = Files.getOwner(path);
+ if (view != null && owner != null) {
+ try {
+ // Remove all access rights from non-owners.
+ List<AclEntry> restricted = new ArrayList<>();
+ for (AclEntry acl : view.getAcl()) {
+ if (owner.equals(acl.principal()) || AclEntryType.DENY.equals(acl.type())) {
+ restricted.add(acl);
+ } else {
+ // We can't use DENY access: if the owner is member of a group and we deny the group
+ // access, the owner won't be able to perform the access. Instead of denying permissions
+ // simply allow nothing.
+ restricted.add(AclEntry.newBuilder()
+ .setType(AclEntryType.ALLOW)
+ .setPrincipal(acl.principal())
+ .setPermissions(Collections.emptySet())
+ .build());
+ }
+ }
+ view.setAcl(restricted);
+ return;
+ } catch (IOException | SecurityException e) {
+ t = e;
+ }
+ }
+ } else {
+ File file = path.toFile();
+ if (!file.setExecutable(false)) {
+ log.debug("Host key file {}: cannot set non-executable", path);
+ }
+
+ boolean success = file.setWritable(false, false) && file.setWritable(true, true);
+ success = file.setReadable(false, false) && file.setReadable(true, true) && success;
+ if (success) {
+ return;
+ }
+ }
+ log.warn("Host key file {}: cannot set file permissions correctly (readable and writeable only by owner)", path, t);
+ }
+
protected abstract void doWriteKeyPair(
NamedResource resourceKey, KeyPair kp, OutputStream outputStream)
throws IOException, GeneralSecurityException;
@@ -305,6 +361,8 @@ public abstract class AbstractGeneratorHostKeyProvider
} else if (keySize != 0) {
generator.initialize(keySize);
log.info("generateKeyPair({}) generating host key - size={}", algorithm, keySize);
+ } else {
+ log.info("generateKeyPair({}) generating host key", algorithm);
}
return generator.generateKeyPair();
diff --git a/sshd-common/src/main/java/org/apache/sshd/server/keyprovider/SimpleGeneratorHostKeyProvider.java b/sshd-common/src/main/java/org/apache/sshd/server/keyprovider/SimpleGeneratorHostKeyProvider.java
index 95c6bb0a1..38ba9113b 100644
--- a/sshd-common/src/main/java/org/apache/sshd/server/keyprovider/SimpleGeneratorHostKeyProvider.java
+++ b/sshd-common/src/main/java/org/apache/sshd/server/keyprovider/SimpleGeneratorHostKeyProvider.java
@@ -18,26 +18,36 @@
*/
package org.apache.sshd.server.keyprovider;
+import java.io.BufferedInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.ObjectInputStream;
-import java.io.ObjectOutputStream;
+import java.io.ObjectStreamClass;
+import java.io.ObjectStreamConstants;
import java.io.OutputStream;
+import java.io.StreamCorruptedException;
import java.nio.file.Path;
import java.security.GeneralSecurityException;
import java.security.KeyPair;
import java.security.spec.InvalidKeySpecException;
import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
import org.apache.sshd.common.NamedResource;
+import org.apache.sshd.common.config.keys.loader.openssh.OpenSSHKeyPairResourceParser;
+import org.apache.sshd.common.config.keys.writer.openssh.OpenSSHKeyPairResourceWriter;
import org.apache.sshd.common.session.SessionContext;
/**
- * TODO Add javadoc
+ * A simple implementation of an {@link AbstractGeneratorHostKeyProvider} that writes and reads host keys using the
+ * OpenSSH file format. Legacy keys written by earlier implementations used Java serialization. De-serializing is
+ * restricted to a small number of classes known to exist in serialized {@link KeyPair}s.
*
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
*/
public class SimpleGeneratorHostKeyProvider extends AbstractGeneratorHostKeyProvider {
+
public SimpleGeneratorHostKeyProvider() {
super();
}
@@ -49,23 +59,100 @@ public class SimpleGeneratorHostKeyProvider extends AbstractGeneratorHostKeyProv
@Override
protected Iterable<KeyPair> doReadKeyPairs(SessionContext session, NamedResource resourceKey, InputStream inputStream)
throws IOException, GeneralSecurityException {
- KeyPair kp;
- try (ObjectInputStream r = new ObjectInputStream(inputStream)) {
- try {
- kp = (KeyPair) r.readObject();
- } catch (ClassNotFoundException e) {
- throw new InvalidKeySpecException("Missing classes: " + e.getMessage(), e);
+ try (BufferedInputStream in = new BufferedInputStream(inputStream)) {
+ if (isJavaSerialization(in, resourceKey)) {
+ try (ObjectInputStream r = new ValidatingObjectInputStream(in)) {
+ return Collections.singletonList((KeyPair) r.readObject());
+ } catch (ClassNotFoundException e) {
+ throw new InvalidKeySpecException(
+ "Cannot de-serialize " + resourceKey + ": missing classes: " + e.getMessage(), e);
+ }
+ } else {
+ OpenSSHKeyPairResourceParser reader = new OpenSSHKeyPairResourceParser();
+ return reader.loadKeyPairs(null, resourceKey, null, in);
}
}
+ }
- return Collections.singletonList(kp);
+ private boolean isJavaSerialization(BufferedInputStream in, NamedResource resourceKey) throws IOException {
+ in.mark(2);
+ try {
+ byte[] magicBytes = new byte[2];
+ int length = in.read(magicBytes);
+ if (length != 2) {
+ throw new StreamCorruptedException("File " + resourceKey + " is not a host key");
+ }
+ short magic = (short) (((magicBytes[0] & 0xFF) << 8) | (magicBytes[1] & 0xFF));
+ return magic == ObjectStreamConstants.STREAM_MAGIC;
+ } finally {
+ in.reset();
+ }
}
@Override
protected void doWriteKeyPair(NamedResource resourceKey, KeyPair kp, OutputStream outputStream)
throws IOException, GeneralSecurityException {
- try (ObjectOutputStream w = new ObjectOutputStream(outputStream)) {
- w.writeObject(kp);
+ OpenSSHKeyPairResourceWriter writer = new OpenSSHKeyPairResourceWriter();
+ try (OutputStream out = outputStream) {
+ writer.writePrivateKey(kp, "host key", null, out);
}
}
+
+ private static class ValidatingObjectInputStream extends ObjectInputStream {
+
+ private static final Set<String> ALLOWED = new HashSet<>();
+
+ static {
+ ALLOWED.add("[B"); // byte[], used in BC EC key serialization
+
+ ALLOWED.add("java.lang.Enum");
+ ALLOWED.add("java.lang.Number");
+ ALLOWED.add("java.lang.String");
+
+ ALLOWED.add("java.math.BigInteger"); // Used in BC DSA/RSA
+
+ ALLOWED.add("java.security.KeyPair");
+ ALLOWED.add("java.security.PublicKey");
+ ALLOWED.add("java.security.PrivateKey");
+ ALLOWED.add("java.security.KeyRep");
+ ALLOWED.add("java.security.KeyRep$Type");
+
+ ALLOWED.add("org.bouncycastle.jcajce.provider.asymmetric.dsa.BCDSAPrivateKey");
+ ALLOWED.add("org.bouncycastle.jcajce.provider.asymmetric.dsa.BCDSAPublicKey");
+ ALLOWED.add("org.bouncycastle.jcajce.provider.asymmetric.rsa.BCRSAPrivateCrtKey");
+ ALLOWED.add("org.bouncycastle.jcajce.provider.asymmetric.rsa.BCRSAPrivateKey");
+ ALLOWED.add("org.bouncycastle.jcajce.provider.asymmetric.rsa.BCRSAPublicKey");
+ ALLOWED.add("org.bouncycastle.jcajce.provider.asymmetric.ec.BCECPrivateKey");
+ ALLOWED.add("org.bouncycastle.jcajce.provider.asymmetric.ec.BCECPublicKey");
+
+ ALLOWED.add("com.android.org.bouncycastle.jcajce.provider.asymmetric.dsa.BCDSAPrivateKey");
+ ALLOWED.add("com.android.org.bouncycastle.jcajce.provider.asymmetric.dsa.BCDSAPublicKey");
+ ALLOWED.add("com.android.org.bouncycastle.jcajce.provider.asymmetric.rsa.BCRSAPrivateCrtKey");
+ ALLOWED.add("com.android.org.bouncycastle.jcajce.provider.asymmetric.rsa.BCRSAPrivateKey");
+ ALLOWED.add("com.android.org.bouncycastle.jcajce.provider.asymmetric.rsa.BCRSAPublicKey");
+ ALLOWED.add("com.android.org.bouncycastle.jcajce.provider.asymmetric.ec.BCECPrivateKey");
+ ALLOWED.add("com.android.org.bouncycastle.jcajce.provider.asymmetric.ec.BCECPublicKey");
+
+ // net.i2p EdDSA keys cannot be serialized anyway; so no need to whitelist any of their classes.
+ // They use the default serialization, which writes a great many different classes, but at least
+ // one of them does not implement Serializable, and thus writing runs into a NotSerializableException.
+ }
+
+ ValidatingObjectInputStream(InputStream in) throws IOException {
+ super(in);
+ }
+
+ @Override
+ protected Class<?> resolveClass(ObjectStreamClass desc) throws IOException, ClassNotFoundException {
+ validate(desc.getName());
+ return super.resolveClass(desc);
+ }
+
+ private void validate(String className) throws IOException {
+ if (!ALLOWED.contains(className)) {
+ throw new IOException(className + " blocked for deserialization");
+ }
+ }
+ }
+
}
diff --git a/sshd-common/src/test/java/org/apache/sshd/server/keyprovider/SimpleGeneratorHostKeyProviderTest.java b/sshd-common/src/test/java/org/apache/sshd/server/keyprovider/SimpleGeneratorHostKeyProviderTest.java
index f0d388732..55c3064b2 100644
--- a/sshd-common/src/test/java/org/apache/sshd/server/keyprovider/SimpleGeneratorHostKeyProviderTest.java
+++ b/sshd-common/src/test/java/org/apache/sshd/server/keyprovider/SimpleGeneratorHostKeyProviderTest.java
@@ -19,6 +19,7 @@
package org.apache.sshd.server.keyprovider;
import java.io.IOException;
+import java.io.ObjectOutputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.security.GeneralSecurityException;
@@ -88,7 +89,13 @@ public class SimpleGeneratorHostKeyProviderTest extends JUnitTestSupport {
new ECGenParameterSpec("P-521"));
}
- private Path testSimpleGeneratorHostKeyProvider(
+ @Test
+ public void testEdDSA() throws IOException, GeneralSecurityException {
+ Assume.assumeTrue("EdDSA not supported", SecurityUtils.isEDDSACurveSupported());
+ testSimpleGeneratorHostKeyProvider(SecurityUtils.EDDSA, KeyPairProvider.SSH_ED25519, -1, null);
+ }
+
+ private void testSimpleGeneratorHostKeyProvider(
String algorithm, String keyType, int keySize, AlgorithmParameterSpec keySpec)
throws IOException, GeneralSecurityException {
Path path = initKeyFileLocation(algorithm);
@@ -97,7 +104,16 @@ public class SimpleGeneratorHostKeyProviderTest extends JUnitTestSupport {
KeyPair kpRead = invokeSimpleGeneratorHostKeyProvider(path, algorithm, keyType, keySize, keySpec);
assertKeyPairEquals("Mismatched write/read key pairs", kpWrite, kpRead);
- return path;
+
+ if (!KeyPairProvider.SSH_ED25519.equals(keyType)) {
+ // Try the old way: use Java serialization. net.i2p EdDSA keys cannot be serialized.
+ path = initKeyFileLocation(algorithm, "ser");
+ try (ObjectOutputStream out = new ObjectOutputStream(Files.newOutputStream(path))) {
+ out.writeObject(kpWrite);
+ }
+ kpRead = invokeSimpleGeneratorHostKeyProvider(path, algorithm, keyType, keySize, keySpec);
+ assertKeyPairEquals("Mismatched serialized/deserialized key pairs", kpWrite, kpRead);
+ }
}
private static KeyPair invokeSimpleGeneratorHostKeyProvider(
@@ -135,8 +151,12 @@ public class SimpleGeneratorHostKeyProviderTest extends JUnitTestSupport {
}
private Path initKeyFileLocation(String algorithm) throws IOException {
+ return initKeyFileLocation(algorithm, "key");
+ }
+
+ private Path initKeyFileLocation(String algorithm, String extension) throws IOException {
Path path = assertHierarchyTargetFolderExists(getTempTargetRelativeFile(getClass().getSimpleName()));
- path = path.resolve(algorithm + "-simple.key");
+ path = path.resolve(algorithm + "-simple." + extension);
Files.deleteIfExists(path);
return path;
}
[mina-sshd] 02/02: Minor comment fix
Posted by tw...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
twolf pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 1ccde6cdfe72adf13ef9dd49138434a74aabd784
Author: Thomas Wolf <tw...@apache.org>
AuthorDate: Wed Nov 9 12:25:20 2022 +0100
Minor comment fix
---
.../src/main/java/org/apache/sshd/common/util/buffer/BufferUtils.java | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/BufferUtils.java b/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/BufferUtils.java
index cdb583153..b1c68a0fe 100644
--- a/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/BufferUtils.java
+++ b/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/BufferUtils.java
@@ -35,7 +35,7 @@ import org.apache.sshd.common.util.io.IoUtils;
import org.apache.sshd.common.util.logging.SimplifiedLog;
/**
- * TODO Add javadoc
+ * General utilities for working with byte-encoded data.
*
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
*/