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

(GH-399) client certificate support #481

Closed
wants to merge 1 commit into from
Closed

(GH-399) client certificate support #481

wants to merge 1 commit into from

Conversation

et1975
Copy link

@et1975 et1975 commented Nov 17, 2015

Please consider this merge for #399.

It requires chocolatey/nuget-chocolatey/pull/3/ to be pre-built and included as well.

Closes #399

@@ -384,6 +384,8 @@ public sealed class SourcesCommandConfiguration
public string Username { get; set; }
public string Password { get; set; }
public int Priority { get; set; }
public string Certificate { get; set; }
public string CertificatePassword { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Will you have both a user name / password and a certificate / password?

In cases like this maybe it makes sense that EncryptedPassword is used for both.

Copy link
Author

Choose a reason for hiding this comment

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

I see the two authentication methods as complimentary so it's quite possible for the passwords to be different.

Copy link
Member

Choose a reason for hiding this comment

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

Are you saying sometimes you would authenticate to the _same_ source with user name / password and sometimes you would authenticate with the certificate / password? Because I'm having a really hard time seeing that.

Copy link
Member

Choose a reason for hiding this comment

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

I'd also see the above as a bug if it didn't authenticate the same way every time.

Copy link
Author

Choose a reason for hiding this comment

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

No, like with proxy authentication, you can have different credentials for for different nodes along the way (btw, the certificate password is not and should not be sent along - it's just to open local certificate file). My use case - reverse proxy configured to do TLS and authenticate with certificates - server & client certificates exchange establishes HTTPS, NuGet server behind it - authorizes. Given that HTTPS termination is commonly done with the proxy, the HTTP server behind it won't even get a say in the negotiations.
Hope that makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the certificate be validated with the remote server or am I missing something?

So what you are stating is that it is likely in a single call for a source it will use both the certificate AND the username/password?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the public key from the certificate will be validated, by whatever is doing HTTPS termination, which may or may not be the NuGet server. The certificate validation does not however use password, but the chain of trust and revocation lists.
Yes, you could have the need for either/both, depending on how the server is implemented.

@ferventcoder ferventcoder changed the title GH-399 client certificate support (GH-399) client certificate support Nov 19, 2015
@ferventcoder
Copy link
Member

@et1975 have you had a chance to read over CONTRIBUTING? Please do and update your git commit with (GH-399) instead of GH-399 and include a nice git message body about what this change is and why it is necessary, etc. Thanks for your work so far!

@ferventcoder
Copy link
Member

The commit message is much better. It didn't really do line breaks at 72 characters though. We are a bit of a stickler for those.

@ferventcoder
Copy link
Member

I think you also need to compile and replace nuget-chocolatey so that builds here will pass.

@et1975
Copy link
Author

et1975 commented Nov 19, 2015

Are you reviewing disassembled binaries submitted this way? Cause otherwise there's nothing stopping me from packing it full of trojans, is there?

@ferventcoder
Copy link
Member

@et1975 I won't accept the binary changes. I just want them here to validate that in theory builds will work. :)

@ferventcoder
Copy link
Member

Speaking of which, there is a signer there that will use the default strong name key - you'll need to use it or things won't build. Which you probably already have seen.

@et1975
Copy link
Author

et1975 commented Nov 23, 2015

I think the outstanding failures are due to some environmental factors, no?

@ferventcoder
Copy link
Member

Looks like it:

Errors and Failures:
1) Test Error : chocolatey.tests.integration.infrastructure.filesystem.DotNetFileSystemSpecs+when_finding_paths_to_executables_with_dotNetFileSystem.GetExecutablePath_should_find_existing_executable
   Should.Core.Exceptions.EqualException : Assert.Equal() Failure
Position: First difference is at position 3
Expected: C:\Windows\system32\cmd.exe
Actual:   C:\windows\system32\cmd.exe
   at chocolatey.tests.integration.infrastructure.filesystem.DotNetFileSystemSpecs.when_finding_paths_to_executables_with_dotNetFileSystem.GetExecutablePath_should_find_existing_executable() in c:\projects\choco\src\chocolatey.tests.integration\infrastructure\filesystem\DotNetFileSystemSpecs.cs:line 67

2) Test Error : chocolatey.tests.integration.infrastructure.filesystem.DotNetFileSystemSpecs+when_finding_paths_to_executables_with_dotNetFileSystem.GetExecutablePath_should_find_existing_executable_with_extension
   Should.Core.Exceptions.EqualException : Assert.Equal() Failure
Position: First difference is at position 3
Expected: C:\Windows\system32\cmd.exe
Actual:   C:\windows\system32\cmd.exe
   at chocolatey.tests.integration.infrastructure.filesystem.DotNetFileSystemSpecs.when_finding_paths_to_executables_with_dotNetFileSystem.GetExecutablePath_should_find_existing_executable_with_extension() in c:\projects\choco\src\chocolatey.tests.integration\infrastructure\filesystem\DotNetFileSystemSpecs.cs:line 77

@ferventcoder
Copy link
Member

Remove your second commit please. I fixed those test issues.

@ferventcoder
Copy link
Member

Ping @et1975 - can you remove your second commit here?

@et1975
Copy link
Author

et1975 commented Dec 23, 2015

@ferventcoder I believe I did, the pull request shows only one commit.
Where do you see a problem?

@ferventcoder
Copy link
Member

Ah right on. The issues lie in rebase - any chance you can rebase your commits against current? If not I can do it.

@@ -164,7 +164,7 @@ public void GetExecutablePath_should_find_existing_executable()
[Fact]
public void GetExecutablePath_should_find_existing_executable_with_extension()
{
FileSystem.get_executable_path("cmd.exe").ShouldEqual(
FileSystem.get_executable_path("cmd.exe").ToLower().ShouldEqual(
Copy link
Member

Choose a reason for hiding this comment

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

weird.

Copy link
Author

Choose a reason for hiding this comment

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

I've seen \Windows and \windows returned, I guess it depends on how OS was installed...

Copy link
Member

Choose a reason for hiding this comment

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

It's more weird in that you are shifting it to lower and asking if it is equal to a string with upppercase.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, appologies, copy-paste strikes again, although I thought I reverted my
unit-test touches...
Hm, there are two copies of those tests and I guess
chocolatey.tests.infrastructure.filesystem aren't being executed?

Copy link
Member

Choose a reason for hiding this comment

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

Both get executed.

Copy link
Member

Choose a reason for hiding this comment

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

That's why it's a bit weird to me. :)

Copy link
Author

Choose a reason for hiding this comment

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

... pushed the update

Extends authentication by adding options to sources and commands
to specify client certificate and its password, as well as
implementing new IClientCertificateProvider from choco/nuget-chocolatey
to lookup those options based on the Uri requested.
These options are complimentary and can be used in addition to
username/password assuming the server implements it.
+ unit tests fix to use lowercased %ComSpec% when looking for cmd.exe
@ferventcoder
Copy link
Member

Merged into stable at 026df76

@coveralls
Copy link

Coverage Status

Coverage remained the same at 50.127% when pulling 25066f1 on Prolucid:stable into dbf0d22 on chocolatey:stable.

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

Successfully merging this pull request may close these issues.

3 participants