Leon/issue 40 and 42 #43
Conversation
WalkthroughTLS setup now requires explicit Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as tapir-cli
participant Cfg as Config
participant FS as Filesystem
participant TLS as TLS Builder
participant API as API Client
CLI->>Cfg: Load config
alt UseTLS == true
Cfg-->>CLI: certs.cert, certs.key, certs.cacertfile
CLI->>FS: os.Stat(cert), os.Stat(key), os.Stat(cacert)
alt Any missing/inaccessible
CLI-->>CLI: Log fatal and exit
else All accessible
CLI->>TLS: NewClientConfig(cacert, key, cert)
alt TLS init fails
CLI-->>CLI: Log fatal and exit
else Success
CLI->>API: SetupTLS(tlsConfig)
API-->>CLI: Client ready
end
end
else UseTLS == false
CLI->>API: Setup()
API-->>CLI: Client ready
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/root.go (1)
139-142: Don’t ship with InsecureSkipVerify = true by default. Gate it via config and default to safe.This disables hostname/cert verification and enables trivial MITM. Make it opt-in via config (e.g., cli.tapir-pop.tls.insecure_skip_verify).
Apply:
- tlsConfig.InsecureSkipVerify = true + if viper.GetBool("cli.tapir-pop.tls.insecure_skip_verify") { + tlsConfig.InsecureSkipVerify = true + }Nit: in the comment above, s/VerifyPerrCertificate/VerifyPeerCertificate/.
🧹 Nitpick comments (2)
cmd/root.go (2)
121-127: Namespace TLS config keys to avoid collisions and improve clarity.Using top-level keys like certs.cert may clash with other components. Prefer a scoped path such as cli.tapir-pop.tls.{cert,key,cacertfile}.
Apply:
- certPath := viper.GetString("certs.cert") - keyPath := viper.GetString("certs.key") - caCertPath := viper.GetString("certs.cacertfile") + certPath := viper.GetString("cli.tapir-pop.tls.cert") + keyPath := viper.GetString("cli.tapir-pop.tls.key") + caCertPath := viper.GetString("cli.tapir-pop.tls.cacertfile") - if certPath == "" || keyPath == "" || caCertPath == "" { - log.Fatalf("Error: missing TLS config keys: certs.cert, certs.key and/or certs.cacertfile in %s", viper.ConfigFileUsed()) - } + if certPath == "" || keyPath == "" || caCertPath == "" { + log.Fatalf("Error: missing TLS config keys: cli.tapir-pop.tls.cert, cli.tapir-pop.tls.key and/or cli.tapir-pop.tls.cacertfile in %s", viper.ConfigFileUsed()) + }
128-132: Avoid TOCTOU and non-reliable readability checks; rely on load step or open files.os.Stat doesn’t guarantee readability and introduces TOCTOU. Either drop the pre-check and rely on NewClientConfig errors, or open/close files to assert readability.
Option A (simpler — rely on load):
- for _, f := range []string{certPath, keyPath, caCertPath} { - if _, statErr := os.Stat(f); statErr != nil { - log.Fatalf("Error: TLS file not accessible: %s (%v)", f, statErr) - } - }Option B (assert readability):
+ for _, f := range []string{certPath, keyPath, caCertPath} { + fd, openErr := os.Open(f) + if openErr != nil { + log.Fatalf("Error: TLS file not readable: %s (%v)", f, openErr) + } + _ = fd.Close() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
cmd/root.go(1 hunks)go.mod(1 hunks)rpm/SPECS/tapir-cli.spec(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T12:03:40.980Z
Learnt from: zluudg
PR: dnstapir/cli#41
File: rpm/SPECS/tapir-cli.spec:42-42
Timestamp: 2025-08-14T12:03:40.980Z
Learning: User zluudg prefers not to auto-start or auto-enable systemd services in the dnstapir/cli RPM package, preferring manual activation instead.
Applied to files:
rpm/SPECS/tapir-cli.spec
🔇 Additional comments (2)
rpm/SPECS/tapir-cli.spec (1)
19-22: LGTM: disables debuginfo on RHEL ≥ 9.The conditional %global debug_package %{nil} is correct and early enough in the spec. This also aligns with your prior preference to avoid opinionated post-install behaviors.
cmd/root.go (1)
133-136: Request NewClientConfig Signature
Please fetch the source fortapir.NewClientConfigat commit 19067e68eca0 and share its exact function signature (parameter names and order), so we can confirm whether it expects(ca, key, cert)or(ca, cert, key).
zluudg
force-pushed
the
leon/issue-40-and-42
branch
from
1ffacad to
648840a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/root.go (1)
140-141: Don’t hardcode InsecureSkipVerify; gate it via config.This disables server verification globally. Make it opt-in via a config flag.
- tlsConfig.InsecureSkipVerify = true + if viper.GetBool("certs.insecure_skip_verify") { + tlsConfig.InsecureSkipVerify = true + }
🧹 Nitpick comments (1)
cmd/root.go (1)
128-132: Harden TLS file checks: ensure regular files and readability.
os.Statwon’t catch directories or unreadable files. Add checks for regular file type and openability.- for _, f := range []string{certPath, keyPath, caCertPath} { - if _, statErr := os.Stat(f); statErr != nil { - log.Fatalf("Error: TLS file not accessible: %s (%v)", f, statErr) - } - } + for _, f := range []string{certPath, keyPath, caCertPath} { + fi, statErr := os.Stat(f) + if statErr != nil { + log.Fatalf("Error: TLS file not accessible: %s (%v)", f, statErr) + } + if !fi.Mode().IsRegular() { + log.Fatalf("Error: TLS path is not a regular file: %s", f) + } + if fd, openErr := os.Open(f); openErr != nil { + log.Fatalf("Error: TLS file not readable: %s (%v)", f, openErr) + } else { + _ = fd.Close() + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
cmd/root.go(1 hunks)go.mod(1 hunks)rpm/SPECS/tapir-cli.spec(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rpm/SPECS/tapir-cli.spec
🔇 Additional comments (2)
go.mod (1)
8-8: tapir bump: verifiedgo mod tidy && go build succeeded; NewClientConfig present with signature: func NewClientConfig(caFile, keyFile, certFile string) (*tls.Config, error).
cmd/root.go (1)
133-134: Resolved — NewClientConfig order is (caFile, keyFile, certFile); tapir.NewClientConfig(caCertPath, keyPath, certPath) is correct.
Summary by CodeRabbit