(#3565 #2421) Avoid credential bleed from saved sources with the same hostname #3568
Conversation
gep13
changed the title
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.
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.
|
Converting this PR to draft while I add and validate Pester tests. |
|
I've updated the Pester tests once I got them sorted in Test Kitchen. This is once again ready for review. |
| [OneTimeSetUp] | ||
| public async Task OneTimeSetup() | ||
| { | ||
| await With(); | ||
| } |
There was a problem hiding this comment.
This should be a Because clause, not a separate OneTimeSetup.
There are no guarantee in what order a OneTimeSetup will be ran when multiple ones are specified (both Context and Because is in a single OneTimeSetup).
| [OneTimeSetUp] | |
| public async Task OneTimeSetup() | |
| { | |
| await With(); | |
| } | |
| public override void Because() | |
| { | |
| With().Wait(); | |
| } | |
At some point, if we want to test async code more often, then we should think about introducing an async TinySpec instead of doing this (You can look at the AyncTinySpec class I created for Agent).
| public override void Because() | ||
| { | ||
| Configuration.Sources = Configuration.ExplicitSources = TargetSourceUrl; | ||
| Configuration.SourceCommand.Username = "user"; | ||
| Configuration.SourceCommand.Password = "totally_secure_password!!!"; | ||
| } |
There was a problem hiding this comment.
This is part of the setup, not the actual run of a code. As such this should be in a Context method.
| public override void Because() | |
| { | |
| Configuration.Sources = Configuration.ExplicitSources = TargetSourceUrl; | |
| Configuration.SourceCommand.Username = "user"; | |
| Configuration.SourceCommand.Password = "totally_secure_password!!!"; | |
| } | |
| public override void Context() | |
| { | |
| base.Context(); | |
| Configuration.Sources = Configuration.ExplicitSources = TargetSourceUrl; | |
| Configuration.SourceCommand.Username = "user"; | |
| Configuration.SourceCommand.Password = "totally_secure_password!!!"; | |
| } | |
| [Fact] | ||
| public void Creates_a_valid_credential() | ||
| { | ||
| Result.Should().NotBeNull(); | ||
| } |
There was a problem hiding this comment.
This does not test if the credentials are valid, just that they were created.
We also commonly have prefixed assertions with Should_ (this part is also true for all other methods created in this file, I am just not commenting on it right now). Additionally, each word should have the first letter in uppercase (it wasn't done when converting to FluentAssertion due to the amount of time it would take).
| [Fact] | |
| public void Creates_a_valid_credential() | |
| { | |
| Result.Should().NotBeNull(); | |
| } | |
| [Fact] | |
| public void Should_Create_Credentials() | |
| { | |
| Result.Should().NotBeNull(); | |
| } | |
There was a problem hiding this comment.
This has been updated.
A note on the naming and casing: should it not be PascalCase as well?
There was a problem hiding this comment.
A note on the naming and casing: should it not be PascalCase as well?
Not for tests, as the names of tests can become large, it will be easier to read these names when the words are separated by an underscore.
| public override void Because() | ||
| { | ||
| Configuration.Sources = TargetSourceUrl; | ||
| Configuration.ExplicitSources = TargetSourceName; | ||
| } |
There was a problem hiding this comment.
This should be part of the context (ie the setup), not the because method.
| public override void Because() | |
| { | |
| Configuration.Sources = TargetSourceUrl; | |
| Configuration.ExplicitSources = TargetSourceName; | |
| } | |
| public override void Context() | |
| { | |
| base.Context(); | |
| Configuration.Sources = TargetSourceUrl; | |
| Configuration.ExplicitSources = TargetSourceName; | |
| } | |
| public override void Because() | ||
| { | ||
| Configuration.Sources = Configuration.ExplicitSources = TargetSourceUrl; | ||
| } |
There was a problem hiding this comment.
Again, should be Context.
| public override void Because() | |
| { | |
| Configuration.Sources = Configuration.ExplicitSources = TargetSourceUrl; | |
| } | |
| public override void Context() | |
| { | |
| base.Context(); | |
| Configuration.Sources = Configuration.ExplicitSources = TargetSourceUrl; | |
| } | |
| configuration.Sources = configuration.ExplicitSources = option.UnquoteSafe(); | ||
| configuration.ListCommand.ExplicitSource = true; |
There was a problem hiding this comment.
Why are we storing a Source and an ExplicitSource, at the same time that we are setting a boolean property called ExplicitSource?
Also, what happens if the source that is passed is the named source of a configuration? Or one of the alternative sources?
Should that also be considered an explicit source?
There was a problem hiding this comment.
In the credential provider, the first thing it does it try to look up any explicitly set named sources, and then the remainder as URIs. I if it's an alternative source it'll be handled before getting to the credential provider and so will not matter if it's set as an explicit source.
There was a problem hiding this comment.
I am ok with the introduction of the ExplicitSources property on the top-level configuration object, but that does bring up the question of whether the ExplicitSource boolean is needed anymore on the ListCommand object. Should we not replace the usage of ExplicitSource in the ChocolateyListCommand class, and replace if with having anything in the ExplicitSources property? Perhaps mark ExplicitSource as obsolete, and then we can remove it in future breaking change?
| .ToList(); | ||
| if (namedExplicitSources?.Count > 0) |
There was a problem hiding this comment.
Should be a newline here.
| .ToList(); | |
| if (namedExplicitSources?.Count > 0) | |
| .ToList(); | |
| if (namedExplicitSources?.Count > 0) | |
| source = _config.MachineSources | ||
| .Where(s => namedExplicitSources.Contains(s.Name) && new Uri(s.Key.TrimEnd('/')) == trimmedTargetUri) | ||
| .FirstOrDefault(); |
There was a problem hiding this comment.
Why are we doing case sensitive checking here?
The Contains method is case sensitive, and as the comment says it does a case sensitive match on the url (just not the hostname).
That do not make sense, and I would think it is a case insensitive match we would want.
There was a problem hiding this comment.
The reason for doing a case sensitive match on everything but the hostname is that the hostname is handled by DNS and is case insensitive, but the rest of the URL is up to the responding server which may or may not be case insensitive. For example: https://docs.chocolatey.org/en-us/central-management/ is valid while https://docs.chocolatey.org/en-us/central-managemenT/ is not. But if you were to case-insensitive compare them they are identical.
There was a problem hiding this comment.
While that is true for the documentation site we have, that is actually quite uncommon.
If we take the api we have, the URL https://community.chocolatey.org/api/v2/ returns the exact same information as https://community.chocolatey.org/API/v2/.
But even if they were not returning the same information, it would not be the point.
The point is, we are looking up information about a URL in the machine sources configuration, and it is not guaranteed that a user will remember the exact case of the URI that was configured but still intend to use it. By not allowing the case to be different, we are preventing this configured URL to be picked up. We should also use the URL as specified in the machine configuration (and if I am reading everything correctly, we are doing that), so even if the server would accept one case but not the other, it wouldn't be a problem since it already uses the case that is configured on the machine object.
There is also the case that with this change, a user can not use a named source that differs in casing either. Ie, they can not pass the source Chocolatey when the name is configured to be chocolatey. This is something that is changing the behavior of what is currently available, as currently using a named source with a different case will resolve to a configured URL.
| // Note: This behaviour remains as removing it would be a breaking change, but we may want | ||
| // to remove this in a future version, as specifying an explicit URL should potentially |
There was a problem hiding this comment.
If there has been a decision to remove this, or potentially removing it, we would need an issue to be created to keep track of this.
There was a problem hiding this comment.
I think we agree that we would add an issue to track this discussion, and point to it in this comment in the code, right?
| .Where(s => !string.IsNullOrWhiteSpace(s.Username) | ||
| && !string.IsNullOrWhiteSpace(s.EncryptedPassword) | ||
| && Uri.TryCreate(s.Key.TrimEnd('/'), UriKind.Absolute, out var trimmedSourceUri) | ||
| && trimmedSourceUri == trimmedTargetUri) |
There was a problem hiding this comment.
Coming back to the comment further up, if this is doing a case-sensitive match on the URL (except the hostname), then this type of comparison is probably not what we would want.
Add Pester tests to ensure we don't inadvertently bleed configured credentials into scenarios where they should not be used.
The query string used by some commands seems to be ever so slightly different depending on where they are run. This removes the query string from the tests as these tests are not concerned with the entire message, just the initial part of the message.
| [string]$Name = "*", | ||
|
|
||
| [Parameter()] | ||
| [switch]$All |
There was a problem hiding this comment.
Am I missing where this is being used?
| # Skip the download command if chocolatey.extension is not installed. | ||
| Context 'Command (<Command>)' -Skip:($Command -eq 'download' -and -not $HasLicensedExtension) { | ||
| BeforeAll { | ||
| # Picked a package that is on `hermes-setup` but not on `hermes`. |
There was a problem hiding this comment.
How are we going to enforce this going forward?
Description Of Changes
--sourceon the command line are, and start making use of it here to aid with looking up source credentials.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.testsproject and make sure they pass.Cory added: Ran
CredentialProvidertag in Test Kitchen.Operating Systems Testing
Win10
Change Types Made
Change Checklist
Related Issue
Fixes #3565
Fixes #2421 (different symptom, same issue, likely somewhat duplicate issue)