Skip to content

Conversation

@markekraus
Copy link
Contributor

Fixes #4544

HttpClientHandler now supports client certificates. Enabling client certificates in Core for Invoke-WebRequest and Invoke-RestMethod makes it possible to do certificate authentication on Linux.

@msftclas
Copy link

@markekraus,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

Copy link
Member

Choose a reason for hiding this comment

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

We already have a common module to add certificates here:
https://github.com/PowerShell/PowerShell/blob/master/test/powershell/Modules/Microsoft.PowerShell.Security/certificateCommon.psm1

We shouldn't duplicate the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TravisEz13 I tested with that. But the cert in New-GoodCertificate does not contain the proper key usage to be used for SSL/TLS Auth. It is issued for only Document Encryption. I was afraid if I change the cert in certificateCommon.psm1 I might be break tests that depend on that restriction. Thus, I added a new function in WebCmdlets.Tests.ps1 instead. What do you suggest?

Copy link
Member

Choose a reason for hiding this comment

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

Move the module to a common location and add your function to it, with descriptions of each cert (at least what you learned.) At the minimum, file an issue that this work needs to be done to avoid future duplication of effort. Even then, I would suggest adding the comments to each now.

Copy link
Member

Choose a reason for hiding this comment

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

@markekraus Can you address this feedback please?

Copy link
Contributor Author

@markekraus markekraus Aug 16, 2017

Choose a reason for hiding this comment

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

@adityapatwardhan I did here and with 305c048. Was that insufficient?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry missed that.

@markekraus
Copy link
Contributor Author

@TravisEz13 I think what may need to be done is issue a self-signed cert that has all the key usage and enhanced key usages required by all the tests, a kind of one-cert-to-rule-them-all and then also move the module to a common area etc.

For now I have added comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest you check to see if you can mock using WebRequest.RegisterPrefix instead of relying on an external site. We find external sites make the tests unreliable. See #2504

Copy link
Member

Choose a reason for hiding this comment

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

Especially introducing a dependency on a new site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. But I lack the skill to implement this. I would need guidance or assistance.

Copy link
Collaborator

@iSazonov iSazonov Aug 12, 2017

Choose a reason for hiding this comment

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

It seems @SteveL-MSFT have plans to add HTTPS support in our test HttpListener.psm1.
I like it more than adding hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iSazonov awesome. that would be better. It will probably also need some way to ensure the client authentication cert is received by the listener too or to enforce certificate authentication on the listener so it can fail if one is not provided.

@dantraMSFT I'm not entirely sure WebRequest.RegisterPrefix will work on Core. Unless I'm missing something, I don't think it would affect anything being done with the new classes under System.Net.Http in use in Core. I was able to make a mockup with it, but it only works on Windows PowerShell (tested on 5.1). Unless I'm going totally in the wrong direction?

Copy link
Member

Choose a reason for hiding this comment

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

I'm open to that, but I don't think we should be regularly
running the tests without doing one one the other either:

  1. Get explicit permission to use the site for automated tested
  2. find an alternative
  3. mark the tests a pending until the issue is resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TravisEz13 Ok. Let me look into docker/aspnetcore. If I can't make heads or tails of it, I will mark the test pending and create an issue.

On the docker topic, would it be best to include a subfolder with a dockerfile and whatever assets are needed, have the test code run the docker build, run the container, do the relevant test, and tear down the container? Or or there some better way to do that? I imagine that could be a rather slow test.

Copy link
Member

Choose a reason for hiding this comment

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

It would definitely make these slow tests and I think feature tests. There are some tests that use docker here: https://github.com/PowerShell/PowerShell/tree/master/test/docker/networktest But those tests, are meant to be a different suite.

I think @iSazonov might be right. I think option 3 might be the best option. Mark the tests as pending, and file an issue. If you are still willing to work on trying to fix the tests, I'd love to work with you. Assuming @adityapatwardhan (the maintainer for the PR) agrees.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and add a comment with the issue number saying why the tests are marked pending.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TravisEz13 Issue #4609 created, tests set to pending, and comments added. I'm still interested in figuring this out, I'm just unsure of the time it will take me to get up to speed on aspnetcore enough to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest explicitly setting ClientCertificateOptions to ClientCertificateOption.Manual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

@adityapatwardhan
Copy link
Member

@TravisEz13 @dantraMSFT Can you have another look please?

Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

We need to find some solution to deal with https://prod.idrix.eu/secure/

Copy link
Member

Choose a reason for hiding this comment

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

I think we need some explicit statement linked to the site that we can use the site in our tests.

Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

I'm ok with this assuming Aditya agrees

Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

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

LGTM

@adityapatwardhan
Copy link
Member

Had a chat with @TravisEz13 about the testing challenges. Now that #4609 is opened, I can merge this.

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.

6 participants