SSH host key check

[tribut]

Allows specifying the SSH host key as follows:

dbs:
  - path: /path/to/local/db
    replicas:
      - url: sftp://USER@HOST:PORT/PATH
        key-path: /path/to/id_rsa
        host-key: ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIMvvypUkBrS9RCyV//p+UFCLg8yKNtTu/ew/cV6XXAAP

(key can be found in /etc/ssh/ssh_host_*.pub on the server)

The change is backwards-compatible and fixes #602 (GHSA-qpgw-j75c-j585).

If there is interest in this patch, I can update the documentation accordingly.

[tribut]

Strictly speaking, implementing TOFU would be the better fix and more in line with user expectations around SSH, but given that it would be more complex and user would have to touch their config anyway (to add persistent storage for the known hosts db) it didn't seem worth it to me. Let me know if you prefer that route.

[asymmetric]

For reference, there is a similar fix in this commit, which is not on the main branch.

[tribut]

Because this is apparently getting some attention, I want to mention that I don't particularly care if this PR or the other commit end up being merged (though I still think reading the host keys from files adds unnecessary complexity).

What I do think is important is to warn the user when host key checking is not done (which the other commit currently does not do). When using ssh, host key checking is the expectation and so we should at least make users aware that they are using a hijackable connection.

Thanks for your work on this project!

[tribut]

One more thing I remember now: The way both this PR and the other commit work, only the first host key offered by the server is checked. This means you have to use a specific host key (such as ecdsa), the others will be rejected. I would take another look at this if you want, but it's really a minor inconvenience and I would love to see this feature merged as-is with additional improvements later.

[corylanou]

Hi @tribut, thank you so much for this PR! This is a really important security fix that addresses CVE-2024-41254/GHSA-qpgw-j75c-j585, and we really appreciate you taking the time to implement this.

The implementation looks great:

• ✅ Properly addresses the security vulnerability by adding host key verification
• ✅ Maintains backward compatibility with a helpful warning when host key is not provided
• ✅ Clean implementation using ssh.FixedHostKey() with proper error handling
• ✅ Code builds and passes existing tests

This is excellent work! We have a few recommendations that would help round out the PR, but if you don't have the time, we're more than happy to take it from here and add these ourselves:

1. Test Coverage

It would be great to add a test case to cmd/litestream/main_test.go. Here's a suggested test that could go after the TestNewGCSReplicaFromConfig function:

func TestNewSFTPReplicaFromConfig(t *testing.T) {
    t.Run("WithHostKey", func(t *testing.T) {
        config := &main.ReplicaConfig{
            URL:     "sftp://user@example.com:22/path/to/backup",
            HostKey: "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIMvvypUkBrS9RCyV//p+UFCLg8yKNtTu/ew/cV6XXAAP",
        }
        _, err := main.NewReplicaFromConfig(config, nil)
        if err != nil {
            t.Fatal(err)
        }
        // Note: We can't easily test the actual client fields without refactoring
        // to export them or using reflection, but this tests that configuration parsing works
    })

    t.Run("WithoutHostKey", func(t *testing.T) {
        config := &main.ReplicaConfig{
            URL: "sftp://user@example.com:22/path/to/backup",
        }
        _, err := main.NewReplicaFromConfig(config, nil)
        if err != nil {
            t.Fatal(err)
        }
    })

    t.Run("InvalidHostKey", func(t *testing.T) {
        config := &main.ReplicaConfig{
            URL:     "sftp://user@example.com:22/path/to/backup",
            HostKey: "invalid-key-format",
        }
        // This should succeed at config parsing level, error would occur during Init
        _, err := main.NewReplicaFromConfig(config, nil)
        if err != nil {
            t.Fatal(err)
        }
    })
}

2. Documentation Example

It would be helpful to update the example configuration in etc/litestream.yml to show the new host-key field. Here's what we could add after line 9:

#      - url: sftp://user@host:22/path  # SFTP-based replication
#        key-path: /home/user/.ssh/id_rsa
#        # Optional: SSH host key for verification (recommended for security)
#        # Get this from the server's /etc/ssh/ssh_host_*.pub file
#        # Example formats:
#        # host-key: ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIMvvypUkBrS9RCyV//p+UFCLg8yKNtTu/ew/cV6XXAAP
#        # host-key: ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQ...

3. Integration Testing

If you've had a chance to test this with an actual SFTP server, it would be great to know that:

• Connection succeeds with correct host key
• Connection fails with incorrect host key
• Warning appears when no host key is specified

Again, thank you for this excellent contribution! Please let us know if you'd like to add these suggestions yourself, or if you'd prefer we take care of them. Either way works perfectly for us - we're just excited to get this security fix merged!

[corylanou]

Review Summary & Status Update

After reviewing this PR and analyzing the current codebase, I want to provide a status update on this important security fix:

Current Situation

• Security Issue Still Present: The main branch continues to use ssh.InsecureIgnoreHostKey() without any host key verification (sftp/replica_client.go:75)
• This PR is Still Needed: The functionality proposed here has NOT been merged and addresses CVE-2024-41254/GHSA-qpgw-j75c-j585

Analysis of Similar Work

The referenced "similar commit" (f6c8590) from Jan 2022:

• Was never merged into main
• Used a different approach (HostKeyPath reading from file vs this PR's direct host-key string in config)
• Lacked the important warning when host key verification is disabled

Why This PR Should Be Merged

1. Critical Security Fix: Addresses a known vulnerability where SFTP connections are susceptible to MITM attacks
2. Better Implementation:
   • Direct key in config is simpler than requiring a separate file
   • Includes crucial warning when host key checking is disabled (meeting user expectations for SSH)
   • Maintains backward compatibility
3. Clean Code: The implementation is straightforward and follows the existing codebase patterns

Recommended Next Steps

@tribut's implementation is excellent as-is. The suggestions from @corylanou above (tests, documentation) would make this production-ready.

Timeline: We'll give this a week for any updates from @tribut. If there's no response by then, we're happy to:

1. Apply this PR's changes
2. Add the test coverage
3. Update the documentation examples
4. Get this security fix merged

Thank you @tribut for this important contribution! Please let us know if you'd like to complete the additions yourself, or if you'd prefer we take it from here. Either way works - we just want to ensure this security fix gets into the codebase.

cc @benbjohnson @corylanou for visibility on this security fix status

[tribut]

Hey, thanks for getting back on this. I can add both the documentation and tests as well as an updated implementation that avoids verifying the first ssh host key only (cf. #609 (comment)) sometime this week.
If you want to get it done earlier, feel free to do it without me. I would appreciate a short ping in that case.

[asymmetric]

Is this gonna land in 0.5.0, for which there's an RC out?

[corylanou]

Is this gonna land in 0.5.0, for which there's an RC out?

Yes

[tribut]

Sorry this took a bit longer than promised. The AI generated suggestions in #609 (comment) didn't actually test the host key check.

I've now added tests for both the configuration as well as checks for matching host key, invalid host key and no host key validation configured.

Not sure where the source for https://litestream.io/reference/config/#sftp-replica lives, but we should definitely add an example there.

[github-actions]

✅ Manual integration tests have been run by @corylanou

View test results

[asymmetric]

Is this gonna land in 0.5.0, for which there's an RC out?

Yes

@corylanou seems like this didn't land in 0.5.0 after all?

[corylanou]

Is this gonna land in 0.5.0, for which there's an RC out?

Yes

@corylanou seems like this didn't land in 0.5.0 after all?

Sorry about that, we had a lot in the air and we needed to get a release with at least the current feature set. This will be one of the top things I work on as soon as we stabilize the current RC. Thanks for the efforts, they are greatly appreciated. We didn't forget you :-).

[benbjohnson]

Sorry for the delay on this! This looks great and it's merged in now. It'll go into v0.5.3.