Skip to content

Commit

Permalink
Remove JSch from server module
Browse files Browse the repository at this point in the history
Motivation:
Using Apache MINA instead of JSch is recommended by JGit team.

Modification:
- Use Apache MINA instead of JSch for server module.
  - JSch is now only used for testing.

Result:
- JSch gone.
  • Loading branch information
minwoox committed Aug 24, 2023
1 parent 580c77b commit 4d82fb8
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 93 deletions.
2 changes: 1 addition & 1 deletion dependencies.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# If its classes are exposed in Javadoc, update offline links as well.
#
[versions]
armeria = "1.25.1"
armeria = "1.24.3"
assertj = "3.24.2"
awaitility = "4.2.0"
bouncycastle = "1.76"
Expand Down
4 changes: 2 additions & 2 deletions dist/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ task copyLib(group: 'Build',
FileCollection jgits = layout.files { libDir.listFiles() }.filter { File file ->
file.name.contains("jgit")
}
// org.eclipse.jgit-6.x and org.eclipse.jgit.ssh.jsch-6.x are required.
if (jgits.size() != 2 || !jgits.any { File file -> file.name.startsWith("org.eclipse.jgit-6") }) {
// org.eclipse.jgit-6.x is required.
if (jgits.size() != 1 || !jgits[0].name.startsWith("org.eclipse.jgit-6")) {
throw new IllegalStateException("Unexpected JGit JARs: $jgits.files")
}
}
Expand Down
1 change: 0 additions & 1 deletion server-mirror-git/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ dependencies {
implementation libs.bouncycastle.bcprov
implementation libs.eddsa
implementation libs.jgit6
implementation libs.jgit.ssh.jsch6
implementation libs.mina.sshd.core
implementation libs.mina.sshd.git
}
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ private GitWithAuth openGit(File workDir) throws Exception {
final String jGitUri;
if (scheme.startsWith("git+")) {
if (scheme.equals(SCHEME_GIT_SSH)) {
// JSch requires the username to be included in the URI.
// Requires the username to be included in the URI.
final String username;
if (credential() instanceof PasswordMirrorCredential) {
username = ((PasswordMirrorCredential) credential()).username();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

import javax.annotation.Nullable;

import org.apache.sshd.client.ClientBuilder;
import org.apache.sshd.client.SshClient;
import org.apache.sshd.client.config.hosts.HostConfigEntryResolver;
Expand Down Expand Up @@ -64,18 +66,10 @@
import org.eclipse.jgit.transport.SshTransport;
import org.eclipse.jgit.transport.URIish;
import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider;
import org.eclipse.jgit.transport.ssh.jsch.JschConfigSessionFactory;
import org.eclipse.jgit.transport.ssh.jsch.OpenSshConfig;
import org.eclipse.jgit.transport.ssh.jsch.OpenSshConfig.Host;
import org.eclipse.jgit.util.FS;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.jcraft.jsch.HostKey;
import com.jcraft.jsch.HostKeyRepository;
import com.jcraft.jsch.Session;
import com.jcraft.jsch.UserInfo;

import com.linecorp.centraldogma.server.MirrorException;
import com.linecorp.centraldogma.server.internal.IsolatedSystemReader;
import com.linecorp.centraldogma.server.internal.JGitUtil;
Expand All @@ -88,8 +82,6 @@ final class GitWithAuth extends Git {

private static final Logger logger = LoggerFactory.getLogger(GitWithAuth.class);

private static final OpenSshConfig EMPTY_CONFIG = emptySshConfig();

private static final KeyPairResourceParser keyPairResourceParser = KeyPairResourceParser.aggregate(
// Use BouncyCastle resource parser to support non-standard formats as well.
SecurityUtils.getBouncycastleKeyPairResourceParser(),
Expand Down Expand Up @@ -213,7 +205,8 @@ public LsRemoteCommand lsRemote() {
final Collection<KeyPair> keyPairs;
try {
keyPairs = keyPairResourceParser.loadKeyPairs(null, NamedResource.ofName(cred.username()),
passwordProvider(cred), cred.privateKey());
passwordProvider(cred.passphrase()),
cred.privateKey());
} catch (IOException | GeneralSecurityException e) {
throw new MirrorException("Unexpected exception while loading private key. username: " +
cred.username() + ", publicKey: " +
Expand Down Expand Up @@ -262,32 +255,50 @@ protected ClientSession createClientSession(
});
}

// TODO remove jsch dependency completely.
private static <T extends TransportCommand<?, ?>> void configureSsh(T cmd, PasswordMirrorCredential cred) {
cmd.setTransportConfigCallback(transport -> {
final SshTransport sshTransport = (SshTransport) transport;
final JschConfigSessionFactory sessionFactory = new JschConfigSessionFactory() {
final GitSshdSessionFactory factory = new GitSshdSessionFactory() {

Check warning on line 260 in server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java

View check run for this annotation

Codecov / codecov/patch

server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java#L260

Added line #L260 was not covered by tests
@Override
protected void configure(Host host, Session session) {
public RemoteSession getSession(URIish uri, CredentialsProvider credentialsProvider,
FS fs, int tms) throws TransportException {
try {
session.setHostKeyRepository(new MirrorHostKeyRepository());
session.setPassword(cred.password());
} catch (MirrorException e) {
throw e;
return new GitSshdSession(uri, NoopCredentialsProvider.INSTANCE, fs, tms) {

Check warning on line 265 in server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java

View check run for this annotation

Codecov / codecov/patch

server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java#L265

Added line #L265 was not covered by tests
@Override
protected SshClient createClient() {
// Not an Armeria but an SSHD client.
final ClientBuilder builder = ClientBuilder.builder();

Check warning on line 269 in server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java

View check run for this annotation

Codecov / codecov/patch

server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java#L269

Added line #L269 was not covered by tests
// Do not use local file system.
builder.hostConfigEntryResolver(HostConfigEntryResolver.EMPTY);
builder.fileSystemFactory(NoneFileSystemFactory.INSTANCE);

Check warning on line 272 in server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java

View check run for this annotation

Codecov / codecov/patch

server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java#L271-L272

Added lines #L271 - L272 were not covered by tests
// Do not verify the server key.
builder.serverKeyVerifier((clientSession, remoteAddress, serverKey) -> true);
builder.filePasswordProvider(passwordProvider(cred.password()));
return builder.build();

Check warning on line 276 in server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java

View check run for this annotation

Codecov / codecov/patch

server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java#L274-L276

Added lines #L274 - L276 were not covered by tests
}

@Override
protected ClientSession createClientSession(
SshClient clientInstance, String host, String username, int port,
String... passwords) throws IOException, InterruptedException {
if (port <= 0) {
port = 22; // Use the SSH default port it unspecified.

Check warning on line 284 in server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java

View check run for this annotation

Codecov / codecov/patch

server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java#L284

Added line #L284 was not covered by tests
}
return super.createClientSession(clientInstance, host, username,

Check warning on line 286 in server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java

View check run for this annotation

Codecov / codecov/patch

server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java#L286

Added line #L286 was not covered by tests
port, passwords);
}
};
} catch (Exception e) {
throw new MirrorException(e);
throw new TransportException("Unable to connect to: " + uri +

Check warning on line 291 in server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java

View check run for this annotation

Codecov / codecov/patch

server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java#L291

Added line #L291 was not covered by tests
" CredentialsProvider: " + credentialsProvider, e);
}
}
};

// Disable the default SSH config file lookup.
sessionFactory.setConfig(EMPTY_CONFIG);
sshTransport.setSshSessionFactory(sessionFactory);
final SshTransport sshTransport = (SshTransport) transport;
sshTransport.setSshSessionFactory(factory);

Check warning on line 297 in server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java

View check run for this annotation

Codecov / codecov/patch

server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java#L296-L297

Added lines #L296 - L297 were not covered by tests
});
}

private static FilePasswordProvider passwordProvider(PublicKeyMirrorCredential cred) {
final String passphrase = cred.passphrase();
private static FilePasswordProvider passwordProvider(@Nullable String passphrase) {
if (passphrase == null) {
return FilePasswordProvider.EMPTY;
}
Expand Down Expand Up @@ -320,45 +331,6 @@ public void beginTask(String title, int totalWork) {
}
}

protected static final class MirrorHostKeyRepository implements HostKeyRepository {

private static final HostKey[] EMPTY_HOST_KEYS = new HostKey[0];

MirrorHostKeyRepository() {
// TODO(trustin): Store the hostkeys in the meta repository.
}

@Override
public int check(String host, byte[] key) {
return OK;
}

@Override
public void add(HostKey hostkey, UserInfo ui) {}

@Override
public void remove(String host, String type) {}

@Override
public void remove(String host, String type, byte[] key) {}

@Override
public String getKnownHostsRepositoryID() {
return getClass().getSimpleName();
}

@Override
public HostKey[] getHostKey() {
throw new UnsupportedOperationException();
}

@Override
public HostKey[] getHostKey(String host, String type) {
// TODO(trustin): Store the hostkeys in the meta repository.
return EMPTY_HOST_KEYS;
}
}

private static final class NoopCredentialsProvider extends CredentialsProvider {

static final CredentialsProvider INSTANCE = new NoopCredentialsProvider();
Expand All @@ -378,24 +350,4 @@ public boolean get(URIish uri, CredentialItem... items) throws UnsupportedCreden
return false;

Check warning on line 350 in server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java

View check run for this annotation

Codecov / codecov/patch

server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitWithAuth.java#L350

Added line #L350 was not covered by tests
}
}

/**
* Returns an empty {@link OpenSshConfig}.
*
* <p>The default {@link OpenSshConfig} reads the SSH config in `~/.ssh/config` and converts the identity
* files into {@code com.jcraft.jsch.KeyPair}. Since JSch does not support Ed25519, `KeyPair.load()`
* raise an exception if Ed25519 is used locally. Plus, Central Dogma uses
* {@link PublicKeyMirrorCredential}, we need to provide an empty config for an isolated environment.
*/
private static OpenSshConfig emptySshConfig() {
final File emptyConfigFile;
try {
emptyConfigFile = File.createTempFile("dogma", "empty-ssh-config");
emptyConfigFile.deleteOnExit();
} catch (IOException e) {
throw new IllegalStateException(e);
}

return new OpenSshConfig(emptyConfigFile.getParentFile(), emptyConfigFile);
}
}
3 changes: 0 additions & 3 deletions server/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@ dependencies {
api libs.armeria.thrift09
testImplementation libs.armeria.junit5

// JSch
implementation libs.jsch

// Caffeine
implementation libs.caffeine

Expand Down

0 comments on commit 4d82fb8

Please sign in to comment.