-
Notifications
You must be signed in to change notification settings - Fork 8.1k
[Feature] Add Certificate Authentication Support for WebCmdlets #4546
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
[Feature] Add Certificate Authentication Support for WebCmdlets #4546
Conversation
|
@markekraus, |
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.
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.
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.
@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?
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.
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.
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.
@markekraus Can you address this feedback please?
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.
@adityapatwardhan I did here and with 305c048. Was that insufficient?
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 missed that.
|
@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. |
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 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
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.
Especially introducing a dependency on a new site.
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 agree. But I lack the skill to implement this. I would need guidance or assistance.
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.
It seems @SteveL-MSFT have plans to add HTTPS support in our test HttpListener.psm1.
I like it more than adding hook.
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.
@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?
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'm open to that, but I don't think we should be regularly
running the tests without doing one one the other either:
- Get explicit permission to use the site for automated tested
- find an alternative
- mark the tests a pending until the issue is 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.
@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.
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.
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.
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.
Oh, and add a comment with the issue number saying why the tests are marked pending.
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.
@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.
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.
Suggest explicitly setting ClientCertificateOptions to ClientCertificateOption.Manual.
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.
added.
|
@TravisEz13 @dantraMSFT Can you have another look please? |
eaec678 to
d6375c1
Compare
TravisEz13
left a 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.
We need to find some solution to deal with https://prod.idrix.eu/secure/
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 think we need some explicit statement linked to the site that we can use the site in our tests.
TravisEz13
left a 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.
I'm ok with this assuming Aditya agrees
adityapatwardhan
left a 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.
LGTM
|
Had a chat with @TravisEz13 about the testing challenges. Now that #4609 is opened, I can merge this. |
Fixes #4544
HttpClientHandlernow supports client certificates. Enabling client certificates in Core forInvoke-WebRequestandInvoke-RestMethodmakes it possible to do certificate authentication on Linux.