This is an automated email from the ASF dual-hosted git repository. lgoldstein pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 644a80a5d8f54d19fd10949d8c54bf02d317c8d7 Author: Lyor Goldstein <lgoldst...@apache.org> AuthorDate: Mon May 4 17:26:55 2020 +0300 [SSHD-984] Fixed some minor coding issues to make OpenSSHKeyPairResourceWriter conform to SSHD style --- CHANGES.md | 4 +- .../common/config/keys/PrivateKeyEntryDecoder.java | 18 +- .../config/keys/writer/KeyPairResourceWriter.java | 66 ++--- .../openssh/OpenSSHKeyEncryptionContext.java | 81 +----- .../openssh/OpenSSHKeyPairResourceWriter.java | 136 +++++----- .../util/io/SecureByteArrayOutputStream.java | 4 +- .../OpenSSHEd25519PrivateKeyEntryDecoder.java | 3 +- .../openssh/OpenSSHKeyPairResourceWriterTest.java | 291 +++++++++++---------- .../apache/sshd/util/test/JUnitTestSupport.java | 27 +- 9 files changed, 291 insertions(+), 339 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 7022e45..0fbc103 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -33,4 +33,6 @@ where the former validate the messages and deal with the idle timeout, and the l * [SSHD-977](https://issues.apache.org/jira/browse/SSHD-977) - Apply consistent logging policy to caught exceptions -* [SSHD-660](https://issues.apache.org/jira/browse/SSHD-660) - Added support for server-side signed certificate keys \ No newline at end of file +* [SSHD-660](https://issues.apache.org/jira/browse/SSHD-660) - Added support for server-side signed certificate keys + +* [SSHD-984](https://issues.apache.org/jira/browse/SSHD-984) - Utility method to export KeyPair in OpenSSH format \ No newline at end of file diff --git a/sshd-common/src/main/java/org/apache/sshd/common/config/keys/PrivateKeyEntryDecoder.java b/sshd-common/src/main/java/org/apache/sshd/common/config/keys/PrivateKeyEntryDecoder.java index 4deccc8..0e61acc 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/config/keys/PrivateKeyEntryDecoder.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/config/keys/PrivateKeyEntryDecoder.java @@ -22,7 +22,6 @@ package org.apache.sshd.common.config.keys; import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; -import java.io.OutputStream; import java.io.StreamCorruptedException; import java.security.GeneralSecurityException; import java.security.PrivateKey; @@ -122,16 +121,15 @@ public interface PrivateKeyEntryDecoder<PUB extends PublicKey, PRV extends Priva throws IOException, GeneralSecurityException; /** - * Encodes the {@link PrivateKey} using the {@code OpenSSH} format - same one - * used by the {@code decodePublicKey} method(s) + * Encodes the {@link PrivateKey} using the {@code OpenSSH} format - same one used by the {@code decodePublicKey} + * method(s) * - * @param s The {@link SecureByteArrayOutputStream} to write the data to. - * @param key The {@link PrivateKey} - may not be {@code null} - * @param pubKey The {@link PublicKey} belonging to the private key - must be - * non-{@code null} if {@link #isPublicKeyRecoverySupported() - * public key recovery} is not supported - * @return The key type value - one of the {@link #getSupportedKeyTypes()} or - * {@code null} if encoding not supported + * @param s The {@link SecureByteArrayOutputStream} to write the data to. + * @param key The {@link PrivateKey} - may not be {@code null} + * @param pubKey The {@link PublicKey} belonging to the private key - must be non-{@code null} if + * {@link #isPublicKeyRecoverySupported() public key recovery} is not supported + * @return The key type value - one of the {@link #getSupportedKeyTypes()} or {@code null} if encoding + * not supported * @throws IOException If failed to generate the encoding */ default String encodePrivateKey(SecureByteArrayOutputStream s, PRV key, PUB pubKey) throws IOException { diff --git a/sshd-common/src/main/java/org/apache/sshd/common/config/keys/writer/KeyPairResourceWriter.java b/sshd-common/src/main/java/org/apache/sshd/common/config/keys/writer/KeyPairResourceWriter.java index c1be690..9201aea 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/config/keys/writer/KeyPairResourceWriter.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/config/keys/writer/KeyPairResourceWriter.java @@ -26,59 +26,59 @@ import java.security.KeyPair; import java.security.PublicKey; import org.apache.sshd.common.config.keys.loader.PrivateKeyEncryptionContext; -import org.apache.sshd.common.util.io.SecureByteArrayOutputStream; /** - * A {@code KeyPairResourceWriter} can serialize keys to an external - * representation. + * A {@code KeyPairResourceWriter} can serialize keys to an external representation. * - * @param <OPTIONS> The type of {@link PrivateKeyEncryptionContext} to use with - * this {@code KeyPairResourceWriter}. + * @param <OPTIONS> The type of {@link PrivateKeyEncryptionContext} to use with this {@code KeyPairResourceWriter}. */ public interface KeyPairResourceWriter<OPTIONS extends PrivateKeyEncryptionContext> { - /** - * Writes a serialization of a private key from a given {@link KeyPair} to a - * given {@link SecureByteArrayOutputStream}. + * Writes a serialization of a private key from a given {@link KeyPair} to a given {@link OutputStream}. * - * @param key to write the private key of - * @param comment to write with the private key - * @param options for writing the key; may be {@code null} if no encryption is - * wanted. The caller is responsible for - * {@link PrivateKeyEncryptionContext#clear() clearing} the - * options when no longer needed. If the passphrase obtained from - * the context is {@code null} or an empty array (length zero or - * containing only whitespace), the key is written unencrypted. - * @param out to write to - * @return a byte array containing the serialized private key data - * @throws GeneralSecurityException if the key is inconsistent or unknown, or - * the encryption specified cannot be applied + * @param key to write the private key of + * @param comment to write with the private key + * @param options for writing the key; may be {@code null} if no encryption is wanted. The caller + * is responsible for clearing the options when no longer needed. If the passphrase + * obtained from the context is {@code null} or an empty/blank string (length zero + * or containing only whitespace), the key is written unencrypted. + * @param out The {@link OutputStream} to write to - recommend using a + * {@code SecureByteArrayOutputStream} in order to reduce sensitive data exposure + * in memory + * @throws GeneralSecurityException if the key is inconsistent or unknown, or the encryption specified cannot be + * applied * @throws IOException if the key cannot be written */ - void writePrivateKey(KeyPair key, String comment, OPTIONS options, SecureByteArrayOutputStream out) + void writePrivateKey(KeyPair key, String comment, OPTIONS options, OutputStream out) throws IOException, GeneralSecurityException; /** - * Writes a serialization of a public key from a given {@link KeyPair} to a - * given {@link OutputStream}. + * Writes a serialization of a public key from a given {@link KeyPair} to a given {@link OutputStream}. * - * @param key to write the public key of - * @param comment to write with the public key - * @param out to write to + * @param key to write the public key of + * @param comment to write with the public key + * @param out The {@link OutputStream} to write to - recommend using a + * {@code SecureByteArrayOutputStream} in order to reduce sensitive data exposure + * in memory * @throws GeneralSecurityException if the key is unknown * @throws IOException if the key cannot be written */ - void writePublicKey(KeyPair key, String comment, OutputStream out) throws IOException, GeneralSecurityException; + default void writePublicKey(KeyPair key, String comment, OutputStream out) + throws IOException, GeneralSecurityException { + writePublicKey(key.getPublic(), comment, out); + } /** - * Writes a serialization of a {@link PublicKey} to a given - * {@link OutputStream}. + * Writes a serialization of a {@link PublicKey} to a given {@link OutputStream}. * - * @param key to write - * @param comment to write with the key - * @param out to write to + * @param key to write + * @param comment to write with the key + * @param out The {@link OutputStream} to write to - recommend using a + * {@code SecureByteArrayOutputStream} in order to reduce sensitive data exposure + * in memory * @throws GeneralSecurityException if the key is unknown * @throws IOException if the key cannot be written */ - void writePublicKey(PublicKey key, String comment, OutputStream out) throws IOException, GeneralSecurityException; + void writePublicKey(PublicKey key, String comment, OutputStream out) + throws IOException, GeneralSecurityException; } diff --git a/sshd-common/src/main/java/org/apache/sshd/common/config/keys/writer/openssh/OpenSSHKeyEncryptionContext.java b/sshd-common/src/main/java/org/apache/sshd/common/config/keys/writer/openssh/OpenSSHKeyEncryptionContext.java index 78313a6..477211a 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/config/keys/writer/openssh/OpenSSHKeyEncryptionContext.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/config/keys/writer/openssh/OpenSSHKeyEncryptionContext.java @@ -19,28 +19,22 @@ package org.apache.sshd.common.config.keys.writer.openssh; -import java.util.Arrays; - import org.apache.sshd.common.config.keys.loader.PrivateKeyEncryptionContext; import org.apache.sshd.common.util.ValidateUtils; /** - * A {@link PrivateKeyEncryptionContext} for use with a - * {@link OpenSSHKeyPairResourceWriter}. + * A {@link PrivateKeyEncryptionContext} for use with a {@link OpenSSHKeyPairResourceWriter}. */ public class OpenSSHKeyEncryptionContext extends PrivateKeyEncryptionContext { /** Default number of bcrypt KDF rounds to apply. */ public static final int DEFAULT_KDF_ROUNDS = 16; - private static final String AES = "AES"; - - private char[] passphrase; + public static final String AES = "AES"; private int kdfRounds = DEFAULT_KDF_ROUNDS; public OpenSSHKeyEncryptionContext() { - super(); setCipherMode("CTR"); // Set default to CTR, as in OpenSSH } @@ -51,58 +45,11 @@ public class OpenSSHKeyEncryptionContext extends PrivateKeyEncryptionContext { @Override public void setCipherName(String value) { - ValidateUtils.checkTrue(value != null && value.equalsIgnoreCase(AES), + ValidateUtils.checkTrue((value != null) && value.equalsIgnoreCase(AES), "OpenSSHKeyEncryptionContext works only with AES encryption"); } /** - * {@inheritDoc} - * - * @deprecated Use {@link #getPassphrase()} instead - */ - @Deprecated - @Override - public String getPassword() { - return passphrase == null ? null : new String(passphrase); - } - - /** - * {@inheritDoc} - * - * @deprecated Use {@link #setPassphrase()} instead - */ - @Deprecated - @Override - public void setPassword(String value) { - setPassphrase(value.toCharArray()); - } - - /** - * Retrieves a <em>copy</em> of the internally stored passphrase. The caller is - * responsible for clearing the returned array when it is no longer needed. - * - * @return the passphrase - */ - public char[] getPassphrase() { - return passphrase == null ? null : passphrase.clone(); - } - - /** - * Stores a <em>copy</em> of the passphrase. The caller is responsible for - * eventually clearing the array passed as an argument, and for {@link #clear() - * clearing} this {@link OpenSSHKeyEncryptionContext} once it is no longer - * needed. - * - * @param passphrase to store - */ - public void setPassphrase(char[] passphrase) { - if (this.passphrase != null) { - Arrays.fill(this.passphrase, '\000'); - } - this.passphrase = passphrase == null ? null : passphrase.clone(); - } - - /** * Retrieves the number of KDF rounds to apply. * * @return the default number of KDF rounds, >= {@link #DEFAULT_KDF_ROUNDS} @@ -112,8 +59,7 @@ public class OpenSSHKeyEncryptionContext extends PrivateKeyEncryptionContext { } /** - * Sets the number of KDF rounds to apply. If smaller than the - * {@link #DEFAULT_KDF_ROUNDS}, set that default. + * Sets the number of KDF rounds to apply. If smaller than the {@link #DEFAULT_KDF_ROUNDS}, set that default. * * @param rounds number of rounds to apply */ @@ -122,26 +68,9 @@ public class OpenSSHKeyEncryptionContext extends PrivateKeyEncryptionContext { } /** - * Retrieves the cipher's factory name. - * - * @return the name + * @return the cipher's factory name. */ protected String getCipherFactoryName() { return getCipherName().toLowerCase() + getCipherType() + '-' + getCipherMode().toLowerCase(); } - - /** - * Clears internal sensitive data (the password and the init vector). - */ - public void clear() { - if (this.passphrase != null) { - Arrays.fill(this.passphrase, '\000'); - this.passphrase = null; - } - byte[] iv = getInitVector(); - if (iv != null) { - Arrays.fill(iv, (byte) 0); - } - } - } diff --git a/sshd-common/src/main/java/org/apache/sshd/common/config/keys/writer/openssh/OpenSSHKeyPairResourceWriter.java b/sshd-common/src/main/java/org/apache/sshd/common/config/keys/writer/openssh/OpenSSHKeyPairResourceWriter.java index bc8ae46..6251778 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/config/keys/writer/openssh/OpenSSHKeyPairResourceWriter.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/config/keys/writer/openssh/OpenSSHKeyPairResourceWriter.java @@ -48,28 +48,32 @@ import org.apache.sshd.common.config.keys.loader.PrivateKeyEncryptionContext; import org.apache.sshd.common.config.keys.loader.openssh.OpenSSHKeyPairResourceParser; import org.apache.sshd.common.config.keys.loader.openssh.OpenSSHParserContext; import org.apache.sshd.common.config.keys.loader.openssh.kdf.BCrypt; +import org.apache.sshd.common.config.keys.loader.openssh.kdf.BCryptKdfOptions; import org.apache.sshd.common.config.keys.writer.KeyPairResourceWriter; +import org.apache.sshd.common.util.GenericUtils; import org.apache.sshd.common.util.ValidateUtils; import org.apache.sshd.common.util.io.SecureByteArrayOutputStream; /** - * A {@link KeyPairResourceWriter} for writing keys in the modern OpenSSH format, using - * the OpenBSD bcrypt KDF for passphrase-protected encrypted private keys. + * A {@link KeyPairResourceWriter} for writing keys in the modern OpenSSH format, using the OpenBSD bcrypt KDF for + * passphrase-protected encrypted private keys. */ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenSSHKeyEncryptionContext> { - private static final Pattern VERTICALSPACE = Pattern.compile("\\v"); //$NON-NLS-1$ + public static final String DASHES = "-----"; //$NON-NLS-1$ + + public static final int LINE_LENGTH = 70; - private static final String DASHES = "-----"; //$NON-NLS-1$ + public static final OpenSSHKeyPairResourceWriter INSTANCE = new OpenSSHKeyPairResourceWriter(); - private static final int LINE_LENGTH = 70; + private static final Pattern VERTICALSPACE = Pattern.compile("\\v"); //$NON-NLS-1$ public OpenSSHKeyPairResourceWriter() { - super(); + super(); } @Override - public void writePrivateKey(KeyPair key, String comment, OpenSSHKeyEncryptionContext options, SecureByteArrayOutputStream out) + public void writePrivateKey(KeyPair key, String comment, OpenSSHKeyEncryptionContext options, OutputStream out) throws IOException, GeneralSecurityException { ValidateUtils.checkNotNull(key, "Cannot write null key"); String keyType = KeyUtils.getKeyType(key); @@ -94,22 +98,19 @@ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenS } byte[] privateBytes = encodePrivateKey(key, keyType, blockSize, comment); String kdfName = OpenSSHParserContext.NONE_CIPHER; - byte[] kdfOptions = new byte[0]; + byte[] kdfOptions = GenericUtils.EMPTY_BYTE_ARRAY; try (SecureByteArrayOutputStream bytes = new SecureByteArrayOutputStream()) { write(bytes, OpenSSHKeyPairResourceParser.AUTH_MAGIC); bytes.write(0); if (opt != null) { KeyEncryptor encryptor = new KeyEncryptor(opt); opt.setPrivateKeyObfuscator(encryptor); - try { - byte[] encodedBytes = encryptor.applyPrivateKeyCipher(privateBytes, opt, true); - Arrays.fill(privateBytes, (byte) 0); - privateBytes = encodedBytes; - kdfName = "bcrypt"; //$NON-NLS-1$ - kdfOptions = encryptor.getKdfOptions(); - } finally { - opt.clear(); - } + + byte[] encodedBytes = encryptor.applyPrivateKeyCipher(privateBytes, opt, true); + Arrays.fill(privateBytes, (byte) 0); + privateBytes = encodedBytes; + kdfName = BCryptKdfOptions.NAME; + kdfOptions = encryptor.getKdfOptions(); } KeyEntryResolver.encodeString(bytes, cipherName); KeyEntryResolver.encodeString(bytes, kdfName); @@ -125,27 +126,23 @@ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenS out.write('\n'); } - private static OpenSSHKeyEncryptionContext determineEncryption(OpenSSHKeyEncryptionContext options) { - if (options == null) { - return null; - } - char[] passphrase = options.getPassphrase(); - if (passphrase == null) { + public static OpenSSHKeyEncryptionContext determineEncryption(OpenSSHKeyEncryptionContext options) { + CharSequence password = (options == null) ? null : options.getPassword(); + if (GenericUtils.isEmpty(password)) { return null; } - try { - for (char ch : passphrase) { - if (!Character.isWhitespace(ch)) { - return options; - } + + for (int pos = 0, len = password.length(); pos < len; pos++) { + char ch = password.charAt(pos); + if (!Character.isWhitespace(ch)) { + return options; } - } finally { - Arrays.fill(passphrase, '\000'); } + return null; } - private static byte[] encodePrivateKey(KeyPair key, String keyType, int blockSize, String comment) + public static byte[] encodePrivateKey(KeyPair key, String keyType, int blockSize, String comment) throws IOException, GeneralSecurityException { try (SecureByteArrayOutputStream out = new SecureByteArrayOutputStream()) { int check = new SecureRandom().nextInt(); @@ -153,8 +150,9 @@ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenS KeyEntryResolver.encodeInt(out, check); KeyEntryResolver.encodeString(out, keyType); @SuppressWarnings("unchecked") // Problem with generics - PrivateKeyEntryDecoder<PublicKey, PrivateKey> encoder = (PrivateKeyEntryDecoder<PublicKey, PrivateKey>) OpenSSHKeyPairResourceParser - .getPrivateKeyEntryDecoder(keyType); + PrivateKeyEntryDecoder<PublicKey, PrivateKey> encoder + = (PrivateKeyEntryDecoder<PublicKey, PrivateKey>) OpenSSHKeyPairResourceParser + .getPrivateKeyEntryDecoder(keyType); if (encoder.encodePrivateKey(out, key.getPrivate(), key.getPublic()) == null) { throw new GeneralSecurityException("Cannot encode key of type " + keyType); } @@ -173,10 +171,11 @@ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenS } } - private static byte[] encodePublicKey(PublicKey key, String keyType) throws IOException, GeneralSecurityException { + public static byte[] encodePublicKey(PublicKey key, String keyType) + throws IOException, GeneralSecurityException { @SuppressWarnings("unchecked") // Problem with generics. - PublicKeyEntryDecoder<PublicKey, ?> decoder = (PublicKeyEntryDecoder<PublicKey, ?>) KeyUtils - .getPublicKeyEntryDecoder(keyType); + PublicKeyEntryDecoder<PublicKey, ?> decoder + = (PublicKeyEntryDecoder<PublicKey, ?>) KeyUtils.getPublicKeyEntryDecoder(keyType); if (decoder == null) { throw new GeneralSecurityException("Unknown key type: " + keyType); } @@ -186,7 +185,7 @@ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenS } } - private static void write(OutputStream out, byte[] bytes, int lineLength) throws IOException { + public static void write(OutputStream out, byte[] bytes, int lineLength) throws IOException { byte[] encoded = Base64.getEncoder().encode(bytes); Arrays.fill(bytes, (byte) 0); int last = encoded.length; @@ -204,22 +203,8 @@ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenS /** * {@inheritDoc} * - * Writes the public key in the single-line OpenSSH format "key-type pub-key - * comment" without terminating line ending. If the comment has multiple lines, - * only the first line is written. - */ - @Override - public void writePublicKey(KeyPair key, String comment, OutputStream out) - throws IOException, GeneralSecurityException { - writePublicKey(key.getPublic(), comment, out); - } - - /** - * {@inheritDoc} - * - * Writes the public key in the single-line OpenSSH format "key-type pub-key - * comment" without terminating line ending. If the comment has multiple lines, - * only the first line is written. + * Writes the public key in the single-line OpenSSH format "key-type pub-key comment" without terminating line + * ending. If the comment has multiple lines, only the first line is written. */ @Override public void writePublicKey(PublicKey key, String comment, OutputStream out) @@ -236,7 +221,7 @@ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenS write(out, b.toString()); } - private static String firstLine(String text) { + public static String firstLine(String text) { Matcher m = VERTICALSPACE.matcher(text); if (m.find()) { return text.substring(0, m.start()); @@ -244,18 +229,17 @@ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenS return text; } - private static void write(OutputStream out, String s) throws IOException { + public static void write(OutputStream out, String s) throws IOException { out.write(s.getBytes(StandardCharsets.UTF_8)); } /** * A key encryptor for modern-style OpenSSH private keys using the bcrypt KDF. */ - private static class KeyEncryptor extends AESPrivateKeyObfuscator { - - private static final int BCRYPT_SALT_LENGTH = 16; + public static class KeyEncryptor extends AESPrivateKeyObfuscator { + public static final int BCRYPT_SALT_LENGTH = 16; - private final OpenSSHKeyEncryptionContext options; + protected final OpenSSHKeyEncryptionContext options; private byte[] kdfOptions; @@ -265,8 +249,7 @@ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenS /** * Retrieves the KDF options used. Valid only after - * {@link #deriveEncryptionKey(PrivateKeyEncryptionContext, int)} has been - * called. + * {@link #deriveEncryptionKey(PrivateKeyEncryptionContext, int)} has been called. * * @return the number of KDF rounds applied */ @@ -275,13 +258,13 @@ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenS } /** - * Derives an encryption key and set the IV on the {@code context} from the - * passphase provided by the context using the OpenBSD {@link BCrypt} KDF. + * Derives an encryption key and set the IV on the {@code context} from the passphase provided by the context + * using the OpenBSD {@link BCrypt} KDF. * - * @param context for the encryption, provides the passphrase and transports - * other encryption-related information including the IV - * @param keyLength number of key bytes to generate - * @return {@code keyLength} bytes to use as encryption key + * @param context for the encryption, provides the passphrase and transports other encryption-related + * information including the IV + * @param keyLength number of key bytes to generate + * @return {@code keyLength} bytes to use as encryption key */ @Override protected byte[] deriveEncryptionKey(PrivateKeyEncryptionContext context, int keyLength) @@ -300,7 +283,7 @@ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenS byte[] result = null; // "kdf" collects the salt and number of rounds; not sensitive data. try (ByteArrayOutputStream kdf = new ByteArrayOutputStream()) { - pwd = convert(options.getPassphrase()); + pwd = convert(options.getPassword()); bcrypt.pbkdf(pwd, salt, rounds, kdfOutput); KeyEntryResolver.writeRLEBytes(kdf, salt); KeyEntryResolver.encodeInt(kdf, rounds); @@ -318,17 +301,24 @@ public class OpenSSHKeyPairResourceWriter implements KeyPairResourceWriter<OpenS return result; } - private byte[] convert(char[] pass) { - if (pass == null) { - return new byte[0]; + protected byte[] convert(String password) { + if (GenericUtils.isEmpty(password)) { + return GenericUtils.EMPTY_BYTE_ARRAY; + } + + char[] pass = password.toCharArray(); + ByteBuffer bytes; + try { + bytes = StandardCharsets.UTF_8.encode(CharBuffer.wrap(pass)); + } finally { + Arrays.fill(pass, '\0'); } - ByteBuffer bytes = StandardCharsets.UTF_8.encode(CharBuffer.wrap(pass)); + byte[] pwd = new byte[bytes.remaining()]; bytes.get(pwd); if (bytes.hasArray()) { Arrays.fill(bytes.array(), (byte) 0); } - Arrays.fill(pass, '\000'); return pwd; } } diff --git a/sshd-common/src/main/java/org/apache/sshd/common/util/io/SecureByteArrayOutputStream.java b/sshd-common/src/main/java/org/apache/sshd/common/util/io/SecureByteArrayOutputStream.java index a1e9d44..3cd073f 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/util/io/SecureByteArrayOutputStream.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/util/io/SecureByteArrayOutputStream.java @@ -23,8 +23,8 @@ import java.io.ByteArrayOutputStream; import java.util.Arrays; /** - * A {@link ByteArrayOutputStream} that clears its internal buffer after - * resizing and when it is {@link #close() closed}. + * A {@link ByteArrayOutputStream} that clears its internal buffer after resizing and when it is {@link #close() + * closed}. */ public final class SecureByteArrayOutputStream extends ByteArrayOutputStream { diff --git a/sshd-common/src/main/java/org/apache/sshd/common/util/security/eddsa/OpenSSHEd25519PrivateKeyEntryDecoder.java b/sshd-common/src/main/java/org/apache/sshd/common/util/security/eddsa/OpenSSHEd25519PrivateKeyEntryDecoder.java index e9becf4..5e72064 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/util/security/eddsa/OpenSSHEd25519PrivateKeyEntryDecoder.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/util/security/eddsa/OpenSSHEd25519PrivateKeyEntryDecoder.java @@ -130,7 +130,8 @@ public class OpenSSHEd25519PrivateKeyEntryDecoder extends AbstractPrivateKeyEntr } @Override - public String encodePrivateKey(SecureByteArrayOutputStream s, EdDSAPrivateKey key, EdDSAPublicKey pubKey) throws IOException { + public String encodePrivateKey(SecureByteArrayOutputStream s, EdDSAPrivateKey key, EdDSAPublicKey pubKey) + throws IOException { Objects.requireNonNull(key, "No private key provided"); // ed25519 bernstein naming: pk .. public key, sk .. secret key diff --git a/sshd-common/src/test/java/org/apache/sshd/common/config/keys/writer/openssh/OpenSSHKeyPairResourceWriterTest.java b/sshd-common/src/test/java/org/apache/sshd/common/config/keys/writer/openssh/OpenSSHKeyPairResourceWriterTest.java index f52f054..1cce6d6 100644 --- a/sshd-common/src/test/java/org/apache/sshd/common/config/keys/writer/openssh/OpenSSHKeyPairResourceWriterTest.java +++ b/sshd-common/src/test/java/org/apache/sshd/common/config/keys/writer/openssh/OpenSSHKeyPairResourceWriterTest.java @@ -19,14 +19,6 @@ package org.apache.sshd.common.config.keys.writer.openssh; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertThrows; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; - -import java.io.File; -import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -34,6 +26,7 @@ import java.io.StreamCorruptedException; import java.nio.ByteBuffer; import java.nio.channels.ByteChannel; import java.nio.file.Files; +import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.security.GeneralSecurityException; import java.security.KeyPair; @@ -43,9 +36,12 @@ import java.security.spec.AlgorithmParameterSpec; import java.security.spec.ECGenParameterSpec; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.List; import java.util.Objects; +import net.i2p.crypto.eddsa.EdDSAKey; +import net.i2p.crypto.eddsa.spec.EdDSAGenParameterSpec; import org.apache.sshd.common.config.keys.AuthorizedKeyEntry; import org.apache.sshd.common.config.keys.FilePasswordProvider; import org.apache.sshd.common.config.keys.KeyUtils; @@ -53,86 +49,71 @@ import org.apache.sshd.common.config.keys.PublicKeyEntryResolver; import org.apache.sshd.common.util.io.SecureByteArrayOutputStream; import org.apache.sshd.common.util.io.resource.PathResource; import org.apache.sshd.common.util.security.SecurityUtils; +import org.apache.sshd.util.test.JUnit4ClassRunnerWithParametersFactory; +import org.apache.sshd.util.test.JUnitTestSupport; +import org.apache.sshd.util.test.NoIoTestCase; import org.junit.Before; +import org.junit.FixMethodOrder; import org.junit.Rule; import org.junit.Test; +import org.junit.experimental.categories.Category; import org.junit.rules.TemporaryFolder; import org.junit.runner.RunWith; +import org.junit.runners.MethodSorters; import org.junit.runners.Parameterized; -import org.junit.runners.Parameterized.Parameter; import org.junit.runners.Parameterized.Parameters; +import org.junit.runners.Parameterized.UseParametersRunnerFactory; -import net.i2p.crypto.eddsa.EdDSAKey; -import net.i2p.crypto.eddsa.spec.EdDSAGenParameterSpec; - -@RunWith(Parameterized.class) -public class OpenSSHKeyPairResourceWriterTest { - +@FixMethodOrder(MethodSorters.NAME_ASCENDING) +@RunWith(Parameterized.class) // see https://github.com/junit-team/junit/wiki/Parameterized-tests +@UseParametersRunnerFactory(JUnit4ClassRunnerWithParametersFactory.class) +@Category({ NoIoTestCase.class }) +public class OpenSSHKeyPairResourceWriterTest extends JUnitTestSupport { @Rule public TemporaryFolder folder = new TemporaryFolder(); - private static class TestData { - public final String algorithm; - - public final String provider; - - public final int keySize; - - public final AlgorithmParameterSpec spec; - - public TestData(String algorithm, int keySize, - AlgorithmParameterSpec spec) { - this(algorithm, null, keySize, spec); - } - - public TestData(String algorithm, String provider, int keySize, - AlgorithmParameterSpec spec) { - this.algorithm = algorithm; - this.provider = provider; - this.keySize = keySize; - this.spec = spec; - } + private final TestData data; + private KeyPair testKey; - @Override - public String toString() { - return algorithm + '-' + keySize - + (provider == null ? "" : '(' + provider + ')'); - } + public OpenSSHKeyPairResourceWriterTest(TestData data) { + this.data = data; } @Parameters(name = "{0}") - public static Object[] getParams() { + public static Collection<Object[]> parameters() { List<TestData> result = new ArrayList<>(); result.add(new TestData("RSA", 1024, null)); result.add(new TestData("RSA", 2048, null)); result.add(new TestData("DSA", 1024, null)); - result.add( - new TestData("ECDSA", 256, - new ECGenParameterSpec("secp256r1"))); - result.add( - new TestData("ECDSA", 384, - new ECGenParameterSpec("secp384r1"))); - result.add( - new TestData("ECDSA", 521, - new ECGenParameterSpec("secp521r1"))); - result.add(new TestData("EDDSA", "EdDSA", 25519, - new EdDSAGenParameterSpec("Ed25519"))); - // Note: BC also has an EDDSA provider, but that one returns - // "Ed25519" as algorithm from its keys, while the one in - // net.i2p.crypto.eddsa gives keys with "EDDSA" as algorithm. - // sshd handles only the latter. - return result.toArray(); - } - - @Parameter - public TestData data; - - private KeyPair testKey; + if (SecurityUtils.isECCSupported()) { + result.add( + new TestData( + "ECDSA", 256, + new ECGenParameterSpec("secp256r1"))); + result.add( + new TestData( + "ECDSA", 384, + new ECGenParameterSpec("secp384r1"))); + result.add( + new TestData( + "ECDSA", 521, + new ECGenParameterSpec("secp521r1"))); + } + if (SecurityUtils.isEDDSACurveSupported()) { + // Note: BC also has an EDDSA provider, but that one returns + // "Ed25519" as algorithm from its keys, while the one in + // net.i2p.crypto.eddsa gives keys with "EDDSA" as algorithm. + // sshd handles only the latter. + result.add(new TestData( + "EDDSA", "EdDSA", 25519, + new EdDSAGenParameterSpec("Ed25519"))); + } - private OpenSSHKeyPairResourceWriter writer = new OpenSSHKeyPairResourceWriter(); + return parameterize(result); + } @Before - public void generateKey() throws Exception { + public void setUp() throws Exception { KeyPairGenerator generator; if (data.provider == null) { generator = KeyPairGenerator.getInstance(data.algorithm); @@ -161,9 +142,9 @@ public class OpenSSHKeyPairResourceWriterTest { return KeyUtils.compareKeyPairs(a, b); } - private static void writeToFile(File file, byte[] sensitiveData) + private static void writeToFile(Path file, byte[] sensitiveData) throws IOException { - try (ByteChannel out = Files.newByteChannel(file.toPath(), + try (ByteChannel out = Files.newByteChannel(file, StandardOpenOption.CREATE, StandardOpenOption.WRITE)) { ByteBuffer buf = ByteBuffer.wrap(sensitiveData); while (buf.hasRemaining()) { @@ -175,103 +156,99 @@ public class OpenSSHKeyPairResourceWriterTest { } @Test - public void privateNoEncryption() throws Exception { - File tmp = folder.newFile(); + public void testWritePrivateKeyNoEncryption() throws Exception { + Path tmp = getTemporaryOutputFile(); try (SecureByteArrayOutputStream out = new SecureByteArrayOutputStream()) { - writer.writePrivateKey(testKey, "a comment", null, out); + OpenSSHKeyPairResourceWriter.INSTANCE.writePrivateKey(testKey, "a comment", null, out); writeToFile(tmp, out.toByteArray()); } - try (InputStream in = Files.newInputStream(tmp.toPath())) { + try (InputStream in = Files.newInputStream(tmp)) { KeyPair key = SecurityUtils.loadKeyPairIdentities(null, - new PathResource(tmp.toPath()), in, null).iterator().next(); - assertNotNull(key); + new PathResource(tmp), in, null).iterator().next(); + assertNotNull("No key pair parsed", key); + assertKeyPairEquals("Mismatched recovered keys", testKey, key); assertTrue("Keys should be equal", compare(key, testKey)); } } @Test - public void privateNoPassword() throws Exception { - File tmp = folder.newFile(); - OpenSSHKeyEncryptionContext options = new OpenSSHKeyEncryptionContext(); + public void testWritePrivateKeyNoPassword() throws Exception { + Path tmp = getTemporaryOutputFile(); try (SecureByteArrayOutputStream out = new SecureByteArrayOutputStream()) { - writer.writePrivateKey(testKey, "a comment", options, out); + OpenSSHKeyEncryptionContext options = new OpenSSHKeyEncryptionContext(); + OpenSSHKeyPairResourceWriter.INSTANCE.writePrivateKey(testKey, "a comment", options, out); writeToFile(tmp, out.toByteArray()); - } finally { - options.clear(); } - try (InputStream in = Files.newInputStream(tmp.toPath())) { + try (InputStream in = Files.newInputStream(tmp)) { KeyPair key = SecurityUtils.loadKeyPairIdentities(null, - new PathResource(tmp.toPath()), in, null).iterator().next(); - assertNotNull(key); + new PathResource(tmp), in, null).iterator().next(); + assertNotNull("No key pair parsed", key); + assertKeyPairEquals("Mismatched recovered keys", testKey, key); assertTrue("Keys should be equal", compare(key, testKey)); } } @Test - public void privateEncryptedAesCbc128() throws Exception { - File tmp = folder.newFile(); - OpenSSHKeyEncryptionContext options = new OpenSSHKeyEncryptionContext(); + public void testWritePrivateKeyEncryptedAesCbc128() throws Exception { + Path tmp = getTemporaryOutputFile(); try (SecureByteArrayOutputStream out = new SecureByteArrayOutputStream()) { - options.setPassphrase("nonsense".toCharArray()); + OpenSSHKeyEncryptionContext options = new OpenSSHKeyEncryptionContext(); + options.setPassword("nonsense"); options.setCipherName("AES"); options.setCipherMode("CBC"); options.setCipherType("128"); - writer.writePrivateKey(testKey, "a comment", options, out); + OpenSSHKeyPairResourceWriter.INSTANCE.writePrivateKey(testKey, "a comment", options, out); writeToFile(tmp, out.toByteArray()); - } finally { - options.clear(); } - try (InputStream in = Files.newInputStream(tmp.toPath())) { + try (InputStream in = Files.newInputStream(tmp)) { KeyPair key = SecurityUtils - .loadKeyPairIdentities(null, new PathResource(tmp.toPath()), + .loadKeyPairIdentities(null, new PathResource(tmp), in, FilePasswordProvider.of("nonsense")) .iterator().next(); - assertNotNull(key); + assertNotNull("No key pair parsed", key); + assertKeyPairEquals("Mismatched recovered keys", testKey, key); assertTrue("Keys should be equal", compare(key, testKey)); } } @Test - public void privateEncryptedAesCtr256() throws Exception { - File tmp = folder.newFile(); - OpenSSHKeyEncryptionContext options = new OpenSSHKeyEncryptionContext(); + public void testWritePrivateKeyEncryptedAesCtr256() throws Exception { + Path tmp = getTemporaryOutputFile(); try (SecureByteArrayOutputStream out = new SecureByteArrayOutputStream()) { - options.setPassphrase("nonsense".toCharArray()); + OpenSSHKeyEncryptionContext options = new OpenSSHKeyEncryptionContext(); + options.setPassword("nonsense"); options.setCipherName("AES"); options.setCipherMode("CTR"); options.setCipherType("256"); - writer.writePrivateKey(testKey, "a comment", options, out); + OpenSSHKeyPairResourceWriter.INSTANCE.writePrivateKey(testKey, "a comment", options, out); writeToFile(tmp, out.toByteArray()); - } finally { - options.clear(); } - try (InputStream in = Files.newInputStream(tmp.toPath())) { + try (InputStream in = Files.newInputStream(tmp)) { KeyPair key = SecurityUtils - .loadKeyPairIdentities(null, new PathResource(tmp.toPath()), + .loadKeyPairIdentities(null, new PathResource(tmp), in, FilePasswordProvider.of("nonsense")) .iterator().next(); - assertNotNull(key); + assertNotNull("No key pair parsed", key); + assertKeyPairEquals("Mismatched recovered keys", testKey, key); assertTrue("Keys should be equal", compare(key, testKey)); } } @Test - public void privateEncryptedWrongPassword() throws Exception { - File tmp = folder.newFile(); - OpenSSHKeyEncryptionContext options = new OpenSSHKeyEncryptionContext(); + public void testWritePrivateKeyEncryptedWrongPassword() throws Exception { + Path tmp = getTemporaryOutputFile(); try (SecureByteArrayOutputStream out = new SecureByteArrayOutputStream()) { - options.setPassphrase("nonsense".toCharArray()); + OpenSSHKeyEncryptionContext options = new OpenSSHKeyEncryptionContext(); + options.setPassword("nonsense"); options.setCipherName("AES"); options.setCipherMode("CTR"); options.setCipherType("256"); - writer.writePrivateKey(testKey, "a comment", options, out); + OpenSSHKeyPairResourceWriter.INSTANCE.writePrivateKey(testKey, "a comment", options, out); writeToFile(tmp, out.toByteArray()); - } finally { - options.clear(); } - try (InputStream in = Files.newInputStream(tmp.toPath())) { + try (InputStream in = Files.newInputStream(tmp)) { SecurityUtils.loadKeyPairIdentities(null, - new PathResource(tmp.toPath()), in, + new PathResource(tmp), in, FilePasswordProvider.of("wrong")); fail("Expected an exception"); } catch (StreamCorruptedException | GeneralSecurityException e) { @@ -280,29 +257,26 @@ public class OpenSSHKeyPairResourceWriterTest { } @Test - public void privateEncryptedNoPassword() throws Exception { - File tmp = folder.newFile(); - OpenSSHKeyEncryptionContext options = new OpenSSHKeyEncryptionContext(); + public void testWritePrivateKeyEncryptedNoPassword() throws Exception { + Path tmp = getTemporaryOutputFile(); try (SecureByteArrayOutputStream out = new SecureByteArrayOutputStream()) { - options.setPassphrase("nonsense".toCharArray()); + OpenSSHKeyEncryptionContext options = new OpenSSHKeyEncryptionContext(); + options.setPassword("nonsense"); options.setCipherName("AES"); options.setCipherMode("CTR"); options.setCipherType("256"); - writer.writePrivateKey(testKey, "a comment", options, out); + OpenSSHKeyPairResourceWriter.INSTANCE.writePrivateKey(testKey, "a comment", options, out); writeToFile(tmp, out.toByteArray()); - } finally { - options.clear(); } - try (InputStream in = Files.newInputStream(tmp.toPath())) { + try (InputStream in = Files.newInputStream(tmp)) { assertThrows(GeneralSecurityException.class, () -> SecurityUtils.loadKeyPairIdentities(null, - new PathResource(tmp.toPath()), in, null)); + new PathResource(tmp), in, null)); } } - private void checkPublicKey(File tmp, String comment) throws Exception { - List<AuthorizedKeyEntry> keysRead = AuthorizedKeyEntry - .readAuthorizedKeys(tmp.toPath()); + private void checkPublicKey(Path tmp, String comment) throws Exception { + List<AuthorizedKeyEntry> keysRead = AuthorizedKeyEntry.readAuthorizedKeys(tmp); assertEquals("Unexpected list size", 1, keysRead.size()); AuthorizedKeyEntry entry = keysRead.get(0); String readComment = entry.getComment(); @@ -318,41 +292,76 @@ public class OpenSSHKeyPairResourceWriterTest { } @Test - public void publicWithComment() throws Exception { - File tmp = folder.newFile(); - try (OutputStream out = new FileOutputStream(tmp)) { - writer.writePublicKey(testKey, "a comment", out); + public void testWritePublicKeyWithComment() throws Exception { + Path tmp = getTemporaryOutputFile(); + try (OutputStream out = Files.newOutputStream(tmp)) { + OpenSSHKeyPairResourceWriter.INSTANCE.writePublicKey(testKey, "a comment", out); } checkPublicKey(tmp, "a comment"); } @Test - public void publicWithMultilineComment() throws Exception { - File tmp = folder.newFile(); - try (OutputStream out = new FileOutputStream(tmp)) { - writer.writePublicKey(testKey, + public void testWritePublicKeyWithMultilineComment() throws Exception { + Path tmp = getTemporaryOutputFile(); + try (OutputStream out = Files.newOutputStream(tmp)) { + OpenSSHKeyPairResourceWriter.INSTANCE.writePublicKey(testKey, "a comment" + System.lineSeparator() + "second line", out); } assertEquals("Unexpected number of lines", 1, - Files.readAllLines(tmp.toPath()).size()); + Files.readAllLines(tmp).size()); checkPublicKey(tmp, "a comment"); } @Test - public void publicNoComment() throws Exception { - File tmp = folder.newFile(); - try (OutputStream out = new FileOutputStream(tmp)) { - writer.writePublicKey(testKey, null, out); + public void testWritePublicKeyNoComment() throws Exception { + Path tmp = getTemporaryOutputFile(); + try (OutputStream out = Files.newOutputStream(tmp)) { + OpenSSHKeyPairResourceWriter.INSTANCE.writePublicKey(testKey, null, out); } checkPublicKey(tmp, null); } @Test - public void publicEmptyComment() throws Exception { - File tmp = folder.newFile(); - try (OutputStream out = new FileOutputStream(tmp)) { - writer.writePublicKey(testKey, "", out); + public void testWritePublicKeyEmptyComment() throws Exception { + Path tmp = getTemporaryOutputFile(); + try (OutputStream out = Files.newOutputStream(tmp)) { + OpenSSHKeyPairResourceWriter.INSTANCE.writePublicKey(testKey, "", out); } checkPublicKey(tmp, null); } + + private Path getTemporaryOutputFile() throws IOException { + Path dir = assertHierarchyTargetFolderExists(getTempTargetFolder()); + return dir.resolve(getCurrentTestName()); + } + + @SuppressWarnings("checkstyle:VisibilityModifier") + private static class TestData { + public final String algorithm; + + public final String provider; + + public final int keySize; + + public final AlgorithmParameterSpec spec; + + TestData(String algorithm, int keySize, + AlgorithmParameterSpec spec) { + this(algorithm, null, keySize, spec); + } + + TestData(String algorithm, String provider, int keySize, + AlgorithmParameterSpec spec) { + this.algorithm = algorithm; + this.provider = provider; + this.keySize = keySize; + this.spec = spec; + } + + @Override + public String toString() { + return algorithm + '-' + keySize + + (provider == null ? "" : '(' + provider + ')'); + } + } } diff --git a/sshd-common/src/test/java/org/apache/sshd/util/test/JUnitTestSupport.java b/sshd-common/src/test/java/org/apache/sshd/util/test/JUnitTestSupport.java index e990c5b..55537e0 100644 --- a/sshd-common/src/test/java/org/apache/sshd/util/test/JUnitTestSupport.java +++ b/sshd-common/src/test/java/org/apache/sshd/util/test/JUnitTestSupport.java @@ -49,11 +49,14 @@ import java.util.Collection; import java.util.Collections; import java.util.Iterator; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Objects; import java.util.concurrent.TimeUnit; import java.util.function.BiPredicate; +import org.apache.sshd.common.config.keys.BuiltinIdentities; +import org.apache.sshd.common.config.keys.KeyUtils; import org.apache.sshd.common.keyprovider.KeyPairProvider; import org.apache.sshd.common.util.GenericUtils; import org.apache.sshd.common.util.io.IoUtils; @@ -404,12 +407,23 @@ public abstract class JUnitTestSupport extends Assert { assertKeyEquals(message + "[private]", expected.getPrivate(), actual.getPrivate()); } + public static void assertKeyEncodingEquals(String message, Key expected, Key actual) { + if (expected == actual) { + return; + } + + assertEquals(message + "[format]", expected.getFormat(), actual.getFormat()); + assertArrayEquals(message + "[encoded-data]", expected.getEncoded(), actual.getEncoded()); + } + public static <T extends Key> void assertKeyEquals(String message, T expected, T actual) { if (expected == actual) { return; } - assertEquals(message + "[algorithm]", expected.getAlgorithm(), actual.getAlgorithm()); + assertEquals(message + "[algorithm]", + resolveEffectiveAlgorithm(expected.getAlgorithm()), + resolveEffectiveAlgorithm(actual.getAlgorithm())); if (expected instanceof RSAPublicKey) { assertRSAPublicKeyEquals(message, RSAPublicKey.class.cast(expected), RSAPublicKey.class.cast(actual)); @@ -424,7 +438,16 @@ public abstract class JUnitTestSupport extends Assert { } else if (expected instanceof ECPrivateKey) { assertECPrivateKeyEquals(message, ECPrivateKey.class.cast(expected), ECPrivateKey.class.cast(actual)); } - assertArrayEquals(message + "[encoded-data]", expected.getEncoded(), actual.getEncoded()); + } + + public static String resolveEffectiveAlgorithm(String algorithm) { + if (GenericUtils.isEmpty(algorithm)) { + return algorithm; + } else if (BuiltinIdentities.Constants.ECDSA.equalsIgnoreCase(algorithm)) { + return KeyUtils.EC_ALGORITHM; + } else { + return algorithm.toUpperCase(Locale.ENGLISH); + } } public static void assertRSAPublicKeyEquals(String message, RSAPublicKey expected, RSAPublicKey actual) {