Skip to content

chore(providers/azure): align 5 sibling NewClientWithHTTP constructors with SEC-03 nil-fallback pattern #1304

Description

@cristim

Problem

After PR #1228 (closes #1143, SEC-03) landed, 3 of the 8 Azure NewClientWithHTTP constructors (savingsplans, synapse, managedredis) defensively fall back to httpclient.New() when the injected httpClient is nil. The other 5 -- database, cache, search, cosmosdb, compute -- blindly assign httpClient to the struct field with no nil check:

func NewClientWithHTTP(cred azcore.TokenCredential, subscriptionID, region string, httpClient HTTPClient) *DatabaseClient {
    return &DatabaseClient{
        cred:           cred,
        subscriptionID: subscriptionID,
        region:         region,
        httpClient:     httpClient,
    }
}

If a test (or a future production caller) ever passes nil, these clients will nil-pointer-panic on the first c.httpClient.Do(req) call rather than serving requests through the SSRF-hardened transport.

Why this matters

  • Inconsistency: the SEC-03 issue body (SEC-03: Azure Synapse client falls back to http.DefaultClient (bypasses IMDS-blocking client) on nil injection #1143) noted Synapse was "the only one of the 8 Azure NewClientWithHTTP constructors that substitutes http.DefaultClient." After this fix, 3 of 8 have a hardened nil-fallback and 5 of 8 have none -- still inconsistent, just in the other direction.
  • Defensive consistency: nil-fallback to httpclient.New() is the established cross-cutting pattern memory feedback_azure_use_httpclient_new.md is built around. Lining all 8 up on the same shape reduces the surface where a future caller can stumble.
  • Latent SSRF re-introduction risk: if someone copy-pastes the existing 5 constructors as a template for a new Azure service and adds an if httpClient == nil { httpClient = http.DefaultClient } (matching net/http Go idiom for nil-safety), they re-introduce SEC-03. The 3 hardened constructors are a safer copy template.

Suggested change

Add the same if httpClient == nil { httpClient = httpclient.New() } guard at the top of NewClientWithHTTP in each of the 5 sibling constructors:

  • providers/azure/services/database/client.go
  • providers/azure/services/cache/client.go
  • providers/azure/services/search/client.go
  • providers/azure/services/cosmosdb/client.go
  • providers/azure/services/compute/client.go

Plus a regression test in each package mirroring TestNewClientWithHTTP_NilFallbackIsHardened from synapse/client_test.go / savingsplans/client_test.go / managedredis/client_test.go.

Out of scope

Existing CR findings or other security work on these clients.

Triage

  • type/chore
  • priority/p3
  • severity/low
  • urgency/eventually
  • impact/internal
  • effort/xs
  • triaged

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions