Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-42959] Support Ed25519 and EDDSA host keys #51

Closed
wants to merge 1 commit into from

Conversation

@reedloden
Copy link
Contributor

reedloden commented May 6, 2017

  • Trilead just gained support for Ed25519 and EDDSA algorithms, so update
    the hardcoded list in the manual key verification step to support them.
  • Add a isClass() method to allow using classes in a newer Trilead version
    while still retaining backwards compatibility with old Jenkins versions.
  • Fix a few typos that I noticed while adding this.
@reedloden

This comment has been minimized.

pom.xml Outdated
@@ -73,6 +73,11 @@
<artifactId>ssh-credentials</artifactId>
<version>1.6.1</version>
</dependency>
<dependency>

This comment has been minimized.

Copy link
@paladox

paladox May 6, 2017

This should probably be removed so a separate change adds it. Also this plugin has not been released yet I doint think.

This comment has been minimized.

Copy link
@reedloden

reedloden May 6, 2017

Author Contributor

Yup, not released yet. I can remove it for now and just bump the minimum Jenkins version temporarily so that this passes.

@reedloden reedloden force-pushed the reedloden:more-key-algs branch 3 times, most recently from 954fedf to 2de6d75 May 6, 2017
@reedloden reedloden changed the title JENKINS-42959: Support Ed25519 and EDDSA host keys [JENKINS-42959] Support Ed25519 and EDDSA host keys May 7, 2017
@@ -31,6 +31,8 @@

import com.trilead.ssh2.signature.DSASHA1Verify;
import com.trilead.ssh2.signature.RSASHA1Verify;
import com.trilead.ssh2.signature.ED25519KeyAlgorithm;

This comment has been minimized.

Copy link
@paladox

paladox May 7, 2017

You will want to remove both these imports.

@@ -92,8 +94,16 @@ private static HostKey parseKey(String key) {
RSASHA1Verify.decodeSSHRSAPublicKey(keyValue);
} else if ("ssh-dss".equals(algorithm)) {
DSASHA1Verify.decodeSSHDSAPublicKey(keyValue);
} else if ("ssh-ed25519".equals(algorithm)) {

This comment has been minimized.

Copy link
@paladox

paladox May 7, 2017

This should instead be

        } else if (isClass("ED25519KeyAlgorithm") && "ssh-ed25519".equals(algorithm))** {
             com.trilead.ssh2.signature.ED25519KeyAlgorithm.decodePublicKey(keyValue);
         } else if (isClass("ECDSAKeyAlgorithm") && "ecdsa-sha2-nistp256".equals(algorithm)) {
             com.trilead.ssh2.signature.ECDSAKeyAlgorithm.decodePublicKey(keyValue);
         } else if (isClass("ECDSAKeyAlgorithm") && "ecdsa-sha2-nistp384".equals(algorithm)) {
             com.trilead.ssh2.signature.ECDSAKeyAlgorithm.decodePublicKey(keyValue);
         } else if (isClass("ECDSAKeyAlgorithm") && "ecdsa-sha2-nistp521".equals(algorithm)) {
             com.trilead.ssh2.signature.ECDSAKeyAlgorithm.decodePublicKey(keyValue);
          } else {

then you will want to have a new function probably under this function that does

public boolean isClass(String className) {
try {
Class.forName(className);
return true;
} catch (ClassNotFoundException e) {
return false;
}
}

This comment has been minimized.

Copy link
@reedloden

reedloden May 7, 2017

Author Contributor

Sadly, this doesn't seem to work. Getting this build failure (see latest update to PR):

[ERROR] /scratch/jenkins/workspace/plugins/ssh-slaves-plugin/src/main/java/hudson/plugins/sshslaves/verifiers/ManuallyProvidedKeyVerificationStrategy.java:[96,43] cannot find symbol
[ERROR] symbol:   class ED25519KeyAlgorithm
[ERROR] location: package com.trilead.ssh2.signature
[ERROR] /scratch/jenkins/workspace/plugins/ssh-slaves-plugin/src/main/java/hudson/plugins/sshslaves/verifiers/ManuallyProvidedKeyVerificationStrategy.java:[98,43] cannot find symbol
[ERROR] symbol:   class ECDSAKeyAlgorithm
[ERROR] location: package com.trilead.ssh2.signature
[ERROR] /scratch/jenkins/workspace/plugins/ssh-slaves-plugin/src/main/java/hudson/plugins/sshslaves/verifiers/ManuallyProvidedKeyVerificationStrategy.java:[100,43] cannot find symbol
[ERROR] symbol:   class ECDSAKeyAlgorithm
[ERROR] location: package com.trilead.ssh2.signature
[ERROR] /scratch/jenkins/workspace/plugins/ssh-slaves-plugin/src/main/java/hudson/plugins/sshslaves/verifiers/ManuallyProvidedKeyVerificationStrategy.java:[102,43] cannot find symbol
[ERROR] symbol:   class ECDSAKeyAlgorithm
[ERROR] location: package com.trilead.ssh2.signature

Thoughts? I haven't written Java in ~10 years, so forgive my n00bism here. :)

This comment has been minimized.

Copy link
@coder-hugo

coder-hugo May 8, 2017

You have to invoke the methods via reflection as well. This should work:

    private static HostKey parseKey(String key) {
        if (!key.contains(" ")) {
            throw new IllegalArgumentException(Messages.ManualKeyProvidedHostKeyVerifier_TwoPartKey());
        }
        StringTokenizer tokenizer = new StringTokenizer(key, " ");
        String algorithm = tokenizer.nextToken();
        byte[] keyValue = Base64.decode(tokenizer.nextToken());
        if (null == keyValue) {
            throw new IllegalArgumentException(Messages.ManualKeyProvidedHostKeyVerifier_Base64EncodedKeyValueRequired());
        }
        
        try {
            if ("ssh-rsa".equals(algorithm)) {
                RSASHA1Verify.decodeSSHRSAPublicKey(keyValue);
            } else if ("ssh-dss".equals(algorithm)) {
                DSASHA1Verify.decodeSSHDSAPublicKey(keyValue);
            } else if (isClass("com.trilead.ssh2.signature.ED25519KeyAlgorithm") && "ssh-ed25519".equals(algorithm)) {
                decodePublicKey("com.trilead.ssh2.signature.ED25519KeyAlgorithm", keyValue);
            } else if (isClass("com.trilead.ssh2.signature.ECDSAKeyAlgorithm") && "ecdsa-sha2-nistp256".equals(algorithm)) {
                decodePublicKey("com.trilead.ssh2.signature.ECDSAKeyAlgorithm", keyValue);
            } else if (isClass("com.trilead.ssh2.signature.ECDSAKeyAlgorithm") && "ecdsa-sha2-nistp384".equals(algorithm)) {
                decodePublicKey("com.trilead.ssh2.signature.ECDSAKeyAlgorithm", keyValue);
            } else if (isClass("ECDSAKeyAlgorithm") && "ecdsa-sha2-nistp521".equals(algorithm)) {
                decodePublicKey("com.trilead.ssh2.signature.ECDSAKeyAlgorithm", keyValue);
            } else {
                throw new IllegalArgumentException(Messages.ManualKeyProvidedHostKeyVerifier_UnknownKeyAlgorithm());
            }
        } catch (IOException ex) {
            throw new IllegalArgumentException(Messages.ManualKeyProvidedHostKeyVerifier_KeyValueDoesNotParse(algorithm), ex);
        }  catch (StringIndexOutOfBoundsException ex) {
            // can happen in DSASHA1Verifier with certain values (from quick testing)
            throw new IllegalArgumentException(Messages.ManualKeyProvidedHostKeyVerifier_KeyValueDoesNotParse(algorithm), ex);
        } catch (NoSuchMethodException e) {
            throw new IllegalArgumentException(Messages.ManualKeyProvidedHostKeyVerifier_KeyValueDoesNotParse(algorithm), ex);
        } catch (IllegalAccessException e) {
            throw new IllegalArgumentException(Messages.ManualKeyProvidedHostKeyVerifier_KeyValueDoesNotParse(algorithm), ex);
        } catch (InvocationTargetException e) {
            throw new IllegalArgumentException(Messages.ManualKeyProvidedHostKeyVerifier_KeyValueDoesNotParse(algorithm), ex);
        } catch (ClassNotFoundException e) {
            throw new IllegalArgumentException(Messages.ManualKeyProvidedHostKeyVerifier_KeyValueDoesNotParse(algorithm), ex);
        }

        return new HostKey(algorithm, keyValue);
    }

    private static void decodePublicKey(String className, byte[] keyValue) throws ClassNotFoundException, NoSuchMethodException, InvocationTargetException, IllegalAccessException {
        Class.forName(className).getMethod("decodePublicKey", byte[].class).invoke(keyValue);
    }

Please note also the changes of the isClass calls. Regarding the Javadoc of Class.forName() you have the pass the fully qualified name of the class. Btw. I think it's time to switch the java level from 6 to 7 (pom.xml). So multi-catches will be possible.

This comment has been minimized.

Copy link
@reedloden

reedloden May 8, 2017

Author Contributor

@coder-hugo thanks for the help... now I'm getting:

[ERROR] /scratch/jenkins/workspace/plugins/ssh-slaves-plugin/src/main/java/hudson/plugins/sshslaves/verifiers/ManuallyProvidedKeyVerificationStrategy.java:[124,130] cannot find symbol
[ERROR] symbol:   class InvocationTargetException
[ERROR] location: class hudson.plugins.sshslaves.verifiers.ManuallyProvidedKeyVerificationStrategy
[ERROR] /scratch/jenkins/workspace/plugins/ssh-slaves-plugin/src/main/java/hudson/plugins/sshslaves/verifiers/ManuallyProvidedKeyVerificationStrategy.java:[124,130] cannot find symbol
[ERROR] symbol:   class InvocationTargetException
[ERROR] location: class hudson.plugins.sshslaves.verifiers.ManuallyProvidedKeyVerificationStrategy
[ERROR] /scratch/jenkins/workspace/plugins/ssh-slaves-plugin/src/main/java/hudson/plugins/sshslaves/verifiers/ManuallyProvidedKeyVerificationStrategy.java:[115,18] cannot find symbol
[ERROR] symbol:   class InvocationTargetException
[ERROR] location: class hudson.plugins.sshslaves.verifiers.ManuallyProvidedKeyVerificationStrategy
@reedloden reedloden force-pushed the reedloden:more-key-algs branch 4 times, most recently from ee94cd7 to e2169a0 May 7, 2017
@coder-hugo

This comment has been minimized.

Copy link

coder-hugo commented May 8, 2017

@reedloden I assume you have written this code without a Java IDE. I think it's more easier to get a running code if I provide a complete patch. I just forked the repo and pushed the patched commit here. I'm not sure if it's possible to change the base branch of a PR. If yes you can just switch to the more-key-algs of my fork. Otherwise you have to pull the commit to your repository.

* Trilead just gained support for Ed25519 and EDDSA algorithms, so update
  the hardcoded list in the manual key verification step to support them.
* Add a isClass() method to allow using classes in a newer Trilead version
  while still retaining backwards compatibility with old Jenkins versions.
* Fix a few typos that I noticed while adding this.
@reedloden reedloden force-pushed the reedloden:more-key-algs branch from e2169a0 to f5dc0f7 May 8, 2017
@reedloden

This comment has been minimized.

Copy link
Contributor Author

reedloden commented May 8, 2017

@coder-hugo yup, as I mentioned in one of my comments, I have not written Java in ~10 years. I got hit by this particular bug, so figured I would try to fix it, but that's been a bit more complicated than I originally thought. Thank you so much for the help! :)

@mc1arke

This comment has been minimized.

Copy link
Member

mc1arke commented May 8, 2017

@reedloden @coder-hugo There are other changes needing made to allow the new key types to work properly: we have to set the preferred key types on the connection and really want to make sure we use the new methods in Trilead for parsing key types where possible. These new methods have been built to allow new key types to be added in the future without any dependent users - such as ssh-slaves - having to update a manual mapping of algorithms to Trilead classes.

If you're prepared to wait then I can raise a PR later today containing all the necessary changes to use new Trilead features in a backwards compatible way

Copy link
Member

oleg-nenashev left a comment

Jenkins core minimal dependency needs to be bumped. No problem to do so since it is already a problem due to transient plugin dependencies

@@ -17,7 +17,7 @@

<properties>
<jenkins.version>1.609.1</jenkins.version>
<java.level>6</java.level>
<java.level>7</java.level>

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 8, 2017

Member

🐛 Jenkins core needs to be bumped to 1.625+ then

@@ -39,7 +39,7 @@ SSHLauncher.PortLessThanZero=The port value must be greater than 0
SSHLauncher.PortMoreThan65535=The port value must be less than 65536
ManualTrustingHostKeyVerifier.KeyNotTrusted={0} [SSH] WARNING: The SSH key for this host is not currently trusted. Connections will be denied until this new key is authorised.
ManualTrustingHostKeyVerifier.KeyAutoTrusted={0} [SSH] The SSH key with fingerprint {1} has been automatically trusted for connections to this machine.
ManualTrustingHostKeyVerifier.KeyTrused={0} [SSH] SSH host key matches key seen previously for this host. Connection will be allowed.
ManualTrustingHostKeyVerifier.KeyTrusted={0} [SSH] SSH host key matches key seen previously for this host. Connection will be allowed.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 8, 2017

Member

Translations are OK since there is no translations of this field

throw new IllegalArgumentException(Messages.ManualKeyProvidedHostKeyVerifier_KeyValueDoesNotParse(algorithm), ex);
}

return new HostKey(algorithm, keyValue);
}

private static void decodePublicKey(String className, byte[] keyValue) throws ClassNotFoundException, NoSuchMethodException, InvocationTargetException, IllegalAccessException {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 8, 2017

Member

I would rather handle these exceptions within the method

@@ -92,18 +93,36 @@ private static HostKey parseKey(String key) {
RSASHA1Verify.decodeSSHRSAPublicKey(keyValue);
} else if ("ssh-dss".equals(algorithm)) {
DSASHA1Verify.decodeSSHDSAPublicKey(keyValue);
} else if (isClass("com.trilead.ssh2.signature.ED25519KeyAlgorithm") && "ssh-ed25519".equals(algorithm)) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 8, 2017

Member

I would reorder operations. isClass() is much more heavy than String comparison

throw new IllegalArgumentException(Messages.ManualKeyProvidedHostKeyVerifier_KeyValueDoesNotParse(algorithm), ex);
}

return new HostKey(algorithm, keyValue);
}

private static void decodePublicKey(String className, byte[] keyValue) throws ClassNotFoundException, NoSuchMethodException, InvocationTargetException, IllegalAccessException {
Class.forName(className).getMethod("decodePublicKey", byte[].class).invoke(keyValue);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 8, 2017

Member

Some logging on the FINE/FINEST level would be really useful. Just to know when the new algorithm decoding fails

@reedloden

This comment has been minimized.

Copy link
Contributor Author

reedloden commented May 8, 2017

@mc1arke do you want to just take this over then (especially since it needs so much additional work)? Happy to turn this over to you. :)

@mc1arke

This comment has been minimized.

Copy link
Member

mc1arke commented May 8, 2017

@reedloden I'm happy to take on the change. Since I'll approach it in a different way, I wont be basing it on the contents of this PR, so feel free to close this PR. I also wont be including the fix to the typos in the translations as part of my PR since they're unrelated to the functionality change, so also feel free to raise a separate PR to cover them. Thanks for looking at this though.

@reedloden

This comment has been minimized.

Copy link
Contributor Author

reedloden commented May 8, 2017

Sure, closing this out, and I'll open a new PR just for the typos.

@reedloden reedloden closed this May 8, 2017
reedloden added a commit to reedloden/ssh-slaves-plugin that referenced this pull request May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.