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-43668] - Remove Trilead references #13

Merged
merged 3 commits into from Apr 22, 2017
Merged

[JENKINS-43668] - Remove Trilead references #13

merged 3 commits into from Apr 22, 2017

Conversation

mc1arke
Copy link
Member

@mc1arke mc1arke commented Apr 15, 2017

[JENKINS-43610] Move to latest version of ssh-cli-auth to remove Trilead references from SSHD module

pom.xml Outdated
@@ -28,7 +28,7 @@

<properties>
<jenkins.version>1.580.3</jenkins.version>
<java.level>6</java.level>
<java.level>7</java.level>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needs bumping the core dependency then. I would propose to pick 1.625.x as a first version with Java 7

pom.xml Outdated
@@ -62,20 +62,20 @@
<dependency>
<groupId>org.jenkins-ci.modules</groupId>
<artifactId>ssh-cli-auth</artifactId>
<version>1.1</version>
<version>1.4</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also requires 1.625.3

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

I've just gone with the latest release version, but 1.2 would also work. Would there be any reason to stick with a lower version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only potential issue is that backporting of this change (e.g. SSHD Module 1.12 or above) will also require bumping the ssh-cli-auth module. I do not see any issue with that in this case. The modules have to evolve anyway

if (!sshKey.isAuthorizedKey(getPublicKeySignature(key))) {
LOGGER.fine("Key signature didn't match for the user: "+username+" : "+getPublicKeySignature(key));
if (!sshKey.has(key)) {
LOGGER.fine("Key signature didn't match for the user: "+username);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to continue printing some diagnostics message from key?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed this comment. We can probably print out the whole public key, but the signature generation relies on Trilead which is what we're wanting to remove, so I'm not keen on still trying to generate a signature for logging purposes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signature now retained for the log message, as requested

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Will get it integrated in the next release. Is there a tracking JIRA issue for this?

@oleg-nenashev oleg-nenashev self-assigned this Apr 18, 2017
@oleg-nenashev
Copy link
Member

Maybe it makes sense to convert JENKINS-43610 to EPIC and to add tasks for module. But it is up to you, the current approach is fine for me if you prefer it

@oleg-nenashev oleg-nenashev changed the title Remove Trilead references [JENKINS-43610] - Remove Trilead references Apr 18, 2017
@mc1arke
Copy link
Member Author

mc1arke commented Apr 18, 2017

Agreed on the Epic. JENKINS-43668 now covers this task

@mc1arke mc1arke changed the title [JENKINS-43610] - Remove Trilead references [JENKINS-43668] - Remove Trilead references Apr 18, 2017
@oleg-nenashev
Copy link
Member

Should be merged without squash from what I see.
@mc1arke Are you fine if I leave the commit link to the second PR? Or would you mind changing the commit message?

@mc1arke
Copy link
Member Author

mc1arke commented Apr 20, 2017

I'll amend my commits later today, and will probably squash the last 3 commits into one, so I have one for fixing the local build, and one for using the new signature generation mechanism; unless anyone objects?

@oleg-nenashev oleg-nenashev merged commit 534ebac into jenkinsci:master Apr 22, 2017
@mc1arke mc1arke deleted the JENKINS-43610-remove-trilead-references branch April 22, 2017 14:55
@@ -51,20 +51,20 @@
<dependency>
<groupId>org.jenkins-ci.modules</groupId>
<artifactId>ssh-cli-auth</artifactId>
<version>1.2</version>
<version>1.4</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants