GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Replace test certificates with self-signed certificate generating command by rjmholt · Pull Request #7875 · PowerShell/PowerShell

Conversation

Rob Holt (rjmholt) commented Sep 26, 2018

Copy link

Copy Markdown

Collaborator

PR Summary

Adds a dependency on the newly published SelfSignedCertificate module so that we can construct our own self-signed certificates at test time rather than checking them in.

To do this, the following happens:

Publish-PSTestTools will Save-Module the SelfSignedCertificate from the gallery to
$RepoRoot/test/tools/Modules/
In WebListener.psm1 we import the SelfSignedCertificate module
- I tried both using module at the top of the psm1 and NestedModules in the psd1 here but neither seemed to work
Start-WebListener now builds both client and server certificates when it is called

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
User-facing changes
- Not Applicable
- OR
- User-facing Documentation needed
  - Issue filed - Issue link:
Testing - New and feature
- Not Applicable or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
  - Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          [Feature] Replace test certificates with cert gen module

3762ab6

Rob Holt (rjmholt) requested review from James Truher (JamesWTruher), Steve Lee (SteveL-MSFT), Aditya Patwardhan (adityapatwardhan) and Mark Kraus (markekraus)

September 26, 2018 22:07

Rob Holt (rjmholt) requested review from Travis Plunk (TravisEz13), Andrew (anmenaga) and Dongbo Wang (daxian-dbw) as code owners

September 26, 2018 22:07


          Use fixed certificate module version

2b4e3e2

Steve Lee (SteveL-MSFT) requested changes

Sep 26, 2018

View reviewed changes

Comment thread

.gitignore Outdated

		/staging/
		/Packages/
		*.nuget.props
		/test/tools/Modules/SelfSignedCertificate/

Steve Lee (SteveL-MSFT) Sep 26, 2018

Copy link

Copy Markdown

Member

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.

Even though this module is for test, perhaps it's ok to just use install-module -scope currentuser? Adding it to a repo path breaks our work towards separating tests from dependencies on the repo.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Rob Holt (rjmholt) Sep 26, 2018

Copy link

Copy Markdown

Collaborator Author

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 had that exact discussion with James Truher (@JamesWTruher) and argued that we shouldn't pollute the installed modules because PowerShell was build on a machine. But I will make the change

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

James Truher (JamesWTruher) Sep 27, 2018

Copy link

Copy Markdown

Collaborator

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 should not be polluting the user's space with anything that isn't explicitly testing our behavior for the current user. These tests are very likely to be run on a dev box and we should not be adding stuff to the current user's scope. Further, this is a tool only for our tests. If the user happens to have this already, it still shouldn't use it, we should be using the module that we know we should be using in the location that we control without molesting their environment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Sep 27, 2018

Copy link

Copy Markdown

Member

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'd rather put it into a temp folder then

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

.gitignore Outdated

		*.AppImage

		# Ignore certificates
		*.pfx

Steve Lee (SteveL-MSFT) Sep 26, 2018

Copy link

Copy Markdown

Member

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.

Similarly, the pfx files should be generated outside of the repo structure

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Rob Holt (rjmholt) Sep 26, 2018

Copy link

Copy Markdown

Collaborator Author

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 question is where. We don't have a cross-platform temp dir and I would very much prefer not to put it in $HOME. Ideally it would be on the $TestDrive, but anything that tries to operate across Describe blocks with a generated certificate will fail.

Arguably we should refactor things to do exactly that, but I think that is out of scope of this PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Sep 26, 2018

Copy link

Copy Markdown

Member

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.

For now, Path::GetTempPath() is good enough I think

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/tools/Modules/WebListener/WebListener.psm1 Outdated

		{
		param([string]$CertificatePath)

		$password = ConvertTo-SecureString -Force -AsPlainText 'password'

Steve Lee (SteveL-MSFT) Sep 26, 2018

Copy link

Copy Markdown

Member

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 shouldn't have a hardcoded password. Looks like the GeneratePassword() method isn't available in corefx. I think it's fine to generate a random alphanumeric password.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/tools/Modules/WebListener/WebListener.psm1 Outdated

		@@ -61,10 +119,14 @@ function Start-WebListener
		$appDll = 'WebListener.dll'
		$serverPfx = 'ServerCert.pfx'
		$serverPfxPassword = 'password'

Steve Lee (SteveL-MSFT) Sep 26, 2018

Copy link

Copy Markdown

Member

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.

Maybe be better to store in env var the generated password rather than hardcode

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Rob Holt (rjmholt) and others added 11 commits

September 26, 2018 16:33


          Use correct version parameter for Save-Module

4f5811b


          Install SelfSignedCertificate rather than save it

faea431


          Generate self-signed certificate for protected certificate test

32d483e


          Update certificate-describing documentation

adf0199


          Force installation of the self-signed certificate

bd9a9f9


          Fix certificate test breakage

2478ca2


          Add -AllowClobber to Install-Module

db62e11


          Merge branch 'master' of https://github.com/rjmholt/PowerShell into r…

00a1065

…emove-web-cmdlet-test-certs


          Merge branch 'remove-web-cmdlet-test-certs' of https://github.com/rjm…

7b82ff3

…holt/PowerShell into remove-web-cmdlet-test-certs


          Remove hard-coded test passwords

4c6b542


          [Feature] Hide SelfSignedCertificate module name from spell checker

89e27c7

Rob Holt (rjmholt) force-pushed the remove-web-cmdlet-test-certs branch from 07044ee to 89e27c7 Compare

September 27, 2018 04:01


          [Feature] De-verbosify x509 enum parameters used in cert creation

39abcd9

Ilya (iSazonov) commented Sep 27, 2018

Copy link

Copy Markdown

Collaborator

We slow down CIs. Why we need the change?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          [Feature] Use module-qualified name for cert gen command

59d4344

Steve Lee (SteveL-MSFT) commented Sep 27, 2018

Copy link

Copy Markdown

Member

Ilya (@iSazonov) as a Microsoft project, we can't have secrets (even test secrets) stored statically in the repo

rjmholt, thezim, and iSazonov reacted with thumbs up emoji

All reactions

👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (SteveL-MSFT) commented Sep 27, 2018

Copy link

Copy Markdown

Member

Rob Holt (@rjmholt) the test failures are related to this PR

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Rob Holt (rjmholt) commented Sep 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator Author

Steve Lee (@SteveL-MSFT) Yes I just fixed them 🤞

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          [Feature] Get module name right

70c01d0

Rob Holt (rjmholt) force-pushed the remove-web-cmdlet-test-certs branch from 72256f8 to 70c01d0 Compare

September 27, 2018 17:24

Steve Lee (SteveL-MSFT) approved these changes

Sep 27, 2018

View reviewed changes

Robert Holt added 5 commits

September 27, 2018 16:30


          [Feature] Move SelfSignedCert module back to test tools module path

d92d44c


          [Feature] Correct .gitignore of test tools path

bf39e17


          [Feature] Fix module path usage, as asked by @JamesWTruher

dc41fef


          [Feature] Put cert gen module in temp dir, revert .gitignore changes

8e8e6d8


          [Feature] Fix module path mangling

e539e72

Ilya (iSazonov) reviewed

Sep 28, 2018

View reviewed changes

Ilya (iSazonov) left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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.

Steve Lee (@SteveL-MSFT) I found some samples of hard coded passwords in test files by key "password". (And in New-TestHost too).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

build.psm1 Outdated


		# Path to a directory to store modules we temporarily need to test PowerShell
		$script:TestModuleDirPath = Join-Path ([System.IO.Path]::GetTempPath()) 'PwshTestModules'
		New-Item -Force -ItemType Directory -Path $script:TestModuleDirPath

Ilya (iSazonov) Sep 28, 2018

Copy link

Copy Markdown

Collaborator

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.

Do we need output to console? If no we should New-Item > $null.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rjmholt reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

build.psm1 Outdated


		# Autoload (in subprocess) temporary modules used in our tests
		$command += '$env:PSModulePath = '+"'$TestModulePath$TestModulePathSeparator'" + '+$($env:PSModulePath);'
		$command += '$env:PSModulePath = '+"'$TestModulePath$TestModulePathSeparator$script:TestModuleDirPath$TestModulePathSeparator'" + '+$env:PSModulePath;'

Ilya (iSazonov) Sep 28, 2018

Copy link

Copy Markdown

Collaborator

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 afraid of such expressions. Maybe use Join-Path before the line?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Rob Holt (rjmholt) Sep 28, 2018

Copy link

Copy Markdown

Collaborator Author

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.

Join-Path won't work since we want ; and not \ (or : and not / on *nix). But I'll break this expression up a bit

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          [Feature] Address @iSazonov's feedback

40ac236

Ilya (iSazonov) approved these changes

Sep 28, 2018

View reviewed changes

Rob Holt (rjmholt) assigned Rob Holt (rjmholt) and Aditya Patwardhan (adityapatwardhan) and unassigned Rob Holt (rjmholt)

Sep 28, 2018

Dongbo Wang (daxian-dbw) reviewed

Oct 1, 2018

View reviewed changes

Comment thread

build.psm1 Resolved

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) reviewed

Oct 1, 2018

View reviewed changes

Comment thread

test/powershell/Modules/Microsoft.PowerShell.Security/certificateCommon.psm1 Outdated


		# Hackery to create a new 10 digit random hex string as a password
		$rand = [Random]::new()
		$global:protectedCertPassword = ConvertTo-SecureString -Force -AsPlainText (((1..10).ForEach{ '{0:x}' -f $rand.Next(0xf) }) -join '')

Dongbo Wang (daxian-dbw) Oct 1, 2018

Copy link

Copy Markdown

Member

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.

Better expose it through a function, instead of a global variable. The global variable will linger around even after you remove the module from a session.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rjmholt reacted with thumbs up emoji

All reactions

👍 1 reaction


          [Feature] Change global variable to function-exposed module variable

b5cc1ea

Aditya Patwardhan (adityapatwardhan) requested changes

Oct 1, 2018

View reviewed changes

Comment thread

test/powershell/Modules/Microsoft.PowerShell.Security/certificateCommon.psm1 Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

test/tools/Modules/WebListener/WebListener.psm1 Outdated


		$random = [Random]::new()

		return ((1..$Length).ForEach{ '{0:x}' -f $random.Next(0xf) }) -join ''

Aditya Patwardhan (adityapatwardhan) Oct 1, 2018

Copy link

Copy Markdown

Member

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 code is the same as New-CertificatePassword can we re-use the code?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rjmholt reacted with thumbs up emoji

All reactions

👍 1 reaction

Rob Holt (rjmholt) Oct 1, 2018

Copy link

Copy Markdown

Collaborator Author

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 common module I could put the function to generate a password in that is imported in both the web cmdlet tests and the CmdMessage tests?

Right now the only common module I can think of is the SelfSignedCertificate module, but that's not part of this codebase

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) Oct 1, 2018

Copy link

Copy Markdown

Member

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.

Rob Holt (@rjmholt) How about in here: https://github.com/PowerShell/PowerShell/blob/master/test/tools/Modules/HelpersCommon/HelpersCommon.psm1

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

TravisEz13 and rjmholt reacted with thumbs up emoji

All reactions

👍 2 reactions

Robert Holt added 2 commits

October 1, 2018 11:27


          [Feature] Prevent use of Get-CertificatePassword before New-Certifica…

2a5722b

…tePassword


          [Feature] Factor New-RandomHexString into test helper module

f772d6d

Aditya Patwardhan (adityapatwardhan) approved these changes

Oct 1, 2018

View reviewed changes

Aditya Patwardhan (adityapatwardhan) merged commit 649d342 into PowerShell:master

Oct 1, 2018

Mark Kraus (markekraus) mentioned this pull request

Oct 1, 2018

Move certificateCommon.psm1 to a Common Area and Replace Self-Signed Cert Valid for All tests #4598

Closed

Aditya Patwardhan (adityapatwardhan) pushed a commit to adityapatwardhan/PowerShell that referenced this pull request

Apr 9, 2019


          Replace test certificates with self-signed certificate generating com…

478fe24

…mand (PowerShell#7875)

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Replace test certificates with self-signed certificate generating com…

2cabbaa

…mand (PowerShell#7875)

Reviewers

Dongbo Wang (daxian-dbw)

daxian-dbw left review comments

Steve Lee (SteveL-MSFT)

SteveL-MSFT approved these changes

Aditya Patwardhan (adityapatwardhan)

adityapatwardhan approved these changes

Ilya (iSazonov)

iSazonov approved these changes

Mark Kraus (markekraus)

Awaiting requested review from markekraus

James Truher (JamesWTruher)

Awaiting requested review from JamesWTruher

Andrew (anmenaga)

Awaiting requested review from anmenaga

Travis Plunk (TravisEz13)

Awaiting requested review from TravisEz13

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Aditya Patwardhan (adityapatwardhan)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Rob Holt (rjmholt) commented Sep 26, 2018

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Sep 27, 2018

Uh oh!

Steve Lee (SteveL-MSFT) commented Sep 27, 2018

Uh oh!

Steve Lee (SteveL-MSFT) commented Sep 27, 2018

Uh oh!

Rob Holt (rjmholt) commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ilya (iSazonov) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Rob Holt (rjmholt) commented Sep 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.