-
Notifications
You must be signed in to change notification settings - Fork 139
feat: new launcher using Apache Mina sshd library #570
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
base: main
Are you sure you want to change the base?
Conversation
db78481
to
beb4353
Compare
beb4353
to
38a02a4
Compare
Not sure about the CI errors, 106 test run, and there is no failures. It fails when it try to make the Surefire report
|
Are any tests attempting to write to stdout/stderr (not using |
I do not remember any, but I will review it. |
Jesse was right; the cause was a bunch of outputs to |
0b2fcc4
to
df049bb
Compare
This is the initial implementation; it lacks host key verifications and other features the current launcher has. These features will be added in follow-up PRs. |
Did you mean to update the PR title? |
519bdb8
to
c2c908c
Compare
src/main/java/io/jenkins/plugins/sshbuildagents/ssh/mina/SSHApacheMinaLauncher.java
Show resolved
Hide resolved
src/main/java/io/jenkins/plugins/sshbuildagents/ssh/mina/SSHApacheMinaLauncher.java
Show resolved
Hide resolved
*/ | ||
public boolean getTrackCredentials() { | ||
String trackCredentials = System.getProperty(SSHApacheMinaLauncher.class.getName() + ".trackCredentials"); | ||
return !"false".equalsIgnoreCase(trackCredentials); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe trim trackCredentials
just in case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, if the user is not able to put "true" or "false" in a system property, it is not setting the property correctly, so the property is invalid. You cannot manage all the wrong property values that a user can give you, and only make sense to fix their errors when they are common (which denotes there is another issue).
To provide a io.jenkins.plugins.sshbuildagents.ssh.mina.SSHApacheMinaLauncher.trackCredentials
with a value that you must trim. You have to provide something like -Dio. Jenkins.plugins.sshbuildagents.ssh.mina.SSHApacheMinaLauncher.trackCredentials=" true "
that it is enough bizarre to do not thing anyone is so dumb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use SystemProperties
instead please.
/* | ||
* The MIT License | ||
* | ||
* Copyright (c) 2016, Michael Clarke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why you use this copyright here?
It looks to be a new class written by you. Either you put yourself or nobody but not someone who was not part of writing this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C&P from the old class, so I keep the same header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will review it, as it is likely new content, even though I started by copying it from the old file.
</dependency> | ||
</dependencies> | ||
</dependencyManagement> | ||
|
||
<dependencies> | ||
<dependency> | ||
<groupId>io.jenkins.plugins</groupId> | ||
<artifactId>eddsa-api</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove this dependency and use this new feature from mina-sshd see apache/mina-sshd#639
The project https://github.com/str4d/ed25519-java is not maintained anymore and has some known security issues such str4d/ed25519-java#95
import org.apache.sshd.common.config.keys.loader.pem.PKCS8PEMResourceKeyPairParser; | ||
import org.apache.sshd.common.config.keys.loader.pem.RSAPEMResourceKeyPairParser; | ||
import org.apache.sshd.common.util.security.bouncycastle.BouncyCastleKeyPairResourceParser; | ||
import org.apache.sshd.common.util.security.eddsa.Ed25519PEMResourceKeyParser; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be good to get rid of net.i2p.crypto.eddsa
with using BC
final Node node = computer.getNode(); | ||
final String host = this.host; | ||
final int port = this.port; | ||
if (computer == null || listener == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure it's needed as both have @NonNull
annotation :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I struggle to remove a warning from SpotBugs; this is a leftover. I will remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no worries. not a big blocking problem
Nice PR.
|
this.j.timeout = 0; | ||
} | ||
|
||
@BeforeAll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sonds like a bit redundant with @Testcontainers(disabledWithoutDocker = true)
?
At least until we are confident with this new launcher, it lacks some features the other has, like host verification.
That's the goal; I will stop accepting and processing changes in Trilead because I will focus my time on this new launcher and a way to migrate the old configuration to the latest in a seamless manner. It will take a while for sure. |
List<PosixFilePermission> permissions = new ArrayList<>(); | ||
permissions.add(PosixFilePermission.GROUP_READ); | ||
permissions.add(PosixFilePermission.OWNER_WRITE); | ||
scp.upload(bytes, remotePath, permissions, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to ensure remote path exists first
try (ClientSession session = connect()) { | ||
DefaultScpClient scp = new DefaultScpClient(session); | ||
List<PosixFilePermission> permissions = new ArrayList<>(); | ||
permissions.add(PosixFilePermission.GROUP_READ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure we want to force such perms. Remote server may have mask in place with different permissions. (sftp could simplify as you will not need to set permissions)
|
||
@BeforeAll | ||
static void beforeAll() { | ||
assumeFalse(SystemUtils.IS_OS_WINDOWS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about annotations at class level such @DisabledOnOs(OS.WINDOWS)
@@ -0,0 +1,42 @@ | |||
FROM ubuntu:14.04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file doesn't seem to be used in any test?
@@ -0,0 +1,42 @@ | |||
FROM ubuntu:16.04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
this file doesn't seem to be used in any test?
@@ -0,0 +1,42 @@ | |||
FROM ubuntu:18.04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
ConnectFuture connectionFuture = client.connect(this.credentials.getUsername(), this.host, this.port); | ||
connectionFuture.verify(this.timeoutMillis); | ||
session = connectionFuture.getSession(); | ||
addAuthentication(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should be able to use something
SSHAuthenticator.newInstance(session, credentials).authenticate(TaskListener.NULL)
/* | ||
* The MIT License | ||
* | ||
* Copyright (c) 2016, Michael Clarke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
/* | ||
* The MIT License | ||
* | ||
* Copyright (c) 2016, Michael Clarke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
/* | ||
* The MIT License | ||
* | ||
* Copyright (c) 2016, Michael Clarke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
/* | ||
* The MIT License | ||
* | ||
* Copyright (c) 2016, Michael Clarke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -0,0 +1,26 @@ | |||
package io.jenkins.plugins.sshbuildagents.ssh.mina; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing license but we probably will not need this file anymore
/* | ||
* The MIT License | ||
* | ||
* Copyright (c) 2016, Michael Clarke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
/* | ||
* The MIT License | ||
* | ||
* Copyright (c) 2016, Michael Clarke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
but not sure we really need this file
/* | ||
* The MIT License | ||
* | ||
* Copyright (c) 2004-, all the contributors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah :)
|
||
@Before | ||
public void setup() throws IOException { | ||
Logger.getLogger("org.apache.sshd").setLevel(Level.FINE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test is really slow as the same server is restarted for every single test.
why not using test container here to avoid some collision between server and client code (normally shouldn't be but...)
well I guess the main issue is more the 5 minutes loop :)
connection.setCredentials(credentials); | ||
ShellChannel shellChannel = connection.shellChannel(); | ||
shellChannel.execCommand("sleep 500s"); | ||
for (int i = 0; i < 300; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks that's plenty of time to have a coffee :)
do we really need this? Can't we skip this on normal dev life and run it on CI server only?
Right because it's too easy to talk and do nothing, I have started to put my comments into code with this kuisathaverat#102 |
I will address some issues and then merge them as is. This PR is a WIP new launcher, and this is the first iteration. It is not perfect, and it should not be. I do not want to make this PR an open issue that remains unmerged. Also, it helps to work on different improvements in parallel and track the changes better |
The proposed changes are opposed to the structure I have planned for the launcher, or are not in the scope of this PR.
If you do so, you must at least mark and inform users about the experimental aspect of your PR when they use the new launcher because many features are not working now. I completely understand the rationale behind adding an abstraction layer on top of the SSH connection. In theory, it makes sense and can offer flexibility. Introducing a new abstraction would mean committing to a public API that we'll need to support in the long term. Given that this is currently scoped just to the Jenkins SSH Agents plugin. I'm not sure the added complexity is justified. Also, I'm a bit hesitant about re-implementing the authentication layer from scratch. The existing solution is widely used, battle-tested, and already aligns with security requirements. I wish to have you open to comments as definitely such changes need some discussion and acceptance by the broader community, as this has some large effect on the long term maintenance of the plugin |
* | ||
* @author Ivan Fernandez Calvo | ||
*/ | ||
public interface Connection extends AutoCloseable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need such abstraction here? maybe for simplicity we could use directly a class using Apache Mina.
I can't imagine to what other libraries we could switch?
At least this should be marked as internal
(in the package name?) or clearly documented in the Javadoc as internal. Otherwise we have to commit to support this as a long term API (is it the goal of this Jenkins plugin?)
This pull request introduces several changes to enhance SSH connection management, update dependencies, and improve code formatting. The most notable updates include the addition of new Java interfaces and classes for SSH functionality, modifications to the
pom.xml
file to refine dependencies, and updates to.editorconfig
andJenkinsfile
for better code consistency and build configuration.SSH Connection Management Enhancements:
src/main/java/io/jenkins/plugins/sshbuildagents/ssh/Connection.java
: Added a new interface to manage SSH connections, including methods for executing commands, transferring files, and configuring connection parameters.src/main/java/io/jenkins/plugins/sshbuildagents/ssh/KeyAlgorithm.java
,KeyAlgorithmManager.java
,KnownHosts.java
,ServerHostKeyVerifier.java
,ShellChannel.java
: Introduced new classes and interfaces for managing key algorithms, host verification, and non-interactive SSH sessions. [1] [2] [3] [4] [5]src/main/java/io/jenkins/plugins/sshbuildagents/ssh/mina/FakeURI.java
,KeyAlgorithmManagerImpl.java
: Added classes for testing and implementing key algorithm management. [1] [2]Dependency Updates:
pom.xml
: Refined dependencies by adding new plugins (eddsa-api
,mina-sshd-api
) and excluding redundant ones (trilead-api
). Introduced placeholders for future dependency removals. [1] [2]Code Formatting and Build Configuration:
.editorconfig
: Updated rules for*.java
files, specifying 4-space indentation, LF line endings, and UTF-8 charset.Jenkinsfile
: Changed the Windows build configuration to use JDK 21 instead of JDK 17.Submitter checklist
JENKINS-64106