Skip to content
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

Use dotnet credential provider #8927

Closed

Conversation

trejjam
Copy link
Contributor

@trejjam trejjam commented Jan 29, 2024

Using dotnet credential helper, we do not need to juggle with Nuget.config (current solution works only sometimes).

With Credential Provider, we can use it as it is and do not care much about it. It should just work.

When it does not work:

  • when the repository uses packageSourceMapping
  • when the repository replace global sources <packageSources><clear /></packageSources>
  • probably there is more; I hit these myself

As @brettfo pointed out, this fixes these issues only for Azure DevOps repositories. Not other providers

@trejjam trejjam requested a review from a team as a code owner January 29, 2024 17:53
@github-actions github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Jan 29, 2024
@trejjam trejjam mentioned this pull request Jan 29, 2024
@trejjam trejjam force-pushed the feature/nuget-credential-helper branch from a36ce0b to 19be332 Compare January 29, 2024 20:27
@brettfo
Copy link
Contributor

brettfo commented Jan 31, 2024

As I understand it, this will only work for Azure DevOps package feeds, but not private feeds from other providers (e.g., GitHub, AWS, Artifactory, MyGet, etc.) so the NuGet.Config mangling would still be necessary.

@trejjam
Copy link
Contributor Author

trejjam commented Jan 31, 2024

Oh, I did not consider it. You are right

@trejjam
Copy link
Contributor Author

trejjam commented Feb 11, 2024

It does not solve everything around authorization, but it fixes Azure DevOps auth. Can it be reviewed&merged?

@trejjam trejjam force-pushed the feature/nuget-credential-helper branch 3 times, most recently from f2ae60e to de22d37 Compare February 16, 2024 14:33
@trejjam trejjam force-pushed the feature/nuget-credential-helper branch 4 times, most recently from 39435b6 to 56dccc5 Compare February 22, 2024 16:03
@brettfo
Copy link
Contributor

brettfo commented Feb 22, 2024

@trejjam I only recently learned about how credentials are used in the update process and my understanding is that this won't fix the issue.

When a dependabot job is run, there are 2 separate docker containers that are started, the main one is the NuGet updater (which is this code) and despite there being token fields sometimes present in the code and tests, those fields are explicitly stripped out of this container and the "real" runtime never sees them. All network requests from the updater are funneled through the second container to proxy everything. That container image is what intercepts the network calls and injects the auth tokens.

I'm hesitant to modify the job proxy with a dotnet credential provider, since that job proxy is used by all update types (NPM, Python, etc.), not just .NET.

Is there a situation you're seeing where tokens set in the dependabot.yml file aren't working? I don't recall the exact syntax off the top of my head, but it'll commonly look like this:

# ...
registries:
  azure-devops-feed:
    type: nuget-feed
    url: https://dev.azure.com/...<I forgot what goes here>.../index.json
    username: ${{secrets.MY_FEED_USER_NAME}}
    password: ${{secrets.MY_FEED_TOKEN}}
  # __OR__
  azure-devops-feed-2:
    type: nuget-feed
    url: https://dev.azure.com/...<I forgot what goes here>.../index.json
    token: ${{secrets.MY_FEED_USER_NAME}}:${{secrets.MY_FEED_TOKEN}}
# ...

All of that being said, the manual patching of the user-level NuGet.Config is still necessary because it'll add in the feed url and the job proxy is responsible then for inejcting the token/auth.

@abdulapopoola
Copy link
Member

tagging @jakecoffman in case he has some context on the proxy

@trejjam trejjam force-pushed the feature/nuget-credential-helper branch from 56dccc5 to 7610a8f Compare February 22, 2024 18:53
@trejjam
Copy link
Contributor Author

trejjam commented Feb 22, 2024

I am hitting the issue running our instance of Dependabot. I do not have knowledge of other runtimes, and honestly, I am not searching for it.

The issue, as I stated above, is when we use packageSourceMapping. I do not really understand why this change will not work. When the native updater is executed, it modifies the default nuget.config file on a disk, which handles some basic authorization (it needs to run on the target machine. Otherwise, it does not work). This PR passes the ENV variable to the native Updater, which handles the update process properly. We are using it in this way and it works.

@trejjam
Copy link
Contributor Author

trejjam commented Feb 22, 2024

If you are not familiar with nuget.config and packageSourceMapping: it requires that the key of packageSources.add[key] is matching with key from packageSourceMapping.packageSource[key].
The current Dependabot implementation violates this completely.

@brettfo
Copy link
Contributor

brettfo commented Feb 22, 2024

I guess I'm not understanding what the intent is with this PR. The code makes no change to packageSourceMapping, it simply elevates credentials to an environment variable (and adds the credential helper that reads this variable.)

@trejjam It sounds like you're running dependabot directly from the Ruby code and not through the cli tool, is this correct? If that's the case, then you could be getting tokens/authentication into these code paths (using the CLI tokens/auth is all stripped out.)

So if the issue is just with the environment variable having the correct contents, I have a few comments that I'll add next to the code itself.

endpoint_credentials = []

nuget_credentials.each do |c|
next unless c["token"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Azure DevOps feeds aren't necessarily private and authentication isn't always specified via a token field, it could also come in via username/password, so that'll have to be accounted for.

nuget_credentials.each do |c|
next unless c["token"]

exploded_token = T.must(c["token"]).split(":", 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to the above comment, this shouldn't assume a token will be of the form abc:123; it should be able to handle a token without a colon and/or it should handle separate username/password fields.

password = exploded_token[1]

endpoint_credentials << <<~NUGET_ENDPOINT_CREDENTIAL
{"endpoint":"#{c['url']}", "username":"#{username}", "password":"#{password}"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and a few lines below, use the .to_json method so we don't have to worry about any escaping. I'm not 100% certain on the syntax, but it's something like this:

endpoint_credentials << {
  "endpoint" => c['url'],
  "username" => username,
  "password" => password,
}

Then later on:

env["VSS_NUGET_EXTERNAL_FEED_ENDPOINTS"] = {
 "endpointCredentials" => endpoint_credentials
}.to_json

@brettfo
Copy link
Contributor

brettfo commented Feb 22, 2024

If you are running the Ruby code directly and not through the CLI, it might be worth reviving PR #9004.

def self.get_credentials_to_credential_helper_env(credentials)
env = {}

nuget_credentials = credentials.select { |cred| cred["type"] == "nuget_feed" }
Copy link
Contributor

Choose a reason for hiding this comment

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

This feature only works with NuGet feeds from Azure DevOps, right? If that's the case it would be a good idea to filter credentials to only those feeds so you don't accidentally expose feeds/tokens that aren't ADO to the ADO service.

If I recall, all Azure DevOps NuGet feeds look like https://pkgs.dev.azure.com/... or https://SOME-ORGANIZATION.pkgs.visualstudio.com/....

@rhyskoedijk
Copy link

@brettfo I am still not able to get private NuGet feeds hosted in Azure DevOps to auth correctly using this setup. Can you let me know if I am missing something obvious?

Is there a situation you're seeing where tokens set in the dependabot.yml file aren't working? I don't recall the exact syntax off the top of my head, but it'll commonly look like this:

# ...
registries:
  azure-devops-feed:
    type: nuget-feed
    url: https://dev.azure.com/...<I forgot what goes here>.../index.json
    username: ${{secrets.MY_FEED_USER_NAME}}
    password: ${{secrets.MY_FEED_TOKEN}}
  # __OR__
  azure-devops-feed-2:
    type: nuget-feed
    url: https://dev.azure.com/...<I forgot what goes here>.../index.json
    token: ${{secrets.MY_FEED_USER_NAME}}:${{secrets.MY_FEED_TOKEN}}
# ...

My dependabot.yml config is:

version: 2
registries:
  my-private-feed:
    type: nuget-feed
    url: https://pkgs.dev.azure.com/my-org/_packaging/my-private-feed/nuget/v3/index.json
    token: PAT:${{MY_PAT_TOKEN_ENV_VAR}}
# ...

I am using https://github.com/tinglesoftware/dependabot-azure-devops to run Dependabot (Dockerised) in a DevOps Pipeline.
The pipeline sets environment variable MY_PAT_TOKEN_ENV_VAR with the PAT token and this works ok when Dependabot is querying and checking for new packages.

Checking if MyPrivatePackage 1.0.7 needs updating
🌍 --> GET https://pkgs.dev.azure.com/.../index.json
🌍 <-- 200 https://pkgs.dev.azure.com/.../index.json
🌍 --> GET https://api.nuget.org/v3/.../index.json
🌍 <-- 404 https://api.nuget.org/v3/.../index.json
🌍 --> GET https://pkgs.dev.azure.com/...nuspec
🌍 <-- 200 https://pkgs.dev.azure.com/...nuspec
running NuGet updater:
/opt/nuget/NuGetUpdater/NuGetUpdater.Cli framework-check --project-tfms net481 netstandard2.0 --package-tfms .NETStandard2.0 --verbose
The package is compatible.
🌍 --> GET https://pkgs.dev.azure.com/...nuspec
🌍 <-- 200 https://pkgs.dev.azure.com/....nuspec
No update needed for MyPrivatePackage 1.0.7

However, whenever it encounters a package that needs to be updated and it starts building the project, the build fails with "error NU1301: Unable to load the service index for source https://pkgs.dev.azure.com/.../index.json".

Updating System.IdentityModel.Tokens.Jwt from 7.5.2 to 
running NuGet updater:
/opt/nuget/NuGetUpdater/NuGetUpdater.Cli update --repo-root /home/dependabot/dependabot-updater/tmp/... --solution-or-project /home/dependabot/dependabot-updater/tmp/....csproj --dependency System.IdentityModel.Tokens.Jwt --new-version 7.6.0 --previous-version 7.5.2 --verbose
  No dotnet-tools.json file found.
  No global.json file found.
Running for project file [Library/....csproj]
Updating project [/home/dependabot/dependabot-updater/tmp/....csproj]
  Running for SDK-style project
dotnet build in GetAllPackageDependenciesAsync failed. STDOUT:   Determining projects to restore...
/tmp/package-dependency-resolution_NObvEp/Project.csproj : error NU1301: Unable to load the service index for source https://pkgs.dev.azure.com/.../index.json.
  Failed to restore /tmp/package-dependency-resolution_NObvEp/Project.csproj (in 6.18 sec).

Build FAILED.

/tmp/package-dependency-resolution_NObvEp/Project.csproj : error NU1301: Unable to load the service index for source https://pkgs.dev.azure.com/.../index.json.
    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:07.09

 STDERR: 

    Package [System.IdentityModel.Tokens.Jwt] Does not exist as a dependency in [/home/dependabot/dependabot-updater/tmp/....csproj].
Updating project [/home/dependabot/dependabot-updater/tmp/...csproj]
  Running for SDK-style project
dotnet build in GetAllPackageDependenciesAsync failed. STDOUT:   Determining projects to restore...
/tmp/package-dependency-resolution_zgkjXA/Project.csproj : error NU1301: Unable to load the service index for source https://pkgs.dev.azure.com/.../index.json.
  Failed to restore /tmp/package-dependency-resolution_zgkjXA/Project.csproj (in 6.23 sec).

Build FAILED.

I know very little about the inner working here, but is there some disconnect between the auth provided to the "scanner" and the "updater" services? Is this an issue specific to Azure DevOps feeds? Is there something I'm doing wrong in the config?

Any help would be really appreciated, this is the final barrier for me being able to use this in our Azure DevOps environment.

@brettfo
Copy link
Contributor

brettfo commented Jun 10, 2024

@rhyskoedijk There are 2 things I can think of to try.

  1. If I recall correctly, the syntax to specify an environment variable in a dependabot.yml file requires the prefix of env., so in your example you might need to change ${{ MY_ENV_VAR }} to ${{ env.MY_ENV_VAR }}.
  2. The other thing to consider is Azure DevOps package feeds use what they call a token, but is really a password with an empty username, or rather, I've only ever seen it with an empty user name; I don't know if you can specify a username of PAT or not. In the case where user/pass are specified in the config file they're simply joined with a colon : character into a token, so you might try one of the following:

Either use user/pass separately:

...
    type: nuget-feed
    url: https://dev.azure.com/...<I forgot what goes here>.../index.json
    username: ""
    password: ${{ env.MY_ENV_VAR }}
...

Or pre-combine the empty username and password, which really just means you need a leading colon character:

...
    type: nuget-feed
    url: https://dev.azure.com/...<I forgot what goes here>.../index.json
    token: ":${{ env.MY_ENV_VAR }}" # I'm not sure if the leading colon requires surrounding quotes or not
...

I've seen successful runs with private NuGet feeds of all types (Azure DevOps, AWS CodeArtifact, JFrog Artifactory, etc.) so there's nothing special about Azure DevOps.

I don't know about the dependabot-azure-devops tool you mentioned. Does their support site have similar complaints about Azure DevOps feeds not working?

@brettfo
Copy link
Contributor

brettfo commented Jun 11, 2024

@rhyskoedijk I just thought of something else you could check to verify the token for the ADO package feed. At the very start of the job we try to authenticate against all private NuGet feeds and we log the results of that. Right at the top (commonly within the first 10 or 15 lines) you should see a log line like fetching service index for nuget feed https://<the rest of the url> followed immediately by * authenticating nuget feed request ...... That * authenticating line indicates that we're at least trying to inject the token into the request. The next line, then, will report the status code, and these are standard HTTP response codes. We're looking for code 200 indicating that the request was successful. A 401 or 403 indicates that the token or user/pass was wrong. A 404 means the URL is wrong.

@mburumaxwell
Copy link
Contributor

@brettfo from what I recall authentication inside the GitHub hosted version works differently. The credentials are not passed in plain text to the job. This is not open for anyone to inspect or copy. When running a self hosted version, this functionality is not present so the credentials may be passed as is. This is the case for the Azure DevOps extension mentioned by @rhyskoedijk that I maintain because Azure DevOps does not have an inbuilt version.

Before migration to NuGet/dotnet CLI, this method of passing credentials worked just well enough. Admittedly, the migration needed to have happened to bring support for more scenarios. However, private feeds for azure DevOps seems to be not working as expected and this PR contains the actual solution. @rhyskoedijk offered a workaround at tinglesoftware/dependabot-azure-devops#921 (comment), and it proves as much.

By the way, I may be wrong in my understanding of these components.

@rhyskoedijk
Copy link

@mburumaxwell based on #10360 (comment) from @brettfo, this pull request and the wider issue of private feed auth handling via the "updater" component aren't going to be resolved by making changes to dependabot-core. All remaining code related to private feed auth in the updater is likely to be removed in the future.

The recommendation is to do something similar to dependabot-cli, where the "updater" container is wrapped inside a "proxy" container that auto-injects the auth to outgoing HTTP requests.

@mburumaxwell
Copy link
Contributor

@rhyskoedijk
From what I see the dependabot-action and the dependabot-cli, the proxy needs access to the dependabot API to pull credentials from it but it seems closed source so I can't confirm it. That means we either build the small API compliant with the expected requests or build our own proxy to support that does not need the API.
Also can't seem to get access to any sort of guide around it.

Have you found out anything else on it?

The other alternative is to use of the CLI which seems to support Azure DevOps properly except for the additions like auto approve, policy bypass, auto complete, skip PR creation, etc., which I suspect is used by lots of people.

@brettfo
Copy link
Contributor

brettfo commented Dec 4, 2024

The dependabot CLI starts a proxy container that handles all authentication and that's the way we're moving going forward. Could the behavior this PR is trying to introduce be covered in that scenario?

@rhyskoedijk
Copy link

rhyskoedijk commented Dec 4, 2024

@brettfo yes, if using Dependabot CLI, this change isn't required. This change is only required if using dry-run.rb or invoking the updater scripts manually, both of which are not recommended anymore. I think this could be closed, given that that the aim is to remove all authentication related code from dependabot-core.

Something that tripped me up for a while is that there is still a lot of code in dependabot-core that attempts to inject credentials even though the values won't be populated under normal conditions. e.g.

package_source_credentials << " <#{source_name}>"
package_source_credentials << " <add key=\"Username\" value=\"user\" />"
package_source_credentials << " <add key=\"ClearTextPassword\" value=\"#{c['token']}\" />"
package_source_credentials << " </#{source_name}>"

I assume this will be removed when time permits to avoid future confusion?

@brettfo
Copy link
Contributor

brettfo commented Dec 4, 2024

@rhyskoedijk I think you're correct that the authentication stuff you pointed out could likely be removed. I'll have to experiment a bit to see if it might still be required, but in general we're doing the "proper" NuGet thing and honoring NuGet.Config as it exists in the repo.

@brettfo brettfo closed this Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dotnet:nuget NuGet packages via nuget or dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants