-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix discrepancy with terraform in handling TF_TOKEN
: Optionally transform -
character in hostname for TF_TOKEN to __
#95
Conversation
@@ -57,6 +57,12 @@ func (r *registryResolver) Resolve(ctx context.Context, target fs.FS, opt Option | |||
|
|||
envVar := fmt.Sprintf("TF_TOKEN_%s", strings.ReplaceAll(hostname, ".", "_")) | |||
token = os.Getenv(envVar) | |||
|
|||
if token == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
Hostname can contain hyphens and dots at the same time, so we must apply all the rules to the host before reading the variable from Env.
Could you also add support for non-ASCII characters? You can use the ToASCII function of the Punycode profile to convert non-ASCII to Punycode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, see what you think
…verted at the same time
Now resolves aquasecurity/trivy#6068 |
opt.Debug("Found a token for the registry at %s", hostname) | ||
ascii_hostname, err := idna.ToASCII(hostname) | ||
if err != nil { | ||
opt.Debug("Could not convert hostname %s to a punycode encoded ASCII string so cannot find token for this registry", hostname) | ||
} else { | ||
opt.Debug("No token was found for the registry at %s", hostname) | ||
envVar := fmt.Sprintf("TF_TOKEN_%s", strings.ReplaceAll(ascii_hostname, ".", "_")) | ||
token = os.Getenv(envVar) | ||
|
||
// Dashes in the hostname can optionally be converted to double underscores | ||
if token == "" { | ||
envVar = strings.ReplaceAll(envVar, "-", "__") | ||
token = os.Getenv(envVar) | ||
} | ||
|
||
if token != "" { | ||
opt.Debug("Found a token for the registry at %s", hostname) | ||
} else { | ||
opt.Debug("No token was found for the registry at %s", hostname) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are improving this, what do you think if we refactor this logic out to a function and write a small unit test for it?
We can probably refactor most of it from the point we start parsing for opt.Version
as that logic seems brittle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have refactored my new bit out to a function and added unit tests. Could I ask to not do the refactor for the other part of it? I am not a Go programmer, and the code for getting the version is fairly complex including some web requests.
thanks it's looking good, however we unfortunately will have to re-create this PR in Trivy as we're merging trivy-iac repo into Trivy with this PR here: aquasecurity/trivy#6005 |
Can we close this since you've recreated it in the Trivy repo? Thanks again for doing that. |
Closing in favor of aquasecurity/trivy#6108 |
Fixes aquasecurity/trivy#6067
This issue prevents me from accessing a private terraform module repo with a
-
in the hostname.