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

[FIXED JENKINS-42959] Specify preferred host keys during connect #54

Merged
merged 10 commits into from Jun 12, 2017

Conversation

@mc1arke
Copy link
Member

mc1arke commented May 16, 2017

The remaining fix for JENKINS-42959: adds support for additional host key algorithms where Jenkins contains a newer version of Trilead, and specify the preferred host key algorithm for connecting in-case Jenkins has an old key format saved as the known key.

@mc1arke mc1arke changed the title [FIXED JENKINS-42959] Specify preferred host keys during connect [WiP] [FIXED JENKINS-42959] Specify preferred host keys during connect May 17, 2017
pom.xml Outdated
<dependency>
<groupId>org.jenkins-ci</groupId>
<artifactId>trilead-ssh2</artifactId>
<version>build217-jenkins-9</version>

This comment has been minimized.

Copy link
@paladox

paladox May 17, 2017

Should be build217-jenkins-10 please.

This comment has been minimized.

Copy link
@mc1arke

mc1arke May 17, 2017

Author Member

Nope. We want the oldest version of Trilead that provides the new API. This version is only used during the compile/test phase, not in the package/release phase so the actual version here doesn't impact what end users get, but it needs to be a version that provides the API required in the PR without introducing other API that may not exist in versions bundled in other Jenkins releases.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 17, 2017

Member

Ideally should be replaced by API plugin when it's ready

This comment has been minimized.

Copy link
@mc1arke

mc1arke May 17, 2017

Author Member

Agreed. I'll pick up the API plugin work as soon as I get to the bottom of any major outstanding Trilead issues.

@paladox

This comment has been minimized.

Copy link

paladox commented May 17, 2017

Tests failures seem to be because the build is set as unstable.

@mc1arke

This comment has been minimized.

Copy link
Member Author

mc1arke commented May 17, 2017

The build is unstable due to the test failure, which is what is blocking this PR from being sent for full approval.

@mc1arke mc1arke changed the title [WiP] [FIXED JENKINS-42959] Specify preferred host keys during connect [FIXED JENKINS-42959] Specify preferred host keys during connect May 17, 2017
@mc1arke

This comment has been minimized.

Copy link
Member Author

mc1arke commented May 17, 2017

Tests fixed. Feel free to chip in with reviews.

Copy link
Member

oleg-nenashev left a comment

IMHO exception handling is error-prone in this PR. It also contributes new public API, which can be misused in the current state.

private static TrileadVersionSupport createaVersion9Instance() {
try {
return (TrileadVersionSupport) Class.forName("hudson.plugins.sshslaves.verifiers.JenkinsTrilead9VersionSupport").newInstance();
} catch (ReflectiveOperationException e) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

Any risk of ClassNotFoundException?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

I would catch all exceptions

This comment has been minimized.

Copy link
@mc1arke

mc1arke May 18, 2017

Author Member

ReflectiveOperationException covers ClassNotFoundException as well as the InvocationException that would be thrown if the class could not be instantiated

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

Though it still does not cover other possible exceptions i n the newInstance() call. https://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#newInstance()

}

KnownHosts knownHosts = new KnownHosts(KNOWN_HOSTS_FILE);
return knownHosts.getPreferredServerHostkeyAlgorithmOrder(((SSHLauncher) computer.getLauncher()).getHost());

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

Unchecked cast. Please avoid it, because there is a risk somebody uses API wrong. And FindBugs will start grumbling once enabled

try {
Thread.currentThread().getContextClassLoader().loadClass("com.trilead.ssh2.signature.KeyAlgorithmManager");
return createaVersion9Instance();
} catch (ReflectiveOperationException e) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

Better to catch all exceptions as well

return new HostKey(algorithm, keyValue);
}
} catch (IOException ex) {
throw new IllegalArgumentException(Messages.ManualKeyProvidedHostKeyVerifier_KeyValueDoesNotParse(algorithm), ex);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

This exception is not documented in TrileadVersionSupport. Instead of RunTime exceptions, I would rather suggest doing correct propagation using checked exceptions. For new APIs it is easy to do so


}

public abstract static class TrileadVersionSupport {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

API of this class is not documented and IMO misused in this PR. If you want to keep this API internal for now, please mark this class and its children as @Restricted(NoExternalUse.class)

This comment has been minimized.

Copy link
@mc1arke

mc1arke May 18, 2017

Author Member

Could you clarify what you mean by 'misused in this PR'? I can add Javadoc, but want to get the API correct. The visibility of the constructors/method in this class should prevent external use without the need for any Restriction annotations.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

I am just a pretty convinced enemy of unchecked exceptions. Especially the undocumented ones. Even with disabled constructor, it is still possible to access this class e.g. by copy-pasting the createaVersion9Instance() code. I would keep the classes restricted if you do not want to bother about external usages now. Better safe than sorry

@mc1arke

This comment has been minimized.

Copy link
Member Author

mc1arke commented May 19, 2017

@oleg-nenashev I think I've resolved all the points you've currently raised. Care to take another look?

Copy link
Member

oleg-nenashev left a comment

👍 Thanks for fixing it

Copy link

paladox left a comment

+1

@paladox

This comment has been minimized.

Copy link

paladox commented Jun 4, 2017

Bump. can this be merged please?

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Jun 10, 2017

Confirmed that this pull request resolves JENKINS-44803 (a duplicate of JENKINS-42959). Thanks very much!

@paladox

This comment has been minimized.

Copy link

paladox commented Jun 10, 2017

@jglick or @oleg-nenashev hi, could you merge and do a new release of this plugin please?

@mc1arke

This comment has been minimized.

Copy link
Member Author

mc1arke commented Jun 11, 2017

@stephenc could you consider reviewing, merging and releasing this please, since you seem to be the only other person with the access - unless you want me to request ssh-slaves release access myself?

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Jun 11, 2017

I think this plugin needs to release before the 2.60.1 LTD. Without this fix, ssh slaves don't connect after upgrade

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jun 11, 2017

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Jun 11, 2017

I can review and release tomorrow. If needed sooner, someone else will need to

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Jun 11, 2017

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jun 12, 2017

@stephenc stephenc merged commit 67bbbff into jenkinsci:master Jun 12, 2017
1 check passed
1 check passed
Jenkins This pull request looks good
Details
@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Jun 12, 2017

@mc1arke you should also seek release permissions access.

@mc1arke can you update the release notes on the wiki page with the details of what is in the 1.18 release that I have just cut.

@@ -786,6 +786,7 @@ public synchronized void launch(final SlaveComputer computer, final TaskListener
public Boolean call() throws InterruptedException {
Boolean rval = Boolean.FALSE;
try {
connection.setServerHostKeyAlgorithms(sshHostKeyVerificationStrategy.getPreferredKeyAlgorithms(computer));

This comment has been minimized.

Copy link
@jglick

jglick Jun 12, 2017

Member

Can cause an NPE: JENKINS-44830

This comment has been minimized.

Copy link
@jglick
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.