-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Support for NuGet feed credentials with only username/password in nuget.config
#10360
Support for NuGet feed credentials with only username/password in nuget.config
#10360
Conversation
@@ -27,32 +27,66 @@ | |||
return if nuget_credentials.empty? | |||
|
|||
File.rename(user_nuget_config_path, temporary_nuget_config_path) | |||
File.write( | |||
user_nuget_config_path, | |||
<<~NUGET_XML |
Check failure
Code scanning / CodeQL
Clear-text storage of sensitive information High
an assignment to source_password
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 assume this is triggered because the variable name contains the word "password"?
Technically this issue is pre-existing since "token" already contained the password and was used in the same way previously (stored as plain-text). The only difference here is I've renamed the variables so that they are more specific and File.write
is not air-gaped by the nuget_config
variable anymore.
Any guidance on how to resolve this in Ruby? I'm not very familiar with the language and am unsure how to avoid this given the point of this class is to inject the plain-text password to the file.
@brettfo could you let me know if this is sane or not before I invest more time in to resolving the linting and codeql issues? After having got this far in to the change, I now suspect the approach of injecting plain-text creds in to the |
@rhyskoedijk We're trying to remove all credential handling from this codebase, especially since all
|
I understand now that this code is deprecated and trying to improve the NuGet username/password/token handling here is not the correct approach. Unfortunately I can't run the CLI tool as I need to run dependabot on Azure DevOps and many of the features I need are not currently supported by dependabot-core (e.g. auto-complete, policy by-pass, merge strategies, etc), but that is unrelated to this issue. I'm going to abandon this change, but based on what you are saying, shouldn't the code that writes "packageSourceCredentials" in to |
What are you trying to accomplish?
Fix #9098; specifically, #9098 (comment)
Fix #8914; specifically, #8914 (comment)
When dependabot uses NuGet.exe to perform the update, it injects registry credentials to
nuget.config
. If a registry requires basic access auth (username/password), there is no way to set the username as it is always hard-coded to "user".For example, when using configuration:
nuget.config
would be:Changing the
dependabot.yml
config to use a basic access auth formattedtoken
for the registry does not fix the issue. Thenuget.config
becomes:There appears to be no way to influence the value of
Username
; Without this, NuGet feeds secured with basic access auth cannot be used if the username is a significant part of the credentials (e.g. nuget.telerik.com)Anything you want to highlight for special attention from reviewers?
There was an existing unit test for this which I've just added more scenarios too.
Should I break this out in to individual tests?
How will you know you've accomplished your goal?
The
nuget.config
has the expected values (see above).Checklist