Improve DSA and RSA key generation bit size. #1600

axmanalad · 2024-10-27T01:04:48Z

Generating a DSA or RSA key originally gave a bit size of 1024, which is not the recommended bit size of 2048 nor the stronger bit size of 4096.
This change allows a DSA-3072 key or an RSA-4096 key to be generated.
Note: DSA-4096 is not possible with the current algorithm of the DSA key generator. Therefore, DSA-3072 is implemented instead of DSA-4096.

Original PR from #1596
Fixes #1464

eclipse-platform-bot · 2024-10-27T01:10:37Z

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

ant/org.eclipse.ant.ui/META-INF/MANIFEST.MF
team/bundles/org.eclipse.jsch.ui/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From 366fe1d2b2623a06958b2c107845dfbd1fec321d Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Sun, 27 Oct 2024 01:16:20 +0000
Subject: [PATCH] Version bump(s) for 4.34 stream


diff --git a/ant/org.eclipse.ant.ui/META-INF/MANIFEST.MF b/ant/org.eclipse.ant.ui/META-INF/MANIFEST.MF
index 5fd411d90f..35363f174a 100644
--- a/ant/org.eclipse.ant.ui/META-INF/MANIFEST.MF
+++ b/ant/org.eclipse.ant.ui/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.ant.ui; singleton:=true
-Bundle-Version: 3.9.500.qualifier
+Bundle-Version: 3.9.600.qualifier
 Bundle-Activator: org.eclipse.ant.internal.ui.AntUIPlugin
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
diff --git a/team/bundles/org.eclipse.jsch.ui/META-INF/MANIFEST.MF b/team/bundles/org.eclipse.jsch.ui/META-INF/MANIFEST.MF
index 30141699d3..c5bccee193 100644
--- a/team/bundles/org.eclipse.jsch.ui/META-INF/MANIFEST.MF
+++ b/team/bundles/org.eclipse.jsch.ui/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.jsch.ui;singleton:=true
-Bundle-Version: 1.5.400.qualifier
+Bundle-Version: 1.5.500.qualifier
 Bundle-Activator: org.eclipse.jsch.internal.ui.JSchUIPlugin
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
-- 
2.47.0

Further information are available in Common Build Issues - Missing version increments.

github-actions · 2024-10-27T01:44:35Z

Test Results

1 755 files ±0 1 755 suites ±0 1h 25m 40s ⏱️ - 9m 50s
4 169 tests ±0 4 147 ✅ +1 22 💤 ±0 0 ❌ - 1
13 104 runs ±0 12 940 ✅ +1 164 💤 ±0 0 ❌ - 1

Results for commit 28c0896. ± Comparison against base commit a982bd3.

♻️ This comment has been updated with latest results.

HannesWell

Thank you for creating this follow-up.

Can you please first make sure that you don't revert changes made before, i.e. your commit Reverted merge changes is removed. If you have trouble compiling, you probably have to update your target-platform. You can do that for example by re-running your Oomph setup in your workspace.

After that, please squash all your changes into one commit with a meaningful message, In the commit message body you ideally also reference the issue fixed with this. E.g. using
Fixes https://github.com/eclipse-platform/eclipse.platform/issues/1464.
If you squash your change please don't include the change the eclipse-platform-bot has pushed to this branch. This should stay separated.

If you subsequently apply changes from the review, please just amend your commit and force-push to the branch.

axmanalad · 2024-11-01T06:31:53Z

Thank you for the feedback. I was able to remove the Reverted merge changes commit and squashed my changes into one commit.

After that, please squash all your changes into one commit with a meaningful message, In the commit message body you ideally also reference the issue fixed with this. E.g. using Fixes https://github.com/eclipse-platform/eclipse.platform/issues/1464. If you squash your change please don't include the change the eclipse-platform-bot has pushed to this branch. This should stay separated.

As someone who is still learning about Git, this comment was very helpful. Let me know if there are any more changes needed to be made.

HannesWell

After that, please squash all your changes into one commit with a meaningful message, In the commit message body you ideally also reference the issue fixed with this. E.g. using Fixes #1464. If you squash your change please don't include the change the eclipse-platform-bot has pushed to this branch. This should stay separated.
As someone who is still learning about Git, this comment was very helpful. Let me know if there are any more changes needed to be made.

You are welcome and thanks for the update:)

But with referencing Fixes https://github.com/eclipse-platform/eclipse.platform/issues/1464 I meant to mention it at the end of the commit message body and not only in the headline. The commit headline should still be a descriptive (half) sentence, just like this PR's name/headline. The following body can then contain more details and the mentioned reference.
The overall commit message could be for example (feel free to further adjust this):

[SSH] Increase bit-size of generated RSA and DSA keys

Generate RSA keys with 4096bit and DSA keys with 3072 (the underlying algorithm doesn't support higher values)

Fixes https://github.com/eclipse-platform/eclipse.platform/issues/1464

Regarding the implementation I have a two comments below.

HannesWell · 2024-11-03T19:19:33Z

.../bundles/org.eclipse.jsch.ui/src/org/eclipse/jsch/internal/ui/preference/PreferencePage.java

+					if (__type == KeyPair.RSA) {
+						kpairComment = _type + "-4096"; //$NON-NLS-1$
+					} else {
+						kpairComment = _type + "-3072"; //$NON-NLS-1$
+					}


Suggested change

if (__type == KeyPair.RSA) {

kpairComment = _type + "-4096"; //$NON-NLS-1$

} else {

kpairComment = _type + "-3072"; //$NON-NLS-1$

}

kpairComment = _type + "-" + keySize; //$NON-NLS-1$

HannesWell · 2024-11-03T19:25:51Z

.../bundles/org.eclipse.jsch.ui/src/org/eclipse/jsch/internal/ui/preference/PreferencePage.java

+							if (__type == KeyPair.RSA) {
+								_kpair[0] = KeyPair.genKeyPair(getJSch(), __type, 4096);
+							} else {
+								_kpair[0] = KeyPair.genKeyPair(getJSch(), __type, 3072);
+							}


As suggested in #1464 (comment), I think the key-size for RSA and DSA should be specified in a static final int constant at the top of this class:

private static final String SSH2_PREFERENCE_PAGE_CONTEXT="org.eclipse.jsch.ui.ssh2_preference_page_context"; //$NON-NLS-1$ private static final int RSA_KEY_SIZE = 4096; private static final int DSA_KEY_SIZE = 3072;

Then you can create a variable in this method, before BusyIndicator.showWhile() that holds the actual keySize used for the specified key type:

int keySize = type == KeyPair.RSA ? RSA_KEY_SIZE : DSA_KEY_SIZE; final JSchException[] _e=new JSchException[1]; BusyIndicator.showWhile(getShell().getDisplay(), () -> {

and then this if-else-block can stay a one-liner:

Suggested change

if (__type == KeyPair.RSA) {

_kpair[0] = KeyPair.genKeyPair(getJSch(), __type, 4096);

} else {

_kpair[0] = KeyPair.genKeyPair(getJSch(), __type, 3072);

}

_kpair[0] = KeyPair.genKeyPair(getJSch(), __type, keySize);

as well as the block below to create the comment.

This implementation allows to generate RSA keys with 4096 bits and DSA keys with 3072 bits. (DSA does not support 4096 bits or higher within the key algorithm) Originally generates 1024 bits for RSA and DSA keys. Fixes eclipse-platform#1464

axmanalad · 2024-11-04T00:29:30Z

But with referencing Fixes #1464 I meant to mention it at the end of the commit message body and not only in the headline. The commit headline should still be a descriptive (half) sentence, just like this PR's name/headline. The following body can then contain more details and the mentioned reference.

I see there was a misunderstanding between my thought and your intention. Thank you for the clarification; I changed how the format of the commit message should look like.

As suggested in #1464 (comment), I think the key-size for RSA and DSA should be specified in a static final int constant at the top of this class:
private static final String SSH2_PREFERENCE_PAGE_CONTEXT="org.eclipse.jsch.ui.ssh2_preference_page_context"; //$NON-NLS-1$
private static final int RSA_KEY_SIZE = 4096;
private static final int DSA_KEY_SIZE = 3072;
Then you can create a variable in this method, before BusyIndicator.showWhile() that holds the actual keySize used for the specified key type:
int keySize = type == KeyPair.RSA ? RSA_KEY_SIZE : DSA_KEY_SIZE;
final JSchException[] _e=new JSchException[1];
BusyIndicator.showWhile(getShell().getDisplay(), () -> {

I really like this implementation of both comments as it looks organized and easier for readability. I readjusted the changes as you have suggested. Thank you for the suggestion.

HannesWell

Thank you for the adjustments.

Looks good and works well now. Nice work. :)

HannesWell force-pushed the team-alex-shawn-chelsy branch from d905cd2 to 3d576c8 Compare October 28, 2024 17:59

HannesWell requested changes Oct 28, 2024

View reviewed changes

axmanalad force-pushed the team-alex-shawn-chelsy branch from 3d576c8 to 1b38c15 Compare October 30, 2024 18:46

HannesWell requested changes Nov 3, 2024

View reviewed changes

axmanalad force-pushed the team-alex-shawn-chelsy branch from 1b38c15 to 28c0896 Compare November 4, 2024 00:12

HannesWell approved these changes Nov 4, 2024

View reviewed changes

HannesWell merged commit 4712d12 into eclipse-platform:master Nov 4, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve DSA and RSA key generation bit size. #1600

Improve DSA and RSA key generation bit size. #1600

axmanalad commented Oct 27, 2024

eclipse-platform-bot commented Oct 27, 2024 •

edited

Loading

github-actions bot commented Oct 27, 2024 •

edited

Loading

HannesWell left a comment

axmanalad commented Nov 1, 2024

HannesWell left a comment

HannesWell Nov 3, 2024

HannesWell Nov 3, 2024

axmanalad commented Nov 4, 2024

HannesWell left a comment

Improve DSA and RSA key generation bit size. #1600

Improve DSA and RSA key generation bit size. #1600

Conversation

axmanalad commented Oct 27, 2024

eclipse-platform-bot commented Oct 27, 2024 • edited Loading

github-actions bot commented Oct 27, 2024 • edited Loading

Test Results

HannesWell left a comment

Choose a reason for hiding this comment

axmanalad commented Nov 1, 2024

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell Nov 3, 2024

Choose a reason for hiding this comment

HannesWell Nov 3, 2024

Choose a reason for hiding this comment

axmanalad commented Nov 4, 2024

HannesWell left a comment

Choose a reason for hiding this comment

eclipse-platform-bot commented Oct 27, 2024 •

edited

Loading

github-actions bot commented Oct 27, 2024 •

edited

Loading