gnodet-bot commented on code in PR #27821:
URL: https://github.com/apache/camel/pull/27821#discussion_r4269272364
##########
components/camel-ssh/src/main/java/org/apache/camel/component/ssh/ResourceBasedSSHKeyVerifier.java:
##########
@@ -18,30 +18,39 @@
import java.io.IOException;
import java.io.InputStream;
-import java.math.BigInteger;
import java.net.InetSocketAddress;
import java.net.SocketAddress;
-import java.security.NoSuchAlgorithmException;
+import java.security.GeneralSecurityException;
import java.security.PublicKey;
-import java.security.spec.InvalidKeySpecException;
-import java.util.Base64;
-import java.util.LinkedList;
+import java.util.Collections;
import java.util.List;
import org.apache.camel.CamelContext;
import org.apache.camel.support.ResourceHelper;
import org.apache.camel.util.IOHelper;
+import org.apache.sshd.client.config.hosts.KnownHostEntry;
import org.apache.sshd.client.keyverifier.ServerKeyVerifier;
import org.apache.sshd.client.session.ClientSession;
-import org.bouncycastle.util.Arrays;
+import org.apache.sshd.common.config.keys.KeyUtils;
+import org.apache.sshd.common.config.keys.PublicKeyEntryResolver;
+import org.apache.sshd.common.util.net.SshdSocketAddress;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* ServerKeyVerifier that takes a camel resource as input file to validate the
server key against.
- *
+ * <p/>
Review Comment:
💡 **Nit:** `<p/>` is not valid HTML — block elements don't self-close. Use
`<p>` (without the slash).
##########
components/camel-ssh/src/main/java/org/apache/camel/component/ssh/ResourceBasedSSHKeyVerifier.java:
##########
@@ -62,22 +71,53 @@ public ResourceBasedSSHKeyVerifier(CamelContext
camelContext, String knownHostsR
@Override
public boolean verifyServerKey(ClientSession sshClientSession,
SocketAddress remoteAddress, PublicKey serverKey) {
log.debug("Trying to find known_hosts file {}", knownHostsResource);
+ List<KnownHostEntry> entries = Collections.emptyList();
InputStream knownHostsInputStream = null;
try {
knownHostsInputStream =
ResourceHelper.resolveMandatoryResourceAsInputStream(camelContext,
knownHostsResource);
- List<String> possibleTokens =
getKnownHostsFileTokensForSocketAddress(remoteAddress);
- log.debug("Trying to match PublicKey against provided known_hosts
file");
- PublicKey matchingKey =
findKeyForServerToken(knownHostsInputStream, possibleTokens);
- if (matchingKey != null) {
- log.debug("Found PublicKey match for server");
- return Arrays.areEqual(matchingKey.getEncoded(),
serverKey.getEncoded());
- }
+ entries =
KnownHostEntry.readKnownHostEntries(knownHostsInputStream, false);
} catch (IOException ioException) {
log.debug(String.format("Could not find known_hosts file %s",
knownHostsResource), ioException);
} finally {
IOHelper.close(knownHostsInputStream);
}
+
+ SshdSocketAddress server = toServerAddress(remoteAddress);
+ boolean hostListed = false;
+ boolean keyListed = false;
+ if (server != null) {
+ log.debug("Trying to match PublicKey of {} against provided
known_hosts file", server);
+ for (KnownHostEntry entry : entries) {
+ if (!entry.isHostMatch(server.getHostName(), server.getPort())
+ || MARKER_CERT_AUTHORITY.equals(entry.getMarker())) {
+ continue;
+ }
+ boolean sameKey = isSameKey(sshClientSession, entry,
serverKey);
+ if (MARKER_REVOKED.equals(entry.getMarker())) {
+ if (sameKey) {
+ log.warn("The key presented by {} is marked as revoked
in the known_hosts file {}", server,
+ knownHostsResource);
+ return false;
+ }
+ } else {
+ hostListed = true;
+ if (sameKey) {
+ keyListed = true;
+ }
+ }
+ }
Review Comment:
⚠️ **Bug: `@revoked`-only host bypasses the listed-host rejection gate with
`failOnUnknownHost=false`**
When a host has *only* `@revoked` entries and the server presents a key that
is **not** in those entries, the code falls through to the `failOnUnknownHost`
branch and accepts the connection when that flag is `false`.
Trace:
1. `isHostMatch(host)` → `true` for the `@revoked` entry
2. `isSameKey(serverKey, revokedKey)` → `false` (different key)
3. `MARKER_REVOKED` branch: `sameKey=false` → no early return, `hostListed`
is **not set**
4. Loop ends: `hostListed=false`, `keyListed=false`
5. Falls through to `failOnUnknownHost` → with `false` → **returns `true`**
→ accepts
The PR description states: *"A host that has entries is rejected when it
presents a key that none of them lists, whatever the value of
`failOnUnknownHost`."* The current code does not implement this for hosts whose
only entries are `@revoked`. The fix is to track revoked entries in
`hostListed` as well — a revoked-only-listed host should still be treated as
"listed" when a non-matching key is presented.
```suggestion
if (MARKER_REVOKED.equals(entry.getMarker())) {
hostListed = true;
if (sameKey) {
log.warn("The key presented by {} is marked as
revoked in the known_hosts file {}", server,
knownHostsResource);
return false;
}
} else {
hostListed = true;
if (sameKey) {
keyListed = true;
}
}
```
And add a test to cover this:
```java
@Test
void rejectsUnknownKeyWhenHostHasOnlyRevokedEntries() throws Exception {
// A host with only @revoked entries still counts as "listed" —
// a different key must not slip through via failOnUnknownHost=false
String knownHosts = "@revoked db.example.com " +
PublicKeyEntry.toString(otherKey) + "\n";
assertFalse(verify(true, "db.example.com", 22, serverKey, knownHosts));
assertFalse(verify(false, "db.example.com", 22, serverKey, knownHosts));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
