Visitar URL original
verify tls certificate for foreign template downloads by SABITHSAHEB · Pull Request #24504 · pulumi/pulumi · GitHub
Skip to content

verify tls certificate for foreign template downloads - #24504

Merged
iwahbe merged 2 commits into
pulumi:masterfrom
SABITHSAHEB:template-download-tls-verify
Sep 3, 2026
Merged

iwahbe merged 2 commits into
pulumi:masterfrom
SABITHSAHEB:template-download-tls-verify

Conversation

@SABITHSAHEB

Copy link
Copy Markdown
Contributor

Summary

DownloadTemplate splits on whether the template URL points at the configured service or somewhere else. For a URL that isn't the service (a github, gitlab, or other registry-hosted template that pulumi new fetches) it builds a fresh client for that host, and it was passing insecure=true to NewClient, which sets InsecureSkipVerify on the transport. That disables certificate verification for the download regardless of what the user configured, so an on-path attacker can present any certificate and swap in template content that then gets written to disk and, for most templates, installed and run. The user never opted into that; every other NewClient call site threads the real insecure setting, and this is the only place that hardcodes it. I noticed it while reading the call sites for the insecure flag. Passing pc.insecure makes the foreign download inherit the caller's TLS setting like the rest of the code, and behavior is unchanged for the normal case where the certificate is valid.

Test plan

  • Added appropriate unit tests - For all changes
  • Added a test in pkg/engine/lifecycletest - For all engine/protocol changes
  • Added a conformance test in pkg/testing/pulumi-test-language - For language protocol changes
  • Added a golden test in pkg/backend/display - For changes to the output renderers

TestDownloadTemplateForeignURLInheritsInsecure captures the flag the fresh client is built with for a non-service URL; it fails before this change (client built with verification off) and passes after.

Validation

  • make lint — clean
  • make test_fast — all pass
  • make tidy_fix — clean
  • make format — clean
  • Relevant SDK tests pass (if SDK changes)
  • make check_proto — clean (if proto changes)

Ran go test ./backend/httpstate/client/ (pass) and gofmt/go vet on the touched files. make lint uses a custom plugin I couldn't build in isolation.

Changelog

  • Changelog entry added. If you do not believe this PR requires a changelog, ask a maintainer to apply
    the impact/no-changelog-required label.

Risk

Extraction/download only. Downloads over https with a valid certificate are unchanged. The only behavior change is for a user who did not opt into insecure transport but relied on the accidental skip for a self-signed foreign template host; they now pass the insecure flag like every other client path. No public API change.

@SABITHSAHEB
SABITHSAHEB requested a review from a team as a code owner August 30, 2026 12:59
@github-actions

Copy link
Copy Markdown

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@tgummerer

Copy link
Copy Markdown
Collaborator

cc @iwahbe as the original implementer. Was there any reason you can remember for always passing true here?

@iwahbe iwahbe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not that I can remember.

@iwahbe
iwahbe added this pull request to the merge queue Sep 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 1, 2026
@iwahbe

iwahbe commented Sep 1, 2026 •

Copy link
Copy Markdown
Member

/run-acceptance-tests
Please view the results of the acceptance tests Here

@iwahbe

iwahbe commented Sep 1, 2026

Copy link
Copy Markdown
Member

@SABITHSAHEB I believe you need to fix a lint error, but after that I'm ready to merge.

@SABITHSAHEB

Copy link
Copy Markdown
Contributor Author

Fixed, switched to t.Context() per usetesting. While rerunning the whole package I noticed the test could also race other parallel tests through the shared newClient hook, so it's serial now with a nolint like the other tests that touch package globals. Package is clean under -race.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@iwahbe

iwahbe commented Sep 2, 2026 •

Copy link
Copy Markdown
Member

/run-acceptance-tests
Please view the results of the acceptance tests Here

@iwahbe
iwahbe enabled auto-merge September 2, 2026 14:50
@iwahbe
iwahbe added this pull request to the merge queue Sep 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 2, 2026
@iwahbe
iwahbe added this pull request to the merge queue Sep 3, 2026
Merged via the queue into pulumi:master with commit 23f67d0 Sep 3, 2026
7 checks passed
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.

3 participants