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

Add support for dynamic certifcates in Web Binding #307

Merged
merged 9 commits into from
Oct 17, 2017
Merged

Add support for dynamic certifcates in Web Binding #307

merged 9 commits into from
Oct 17, 2017

Conversation

ChrisLGardner
Copy link

@ChrisLGardner ChrisLGardner commented May 3, 2017

This addresses #201 by using @PlagueHO's Find-Certificate function from xCertificate to search for a Certificate based on the Subject.

Further parameters from Find-Certificate can be exposed if needed but it feels like Subject is the main one likely to be known when writing the configuration script.


This change is Reviewable

@ChrisLGardner
Copy link
Author

@kwirkykat or @tysonjhayes able to review this?

Thanks

@PlagueHO
Copy link
Member

@ChrisLGardner - I'm in transit for a few weeks so won't have much time, but if @tysonjhayes can't make it to this by the time I'm back to my usual capacity, I'll review for you. Thanks for submitting - great feature BTW!

@tysonjhayes
Copy link

Reviewed 7 of 10 files at r1.
Review status: 7 of 10 files reviewed at latest revision, 1 unresolved discussion.


Tests/TestHelper/CommonTestHelper.psm1, line 115 at r1 (raw file):

    )

    $newSelfSignedCertURL = 'https://gallery.technet.microsoft.com/scriptcenter/Self-signed-certificate-5920a7c6/file/101251/2/New-SelfSignedCertificateEx.zip'

I'm not sure I'm super comfortable with this. We're downloading some file, unzipping it and using it? I'm not sure our license allows this and we're effectively shipping this with our product. Has this methodology been used else where? Is this the best way to generate this?


Comments from Reviewable

@PlagueHO
Copy link
Member

Hi @tysonjhayes - this was actually a method I implemented for xCertificate. The reason is that the full New-SelfSignedScript function wasn't added till Windows 10 - so anyone running tests on anything older would run into test failures. The older New-SelfSignedScript in Windows Server 8.1 and Windows Server 2012 R2 doesn't allow creation of the correct types of certs and doesn't expose most properties.

I personally don't like retrieving this script from the script center either to run these tests and I have asked the creator of the script (an MVP) to upload to the gallery (https://gallery.technet.microsoft.com/scriptcenter/Self-signed-certificate-5920a7c6/view/Discussions) to make it easier (and more reliable) for us to consume, but he has refused stating "I don't consider PowerShell Gallery suitable for standalone scripts." - which I have pointed out is incorrect.

Also, as xCertificate has changed over to a harness/auto-doc format type repo (like xNetworking) we don't distribute the tests anymore - so the only place this code will be is in the repo.

But I'm definitely with you - I'm not completely comfortable either - just haven't found a better work around yet 😢 .Although theoretically we could just write a version of his script ourselves and publish it to the PS Gallery 😁 - but that is a fair bit of work that could be avoided if we could get the existing script to the gallery!

@koarn
Copy link

koarn commented Sep 5, 2017

:lgtm:


Review status: 7 of 10 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@koarn
Copy link

koarn commented Sep 18, 2017

Hi guys, is there any progress on these reviews? Would love to see these features added :)


Review status: 7 of 10 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@PlagueHO
Copy link
Member

Hi @tysonjhayes - do you think you'll get a chance to take a look through this one? No worries if not, I'm happy to try and get to this one over the weekend.

@ChrisLGardner
Copy link
Author

Review status: 7 of 10 files reviewed at latest revision, 1 unresolved discussion.


Tests/TestHelper/CommonTestHelper.psm1, line 115 at r1 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

I'm not sure I'm super comfortable with this. We're downloading some file, unzipping it and using it? I'm not sure our license allows this and we're effectively shipping this with our product. Has this methodology been used else where? Is this the best way to generate this?

I'm not happy about it either, for the same reasons as mentioned by @PlagueHO there are only so many things we can do about this.

Do we publish a version to the gallery ourselves? Do we store a copy with the tests (that has to be maintained on it's own)? Do we possibly add it to the common DSC tests so it can be used in many other resources should they need the cert?


Comments from Reviewable

@tysonjhayes
Copy link

Reviewed 3 of 10 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Tests/TestHelper/CommonTestHelper.psm1, line 115 at r1 (raw file):

Previously, ChrisLGardner (Chris Gardner) wrote…

I'm not happy about it either, for the same reasons as mentioned by @PlagueHO there are only so many things we can do about this.

Do we publish a version to the gallery ourselves? Do we store a copy with the tests (that has to be maintained on it's own)? Do we possibly add it to the common DSC tests so it can be used in many other resources should they need the cert?

I would feel better adding it to the common DSC tests, or at least putting it in the gallery. I look at this as it could just up and disappear at any point, and we didn't write the code. So we're relying on something that could go away and on something that we didn't write ourselves. Just doesn't feel great? Honestly I'll take this in order to merge but I'd like to see us move away from it.


Comments from Reviewable

@tysonjhayes
Copy link

I guess I'm more or less signed off. I don't love this but I'm at a loss on how to do it better? Any thoughts from anyone else before I merge?


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@ChrisLGardner
Copy link
Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


Tests/TestHelper/CommonTestHelper.psm1, line 115 at r1 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

I would feel better adding it to the common DSC tests, or at least putting it in the gallery. I look at this as it could just up and disappear at any point, and we didn't write the code. So we're relying on something that could go away and on something that we didn't write ourselves. Just doesn't feel great? Honestly I'll take this in order to merge but I'd like to see us move away from it.

I'll raise an issue on the DSC Resources repo and try to put together a PR for it as it would be useful for a few resources to have this sort of functionality.


Comments from Reviewable

@koarn
Copy link

koarn commented Oct 17, 2017

@tysonjhayes, looks like you're good to merge?


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@tysonjhayes tysonjhayes merged commit b9d414f into dsccommunity:dev Oct 17, 2017
@dan-escott
Copy link

Thanks for completing this - I've been watching this for a while :)

When can we expect an official build of this in PSGallery?

@ChrisLGardner ChrisLGardner deleted the issue-201-certs branch October 19, 2017 11:05
@kwirkykat
Copy link
Contributor

@dandiddy Next DSC Resource Kit release is November 15, so you should see it up in PSGallery on November 16.

@achern
Copy link

achern commented Oct 30, 2017

@ChrisLGardner There's a small issue with this merge. The message definitions for Find-Certificate weren't copied into Helper.psm1's LocalizedData, so it doesn't log anything.

@achern
Copy link

achern commented Oct 30, 2017

Also, FriendlyName would also be a good candidate to select a certificate with. I think it's what the IIS menus use.

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.

9 participants