-
Notifications
You must be signed in to change notification settings - Fork 693
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
Add an allowInsecureConnections property in NuGet.Config #5343
Add an allowInsecureConnections property in NuGet.Config #5343
Conversation
src/NuGet.Core/NuGet.Configuration/Utility/ConfigurationContants.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.Configuration/Utility/ConfigurationContants.cs
Outdated
Show resolved
Hide resolved
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.
Should AsSourceItem_WorksCorrectly() and Clone_CopiesAllPropertyValuesFromSource() be updated in NuGet.Client\test\NuGet.Core.Tests\NuGet.Configuration.Test\PackageSourceTests.cs
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.
PR template not filled out.
In particular, since you're changing nuget.config, we need a docs issue to update the nuget.config reference doc at a minimum. If we want this documented someone in addition to the nuget.config reference, then the docs issue should say where else it should be documented.
@@ -19,6 +19,8 @@ public class PackageSource : IEquatable<PackageSource> | |||
/// </summary> | |||
public const int DefaultProtocolVersion = 2; | |||
|
|||
public const bool DefaultAllowInsecureConnections = false; |
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 don't see this as being very useful as a public API: https://github.com/NuGet/NuGet.Client/blob/dev/docs/nuget-sdk.md#changes-to-apis
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.
Thanks! Fixed. It's no more a public API.
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.
Sorry, had to change it back into a public API, as PackageSourceContextInfoFormatter.cs needs this.
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 might be having an overly strong, or just unpopular, opinion, so consider this a nitpick. I'm curious what other peoples opinions are. But, I still don't think it's a good/useful public API.
default(bool)
is false
, so providing a different way to get the same const value isn't useful. Providing a const for a non-obvious value makes sense, which is why DefaultProtocolVersion
isn't so bad. But I think it should be obvious to anyone who understands basic security principals that allowing insecure connections is not default. And therefore I can't imagine this ever changing in the future, and changing defaults is a good reason to have pubic APIs.
But on the topic of changing defaults, from an API design point of view, I think a const is problematic for providing default values. The default protocol version above has the same problem. If in the future we want to change the defaults, then it will only impact projects that update their package version and recompile their project. Consts get "burned in" at compile time. So, if I have Zivkan.Utils
that compiled against this false
default, then someone using my package uses my package, but a newer version of NuGet.Configuration
that changed the default, my Zivkan.Utils
library will continue using the old, now incorrect, value at runtime. If we want to provide defaults as public APIs, get-only properties, or static readonly
fields will cause assemblies that reference NuGet.Configuraiton
to read the updated defaults at runtime.
The first BCL example to use as prior art that I can think of is HttpClient.DefaultRequestVersion
and HttpRequestMessage.Version
. The docs say what the default is if you don't explicitly set the value youself, but there is no static or const API to get the default. But maybe the default HTTP protocol version isn't a good example, because it's not serialized to disk.
The first example I could think of a value that is serialized to disk is how before .NET Core 1.0, all MSBuild project files had to have XML namespaces. But MSBuild's APIs don't appear to have a public API to get the default namespace. All examples I see in a quick search are private
or internal
, and they've duplicated it in a few assemblies, rather than having a single public API that's referenced from all other assemblies: https://github.com/search?q=repo%3Adotnet%2Fmsbuild%20http%3A%2F%2Fschemas.microsoft.com%2Fdeveloper%2Fmsbuild%2F2003&type=code
I think it's perfectly acceptable to use bool allowInsecureConnections = false
, rather than this const across assembly boundaries, in PackageSourceContextInfoFormatter
.
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.
Thanks! Fixed.
@@ -698,7 +709,8 @@ public void SavePackageSources(IEnumerable<PackageSource> sources, PackageSource | |||
|
|||
if (existingSettingsLookup != null && | |||
existingSettingsLookup.TryGetValue(source.Name, out existingSourceItem) && | |||
ReadProtocolVersion(existingSourceItem) == source.ProtocolVersion) | |||
ReadProtocolVersion(existingSourceItem) == source.ProtocolVersion && | |||
ReadAllowInsecureConnections(existingSourceItem) == source.AllowInsecureConnections) |
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.
This is buggy, and exactly what I'm fixing in #5334
Unfortunately, although I addressed all feedback 2 days ago, nobody has reviewed or approved the PR since then, so it's still not merged 😞
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.
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.
@zivkan can you reach out to @aortiz-msft directly?
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.
Thanks for reminding me about this recent change! This is fixed.
{ | ||
} | ||
|
||
public SourceItem(string key, string value, string protocolVersion = "", string allowInsecureConnections = "") |
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.
The optional parameters no longer provide value. If someone has source code calling new SourceItem(key, value)
, it's going to use the 2 string overload, not this one with the default parameters. Same for the 3 string new Source(key, value, protocolVersion)
overload, it's not going to use this 4 string overload. Therefore, the only way to call this overload is to explicitly pass in all 4 parameters, at which point none of them are using the default values.
Well, that's not entirely true. It's possible to call new SourceItem(key, value, allowInsecureConnections: "true")
, but since this is a new API, there's obviously not going to be any existing code doing this.
It also avoid potential future issues with public APIs and default parameter analyzer violations.
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.
This comment hasn't been addressed. I feel strongly that the default values should be removed, so we don't get those analyzer warnings if trying to add more overloads in the future. Honestly, in my opinion, public APIs should never use default values. Leave it for private and internal APIs only.
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.
Thanks! Fixed. Removed the public API with optional arguments.
@@ -23,13 +23,41 @@ public string ProtocolVersion | |||
set => AddOrUpdateAttribute(ConfigurationConstants.ProtocolVersionAttribute, value); | |||
} | |||
|
|||
public SourceItem(string key, string value, string protocolVersion = "") | |||
public string AllowInsecureConnections |
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.
You should check what happens when the nuget.config already has a value for allowInsecureConnections
, and then you edit sources in VS's options page. I think VS will strip out the allowInsecureConnections
value, so I think you need to add this property into PackageSourceContextInfo
, its MsgPack formatter, and possibly NuGetSourcesService.GetPackageSourcesToUpdate
, as I did in this PR: #5334
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.
Thanks! That's very helpful! Fixed and added tests. Manually tested in VS, the non-default value of allowInsecureConnections
will not be stripped out when updating in VS options page. The default value(false) and invalid value(e.g. "random", will be changed into default value) will be removed after updating, as an expected normalization work. (Just like the protocal Version "2" will be stripped out after updating).
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.
There are two tests validating that sourceItem can be parsed successfully with allowInsecureConnections
, but there is no test validating that it gets serialized back to nuget.config correctly
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.
Thanks! Added a test case SourceItem_AsXNode_ReturnsExpectedXNode
.
test/NuGet.Core.Tests/NuGet.Packaging.Test/RuntimeModelTests/RuntimeDescriptionTests.cs
Outdated
Show resolved
Hide resolved
@heng-liu I didn't see an update on this in the latest iteration. |
[InlineData(true, "2", "true")] | ||
[InlineData(false, null, "true")] | ||
[InlineData(false, "", "true")] | ||
public void LoadPackageSources_ReadsSourcesWithAllowInsecureConnectionsFromPackageSourceSections(bool useStaticMethod, string protocolVersion, string allowInsecureConnections) |
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 know people like keeping code to a minimum, but I would say this is a good time to create multiple tests. When there's significant branching in Assertions, that makes it hard to understand what it means when the test fails. Also, the name of the method doesn't indicate the criteria and success conditions.
eg, we could have (just example wordings):
LoadPackageSources_WithNullProtocolVersion_LoadsDefault
LoadPackageSources_WithNullAllowInsecureConnections_LoadsDefault
...
LoadPackageSources_WithNotNullProtocolVersion_LoadsValue
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.
+1 to above comment.
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.
Thanks! Fixed.
</packageSources> | ||
</configuration>"; | ||
|
||
var expectedValues = new List<SourceItem>() | ||
{ | ||
new SourceItem("nugetorg","http://serviceIndexorg.test/api/index.json"), | ||
new SourceItem("nuget2","http://serviceIndex.test/api/index.json", "3" ), | ||
new SourceItem("nuget2","http://serviceIndex.test2/api/index.json", "3" ), | ||
new SourceItem("nuget3","http://serviceIndex.test3/api/index.json", protocolVersion: "3", allowInsecureConnections: "true" ), |
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.
what happens if we specify allowInsecureConnections
in a different case? I assume that setting will be applied correctly but wanted to make sure.
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.
Thanks for reviewing! Yes, it works for a different case. I changed one line to use Uppercase.
4885d86
to
6211314
Compare
Thanks! Created a doc issue and fill out the PR template. |
Thanks! Updated the test case in the above test cases.
|
I spent some time on investigating the tricky test AsSourceItem_WorksCorrectly() and sorry for the delay! |
@@ -33,6 +34,7 @@ private PackageSourceContextInfoFormatter() | |||
string? description = null; | |||
int originalHashCode = 0; | |||
int protocolVersion = PackageSource.DefaultProtocolVersion; | |||
bool allowInsecureConnections = PackageSource.DefaultAllowInsecureConnections; |
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.
this variable is updated when reading all the properties, but it's not used when creating a new instance of PackageSourceContextInfo
around like 77.
Since no tests failed, this means there's also missing and/or buggy tests. A good way to avoid these types of accidents is to write tests first, that fail, and then change the product code so the test passes (test driven development, TDD).
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.
Good catch! Fixed both the src and test.
The original test is just comparing the name and source, changed it into comparing the hashcode.
@@ -19,6 +19,8 @@ public class PackageSource : IEquatable<PackageSource> | |||
/// </summary> | |||
public const int DefaultProtocolVersion = 2; | |||
|
|||
public const bool DefaultAllowInsecureConnections = false; |
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 might be having an overly strong, or just unpopular, opinion, so consider this a nitpick. I'm curious what other peoples opinions are. But, I still don't think it's a good/useful public API.
default(bool)
is false
, so providing a different way to get the same const value isn't useful. Providing a const for a non-obvious value makes sense, which is why DefaultProtocolVersion
isn't so bad. But I think it should be obvious to anyone who understands basic security principals that allowing insecure connections is not default. And therefore I can't imagine this ever changing in the future, and changing defaults is a good reason to have pubic APIs.
But on the topic of changing defaults, from an API design point of view, I think a const is problematic for providing default values. The default protocol version above has the same problem. If in the future we want to change the defaults, then it will only impact projects that update their package version and recompile their project. Consts get "burned in" at compile time. So, if I have Zivkan.Utils
that compiled against this false
default, then someone using my package uses my package, but a newer version of NuGet.Configuration
that changed the default, my Zivkan.Utils
library will continue using the old, now incorrect, value at runtime. If we want to provide defaults as public APIs, get-only properties, or static readonly
fields will cause assemblies that reference NuGet.Configuraiton
to read the updated defaults at runtime.
The first BCL example to use as prior art that I can think of is HttpClient.DefaultRequestVersion
and HttpRequestMessage.Version
. The docs say what the default is if you don't explicitly set the value youself, but there is no static or const API to get the default. But maybe the default HTTP protocol version isn't a good example, because it's not serialized to disk.
The first example I could think of a value that is serialized to disk is how before .NET Core 1.0, all MSBuild project files had to have XML namespaces. But MSBuild's APIs don't appear to have a public API to get the default namespace. All examples I see in a quick search are private
or internal
, and they've duplicated it in a few assemblies, rather than having a single public API that's referenced from all other assemblies: https://github.com/search?q=repo%3Adotnet%2Fmsbuild%20http%3A%2F%2Fschemas.microsoft.com%2Fdeveloper%2Fmsbuild%2F2003&type=code
I think it's perfectly acceptable to use bool allowInsecureConnections = false
, rather than this const across assembly boundaries, in PackageSourceContextInfoFormatter
.
{ | ||
} | ||
|
||
public SourceItem(string key, string value, string protocolVersion = "", string allowInsecureConnections = "") |
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.
This comment hasn't been addressed. I feel strongly that the default values should be removed, so we don't get those analyzer warnings if trying to add more overloads in the future. Honestly, in my opinion, public APIs should never use default values. Leave it for private and internal APIs only.
[Theory] | ||
[InlineData(true)] | ||
[InlineData(false)] | ||
public void LoadPackageSources_ReadsSourcesWithRandomAllowInsecureConnectionsFromPackageSourceSections_LoadsDefault(bool useStaticMethod) |
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.
public void LoadPackageSources_ReadsSourcesWithRandomAllowInsecureConnectionsFromPackageSourceSections_LoadsDefault(bool useStaticMethod) | |
public void LoadPackageSources_ReadsSourcesWithInvalidAllowInsecureConnectionsFromPackageSourceSections_LoadsDefault(bool useStaticMethod) |
having a test name saying "with random ..." makes it sound like you're going to use System.Random
to auto-generate random values to test with (basically, fuzz testing), which is not what this test is doing.
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.
Thanks! Fixed.
settings | ||
.Setup(s => s.GetSection("packageSourceCredentials")) | ||
.Returns(new VirtualSettingSection("packageSourceCredentials", | ||
new CredentialsItem("Source", "source-user", "source-password", isPasswordClearText: true, validAuthenticationTypes: null))); |
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.
Is there a reason I can't think of why credentials are being tested in addition to the allow insecure connection setting? The test name doesn't say anything about credentials, and I would expect that other tests already validate credentials. Could all the tests be 6 lines shorter by removing the credentials stuff? (and anything else that's already being tested elsewhere/is not relevant to the name of the test)
I found the credentials distracting, making it slower to read and understand the tests.
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.
Thanks! Fixed.
.Returns(new List<string>()); | ||
// Act |
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.
nitpick: empty line between these two. Same issue in ther other 2 tests as well.
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.
Thanks! Fixed.
@@ -76,18 +83,58 @@ public void SourceItem_ParsedSuccessfully() | |||
var section = settingsFile.GetSection("packageSources"); | |||
section.Should().NotBeNull(); | |||
|
|||
var children = section.Items.ToList(); | |||
var children = section.Items.Select(c => c as SourceItem).ToList(); |
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.
please don't use as
to cast, unless you handle nulls 😭
var children = section.Items.Select(c => c as SourceItem).ToList(); | |
var children = section.Items.Cast<SourceItem>().ToList(); |
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.
Thanks! Fixed.
new XElement("add", | ||
new XAttribute("key", "nuget4"), | ||
new XAttribute("value", "http://serviceIndex.test4/api/index.json"), | ||
new XAttribute("protocolVersion", "3")))); |
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 find this more difficult to read than const string expectedXml = @"<configuration> ...
. Whether you use XElement.Parse
or serialize the output of AsXNode
to a string and then compare strings, I don't think it matters too much. A good way to choose between the two is to make the test fail, see what the exception message is, and then pick the one that is easier to understand.
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.
Thanks! Fixed.
Thanks for all the comments! I think all the comments are addressed. Pls help review it again. Thanks! |
@@ -13,6 +13,7 @@ internal sealed class PackageSourceContextInfoFormatter : NuGetMessagePackFormat | |||
private const string SourcePropertyName = "source"; | |||
private const string IsEnabledPropertyName = "isenabled"; | |||
private const string ProtocolVersionPropertyName = "protocolversion"; | |||
private const string AllowInsecureConnectionsPropertyName = "allowInsecureConnections"; |
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 like your use of camel-casing better, but should this be lowercase to be consistent?
new AuthorizationServiceClient(Mock.Of<IAuthorizationService>()), | ||
packageSourceProvider.Object); | ||
|
||
List<PackageSourceContextInfo> updatedSources = new(1) |
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.
nit:
List<PackageSourceContextInfo> updatedSources = new(1) | |
List<PackageSourceContextInfo> updatedSources = new(capacity: 1) |
@@ -93,6 +93,7 @@ private IReadOnlyList<PackageSource> GetPackageSourcesToUpdate(IReadOnlyList<Pac | |||
if (packageSource.Name.Equals(packageSourceContextInfo.Name, StringComparison.InvariantCulture) | |||
&& packageSource.Source.Equals(packageSourceContextInfo.Source, StringComparison.InvariantCulture) | |||
&& packageSource.ProtocolVersion == packageSourceContextInfo.ProtocolVersion | |||
&& packageSource.AllowInsecureConnections == packageSourceContextInfo.AllowInsecureConnections |
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.
The comment above, which I don't think is useful, is now inaccurate.
// If key properties have not changed, we don't need to do anything
// If identifying properties have not changed, we don't need to do anything
// If Name/Source/IsEnabled/ProtocolVersion/AllowInsecureConnections has not changed, we don't need to do
anything- delete the comment
I vote for 4 or 1.
This comment is already addressed(the PR template is filled and creating a doc issue as well).
Bug
Fixes: NuGet/Home#12786
Regression? No.
Description
Add an
allowInsecureConnections
property intopackageSources
section in NuGet.Config files, as below:The default value will be false.
Spec: https://github.com/NuGet/Home/blob/dev/proposed/2023/InsecureConnectionsDisableCertificateValidation.md#package-source-nuget-config
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation