feat(cli): implement persistent configuration and grpc client setup (#146, #2)#2799
Open
astraxghost wants to merge 2 commits intoPermify:masterfrom
Open
feat(cli): implement persistent configuration and grpc client setup (#146, #2)#2799astraxghost wants to merge 2 commits intoPermify:masterfrom
astraxghost wants to merge 2 commits intoPermify:masterfrom
Conversation
📝 WalkthroughWalkthroughReplaces the old permify entrypoint by adding Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / CLI
participant ConfigureCmd as NewConfigureCommand
participant FS as File System (~/.permify/credentials)
participant GRPCCfg as ConfiguredGRPCClient
User->>ConfigureCmd: run `permify configure`
ConfigureCmd->>User: prompt Endpoint, API token, Cert Path, Cert Key
User->>ConfigureCmd: provide values
ConfigureCmd->>FS: marshal YAML and save (0600)
FS-->>ConfigureCmd: saved
ConfigureCmd->>User: confirm saved
User->>GRPCCfg: later request dial options
GRPCCfg->>FS: read credentials
FS-->>GRPCCfg: YAML data
GRPCCfg->>GRPCCfg: unmarshal and configure token/TLS/insecure
GRPCCfg-->>User: return dial options & endpoint
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (1)
pkg/cmd/configure.go (1)
26-33: Consider returning an error instead of callingos.Exit.Calling
os.Exitin a utility function makes it difficult to test and limits reusability. Consider returning an error and letting the caller handle it.♻️ Proposed refactor
-func getCredentialsPath() string { +func getCredentialsPath() (string, error) { home, err := os.UserHomeDir() if err != nil { - fmt.Println("Error getting home directory:", err) - os.Exit(1) + return "", fmt.Errorf("error getting home directory: %w", err) } - return filepath.Join(home, ".permify", "credentials") + return filepath.Join(home, ".permify", "credentials"), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/configure.go` around lines 26 - 33, The getCredentialsPath function currently exits the process on os.UserHomeDir failure; change its signature to getCredentialsPath() (string, error), return an empty string and the error instead of calling os.Exit, and update all callers to handle the error (log/propagate/exit at top-level). Ensure the function still returns the joined path on success (nil error), and update any tests or callsites that assumed a no-error signature to check the returned error from getCredentialsPath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/permify/main.go`:
- Around line 27-29: Replace the Spanish inline comment "Comando inyectado"
above the root.AddCommand(cmd.NewConfigureCommand()) call with a concise English
comment (e.g., "Injected command" or "Add configure command") and fix formatting
to follow project comment style (use English, proper spacing and punctuation) so
the comment sits on its own line immediately before
root.AddCommand(cmd.NewConfigureCommand()).
In `@pkg/cmd/configure.go`:
- Around line 95-97: TokenAuth.RequireTransportSecurity currently returns false
allowing bearer tokens over plaintext; change it to enforce TLS by returning
true (or make it conditional via a clearly named configuration flag like
TokenAuth.allowInsecure or an ENV/option checked in RequireTransportSecurity)
and, if you keep an insecure mode for dev, ensure RequireTransportSecurity
documents/reads that flag and that callers log a clear warning when tokens are
used without TLS; update the TokenAuth type and its constructor/options
accordingly so the behavior is explicit and configurable rather than silently
insecure.
- Around line 117-128: The TLS config created when creds.CertPath and
creds.CertKey are set lacks a minimum TLS version; update the tlsConfig (the
variable created after tls.LoadX509KeyPair) to include MinVersion:
tls.VersionTLS12 (or tls.VersionTLS13 for stricter security) before calling
credentials.NewTLS and appending grpc.WithTransportCredentials to opts so the
client will refuse older TLS protocols; leave the insecure.NewCredentials branch
unchanged.
- Around line 42-52: The calls to reader.ReadString('\n') in configure.go ignore
returned errors and can produce empty/invalid inputs; update each read (for
endpoint, apiToken, certPath, certKey) to capture the (string, error) result,
check the error, and handle it (e.g., print a clear error via
fmt.Fprintln(os.Stderr, ...) and return or os.Exit(1)). Specifically modify the
blocks where endpoint := reader.ReadString, apiToken := reader.ReadString,
certPath := reader.ReadString, and certKey := reader.ReadString to use endpoint,
err := reader.ReadString('\n') (and likewise for the others), then if err != nil
handle/report and stop further processing.
---
Nitpick comments:
In `@pkg/cmd/configure.go`:
- Around line 26-33: The getCredentialsPath function currently exits the process
on os.UserHomeDir failure; change its signature to getCredentialsPath() (string,
error), return an empty string and the error instead of calling os.Exit, and
update all callers to handle the error (log/propagate/exit at top-level). Ensure
the function still returns the joined path on success (nil error), and update
any tests or callsites that assumed a no-error signature to check the returned
error from getCredentialsPath.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/permify/main.gocmd/permify/permify.gopermifypkg/cmd/configure.go
💤 Files with no reviewable changes (1)
- cmd/permify/permify.go
Comment on lines
+42
to
+52
| fmt.Print("Endpoint (e.g., localhost:3478): ") | ||
| endpoint, _ := reader.ReadString('\n') | ||
|
|
||
| fmt.Print("API Token: ") | ||
| apiToken, _ := reader.ReadString('\n') | ||
|
|
||
| fmt.Print("Cert Path (optional): ") | ||
| certPath, _ := reader.ReadString('\n') | ||
|
|
||
| fmt.Print("Cert Key (optional): ") | ||
| certKey, _ := reader.ReadString('\n') |
There was a problem hiding this comment.
Handle errors from ReadString to avoid silent failures.
The errors from reader.ReadString('\n') are silently ignored. If stdin encounters an I/O error or EOF unexpectedly, the user won't be notified and may end up with empty/corrupted credentials.
🛡️ Proposed fix to handle read errors
fmt.Print("Endpoint (e.g., localhost:3478): ")
- endpoint, _ := reader.ReadString('\n')
+ endpoint, err := reader.ReadString('\n')
+ if err != nil {
+ fmt.Println("Error reading endpoint:", err)
+ os.Exit(1)
+ }
fmt.Print("API Token: ")
- apiToken, _ := reader.ReadString('\n')
+ apiToken, err := reader.ReadString('\n')
+ if err != nil {
+ fmt.Println("Error reading API token:", err)
+ os.Exit(1)
+ }
fmt.Print("Cert Path (optional): ")
- certPath, _ := reader.ReadString('\n')
+ certPath, err := reader.ReadString('\n')
+ if err != nil {
+ fmt.Println("Error reading cert path:", err)
+ os.Exit(1)
+ }
fmt.Print("Cert Key (optional): ")
- certKey, _ := reader.ReadString('\n')
+ certKey, err := reader.ReadString('\n')
+ if err != nil {
+ fmt.Println("Error reading cert key:", err)
+ os.Exit(1)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fmt.Print("Endpoint (e.g., localhost:3478): ") | |
| endpoint, _ := reader.ReadString('\n') | |
| fmt.Print("API Token: ") | |
| apiToken, _ := reader.ReadString('\n') | |
| fmt.Print("Cert Path (optional): ") | |
| certPath, _ := reader.ReadString('\n') | |
| fmt.Print("Cert Key (optional): ") | |
| certKey, _ := reader.ReadString('\n') | |
| fmt.Print("Endpoint (e.g., localhost:3478): ") | |
| endpoint, err := reader.ReadString('\n') | |
| if err != nil { | |
| fmt.Println("Error reading endpoint:", err) | |
| os.Exit(1) | |
| } | |
| fmt.Print("API Token: ") | |
| apiToken, err := reader.ReadString('\n') | |
| if err != nil { | |
| fmt.Println("Error reading API token:", err) | |
| os.Exit(1) | |
| } | |
| fmt.Print("Cert Path (optional): ") | |
| certPath, err := reader.ReadString('\n') | |
| if err != nil { | |
| fmt.Println("Error reading cert path:", err) | |
| os.Exit(1) | |
| } | |
| fmt.Print("Cert Key (optional): ") | |
| certKey, err := reader.ReadString('\n') | |
| if err != nil { | |
| fmt.Println("Error reading cert key:", err) | |
| os.Exit(1) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cmd/configure.go` around lines 42 - 52, The calls to
reader.ReadString('\n') in configure.go ignore returned errors and can produce
empty/invalid inputs; update each read (for endpoint, apiToken, certPath,
certKey) to capture the (string, error) result, check the error, and handle it
(e.g., print a clear error via fmt.Fprintln(os.Stderr, ...) and return or
os.Exit(1)). Specifically modify the blocks where endpoint := reader.ReadString,
apiToken := reader.ReadString, certPath := reader.ReadString, and certKey :=
reader.ReadString to use endpoint, err := reader.ReadString('\n') (and likewise
for the others), then if err != nil handle/report and stop further processing.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
pkg/cmd/configure.go (1)
42-52:⚠️ Potential issue | 🟡 MinorHandle errors from
ReadStringto avoid silent failures.The errors from
reader.ReadString('\n')are still being ignored. If stdin encounters an I/O error or EOF unexpectedly, the user won't be notified and may end up with empty or corrupted credentials.🛡️ Proposed fix to handle read errors
fmt.Print("Endpoint (e.g., localhost:3478): ") - endpoint, _ := reader.ReadString('\n') + endpoint, err := reader.ReadString('\n') + if err != nil { + fmt.Println("Error reading endpoint:", err) + os.Exit(1) + } fmt.Print("API Token: ") - apiToken, _ := reader.ReadString('\n') + apiToken, err := reader.ReadString('\n') + if err != nil { + fmt.Println("Error reading API token:", err) + os.Exit(1) + } fmt.Print("Cert Path (optional): ") - certPath, _ := reader.ReadString('\n') + certPath, err := reader.ReadString('\n') + if err != nil { + fmt.Println("Error reading cert path:", err) + os.Exit(1) + } fmt.Print("Cert Key (optional): ") - certKey, _ := reader.ReadString('\n') + certKey, err := reader.ReadString('\n') + if err != nil { + fmt.Println("Error reading cert key:", err) + os.Exit(1) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/configure.go` around lines 42 - 52, The ReadString calls ignore errors and can silently produce empty/invalid values; update each reader.ReadString('\n') invocation (for variables endpoint, apiToken, certPath, certKey) to capture the returned error, check it, and handle it (return the error from the enclosing function or print a clear error to stderr and exit) instead of discarding it; use the same error-handling pattern for all four reads so input I/O/EoF failures are surfaced to the caller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/permify/main.go`:
- Around line 27-29: There is a gofumpt formatting issue in main.go around the
root.AddCommand(cmd.NewConfigureCommand()) comment—remove the trailing
whitespace before the comment or reformat the file; run gofumpt -w
cmd/permify/main.go (or gofmt/gofumpt on the file) so the whitespace/comment
formatting around root.AddCommand and cmd.NewConfigureCommand() is corrected.
In `@pkg/cmd/configure.go`:
- Line 70: The call os.MkdirAll(credsDir, 0700) is flagged by gofumpt
formatting; update the octal permission literal to the modern Go form or run the
formatter. Replace 0700 with 0o700 (or run gofumpt -w pkg/cmd/configure.go) so
the os.MkdirAll(credsDir, 0o700) invocation matches gofumpt rules.
- Line 95: Replace the Spanish inline comment "// FIX: Blindamos el envío del
token (Security Risk)" with an English equivalent to keep repo-wide consistency;
for example, update the comment near the configure command in
pkg/cmd/configure.go to "// FIX: Protect token transmission (Security Risk)" or
"// FIX: Prevent token exposure during transmission (Security Risk)" so the
intent remains clear and in English.
- Line 123: Replace the Spanish inline comment "Forzamos la versión mínima de
TLS" with an English equivalent (e.g., "Enforce minimum TLS version") in the
same spot where TLS version is forced so the codebase comments remain
consistent; update any nearby related comments that are also Spanish to English
to keep the section uniform and searchable.
- Around line 114-131: The code allows setting creds.APIToken while falling back
to grpc.WithTransportCredentials(insecure.NewCredentials()) when creds.CertPath
and creds.CertKey are empty, which will cause TokenAuth
(RequireTransportSecurity()=true) to fail at dial time; fix by detecting this
case and either return an explicit error when creds.APIToken is set but no TLS
is configured (check creds.APIToken && (creds.CertPath=="" ||
creds.CertKey=="")) OR, to support server-side TLS without client certs, create
a tls.Config with MinVersion: tls.VersionTLS12 and RootCAs loaded from system
cert pool (x509.SystemCertPool()) and use credentials.NewTLS(tlsConfig) instead
of insecure.NewCredentials(); update the branch around the TokenAuth and
transport credential construction (refs: TokenAuth, creds.APIToken,
creds.CertPath, creds.CertKey, tls.Config, credentials.NewTLS,
insecure.NewCredentials) accordingly.
---
Duplicate comments:
In `@pkg/cmd/configure.go`:
- Around line 42-52: The ReadString calls ignore errors and can silently produce
empty/invalid values; update each reader.ReadString('\n') invocation (for
variables endpoint, apiToken, certPath, certKey) to capture the returned error,
check it, and handle it (return the error from the enclosing function or print a
clear error to stderr and exit) instead of discarding it; use the same
error-handling pattern for all four reads so input I/O/EoF failures are surfaced
to the caller.
Comment on lines
+27
to
+29
|
|
||
| // Add configure command | ||
| root.AddCommand(cmd.NewConfigureCommand()) |
There was a problem hiding this comment.
Fix formatting per gofumpt.
Static analysis indicates a formatting issue at line 27 (likely the trailing whitespace before the comment). Run gofumpt -w cmd/permify/main.go to fix.
✏️ Proposed fix
root.AddCommand(cmd.NewRepairCommand())
-
- // Add configure command
+
+ // Add configure command
root.AddCommand(cmd.NewConfigureCommand())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Add configure command | |
| root.AddCommand(cmd.NewConfigureCommand()) | |
| root.AddCommand(cmd.NewRepairCommand()) | |
| // Add configure command | |
| root.AddCommand(cmd.NewConfigureCommand()) |
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 27-27: File is not properly formatted
(gofumpt)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/permify/main.go` around lines 27 - 29, There is a gofumpt formatting
issue in main.go around the root.AddCommand(cmd.NewConfigureCommand())
comment—remove the trailing whitespace before the comment or reformat the file;
run gofumpt -w cmd/permify/main.go (or gofmt/gofumpt on the file) so the
whitespace/comment formatting around root.AddCommand and
cmd.NewConfigureCommand() is corrected.
| credsPath := getCredentialsPath() | ||
| credsDir := filepath.Dir(credsPath) | ||
|
|
||
| if err := os.MkdirAll(credsDir, 0700); err != nil { |
There was a problem hiding this comment.
Fix formatting per gofumpt.
Static analysis indicates this line has a formatting issue. Run gofumpt -w pkg/cmd/configure.go to fix.
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 70-70: File is not properly formatted
(gofumpt)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cmd/configure.go` at line 70, The call os.MkdirAll(credsDir, 0700) is
flagged by gofumpt formatting; update the octal permission literal to the modern
Go form or run the formatter. Replace 0700 with 0o700 (or run gofumpt -w
pkg/cmd/configure.go) so the os.MkdirAll(credsDir, 0o700) invocation matches
gofumpt rules.
| }, nil | ||
| } | ||
|
|
||
| // FIX: Blindamos el envío del token (Security Risk) |
There was a problem hiding this comment.
Use English for code comments.
This comment is in Spanish. For consistency in an open-source project, use English.
✏️ Proposed fix
-// FIX: Blindamos el envío del token (Security Risk)
+// Require transport security to protect Bearer token transmission📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // FIX: Blindamos el envío del token (Security Risk) | |
| // Require transport security to protect Bearer token transmission |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cmd/configure.go` at line 95, Replace the Spanish inline comment "// FIX:
Blindamos el envío del token (Security Risk)" with an English equivalent to keep
repo-wide consistency; for example, update the comment near the configure
command in pkg/cmd/configure.go to "// FIX: Protect token transmission (Security
Risk)" or "// FIX: Prevent token exposure during transmission (Security Risk)"
so the intent remains clear and in English.
Comment on lines
+114
to
+131
| if creds.APIToken != "" { | ||
| opts = append(opts, grpc.WithPerRPCCredentials(TokenAuth{Token: creds.APIToken})) | ||
| } | ||
|
|
||
| if creds.CertPath != "" && creds.CertKey != "" { | ||
| cert, err := tls.LoadX509KeyPair(creds.CertPath, creds.CertKey) | ||
| if err != nil { | ||
| return nil, "", fmt.Errorf("could not load client key pair: %w", err) | ||
| } | ||
| // FIX: Forzamos la versión mínima de TLS | ||
| tlsConfig := &tls.Config{ | ||
| Certificates: []tls.Certificate{cert}, | ||
| MinVersion: tls.VersionTLS12, | ||
| } | ||
| opts = append(opts, grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))) | ||
| } else { | ||
| opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| } |
There was a problem hiding this comment.
Configuration conflict: API token with insecure transport will fail at runtime.
When a user configures an API token but no TLS certificates, TokenAuth (with RequireTransportSecurity() = true) is combined with insecure.NewCredentials(). gRPC will reject this combination at dial time with an error like "transport: cannot send secure credentials on an insecure connection".
Consider either:
- Returning an explicit error if API token is set without TLS configuration
- Supporting server-side TLS (without client certs) by loading system CA roots when only an API token is provided
🛡️ Option 1: Fail early with a clear error message
if creds.APIToken != "" {
+ if creds.CertPath == "" || creds.CertKey == "" {
+ return nil, "", fmt.Errorf("API token requires TLS configuration (cert_path and cert_key must be set)")
+ }
opts = append(opts, grpc.WithPerRPCCredentials(TokenAuth{Token: creds.APIToken}))
}🛡️ Option 2: Support server-side TLS without client certs
if creds.CertPath != "" && creds.CertKey != "" {
cert, err := tls.LoadX509KeyPair(creds.CertPath, creds.CertKey)
if err != nil {
return nil, "", fmt.Errorf("could not load client key pair: %w", err)
}
// Enforce minimum TLS version for security
tlsConfig := &tls.Config{
Certificates: []tls.Certificate{cert},
MinVersion: tls.VersionTLS12,
}
opts = append(opts, grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig)))
+ } else if creds.APIToken != "" {
+ // Use system CA roots for server-side TLS when API token is set
+ tlsConfig := &tls.Config{
+ MinVersion: tls.VersionTLS12,
+ }
+ opts = append(opts, grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig)))
} else {
opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials()))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if creds.APIToken != "" { | |
| opts = append(opts, grpc.WithPerRPCCredentials(TokenAuth{Token: creds.APIToken})) | |
| } | |
| if creds.CertPath != "" && creds.CertKey != "" { | |
| cert, err := tls.LoadX509KeyPair(creds.CertPath, creds.CertKey) | |
| if err != nil { | |
| return nil, "", fmt.Errorf("could not load client key pair: %w", err) | |
| } | |
| // FIX: Forzamos la versión mínima de TLS | |
| tlsConfig := &tls.Config{ | |
| Certificates: []tls.Certificate{cert}, | |
| MinVersion: tls.VersionTLS12, | |
| } | |
| opts = append(opts, grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))) | |
| } else { | |
| opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials())) | |
| } | |
| if creds.APIToken != "" { | |
| if creds.CertPath == "" || creds.CertKey == "" { | |
| return nil, "", fmt.Errorf("API token requires TLS configuration (cert_path and cert_key must be set)") | |
| } | |
| opts = append(opts, grpc.WithPerRPCCredentials(TokenAuth{Token: creds.APIToken})) | |
| } | |
| if creds.CertPath != "" && creds.CertKey != "" { | |
| cert, err := tls.LoadX509KeyPair(creds.CertPath, creds.CertKey) | |
| if err != nil { | |
| return nil, "", fmt.Errorf("could not load client key pair: %w", err) | |
| } | |
| // FIX: Forzamos la versión mínima de TLS | |
| tlsConfig := &tls.Config{ | |
| Certificates: []tls.Certificate{cert}, | |
| MinVersion: tls.VersionTLS12, | |
| } | |
| opts = append(opts, grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))) | |
| } else { | |
| opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials())) | |
| } |
Suggested change
| if creds.APIToken != "" { | |
| opts = append(opts, grpc.WithPerRPCCredentials(TokenAuth{Token: creds.APIToken})) | |
| } | |
| if creds.CertPath != "" && creds.CertKey != "" { | |
| cert, err := tls.LoadX509KeyPair(creds.CertPath, creds.CertKey) | |
| if err != nil { | |
| return nil, "", fmt.Errorf("could not load client key pair: %w", err) | |
| } | |
| // FIX: Forzamos la versión mínima de TLS | |
| tlsConfig := &tls.Config{ | |
| Certificates: []tls.Certificate{cert}, | |
| MinVersion: tls.VersionTLS12, | |
| } | |
| opts = append(opts, grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))) | |
| } else { | |
| opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials())) | |
| } | |
| if creds.APIToken != "" { | |
| opts = append(opts, grpc.WithPerRPCCredentials(TokenAuth{Token: creds.APIToken})) | |
| } | |
| if creds.CertPath != "" && creds.CertKey != "" { | |
| cert, err := tls.LoadX509KeyPair(creds.CertPath, creds.CertKey) | |
| if err != nil { | |
| return nil, "", fmt.Errorf("could not load client key pair: %w", err) | |
| } | |
| // Enforce minimum TLS version for security | |
| tlsConfig := &tls.Config{ | |
| Certificates: []tls.Certificate{cert}, | |
| MinVersion: tls.VersionTLS12, | |
| } | |
| opts = append(opts, grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))) | |
| } else if creds.APIToken != "" { | |
| // Use system CA roots for server-side TLS when API token is set | |
| tlsConfig := &tls.Config{ | |
| MinVersion: tls.VersionTLS12, | |
| } | |
| opts = append(opts, grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))) | |
| } else { | |
| opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials())) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cmd/configure.go` around lines 114 - 131, The code allows setting
creds.APIToken while falling back to
grpc.WithTransportCredentials(insecure.NewCredentials()) when creds.CertPath and
creds.CertKey are empty, which will cause TokenAuth
(RequireTransportSecurity()=true) to fail at dial time; fix by detecting this
case and either return an explicit error when creds.APIToken is set but no TLS
is configured (check creds.APIToken && (creds.CertPath=="" ||
creds.CertKey=="")) OR, to support server-side TLS without client certs, create
a tls.Config with MinVersion: tls.VersionTLS12 and RootCAs loaded from system
cert pool (x509.SystemCertPool()) and use credentials.NewTLS(tlsConfig) instead
of insecure.NewCredentials(); update the branch around the TokenAuth and
transport credential construction (refs: TokenAuth, creds.APIToken,
creds.CertPath, creds.CertKey, tls.Config, credentials.NewTLS,
insecure.NewCredentials) accordingly.
| if err != nil { | ||
| return nil, "", fmt.Errorf("could not load client key pair: %w", err) | ||
| } | ||
| // FIX: Forzamos la versión mínima de TLS |
There was a problem hiding this comment.
Use English for code comments.
This comment is also in Spanish. Use English for consistency.
✏️ Proposed fix
- // FIX: Forzamos la versión mínima de TLS
+ // Enforce minimum TLS version for security📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // FIX: Forzamos la versión mínima de TLS | |
| // Enforce minimum TLS version for security |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cmd/configure.go` at line 123, Replace the Spanish inline comment
"Forzamos la versión mínima de TLS" with an English equivalent (e.g., "Enforce
minimum TLS version") in the same spot where TLS version is forced so the
codebase comments remain consistent; update any nearby related comments that are
also Spanish to English to keep the section uniform and searchable.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces a scalable CLI implementation to manage Permify instances, addressing #146 and #2.
Technical Implementation:
spf13/cobra.~/.permify/credentials(YAML format, 0600 permissions, 0700 for the directory).configurecommand to set up Endpoint, API Token, and optional TLS Certificates./claim #146 #2
Summary by CodeRabbit
New Features
Chores