fix(security): 2 improvements across 2 files#8980
Conversation
- Security: Insecure TLS Configuration - InsecureSkipVerify in Recording Build - Security: Potential Log Injection via Unsanitized User Input in Debug Logging Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
- Security: Insecure TLS Configuration - InsecureSkipVerify in Recording Build - Security: Potential Log Injection via Unsanitized User Input in Debug Logging Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
|
Thank you for your contribution @tomaioo! We will review the pull request and get back to you soon. |
|
@tomaioo please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Pull request overview
This PR is framed as two "security improvements." In cli/azd/cmd/deps_record.go (compiled only under //go:build record) it removes InsecureSkipVerify: true and stops mutating the global http.DefaultTransport/http.DefaultClient, returning a dedicated *http.Client instead. In the azure.ai.agents extension debug.go, it moves the debug log file into the system temp dir and hardens AZD_EXT_DEBUG parsing. The debug.go changes are clean, but the deps_record.go changes break the record build both at compile time and at runtime.
Changes:
- Remove
InsecureSkipVerifyand global transport/client mutation from the record-mode HTTP client, returning a new client instead. - Switch the AI Agents extension debug log to
os.CreateTempand makeisDebugexplicitly handle empty/invalidAZD_EXT_DEBUG. - (Unintended) Leave
crypto/tlsimported but unused, and drop the only mechanism that let azd trust the recording proxy's self-signed certificate.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cli/azd/cmd/deps_record.go |
Drops InsecureSkipVerify and global transport mutation; leaves crypto/tls unused (won't compile under -tags=record) and removes the only trust path for the recording proxy's self-signed cert, breaking functional tests. |
cli/azd/extensions/azure.ai.agents/internal/cmd/debug.go |
Moves debug logs to a temp file and hardens AZD_EXT_DEBUG parsing to match existing conventions; no blocking issues. |
| @@ -29,12 +27,9 @@ func createHttpClient() *http.Client { | |||
| } | |||
|
|
|||
| transport.Proxy = http.ProxyURL(proxyUrl) | |||
| http.DefaultTransport = transport | |||
| } | |||
|
|
|||
| http.DefaultClient.Transport = transport | |||
|
|
|||
| return http.DefaultClient | |||
| return &http.Client{Transport: transport} | |||
| @@ -18,8 +18,6 @@ import ( | |||
|
|
|||
| func createHttpClient() *http.Client { | |||
| transport := http.DefaultTransport.(*http.Transport).Clone() | |||
jongio
left a comment
There was a problem hiding this comment.
This PR removes InsecureSkipVerify from deps_record.go and refactors logging in debug.go, but introduces several issues that need to be addressed before merging.
deps_record.go: This file only compiles under the record build tag (test infrastructure, not production code). The recording proxy uses self-signed certificates, which is why InsecureSkipVerify exists. The same pattern is used intentionally in cli/azd/extensions/azure.ai.agents/internal/pkg/recordproxy/transport_record.go. Removing it without adding the proxy's CA certificate to a custom pool (as the PR description proposes) will break all recorded test sessions with TLS handshake failures. Additionally, removing the only usage of crypto/tls while keeping the import will cause a compile error under the record build tag.
debug.go: The CreateTemp then Close then OpenFile pattern introduces a TOCTOU race condition. The original code was correctly annotated with //nolint:gosec because the filename is derived from time.Now() (not user input). Moving logs to a temp directory with a random suffix also makes them much harder to find when debugging.
| @@ -18,8 +18,6 @@ import ( | |||
|
|
|||
| func createHttpClient() *http.Client { | |||
| transport := http.DefaultTransport.(*http.Transport).Clone() | |||
There was a problem hiding this comment.
Removing this line leaves crypto/tls imported but unused (line 9), which causes a compile error under the record build tag. Beyond that, this InsecureSkipVerify is required: the recording proxy uses a self-signed certificate. Without it (and without adding the proxy's CA to a custom cert pool), every HTTPS request through the proxy will fail with a TLS verification error. The same pattern exists in extensions/azure.ai.agents/internal/pkg/recordproxy/transport_record.go:41-44 for the same reason.
Since deps_record.go only compiles with //go:build record (test infrastructure), it never runs in production. The security risk described in the PR body doesn't apply here.
| @@ -31,11 +31,11 @@ func setupDebugLogging(flags *pflag.FlagSet) func() { | |||
| return func() {} | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This CreateTemp then Close then OpenFile pattern is a TOCTOU (time-of-check-time-of-use) race: between Close() and the subsequent OpenFile(), another process could modify or remove the file. This is actually less secure than the original code.
The original filename (azd-ai-agents-YYYY-MM-DD.log in CWD) was deliberately predictable so developers can find debug logs. Moving to a temp directory with a random suffix (azd-ai-agents-XYZ123.log in os.TempDir()) makes logs effectively unfindable.
The //nolint:gosec annotation on the original was correct: the G304 warning (file path from variable) doesn't apply when the variable is derived entirely from time.Now().Format(), not user input.
|
|
||
| debug, _ := strconv.ParseBool(os.Getenv("AZD_EXT_DEBUG")) | ||
| return debug | ||
| debugEnv := os.Getenv("AZD_EXT_DEBUG") |
There was a problem hiding this comment.
This refactor is functionally identical to the original. strconv.ParseBool("") returns (false, err) and the original debug, _ := ... discards the error, returning false. The new code does the same thing in more lines. Not harmful, but not needed either.
Summary
fix(security): 2 improvements across 2 files
Problem
Severity:
High| File:cli/azd/cmd/deps_record.go:L20The
deps_record.gofile setsInsecureSkipVerify: trueon the TLS configuration when running in record mode. This disables certificate validation, making the application vulnerable to man-in-the-middle attacks. While this is intended for testing with a recording proxy, the code comment indicates this is for 'self-signed certificates, which is what the recording proxy uses.' The issue is that this is a production code path (guarded by build tag but still shipped) that completely disables TLS verification. Additionally,http.DefaultTransportis mutated globally, which affects all HTTP clients in the process.Solution
http.DefaultTransportglobally; create a new transport instance for the specific client. 3. Consider using a more targeted approach liketls.Config.RootCAswith the proxy's certificate rather thanInsecureSkipVerify.Changes
cli/azd/cmd/deps_record.go(modified)cli/azd/extensions/azure.ai.agents/internal/cmd/debug.go(modified)