luigidemasi commented on code in PR #27821:
URL: https://github.com/apache/camel/pull/27821#discussion_r4269349899
##########
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);
Review Comment:
**Critical / P1: Do not turn a discarded known-host entry into permission to
connect.**
With the bundled MINA SSHD 2.20.0, this reader silently discards a valid
OpenSSH entry whose host field is:
```text
![excluded.example.com]:2222,[db.example.com]:2222
```
MINA's pattern parser handles a leading bracket before it handles negation,
so the negated port-qualified pattern makes the entire line invalid to that
parser. OpenSSH's `ssh-keygen -F '[db.example.com]:2222'` recognizes the same
entry.
Consequently, a server that is actually listed is treated as unknown. With
the default `failOnUnknownHost=false`, an arbitrary different key is accepted.
I reproduced this through a real loopback SSH connection: the server presenting
the unlisted key received the synthetic password. The same probe against the
preceding verifier rejects the key before password authentication. With
`failOnUnknownHost=true`, the legitimate listed key is instead rejected.
Please support the valid negated port-qualified syntax and ensure parser
failures cannot silently remove the host's pinning constraint and activate
permissive unknown-host handling. Add regression tests for the legitimate key
and a different key with both values of `failOnUnknownHost`.
_AI-generated review by Codex on behalf of @luigidemasi._
##########
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())
Review Comment:
**Critical / P1: Enforce wildcard revocation on nondefault ports.**
`KnownHostEntry.isHostMatch(host, port)` treats an unqualified pattern as
port 22, including `*`. For a file containing:
```text
[db.example.com]:2222 <key-type> <key-data>
@revoked * <same-key-type> <same-key-data>
```
the normal entry matches but the revocation is skipped. The verifier accepts
the revoked key even with `failOnUnknownHost=true`. A real loopback SSH
connection using this arrangement authenticated and delivered the synthetic
password.
OpenSSH recognizes the wildcard revocation for the port-qualified host; I
confirmed this with `ssh-keygen -F`, and its [known_hosts
specification](https://man.openbsd.org/sshd.8#SSH_KNOWN_HOSTS_FILE_FORMAT)
documents the `@revoked *` form. This is a gap in the revocation support
introduced by this PR; the preceding verifier did not implement revocation.
Please make wildcard matching preserve the OpenSSH revocation semantics for
nondefault ports. Add tests for ports 22 and 2222, both values of
`failOnUnknownHost`, and revocation before and after an otherwise accepted
entry.
_AI-generated review by Codex on behalf of @luigidemasi._
##########
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 {
Review Comment:
**Required / P2: Reject unsupported markers instead of treating them as
ordinary trust entries.**
This `else` also handles every unrecognized marker. A file containing only
`@unknown db.example.com <key-type> <key-data>` accepts that key with
`failOnUnknownHost=true`. I reproduced this with the current verifier; the
preceding implementation rejects it, and OpenSSH does not recognize it as a
trusted entry.
The OpenSSH format permits only `@revoked` and `@cert-authority` markers. A
malformed or unsupported marker must not grant ordinary host-key trust. Please
explicitly distinguish unmarked entries from unsupported markers and reject
unsupported markers without falling through to permissive unknown-host
acceptance. Add negative tests with both values of `failOnUnknownHost`.
_AI-generated review by Codex on behalf of @luigidemasi._
--
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]
