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

Assess the source lookup when a URL is specified at the command line. #3573

Open
2 tasks done
corbob opened this issue Nov 25, 2024 · 2 comments
Open
2 tasks done

Assess the source lookup when a URL is specified at the command line. #3573

corbob opened this issue Nov 25, 2024 · 2 comments

Comments

@corbob
Copy link
Member

corbob commented Nov 25, 2024

Checklist

  • I have verified this is the correct repository for opening this issue.
  • I have verified no other issues exist related to my request.

Is Your Feature Request Related To A Problem? Please describe.

No response

Describe The Solution. Why is it needed?

The behaviour of looking up a source based purely on the URL is bad In General, choco should probably just use the literal command line arguments and not assume you want it to look things up in the config when you did not ask.

Additional Context

No response

Related Issues

@vexx32 vexx32 added this to the 2.x milestone Nov 25, 2024
@vexx32
Copy link
Member

vexx32 commented Nov 25, 2024

Noting that changing this would be a breaking change, but we need to evaluate whether this is something we can meaningfully deprecate in 2.x first.

If so, we should deprecate this in 2.x and then make the break in 3.0, which may require a follow up issue.

This primarily affects the logic used in the ChocolateyNugetCredentialProvider class as discussed in #3568

@vexx32
Copy link
Member

vexx32 commented Nov 26, 2024

Noting some additional discussion points related to this after talking things over with Gary, as well:

  • If we do still want to keep the URL-based credential lookup here, we probably should still do one of the following:
    • Make it case sensitive in the comparison, because the ChocolateyNugetCredentialProvider cannot provide the correctly-cased URL back to NuGet and have it query a different URL.
    • Put this lookup logic somewhere more centralised and use it both here and when initially resolving sources so that NuGet is querying the saved case of the URL.
  • Currently it's possible to save multiple distinct sources under different names with the same URL and variant credentials. NuGet does not allow this at all -- should we continue to? We can allow it with the logic as defined in (#3565 #2421) Avoid credential bleed from saved sources with the same hostname #3568, but is it actually a good idea?
  • Consistency about how --source parameters are handled when a source is disabled or (in licensed) defined as admin-only, etc. Currently we allow explicitly using a URL from a disabled source (because we don't check the config when resolving sources for an explicit URL), but we don't allow explicitly using a URL for a source that's defined as admin-only, when the licensed extension is installed, because it separately checks and filters those even if you've provided an explicit URL to --source.
    • IMO choco itself should not make this much effort to be "a security boundary" -- we definitely shouldn't be using saved credentials from a disabled / disallowed-admin-only source (and we don't, already, for both), but if users are providing an explicit URL I do not think it makes sense to look that up in config and say "that URL is one you're not allowed to use", given they can just... query that with PowerShell anyway, and if they have the credentials they have the credentials, we are not doing anything by preventing using the URL itself. If that's sensitive for a company they should have authentication on that endpoint, end of. That's not choco's job (nor the licensed extension) to prevent.
  • The whole named sources thing also raises the discussion of what does push do with this stuff?
    • Should we allow "naming" of an api key/push url rather than tying it to an explicit source URL? There's been PRs raised for this in the past and rejected.
    • Should the API key be a property of a given source itself rather than a separate thing? (this one's probably not ideal since push URLs can differ from normal source URLs)
    • When using choco push, provide ability to use alias #63

I know this is kind of throwing many wrenches at the pile at this point, but it seems prudent to have a consistent design across all of this as much as possible, so it is likely necessary to discuss and come to an agreement as to what we want to do here, and then go and file a bunch of separate issues for all the necessary pieces that will come from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants