review2

[masahide]

総評

構成は小さく整理されており、並列接続、SSH Agent、出力順制御を分離しようとしている点は良いです。単体テストも主要関数に用意されています。

一方で、セキュリティ既定値、接続失敗時の制御、SSH Agent接続管理には本番利用前に修正すべき問題があります。

重要度 高

1. 脆弱なSSHアルゴリズムを既定で優先している

対象:

• cmd/gopssh/main.go:27-54
• pkg/pssh/pssh.go:242-248

既定値に次の旧式アルゴリズムが含まれています。

• arcfour256
• diffie-hellman-group1-sha1
• diffie-hellman-group14-sha1
• diffie-hellman-group-exchange-sha1
• hmac-sha1-96

さらにリストの前方に配置されているため、接続先が対応している場合は弱い方式が選択される可能性があります。

defaultKexAlgos = []string{
    ssh.InsecureKeyExchangeDH1SHA1,
    ssh.InsecureKeyExchangeDH14SHA1,
    ...
}

defaultCiphersFlags = []string{
    ssh.InsecureCipherRC4256,
    ...
}

通常は ssh.Config の既定値を使用し、レガシー機器への接続時だけ明示的に追加する設計が安全です。

推奨方針:

Config: ssh.Config{}

レガシー対応が必要なら、例えば --legacy-crypto のような明示的なオプションで有効化します。

---

2. 1台の接続失敗で全ホストの処理が中断される

対象:

• pkg/pssh/conwork.go:37-44
• pkg/pssh/pssh.go:257-263
• pkg/pssh/pssh.go:306-352

接続に失敗すると conInstances にエラーを送信します。

if err != nil {
    res.err = fmt.Errorf("cannot connect [%s] err:%s", c.host, err)
    instanceCh <- res
    return
}

受信側では最初のエラーで全体のコンテキストをキャンセルしています。

if ierr := p.getConInstanceErrs(); ierr != nil {
    log.Print(ierr)
    cancel()
}

その結果、100台中1台だけ接続できない場合でも、残り99台のコマンド実行が中止されます。接続に失敗したホストは通常の result にも送られないため、ホストごとの結果表示も不完全です。

並列SSHツールとしては、各ホストの失敗を独立した結果として扱う方が自然です。

if err != nil {
    results <- &result{
        conID: c.id,
        code:  255,
        err:   fmt.Errorf("connect %s: %w", c.host, err),
    }
    return
}

全体キャンセルは、シグナル受信や明示的なフェイルファスト指定時だけに限定するのがよいです。

---

3. SSH Agent接続失敗でライブラリ内部からプロセス終了する

対象:

• pkg/pssh/agent_wrap.go:44-48
• pkg/pssh/agent_wrap.go:86-99

sync.Pool.New から呼ばれる処理で log.Fatal を使用しています。

func (cp *connPools) newConnPool() any {
    ka, err := cp.newKeyAgent()
    if err != nil {
        log.Fatal(err)
    }
    return ka
}

SSH Agentが停止した、ソケットが消えた、接続数制限に達したといった状況で、gopssh全体が即時終了します。log.Fatal は os.Exit を呼ぶため、呼び出し元でエラーを処理できません。

また、dialSocket は一時的でないエラーを握りつぶしています。

authConn, err = cp.netDialer.Dial("unix", cp.sockFile)
if err != nil {
    if terr, ok := err.(TemporaryError); ok && terr.Temporary() {
        return err
    }
}
return nil

永続的なエラーの場合、authConn == nil かつ err == nil になり、実際の原因が失われます。

最低限、次のように元のエラーを返すべきです。

if err != nil {
    if terr, ok := err.(TemporaryError); ok && terr.Temporary() {
        return err
    }
    return backoff.Permanent(err)
}

より良い設計は Get がエラーを返す方式です。

func (cp *connPools) Get() (*keyAgent, error)

---

4. 設定値によってパニックまたは永久待機する

対象:

• pkg/pssh/pssh.go:137-143
• pkg/pssh/agent_wrap.go:52-58
• pkg/pssh/agent_wrap.go:76-84

Concurrency が負の場合は make がパニックします。

p.concurrentGoroutines = make(chan struct{}, p.Concurrency)

MaxAgentConns がゼロの場合は、SSH Agent利用時に永久待機します。

cp.limit = make(chan struct{}, n)

func (cp *connPools) Get() *keyAgent {
    cp.limit <- struct{}{}
    return cp.connPool.Get().(*keyAgent)
}

容量ゼロのチャネルに送信しますが、対応する受信処理は Get が完了した後なので進行できません。

CLI解析後に検証が必要です。

if c.Concurrency < 0 {
    return errors.New("concurrency must be zero or greater")
}
if c.MaxAgentConns <= 0 {
    return errors.New("max agent connections must be greater than zero")
}

重要度 中

5. 既定で無制限の接続と全出力のメモリ保持を行う

対象:

• cmd/gopssh/main.go:66-78
• pkg/pssh/pssh.go:166-175
• pkg/pssh/pssh.go:286-292
• pkg/pssh/sessionwork.go:61-83

Concurrency の既定値はゼロで、全ホストに対して同時にSSH接続します。

また、各ホストの標準出力と標準エラーを bytes.Buffer に全量保存します。

go readStream(ctx, res.stdout, stdout, errChs[0])
go readStream(ctx, res.stderr, stderr, errChs[1])

ホスト数やコマンド出力が大きい場合、ファイルディスクリプタ、メモリ、ネットワーク接続を急激に消費します。

既定の同時実行数を32から64程度に制限し、非ソートモードでは逐次ストリーミングする設計を推奨します。ソートモードでは出力上限や一時ファイルへの退避が必要です。

---

6. ホストファイルのIPv6とコメントを正しく扱えない

対象:

• pkg/pssh/pssh.go:182-199

現在は正規表現でコロンの有無だけを調べています。

var re = regexp.MustCompile(":.+")

if !re.MatchString(res[i]) {
    res[i] += ":22"
}

次のような問題があります。

• ::1 はポート指定済みと判定されるが、SSH接続先としては [::1]:22 が必要
• host: は host::22 になる
• # production servers が複数のホストとして解釈される
• 行単位ではなく空白単位なので、行末コメントを扱えない

行単位で読み込み、コメントを除去してから net.SplitHostPort と net.JoinHostPort を使うべきです。

---

7. 既定のソート出力ではバッファをプールへ返していない

対象:

• pkg/pssh/pssh.go:306-329
• pkg/pssh/pssh.go:339-354

非ソート版では処理後にバッファを返しています。

p.delReslt(res)

ソート版には同じ処理がありません。

p.printResult(resSlise[j], cws[resSlise[j].conID].host)

既定値は SortPrint: true なので、通常利用では sync.Pool がほとんど機能しません。特に標準エラーのバッファは内容を保持したままGC待ちになります。

p.printResult(resSlise[j], ...)
p.delReslt(resSlise[j])
resSlise[j] = nil

とする必要があります。

---

8. 標準エラーが標準出力へ書き込まれる

対象:

• pkg/pssh/pssh.go:55-65
• pkg/pssh/pssh.go:356-381

print は os.Stdout のみを保持し、リモート側の標準エラーもカラー出力経由で標準出力へ出ています。

p.red.Print(res.stderr.String())

これにより、次のような処理でエラー文字列が出力ファイルへ混入します。

gopssh ... > result.txt

stdout と stderr のWriterを分離すべきです。

type print struct {
    stdout io.Writer
    stderr io.Writer
}

また、printResult 内の次の行はWriter抽象化を無視しています。

res.stdout.WriteTo(os.Stdout)

p.output などへ統一する必要があります。

---

9. -i を指定するとSSH Agentが暗黙に無効になる

対象:

• cmd/gopssh/main.go:97
• pkg/pssh/pssh.go:423-430

c.IdentityFileOnly = identityFiles != defaultIdentityFiles

独自鍵を1つ指定しただけで、SSH Agentが使われなくなります。

gopssh -i ~/.ssh/project_key ...

この挙動はフラグ説明に記載されていません。-i とAgentは併用し、Agentを無効化する場合だけ --identities-only のような別フラグを使う方が明確です。

---

10. 接続監視用ゴルーチンが終了しない

対象:

• pkg/pssh/pssh.go:252-263
• pkg/pssh/pssh.go:296-304

conInstances は作成されますが、どこからもクローズされません。

for con := range p.conInstances {
    ...
}

全ホストが成功した場合、このゴルーチンは永久に待機します。CLIプロセスは直後に終了するため目立ちませんが、pkg/pssh をライブラリとして繰り返し呼ぶとゴルーチンリークになります。

前述のとおり、接続失敗も通常の result に統合すれば、このチャネル自体を削除できます。

リリース関連

11. パッケージファイルを上書きすると壊れる可能性がある

対象:

• pack/debpack/main.go:49
• pack/rpmpack/main.go:57

os.OpenFile(filename, os.O_CREATE|os.O_WRONLY, 0644)

os.O_TRUNC がないため、同名の既存ファイルより新しい内容が短い場合、末尾に古いデータが残ります。

os.OpenFile(
    filename,
    os.O_CREATE|os.O_WRONLY|os.O_TRUNC,
    0o644,
)

---

12. prereleaseとlatestダウンロードURLが矛盾している

対象:

• .github/workflows/buildpkg.yml:163-170
• releasenote.template.md:10-26

GitHub Releaseは常に次の指定です。

prerelease: true

一方、リリースノートのインストールURLは releases/latest/download を参照しています。通常、latestはprereleaseを新しい安定版として扱わないため、作成直後の成果物を指しません。

安定版なら prerelease: false にし、prereleaseを継続するならタグ固有のURLを生成する必要があります。

---

13. Homebrew Formulaの内容が生成バージョンと一致しない

対象:

• pack/Formula/template.rb:5-8
• pack/Formula/template.rb:37-39

次の問題があります。

• 説明が別プロジェクトのものになっている
• バージョンが常に 1.0.0
• テストが存在しない -v オプションを使用している

desc "AWS assume role credential wrapper"
version "1.0.0"

test do
  system "#{bin}/gopssh -v"
end

少なくとも次のような内容が必要です。

desc "Parallel SSH client"

test do
  system bin/"gopssh", "-version"
end

テストとCI

14. カバレッジファイルの結合方法が不正

対象:

• Makefile:23-31

cat main_coverage.txt pkg_coverage.txt > coverage.txt

両ファイルに mode: atomic ヘッダーが含まれるため、結合後のファイルを go tool cover が解析できません。これは簡易的なカバレッジファイルで再現できました。

例えば次のように2番目以降のヘッダーを除去します。

head -n 1 main_coverage.txt > coverage.txt
tail -n +2 main_coverage.txt >> coverage.txt
tail -n +2 pkg_coverage.txt >> coverage.txt

より単純には、最初から一度の go test で取得します。

go test -race -covermode=atomic \
  -coverprofile=coverage.txt ./...

---

15. 重要な失敗経路がテストされていない

主な例:

• cmd/gopssh/main_test.go:36-58

  • wantExit を一度も検証していない
  • 非終了ケースでは戻り値も検証していない
  • os.Args とグローバルFlagSetを復元していない

• pkg/pssh/conwork_test.go:63-102

  • 接続エラー時に即座にコンテキストをキャンセルしており、conInstances の内容を検証していない

• pkg/pssh/pssh_test.go

  • HOME やグローバルLoggerを変更したまま復元していない

• .github/workflows/buildpkg.yml:38-49

  • CIでは go test のみで、-race とLintを実行していない

テストでは t.Setenv と t.Cleanup を利用し、グローバル状態を確実に復元するべきです。

実行確認結果

アーカイブ内の全Goファイルを確認し、gofmt -d では差分はありませんでした。

go test -race ./... も試しましたが、プロジェクトがGo 1.26.5を要求している一方、実行環境のローカルGoは1.23.2でした。自動ツールチェーン取得はネットワーク制限で失敗したため、今回はテスト実行結果までは確認できていません。

修正優先順位

最初の修正単位としては次の順が妥当です。

1. 弱いSSHアルゴリズムを既定値から削除
2. ホスト単位で接続エラーを結果化し、全体キャンセルを廃止
3. SSH Agentプールから log.Fatal を除去
4. Concurrency と MaxAgentConns の入力検証
5. 無制限並列と出力バッファリングの上限設定
6. IPv6とコメント対応のホストパーサーへ置換
7. パッケージ生成とCIの修正

現状は、小規模な管理対象へ手動実行する用途では動作し得ますが、不達ホストを含む多数台への実行や、本番運用ツールとして配布する段階では上位4件の修正が必要です。