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.

Sign up for GitHub

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

Jump to bottom

(#3565 #2421) Avoid credential bleed from saved sources with the same hostname #3568

Merged
merged 7 commits into from
Nov 26, 2024
Merged

(#3565 #2421) Avoid credential bleed from saved sources with the same hostname #3568

merged 7 commits into from
Nov 26, 2024

Conversation

vexx32
Copy link
Member

@vexx32 vexx32 commented Nov 19, 2024 •
edited by corbob
Loading

Description Of Changes

  • Rework handling in ChocolateyNugetCredentialProvider to ensure we only match credentials from the full URL
  • Add a config property so we can actually tell what the literal arguments passed to --source on the command line are, and start making use of it here to aid with looking up source credentials.
    • In discussions with Gary, we have agreed the behaviour of looking up a source based purely on the URL here is bad In General, because 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, so this will later be expanded upon and some of the searching functionality here can be removed at a later date.
  • Add unit tests for this credential provider

Motivation and Context

See #3565 - the credentials are being reused in places that they shouldn't.

Testing

Unit tests! :3 Including a regression test specifically for #3565

Open VS and run the tests in the chocolatey.tests project and make sure they pass.

Cory added: Ran CredentialProvider tag in Test Kitchen.

Operating Systems Testing

Win10

Change Types Made

  • Bug fix (non-breaking change).
  • Feature / Enhancement (non-breaking change).
  • Breaking change (fix or feature that could cause existing functionality to change).
  • Documentation changes.
  • PowerShell code changes.

Change Checklist

  • Requires a change to the documentation.
  • Documentation has been updated.
  • Tests to cover my changes, have been added.
  • All new and existing tests passed?
  • PowerShell code changes: PowerShell v3 compatibility checked?

Related Issue

Fixes #3565
Fixes #2421 (different symptom, same issue, likely somewhat duplicate issue)

@vexx32 vexx32 requested a review from corbob November 19, 2024 17:05
@gep13 gep13 changed the title Avoid credential bleed from saved sources with the same hostname (#3565 #2421) Avoid credential bleed from saved sources with the same hostname Nov 20, 2024
@gep13 gep13 changed the base branch from develop to hotfix/2.4.1 November 20, 2024 09:01
@vexx32 @corbob
(chocolatey#3565) Rework NugetCredentialProvider
f307edd 
Previously we looked up any available sources in the config by the
hostname, before falling back to trying an exact match if we had
collisions.

This still allowed credentials to be reused in situations where we don't
actually know if they're applicable; many repository servers will
support different credentials for individual repositories, so we cannot
and should not assume that credentials for one repository will actually
match another repository, nor that users want the credentials to be
shared for both.

It also led to the possibility of users storing one repository first,
and then later specifying a different repository on the same server, and
choco would try to use the stored credentials for the first repository
for the explicitly-entered URL which is nowhere in config.

Instead, we should only match the whole URL (which can be done with
Uri. Equals to ensure that we match hostnames case-insensitively, but
routes case-sensitively), and expect users to provide credentials if
they provide a URL that is not explicitly in the sources.

Additionally, we try to ensure that if a user has named a specific
source, rather than themselves providing a URL at the command line, we
prioritise finding that in the already-configured sources and use that
source if the URL matches the current URL that NuGet requires a
credential for.
@vexx32 @corbob
(chocolatey#3565) Add unit tests for ChocolateyNugetCredentialProvider
d2ac771 
These tests ensure that the use cases we expect to handle in the
credential provider are appropriately handled according to our
expectations, based on the user-provided input and the transformed input
that is left in configuration.Sources once the credential provider
typically gets queried.
@corbob corbob marked this pull request as draft November 20, 2024 19:06
@corbob
Copy link
Member

corbob commented Nov 20, 2024

Converting this PR to draft while I add and validate Pester tests.

@corbob corbob marked this pull request as ready for review November 21, 2024 01:19
@corbob
Copy link
Member

corbob commented Nov 21, 2024

I've updated the Pester tests once I got them sorted in Test Kitchen. This is once again ready for review.

AdmiringWorm
AdmiringWorm requested changes Nov 21, 2024
View reviewed changes
Copy link
Member

@AdmiringWorm AdmiringWorm left a comment

Choose a reason for hiding this comment

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

In general, this looks good. But there are a few things to fix up, or questions needed to be answered.

src/chocolatey.tests/infrastructure.app/nuget/ChocolateyNugetCredentialProviderSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests/infrastructure.app/nuget/ChocolateyNugetCredentialProviderSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests/infrastructure.app/nuget/ChocolateyNugetCredentialProviderSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests/infrastructure.app/nuget/ChocolateyNugetCredentialProviderSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests/infrastructure.app/nuget/ChocolateyNugetCredentialProviderSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey/infrastructure.app/commands/ChocolateyInfoCommand.cs Outdated Show resolved Hide resolved
src/chocolatey/infrastructure.app/nuget/ChocolateyNugetCredentialProvider.cs Outdated Show resolved Hide resolved
src/chocolatey/infrastructure.app/nuget/ChocolateyNugetCredentialProvider.cs Outdated Show resolved Hide resolved
src/chocolatey/infrastructure.app/nuget/ChocolateyNugetCredentialProvider.cs Show resolved Hide resolved
src/chocolatey/infrastructure.app/nuget/ChocolateyNugetCredentialProvider.cs Outdated Show resolved Hide resolved
@corbob corbob force-pushed the source-auth branch 2 times, most recently from 5e0848b to 7a0b627 Compare November 21, 2024 23:26
@corbob corbob mentioned this pull request Nov 25, 2024
Open
2 tasks
@corbob
(chocolatey#3565) Add Pester tests for Credential Provider
7c41c31 
Add Pester tests to ensure we don't inadvertently bleed configured
credentials into scenarios where they should not be used.
@corbob
(chocolatey#3565) Address review comments
0f9e5ad
@corbob
(chocolatey#3565) Deprecate ListCommand.ExplicitSource
667fbc7 
Since we've added an ExplicitSources property to the top-level
configuration object, we do not need the ListCommand.ExplicitSource
property. It is being deprecated here to be removed in version 3. To
determine if an explicit source was provided, we can look at if
ExplicitSources is set.
@corbob corbob mentioned this pull request Nov 25, 2024
Merged
10 tasks
vexx32
vexx32 commented Nov 25, 2024
View reviewed changes
Copy link
Member Author

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

Looks good to me apart from one minor comment. Thank for picking this up, @corbob :3

src/chocolatey/infrastructure.app/nuget/ChocolateyNugetCredentialProvider.cs Outdated Show resolved Hide resolved
gep13
gep13 requested changes Nov 26, 2024
View reviewed changes
Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

I have left a comment about something I think needs to be addressed before we can get this merged in.

AdmiringWorm
AdmiringWorm approved these changes Nov 26, 2024
View reviewed changes
Copy link
Member

@AdmiringWorm AdmiringWorm left a comment

Choose a reason for hiding this comment

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

LGTM, but I think the comment from Gary needs to be answered (updated) before anything is merged.

gep13
gep13 requested changes Nov 26, 2024
View reviewed changes
Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

Minor nit pick, sorry.

tests/pester-tests/features/CredentialProvider.Tests.ps1 Outdated Show resolved Hide resolved
@corbob
(chocolatey#3565) Add comments around Pester tests
30479ce 
Add some comments to the Pester Tests to better describe the purpose of
the test and why some commands are expected to exit 0 and others not.
gep13
gep13 approved these changes Nov 26, 2024
View reviewed changes
Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

LGTM!

@corbob corbob merged commit 4f5bd1d into chocolatey:hotfix/2.4.1 Nov 26, 2024
5 checks passed
@vexx32 vexx32 deleted the source-auth branch November 27, 2024 15:10
@corbob corbob mentioned this pull request Nov 27, 2024
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corbob corbob corbob left review comments

@AdmiringWorm AdmiringWorm AdmiringWorm approved these changes

@gep13 gep13 gep13 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@vexx32 @corbob @gep13 @AdmiringWorm