Remove usage of obsolete trilead-putty key #200

olamy · 2024-03-21T06:13:24Z

Signed-off-by: Olivier Lamy [email protected]

Testing done

Submitter checklist

Give feedback

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue
Options

…-api which was really used Signed-off-by: Olivier Lamy <[email protected]>

jtnord · 2024-03-21T11:23:32Z

I have no idea how often putty style keys are used (mostly generated from windows users I guess), but if we need to the spec is public.

jglick

Following up #199 I guess.

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/SSHUserPrivateKey.java

Signed-off-by: Olivier Lamy <[email protected]>

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/SSHUserPrivateKey.java

…l/BasicSSHUserPrivateKey.java Co-authored-by: Jesse Glick <[email protected]>

…nternal usage only Signed-off-by: Olivier Lamy <[email protected]>

jglick · 2024-03-25T12:37:57Z

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java

+                        String key = reader.toOpenSSH(privateKey, passphrase);
+                        if(key != null) {
+                            privateKeys.add(key);
+                            break;


I think you want to break out of the outer loop (meaning adding a label) rather than the inner loop. Otherwise for a PuTTY key you will add two entries to privateKeys.

(Is there no test coverage for this?)

well I'm really tempted to remove this putty key extension as this is not up to date and doesn't even work with current version of PuTTy.
using something such puttygen -t rsa -b 2048 -o mykey.ppk uttygen -t rsa -b 2048 -o mykey.ppk
you get a file starting with

PuTTY-User-Key-File-3: ssh-rsa

This key will simply generate a NPE because of this line https://github.com/kohsuke/trilead-putty-extension/blob/e928d8b5cc80a35d521b05e8256bdd5100cba616/src/main/java/org/kohsuke/putty/PuTTYKey.java#L149

So I'm just tempted to remove this code which simply doesn't work anymore...

FTR v3 was released with 0.75 which was 2021-05-08 so it almost 3 years ago.

So, in the last 3 years, no single key created with PuTTy could have been used.
I couldn't find any issue reported in Jira...
It looks like we can remove the support of the PuTTy key format.
I'm not sure why someone re-invented the wheel by inventing another format.

https://xkcd.com/927/

Signed-off-by: Olivier Lamy <[email protected]>

jtnord

Seems reasonable to remove, but I'm sure someone will be using an old V2 key from a server configured long ago...
Not sure why putty keys were not using a different Key source to begin with though

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/SSHUserPrivateKey.java

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java

Signed-off-by: Olivier Lamy <[email protected]>

olamy · 2024-03-27T06:54:51Z

Seems reasonable to remove, but I'm sure someone will be using an old V2 key from a server configured long ago...

Easy bet to win :)

jtnord

Seems reasonable to me, but will delay merging to let others have an opinion

salsifis · 2024-03-28T17:12:04Z

Seems reasonable to remove, but I'm sure someone will be using an old V2 key from a server configured long ago... Not sure why putty keys were not using a different Key source to begin with though

I can announce that you won your bet :)

Here is, for those who will encounter this problem, how to convert your older PuTTY credentials to more standard ones:

Open PuTTYgen
Load your key in the PuTTY format (.ppk) using the passphrase
Click “Conversions” › Export OpenSSH key (force new file format)

The resulting file can be used as the private key in a new credentials entry, with the same passphrase.

jglick · 2024-03-28T17:20:56Z

@salsifis I took the liberty of adding your instructions (unconfirmed) to https://github.com/jenkinsci/ssh-credentials-plugin/releases/tag/334.v7732563deee1 for better discoverability. @olamy / @jtnord should this be reverted and the originally proposed extension point reintroduced?

jtnord · 2024-03-28T17:29:38Z

. @olamy / @jtnord should this be reverted and the originally proposed extension point reintroduced?

I am in 2 minds here.
on the one hand I do not like breaking things,
on the other the ep (and the old code as implemented) seemed to violate the kiss principal, and the UI does not purport to support these keys (nor does it purport to support OpenSSH keys - so that may be a moot point, however it is producing only OpenSSH based keys for use).

olamy · 2024-03-29T00:23:35Z

@salsifis congratulations! bravo :)
I agree it's usually not a good idea to break things.
But here the putty key support is kind of obscure (not even clearly documented and coming transitively from some libraries called trilead-....) and really poor as we do not support keys generated by the last version of Putty.
The migration of the key looks to be very easy and thanks @jglick adding it to the release notes.

salsifis · 2024-03-29T07:32:53Z

@olamy @jtnord

In my opinion the main problem here is that this end of support was not announced (no warnings or whatever in the Jenkins interface). We had to look at the Jenkins logs and do some research to understand why an update caused the jobs to abort.

It would be better to have warnings if deprecations are on the table.

jtnord · 2024-04-02T12:00:11Z

@olamy can you add the compatable-since to the hpi plugin configuration (denoting the first version that removed the support) - that way it will at least get the warning to prompt users to go to the release notes.

olamy · 2024-04-03T01:48:03Z

That sounds like a good idea. #202

Make trilead-putty extension as optional, transitive deps via trilead…

ea9d6dc

…-api which was really used Signed-off-by: Olivier Lamy <[email protected]>

olamy marked this pull request as ready for review March 21, 2024 08:35

olamy requested a review from a team as a code owner March 21, 2024 08:35

olamy added the bug label Mar 21, 2024

olamy added enhancement and removed bug labels Mar 21, 2024

jglick reviewed Mar 21, 2024

View reviewed changes

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java Outdated Show resolved Hide resolved

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/SSHUserPrivateKey.java Outdated Show resolved Hide resolved

olamy added 2 commits March 22, 2024 06:23

remove unused import

a596df8

Signed-off-by: Olivier Lamy <[email protected]>

make PrivateKeyReader as NoExternalUse

2a8add7

Signed-off-by: Olivier Lamy <[email protected]>

olamy requested a review from jglick March 22, 2024 08:46

jglick approved these changes Mar 22, 2024

View reviewed changes

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java Outdated Show resolved Hide resolved

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/SSHUserPrivateKey.java Outdated Show resolved Hide resolved

olamy and others added 2 commits March 23, 2024 03:58

Update src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/imp…

e673b18

…l/BasicSSHUserPrivateKey.java Co-authored-by: Jesse Glick <[email protected]>

simplify as btw we do have only one extension and it is limited for i…

c5cff11

…nternal usage only Signed-off-by: Olivier Lamy <[email protected]>

jglick requested changes Mar 25, 2024

View reviewed changes

get rid of PuTTy key support

95a61ad

Signed-off-by: Olivier Lamy <[email protected]>

jtnord reviewed Mar 26, 2024

View reviewed changes

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/SSHUserPrivateKey.java Outdated Show resolved Hide resolved

src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java Outdated Show resolved Hide resolved

cleanup unused imports

a76823e

Signed-off-by: Olivier Lamy <[email protected]>

olamy changed the title ~~Make trilead-putty extension as optional, transitive deps via trilead-api which was really used~~ Remove usage of obsolete trilead-putty key Mar 27, 2024

olamy added removed and removed enhancement labels Mar 27, 2024

olamy requested review from jglick and jtnord March 27, 2024 01:56

jtnord approved these changes Mar 27, 2024

View reviewed changes

jglick approved these changes Mar 27, 2024

View reviewed changes

jglick merged commit 7732563 into jenkinsci:master Mar 27, 2024
15 checks passed

olamy deleted the make-triled-putty-key-optional branch March 29, 2024 00:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove usage of obsolete trilead-putty key #200

Remove usage of obsolete trilead-putty key #200

olamy commented Mar 21, 2024

Submitter checklist

jtnord commented Mar 21, 2024

jglick left a comment

jglick Mar 25, 2024

olamy Mar 26, 2024

jtnord Mar 26, 2024

olamy Mar 26, 2024

jtnord left a comment

olamy commented Mar 27, 2024

jtnord left a comment

salsifis commented Mar 28, 2024

jglick commented Mar 28, 2024

jtnord commented Mar 28, 2024

olamy commented Mar 29, 2024 •

edited

Loading

salsifis commented Mar 29, 2024

jtnord commented Apr 2, 2024 •

edited

Loading

olamy commented Apr 3, 2024

Remove usage of obsolete trilead-putty key #200

Remove usage of obsolete trilead-putty key #200

Conversation

olamy commented Mar 21, 2024

Testing done

Submitter checklist

jtnord commented Mar 21, 2024

jglick left a comment

Choose a reason for hiding this comment

jglick Mar 25, 2024

Choose a reason for hiding this comment

olamy Mar 26, 2024

Choose a reason for hiding this comment

jtnord Mar 26, 2024

Choose a reason for hiding this comment

olamy Mar 26, 2024

Choose a reason for hiding this comment

jtnord left a comment

Choose a reason for hiding this comment

olamy commented Mar 27, 2024

jtnord left a comment

Choose a reason for hiding this comment

salsifis commented Mar 28, 2024

jglick commented Mar 28, 2024

jtnord commented Mar 28, 2024

olamy commented Mar 29, 2024 • edited Loading

salsifis commented Mar 29, 2024

jtnord commented Apr 2, 2024 • edited Loading

olamy commented Apr 3, 2024

olamy commented Mar 29, 2024 •

edited

Loading

jtnord commented Apr 2, 2024 •

edited

Loading