Allow disabling cert validation entirely #3278
Conversation
adreed-msft
requested review from
dphulkar-msft,
gapra-msft,
seanmcc-msft,
souravgupta-msft,
tanyasethi-msft,
vibhansa-msft and
wonwuakpa-msft
as code owners
adreed-msft
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for an --allow-insecure-certs flag to enable debugging with MITM proxies. The flag is hidden and intended only for debugging purposes, allowing TLS certificate verification to be skipped when enabled.
Key Changes
- Introduces a global
common.AllowInsecureCertsboolean flag - Adds
--allow-insecure-certscommand-line flag (hidden) - Configures
TLSClientConfig.InsecureSkipVerifyacross all HTTP clients in the application
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmd/constants.go | Defines the AllowInsecureCertificatesFlag constant |
| cmd/root.go | Adds the hidden --allow-insecure-certs flag and applies it to the GitHub version check HTTP transport |
| common/oauthTokenManager.go | Declares the AllowInsecureCerts global variable and applies it to OAuth HTTP client |
| ste/mgr-JobPartMgr.go | Applies AllowInsecureCerts to the main AzCopy HTTP client |
| ste/performanceAdvisor.go | Applies AllowInsecureCerts to the Azure VM metadata service HTTP client |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Transport: &http.Transport{ | ||
| TLSClientConfig: &tls.Config{ | ||
| InsecureSkipVerify: common.AllowInsecureCerts, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The Azure Instance Metadata Service uses HTTP (not HTTPS), so configuring TLS settings for this client is unnecessary. The TLSClientConfig will have no effect on HTTP requests. Consider removing the TLS configuration for this specific client or adding a comment explaining why it's present despite using HTTP.
| Transport: &http.Transport{ | |
| TLSClientConfig: &tls.Config{ | |
| InsecureSkipVerify: common.AllowInsecureCerts, | |
| }, | |
| }, | |
| Transport: &http.Transport{}, |
| HardlinksFlag = "hardlinks" | ||
|
|
||
| // root command flags | ||
| AllowInsecureCertificatesFlag = "allow-insecure-certs" |
There was a problem hiding this comment.
design question - would this be better as an environment variable over a CLI param?
|
If this is trying to fix the same thing that the other PR is, I'd almost prefer just to have the customer use the Go environment variable. |
Description
Feature / Bug Fix:
Posed as an alternative to #3277, in this case we disable certificate validation entirely when requested. This technically comes with the additional benefit of avoiding any fly-swatting, at the detriment of potentially letting danger in the door.
Related Links:
Type of Change
How Has This Been Tested?
manually tested against mitm proxy