Skip to content

feat(azure): pin TLS 1.2 and HTTPS-only on state storage account - #365

Merged
stevenolen merged 1 commit into
mainfrom
azure-state-account-min-tls
Aug 12, 2026
Merged

feat(azure): pin TLS 1.2 and HTTPS-only on state storage account#365
stevenolen merged 1 commit into
mainfrom
azure-state-account-min-tls

Conversation

@stevenolen

Copy link
Copy Markdown
Contributor

Summary

The Pulumi-state storage account created at bootstrap (CreateStorageAccount in lib/azure/blobstorage.go) left MinimumTLSVersion and EnableHTTPSTrafficOnly at Azure's API defaults, which are TLS 1.0 and no HTTPS enforcement. Those defaults fail customer security baselines evaluated by Azure Policy.

This sets both explicitly at create time:

  • MinimumTLSVersion: TLS1_2
  • EnableHTTPSTrafficOnly: true

Scope

Applies only to newly bootstrapped Azure workloads. The account is created once and guarded by StorageAccountExists, so existing workloads' state accounts are not retrofitted by this change.

Testing

  • go build ./azure/ clean
  • Pre-commit format, lint, and lib tests pass

The Pulumi-state storage account created at bootstrap left
MinimumTLSVersion and EnableHTTPSTrafficOnly at Azure's API defaults
(TLS 1.0, HTTPS not enforced), which fails customer security baselines
evaluated by Azure Policy. Set both explicitly at create time:
TLS 1.2 minimum and secure-transfer-only.
@stevenolen
stevenolen marked this pull request as ready for review August 12, 2026 11:31
@stevenolen
stevenolen requested a review from a team as a code owner August 12, 2026 11:31
@stevenolen
stevenolen enabled auto-merge August 12, 2026 11:31
@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @stevenolen's task in 39s —— View job


Review

  • Gather context
  • Read review guidelines
  • Read changed file
  • Post review feedback

The change is correct and follows existing patterns. Two observations:

Scope gap for existing workloads

The PR description correctly notes that existing accounts aren't retrofitted — StorageAccountExists short-circuits before CreateStorageAccount is called (bootstrap.go:238). This means any already-bootstrapped Azure workload continues to fail Azure Policy checks until someone manually updates those accounts. That's a known limitation, but it's worth deciding whether an UpdateStorageAccount path (or at least operator docs) should accompany this PR, especially if customers are already failing policy evaluations.

Comment verbosity

The inline comments explain the why well, but they're more verbose than needed given the field names are self-documenting (MinimumTLSVersion, EnableHTTPSTrafficOnly). The existing StorageV2 comment in this same block is a good reference for the right level — it explains a non-obvious choice. The two new comments explain defaults that are already stated in the PR description. Suggestion: condense to a single one-liner if the intent is just to document the intentional override, e.g.:

// Explicit security floor; Azure API defaults are TLS 1.0 / HTTP-allowed.
MinimumTLSVersion:      to.Ptr(armstorage.MinimumTLSVersionTLS12),
EnableHTTPSTrafficOnly: to.Ptr(true),

Neither is blocking. The security values themselves are correct and the placement is right.

@Lytol Lytol left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ While this is obviously an improvement, and we should definitely ship it, we still require EnableHTTPSTrafficOnly: false on the other storage account (and it has to stay that way AFAIK because of NFS). Just a note/observation!

@stevenolen
stevenolen added this pull request to the merge queue Aug 12, 2026
Merged via the queue into main with commit 2975a02 Aug 12, 2026
7 checks passed
@stevenolen
stevenolen deleted the azure-state-account-min-tls branch August 12, 2026 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants