Skip to content
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

[SFTP] [v6 Regression] ResourceKnownHostsServerKeyVerifier does not match by IP #8693

Open
jeanblanchard opened this issue Aug 1, 2023 · 11 comments
Labels

Comments

@jeanblanchard
Copy link

In what version(s) of Spring Integration are you seeing this issue?

  • 6.0.5 (Spring Boot 3.0.6)
  • Still in the main branch

Describe the bug

When using a DefaultSftpSessionFactory with knownHostsResource set, the known_hosts are matched only by hostname, and not by IP.

Even if host is set to an IP address in the configuration, only its reverse DNS is matched.

Exception from the log (redacted)

o.a.s.client.session.ClientSessionImpl   : exceptionCaught(ClientSessionImpl[username@1-113-0-203.static-ip.example.com/203.0.113.1:2222])[state=Opened] SshException: Server key did not validate

To Reproduce

Configure a DefaultSftpSessionFactory, with host set to an IP address, and knownHostsResource set to a classpath known_host file with a public key configured for the IP.

Expected behavior

The known_hosts should be matched with the IP from the host config param, like it did in Spring Integration SFTP < 6.

Analyzing a bit more

ResourceKnownHostsServerKeyVerifier.resolveHostNetworkIdentities() calls SshdSocketAddress.toSshdSocketAddress, which always fetches the hostname from the connect address's IP.

This behavior appears to be copied from Apache Mina's KnownHostsServerKeyVerifier. It might well be on purpose on their side to only use hostnames, but this is a regression from the behavior in previous versions of Spring Integration SFTP.

Maybe ResourceKnownHostsServerKeyVerifier.resolveHostNetworkIdentities() should (in addition?) return the raw address from clientSession.getConnectAddress()?

@jeanblanchard jeanblanchard added status: waiting-for-triage The issue need to be evaluated and its future decided type: bug labels Aug 1, 2023
@artembilan
Copy link
Member

It is hard to test this with mocks, so I'd be glad to see more stack trace for that error to determine where exactly we should catch it and fallback to the IP address.

Correct me if I'm wrong.

  1. You have an entry in the known_hosts which has only an IP address: https://en.wikibooks.org/wiki/OpenSSH/Client_Configuration_Files#About_the_Contents_of_the_known_hosts_Files
  2. You configure exactly same IP address for an SFTP session.
  3. This IP address is resolved to its host name, but it doesn't match to the entry in the known_hosts because there is no host name indicator for the key.

As a workaround you can specify that host name in the known_hosts entry according to the doc mentioned above:

It is possible to use a comma-separated list of hosts in the host name field if a host has multiple names

Does it all make sense?

@artembilan artembilan added status: waiting-for-reporter Needs a feedback from the reporter in: sftp and removed status: waiting-for-triage The issue need to be evaluated and its future decided labels Aug 1, 2023
@artembilan
Copy link
Member

Another workaround is to provide an externally configured SshClient:

	/**
	 * Instantiate based on the provided {@link SshClient}, e.g. some extension for HTTP/SOCKS.
	 * @param sshClient the {@link SshClient} instance.
	 * @param isSharedSession true if the session is to be shared.
	 */
	public DefaultSftpSessionFactory(SshClient sshClient, boolean isSharedSession) {

This way you can inject any custom setServerKeyVerifier().
But if you can do that, why just don't come back to us with a contribution for a fix you are asking here for?
I'm telling this because I still don't see the whole picture where we are failing...

@artembilan
Copy link
Member

artembilan commented Aug 3, 2023

Well, I have looked into the previous 5.5.x version which is based on JSCH SSH implementation, and it does nothing for the provided host or the entry from the known_hosts.
It is just:

return hosts.regionMatches(true, i, _host, 0, hostlen);

Where _host is the value we provide for DefaultSftpSessionFactory and hosts is the host part of the known_hosts entry, like in the sample of the mentioned doc:

[anga.funkfeuer.at]:2022,[78.41.115.130]:2022 ssh-rsa AAAAB...fgTHaojQ==	

So, no way it is going to work in that version if there is no IP variant in the entry.
Therefore it is hard to claim that it worked before since it has met your current requirements, but may fail in other use-cases.

But that is only if I look into a proper direction for what I'd like to hear from you back.

Thank

@sebastianfilke
Copy link

We have the exact same issue. If i only downgrade from version 6.1.5 spring-integration-sftp to version 5.5.20 it works! i have normal known_hosts file with a host name without ip-adress.

Of course if have to change a bit the code because of the missing IntegrationFlows in version 6.1.5. for example if have to change:

@Bean
    public IntegrationFlow sftpSendFlow(SessionFactory<SftpClient.DirEntry> sftpSessionFactory) {
        return IntegrationFlow.from("sftpSendChannel").handle(Sftp.outboundAdapter(sftpSessionFactory, FileExistsMode.REPLACE).useTemporaryFileName(false)
                .remoteDirectoryExpression("headers['remoteDirectory']").fileNameGenerator(SftpEndpointFactory::createFileName)).get();
    }

back to

@Bean
    public IntegrationFlow sftpSendFlow(SessionFactory<ChannelSftp.LsEntry> sftpSessionFactory) {
        return IntegrationFlows.from("sftpSendChannel").handle(Sftp.outboundAdapter(sftpSessionFactory, FileExistsMode.REPLACE).useTemporaryFileName(false)
                .remoteDirectoryExpression("headers['remoteDirectory']").fileNameGenerator(SftpEndpointFactory::createFileName)).get();
    }

or change the

@Bean
    public SessionFactory<SftpClient.DirEntry> sftpSessionFactory(SftpDokumenterkennungConfig sftpDokumenterkennungConfig) {
        DefaultSftpSessionFactory defaultSftpSessionFactory = new DefaultSftpSessionFactory();
        defaultSftpSessionFactory.setKnownHostsResource(new ClassPathResource(KNOWN_HOSTS_FILE_NAME));
        defaultSftpSessionFactory.setHost(sftpDokumenterkennungConfig.getHost());
        defaultSftpSessionFactory.setPort(sftpDokumenterkennungConfig.getPort());
        defaultSftpSessionFactory.setUser(sftpDokumenterkennungConfig.getUser());
        defaultSftpSessionFactory.setPassword(sftpDokumenterkennungConfig.getPassword());

        return new CachingSessionFactory<>(defaultSftpSessionFactory);
    }

back to

@Bean
    public SessionFactory<ChannelSftp.LsEntry> sftpSessionFactory(SftpDokumenterkennungConfig sftpDokumenterkennungConfig) {
        DefaultSftpSessionFactory defaultSftpSessionFactory = new DefaultSftpSessionFactory();
        defaultSftpSessionFactory.setKnownHostsResource(new ClassPathResource(KNOWN_HOSTS_FILE_NAME));
        defaultSftpSessionFactory.setHost(sftpDokumenterkennungConfig.getHost());
        defaultSftpSessionFactory.setPort(sftpDokumenterkennungConfig.getPort());
        defaultSftpSessionFactory.setUser(sftpDokumenterkennungConfig.getUser());
        defaultSftpSessionFactory.setPassword(sftpDokumenterkennungConfig.getPassword());

        return new CachingSessionFactory<>(defaultSftpSessionFactory);
    }

but thats only very smal changes like i mentioned IntegrationFlows to IntegrationFlow or SftpClient.DirEntry to ChannelSftp.LsEntry

@artembilan
Copy link
Member

Sorry, @sebastianfilke , but that's not helpful.
What you show is a change between Spring Integration 5.5.x and 6.x.
The real problem that we have migrated SFTP client from JCraft Jsch to Apache MINA and the last one does not support host resolution as it was in the former one.
Therefore it would be great to have a fix in Apache MINA or as a workaround here in Spring Integration.
However for the workaround I need to understand how to test the problem and know what to do to mitigate it.
No one yet gave me the info is needed to work with.

Perhaps you can share what your known_hosts looks like and how you configure DefaultSftpSessionFactory to connect with that key.

Thanks for understanding!

@laguiar
Copy link

laguiar commented May 17, 2024

Hi @artembilan, is there any update on this topic?

I have the same issue as mentioned above, after migrating SB from 2.x to 3.2.

My known_hosts is structured as:

sftp.server.domain.com ssh-rsa AAAAB3Nz...

and set on DefaultSftpSessionFactory via sessionFactory.setKnownHostsResource(resource);

@artembilan
Copy link
Member

Hi @laguiar !

Thank you for reaching out!

Would you mind to share more info?
What is your DefaultSftpSessionFactory configuration?
How does error looks like?

As you see in all the comments, no one has provided a proper environment to determine where we exactly are failing to find a path for fix.

@laguiar
Copy link

laguiar commented May 17, 2024

Hi @laguiar !

Thank you for reaching out!

Would you mind to share more info? What is your DefaultSftpSessionFactory configuration? How does error looks like?

As you see in all the comments, no one has provided a proper environment to determine where we exactly are failing to find a path for fix.

Sure... it's probably very standard, I don't have much customisation on this component.

    public DefaultSftpSessionFactory defineSftpSessionFactory() {
        var sessionFactory = new DefaultSftpSessionFactory();
        sessionFactory.setHost(host);
        sessionFactory.setPort(port);
        sessionFactory.setUser(user);
        sessionFactory.setPrivateKey(privateKey);
        sessionFactory.setPrivateKeyPassphrase(privateKeyPassphrase);
        sessionFactory.setTimeout(Math.toIntExact(timeout.toMillis()));
        sessionFactory.setAllowUnknownKeys(true);
        sessionFactory.setKnownHostsResource(knownHostsFile);
        var hostConfig = defineHostConfig();
        sessionFactory.setHostConfig(hostConfig);
        return sessionFactory;
    }

    private HostConfigEntry defineHostConfig() {
        var hostConfig = new HostConfigEntry(host, host, port, user);
        hostConfig.setProperty("MaxAuthTries", "3");
        hostConfig.setProperty("PreferredAuthentications", "publickey");
        return hostConfig;
    }

@artembilan
Copy link
Member

And what is that host?
What is the content of your knownHostsFile?
What is the exception you got?

This is still so generic that it does not give any clues what might be wrong in the framework.
And that's why this issue is still opened: no one provides any hints how to reproduce or what exactly doesn't work and has to be fixed respectively.

Thanks for understanding!

@jgormley6
Copy link

jgormley6 commented May 17, 2024

@artembilan
I just encountered a similar issue with my local integration tests. Not sure if it is the issue the original poster had, but it looks like @laguiar might at least be running into the same problem.

I believe my issue is related to this

ServerKeyVerifier serverKeyVerifier =
this.allowUnknownKeys ? AcceptAllServerKeyVerifier.INSTANCE : RejectAllServerKeyVerifier.INSTANCE;
if (this.knownHosts != null) {
serverKeyVerifier = new ResourceKnownHostsServerKeyVerifier(this.knownHosts);
}

Previously, if you specified both setAllowUnknownKeys as true and the setKnownHostsResource the key verification would be skipped regardless of what setKnownHostsResource was.

        // this session factory configuration works with 5.x.x but breaks with 6.x.x
        sessionFactory.setAllowUnknownKeys(true);
        sessionFactory.setKnownHostsResource(knownHostsFile); // knownHostsFile pointing to /dev/null

But now if the knownHosts file is ever not null, the allowUnknownKeys configuration is ignored.

For my case, the integration tests specified the knownHostsFile as /dev/null and allow unknown keys as true as we don't really care for the validations in our local integration tests. Setting the knownHostsFile to be null removed the error.

It's possible that users have a bad knownHosts file and are relying on setAllowUnknownKeys

@artembilan
Copy link
Member

@jgormley6 ,

thank you for sharing your experience, but I don't see how your problem is related to the original report.
For your use-case see respective Javadocs:

	/**
	 * When no {@link #knownHosts} has been provided, set to true to unconditionally allow
	 * connecting to an unknown host or when a host's key has changed (see
	 * {@link #setKnownHostsResource(Resource) knownHosts}). Default false (since 4.2).
	 * Set to true if a knownHosts file is not provided.
	 * @param allowUnknownKeys true to allow connecting to unknown hosts.
	 * @since 4.1.7
	 */
	public void setAllowUnknownKeys(boolean allowUnknownKeys) {

So, the logic you show is correct and it really reflects those Javadocs.

I see an inconvenient with your configuration expectations, but that is what we might revise in the next 6.4 version.

Feel free to raise a new GH issue!
However this one is about host/IP resolution problem which I'm still failed to compile from this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants