Skip to content

Commit c5371ab

Browse files
minwooxminwoox
authored
Merge commit from fork
* Add SSH host key verification for git+ssh:// mirrors Motivation: - `SshGitMirror`'s SSH client unconditionally accepted any host key presented by the remote server (CWE-322), allowing on-path attackers to intercept mirrored repository contents or inject malicious commits. Modifications: - Add `acceptedHostKeys` field to `SshKeyCredential` for per-credential host key pinning using SHA-256 fingerprints. - Add `trustedHostKeys` (hostname → fingerprints map) to `MirroringServicePluginConfig` for server-wide host key configuration. - Replace the accept-all `ServerKeyVerifier` lambda in `SshGitMirror.createSshClient()` with a verifier that computes the SHA-256 fingerprint of the presented key and compares it against the merged set of per-credential and global trusted keys using constant-time comparison. - Reject non-`SshKeyCredential` types (PASSWORD, ACCESS_TOKEN, NONE, null) at `SshGitMirror` construction time. - Remove the unused `PasswordCredential` code path from `SshGitMirror` (`sshRemoteUri()`, `configureCredential()`, `getAcceptedHostKeys()`). Result: - Every outbound SSH mirror connection now verifies the remote server's host key against an explicit allowlist before proceeding to authentication. * Address the comment from @ikhoon
1 parent a38baa1 commit c5371ab

18 files changed

Lines changed: 424 additions & 64 deletions

File tree

‎server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/GitMirrorProvider.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,8 @@ public Mirror newMirror(MirrorContext context) {
4949
context.direction(), context.credential(),
5050
context.localRepo(), context.localPath(),
5151
repositoryUri,
52-
context.gitignore(), context.zone());
52+
context.gitignore(), context.zone(),
53+
context.trustedHostKeys());
5354
}
5455
case SCHEME_GIT_HTTP:
5556
case SCHEME_GIT_HTTPS:

‎server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/SshGitMirror.java‎

Lines changed: 67 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,24 +17,34 @@
1717
package com.linecorp.centraldogma.server.internal.mirror;
1818

1919
import static com.linecorp.centraldogma.server.internal.credential.SshKeyCredential.publicKeyPreview;
20+
import static java.util.Objects.requireNonNull;
2021

2122
import java.io.File;
2223
import java.io.IOException;
24+
import java.net.SocketAddress;
2325
import java.net.URISyntaxException;
2426
import java.security.GeneralSecurityException;
2527
import java.security.KeyPair;
2628
import java.time.Instant;
29+
import java.util.ArrayList;
2730
import java.util.Collection;
31+
import java.util.Collections;
32+
import java.util.List;
33+
import java.util.Map;
34+
import java.util.Set;
35+
import java.util.concurrent.ConcurrentHashMap;
2836

2937
import org.apache.sshd.client.ClientBuilder;
3038
import org.apache.sshd.client.SshClient;
3139
import org.apache.sshd.client.config.hosts.HostConfigEntryResolver;
3240
import org.apache.sshd.client.session.ClientSession;
3341
import org.apache.sshd.common.NamedResource;
3442
import org.apache.sshd.common.config.keys.FilePasswordProvider;
43+
import org.apache.sshd.common.config.keys.KeyUtils;
3544
import org.apache.sshd.common.config.keys.loader.KeyPairResourceParser;
3645
import org.apache.sshd.common.config.keys.loader.openssh.OpenSSHKeyPairResourceParser;
3746
import org.apache.sshd.common.config.keys.loader.pem.PKCS8PEMResourceKeyPairParser;
47+
import org.apache.sshd.common.digest.BuiltinDigests;
3848
import org.apache.sshd.common.file.nonefs.NoneFileSystemFactory;
3949
import org.apache.sshd.common.keyprovider.KeyIdentityProvider;
4050
import org.apache.sshd.common.util.security.SecurityUtils;
@@ -53,11 +63,11 @@
5363

5464
import com.cronutils.model.Cron;
5565
import com.google.common.annotations.VisibleForTesting;
66+
import com.google.common.collect.ImmutableList;
5667

5768
import com.linecorp.centraldogma.common.MirrorException;
5869
import com.linecorp.centraldogma.server.command.CommandExecutor;
5970
import com.linecorp.centraldogma.server.credential.Credential;
60-
import com.linecorp.centraldogma.server.internal.credential.PasswordCredential;
6171
import com.linecorp.centraldogma.server.internal.credential.SshKeyCredential;
6272
import com.linecorp.centraldogma.server.mirror.MirrorDirection;
6373
import com.linecorp.centraldogma.server.mirror.MirrorResult;
@@ -68,6 +78,7 @@
6878
final class SshGitMirror extends AbstractGitMirror {
6979

7080
private static final Logger logger = LoggerFactory.getLogger(SshGitMirror.class);
81+
private static final Set<SocketAddress> warnedAddresses = ConcurrentHashMap.newKeySet();
7182

7283
private static final KeyPairResourceParser keyPairResourceParser = KeyPairResourceParser.aggregate(
7384
// Use BouncyCastle resource parser to support non-standard formats as well.
@@ -81,11 +92,19 @@ final class SshGitMirror extends AbstractGitMirror {
8192
// We might create multiple BouncyCastleRandom later and poll them, if necessary.
8293
private static final BouncyCastleRandom bounceCastleRandom = new BouncyCastleRandom();
8394

95+
private final Map<String, List<String>> trustedHostKeys;
96+
8497
SshGitMirror(String id, boolean enabled, @Nullable Cron schedule, MirrorDirection direction,
8598
Credential credential, Repository localRepo, String localPath,
86-
RepositoryUri remoteUri, @Nullable String gitignore, @Nullable String zone) {
99+
RepositoryUri remoteUri, @Nullable String gitignore, @Nullable String zone,
100+
Map<String, List<String>> trustedHostKeys) {
87101
super(id, enabled, schedule, direction, credential, localRepo, localPath,
88102
remoteUri, gitignore, zone);
103+
this.trustedHostKeys = requireNonNull(trustedHostKeys, "trustedHostKeys");
104+
if (!(credential instanceof SshKeyCredential)) {
105+
throw new MirrorException(
106+
"SSH mirror requires an SSH_KEY credential, but got: " + credential.type());
107+
}
89108
}
90109

91110
@Override
@@ -121,9 +140,7 @@ protected MirrorResult mirrorRemoteToLocal(File workDir, CommandExecutor executo
121140
private URIish sshRemoteUri() throws URISyntaxException {
122141
// Requires the username to be included in the URI.
123142
final String username;
124-
if (credential() instanceof PasswordCredential) {
125-
username = ((PasswordCredential) credential()).username();
126-
} else if (credential() instanceof SshKeyCredential) {
143+
if (credential() instanceof SshKeyCredential) {
127144
username = ((SshKeyCredential) credential()).username();
128145
} else {
129146
username = null;
@@ -145,8 +162,25 @@ private SshClient createSshClient() {
145162
// Do not use local file system.
146163
builder.hostConfigEntryResolver(HostConfigEntryResolver.EMPTY);
147164
builder.fileSystemFactory(NoneFileSystemFactory.INSTANCE);
148-
// Do not verify the server key.
149-
builder.serverKeyVerifier((clientSession, remoteAddress, serverKey) -> true);
165+
166+
// TODO(minwoox): Throw a MirrorException when acceptedHostKeys is empty once all mirrors have
167+
// been migrated to configure host key fingerprints. For now, log a warning and
168+
// accept all host keys to avoid breaking existing mirrors.
169+
final List<String> acceptedHostKeys = getAcceptedHostKeys();
170+
builder.serverKeyVerifier((clientSession, remoteAddress, serverKey) -> {
171+
final String fingerprint = KeyUtils.getFingerPrint(BuiltinDigests.sha256, serverKey);
172+
for (String accepted : acceptedHostKeys) {
173+
if (fingerprint.equals(accepted)) {
174+
return true;
175+
}
176+
}
177+
if (!acceptedHostKeys.isEmpty() && warnedAddresses.add(remoteAddress)) {
178+
logger.warn("Host key verification failed for {} (fingerprint: {}).",
179+
remoteAddress, fingerprint);
180+
}
181+
return acceptedHostKeys.isEmpty();
182+
});
183+
150184
builder.randomFactory(() -> bounceCastleRandom);
151185
final SshClient client = builder.build();
152186
try {
@@ -159,6 +193,30 @@ private SshClient createSshClient() {
159193
}
160194
}
161195

196+
private List<String> getAcceptedHostKeys() {
197+
if (trustedHostKeys.isEmpty()) {
198+
return ImmutableList.of();
199+
}
200+
201+
final List<String> merged = new ArrayList<>();
202+
final String host = remoteRepoUri().getRawAuthority();
203+
// Try exact match first (may include port, e.g. "git.example.com:2222")
204+
final List<String> byAuthority = trustedHostKeys.get(host);
205+
if (byAuthority != null) {
206+
merged.addAll(byAuthority);
207+
}
208+
// Also try hostname-only if authority contains a port
209+
if (host != null && host.contains(":")) {
210+
final String hostOnly = host.substring(0, host.indexOf(':'));
211+
final List<String> byHost = trustedHostKeys.get(hostOnly);
212+
if (byHost != null) {
213+
merged.addAll(byHost);
214+
}
215+
}
216+
217+
return Collections.unmodifiableList(merged);
218+
}
219+
162220
@VisibleForTesting
163221
static ClientSession createSession(SshClient sshClient, URIish uri) {
164222
int port = uri.getPort();
@@ -189,10 +247,8 @@ static ClientSession createSession(SshClient sshClient, URIish uri) {
189247

190248
private void configureCredential(SshClient client) {
191249
final Credential c = credential();
192-
if (c instanceof PasswordCredential) {
193-
client.setFilePasswordProvider(passwordProvider(((PasswordCredential) c).password()));
194-
} else if (c instanceof SshKeyCredential) {
195-
final SshKeyCredential cred = (SshKeyCredential) credential();
250+
if (c instanceof SshKeyCredential) {
251+
final SshKeyCredential cred = (SshKeyCredential) c;
196252
final Collection<KeyPair> keyPairs;
197253
try {
198254
keyPairs = keyPairResourceParser.loadKeyPairs(null, NamedResource.ofName(cred.username()),

‎server-mirror-git/src/test/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMetaRepositoryWithMirrorTest.java‎

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@
4646
import com.linecorp.centraldogma.internal.api.v1.MirrorRequest;
4747
import com.linecorp.centraldogma.server.command.Command;
4848
import com.linecorp.centraldogma.server.credential.Credential;
49-
import com.linecorp.centraldogma.server.internal.credential.PasswordCredential;
49+
import com.linecorp.centraldogma.server.internal.credential.SshKeyCredential;
5050
import com.linecorp.centraldogma.server.internal.storage.repository.RepositoryMetadataException;
5151
import com.linecorp.centraldogma.server.mirror.Mirror;
5252
import com.linecorp.centraldogma.server.mirror.MirrorDirection;
@@ -202,17 +202,15 @@ void testMirror(boolean useRawApi) {
202202
// Ensure the credentials are loaded correctly.
203203

204204
//// Should be matched by 'alice' credential.
205-
assertThat(foo.credential()).isInstanceOf(PasswordCredential.class);
205+
assertThat(foo.credential()).isInstanceOf(SshKeyCredential.class);
206206
//// Should be matched by 'bob' credential.
207-
assertThat(bar.credential()).isInstanceOf(PasswordCredential.class);
207+
assertThat(bar.credential()).isInstanceOf(SshKeyCredential.class);
208208

209-
final PasswordCredential fooCredential = (PasswordCredential) foo.credential();
210-
final PasswordCredential barCredential = (PasswordCredential) bar.credential();
209+
final SshKeyCredential fooCredential = (SshKeyCredential) foo.credential();
210+
final SshKeyCredential barCredential = (SshKeyCredential) bar.credential();
211211

212212
assertThat(fooCredential.username()).isEqualTo("alice");
213-
assertThat(fooCredential.password()).isEqualTo("secret_a");
214213
assertThat(barCredential.username()).isEqualTo("bob");
215-
assertThat(barCredential.password()).isEqualTo("secret_b");
216214
}
217215

218216
@Test
@@ -243,8 +241,8 @@ void testMirrorWithCredentialId() {
243241

244242
final Mirror m = mirrors.get(0);
245243
assertThat(m.localRepo().name()).isEqualTo("qux");
246-
assertThat(m.credential()).isInstanceOf(PasswordCredential.class);
247-
assertThat(((PasswordCredential) m.credential()).username()).isEqualTo("alice");
244+
assertThat(m.credential()).isInstanceOf(SshKeyCredential.class);
245+
assertThat(((SshKeyCredential) m.credential()).username()).isEqualTo("alice");
248246
}
249247

250248
private List<Mirror> findMirrors() {
@@ -257,28 +255,34 @@ private List<Mirror> findMirrors() {
257255
private static List<Change<?>> upsertRawCredentials(String projectName) {
258256
final String aliceCredential = credentialName(projectName, "alice");
259257
final String bobCredential = credentialName(projectName, "bob");
258+
final String dummyKey = "-----BEGIN RSA PRIVATE KEY-----\\ntest\\n-----END RSA PRIVATE KEY-----";
260259
return ImmutableList.of(
261260
Change.ofJsonUpsert(
262261
credentialFile(aliceCredential),
263262
'{' +
264263
" \"name\": \"" + aliceCredential + "\"," +
265-
" \"type\": \"PASSWORD\"," +
264+
" \"type\": \"SSH_KEY\"," +
266265
" \"username\": \"alice\"," +
267-
" \"password\": \"secret_a\"" +
266+
" \"publicKey\": \"ssh-rsa AAAA\"," +
267+
" \"privateKey\": \"" + dummyKey + '"' +
268268
'}'),
269269
Change.ofJsonUpsert(
270270
credentialFile(bobCredential),
271271
'{' +
272272
" \"name\": \"" + bobCredential + "\"," +
273-
" \"type\": \"PASSWORD\"," +
273+
" \"type\": \"SSH_KEY\"," +
274274
" \"username\": \"bob\"," +
275-
" \"password\": \"secret_b\"" +
275+
" \"publicKey\": \"ssh-rsa BBBB\"," +
276+
" \"privateKey\": \"" + dummyKey + '"' +
276277
'}'));
277278
}
278279

279280
private static List<Credential> credentials(String projectName) {
281+
final String dummyKey = "-----BEGIN RSA PRIVATE KEY-----\ntest\n-----END RSA PRIVATE KEY-----";
280282
return ImmutableList.of(
281-
new PasswordCredential(credentialName(projectName, "alice"), "alice", "secret_a"),
282-
new PasswordCredential(credentialName(projectName, "bob"), "bob", "secret_b"));
283+
new SshKeyCredential(credentialName(projectName, "alice"), "alice",
284+
"ssh-rsa AAAA", dummyKey, null),
285+
new SshKeyCredential(credentialName(projectName, "bob"), "bob",
286+
"ssh-rsa BBBB", dummyKey, null));
283287
}
284288
}

‎server-mirror-git/src/test/java/com/linecorp/centraldogma/server/internal/mirror/MirroringTestUtils.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import com.cronutils.parser.CronParser;
2929

3030
import com.linecorp.centraldogma.server.credential.Credential;
31+
import com.linecorp.centraldogma.server.internal.credential.SshKeyCredential;
3132
import com.linecorp.centraldogma.server.mirror.Mirror;
3233
import com.linecorp.centraldogma.server.mirror.MirrorContext;
3334
import com.linecorp.centraldogma.server.mirror.MirrorDirection;
@@ -55,7 +56,8 @@ static <T extends Mirror> T newMirror(String remoteUri, Class<T> mirrorType,
5556

5657
static <T extends Mirror> T newMirror(String remoteUri, Cron schedule,
5758
Repository repository, Class<T> mirrorType) {
58-
final Credential credential = mock(Credential.class);
59+
final Credential credential = remoteUri.startsWith("git+ssh://") ? mock(SshKeyCredential.class)
60+
: mock(Credential.class);
5961
final Mirror mirror =
6062
new GitMirrorProvider().newMirror(
6163
new MirrorContext("mirror-id", true, schedule, MirrorDirection.LOCAL_TO_REMOTE,
@@ -73,7 +75,8 @@ static <T extends Mirror> T newMirror(String remoteUri, Cron schedule,
7375
}
7476

7577
static void assertMirrorNull(String remoteUri) {
76-
final Credential credential = mock(Credential.class);
78+
final Credential credential = remoteUri.startsWith("git+ssh://") ? mock(SshKeyCredential.class)
79+
: mock(Credential.class);
7780
final Mirror mirror = new GitMirrorProvider().newMirror(
7881
new MirrorContext("mirror-id", true, EVERY_MINUTE, MirrorDirection.LOCAL_TO_REMOTE,
7982
credential, mock(Repository.class), "/", URI.create(remoteUri), null, null));

0 commit comments

Comments
 (0)