Changed $ca parameter #43

svenvanrijen · 2017-01-31T13:00:16Z

Changed $ca parameter from $ca = "'$CAServerFQDN$CARootName' " to $ca
= "$CAServerFQDN$CARootName". Old parameter gave errors since
'$CAServerFQDN (example: 'CA01.domain.com) could not be found when
running the script.

This change is

Changed $ca parameter from $ca = "'$CAServerFQDN\$CARootName' " to $ca = "$CAServerFQDN\$CARootName". Old parameter gave errors since '$CAServerFQDN (example: 'CA01.domain.com) could not be found when running the script.

msftclas · 2017-01-31T13:00:20Z

Hi @svenvanrijen, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

BerheAbrha · 2017-02-02T19:02:06Z

DSCResources/MSFT_xCertReq/MSFT_xCertReq.psm1

@@ -128,7 +128,7 @@ function Get-TargetResource
    )

    # The certificate authority, accessible on the local area network
-    $ca = "'$CAServerFQDN\$CARootName'"
+    $ca = "$CAServerFQDN\$CARootName"


Change looks good. Please document your change on the README.md file https://github.com/PowerShell/xCertificate/blob/dev/ReadMe.md

Just all changed lines as discussed under PR 43 and issue 42. Also documented the changes in README.md file as requested.

PlagueHO · 2017-02-02T21:40:27Z

Hi @svenvanrijen - this change will actually cause problems if the $CARootName contains spaces (which is an acceptable CA Root Name name format).

Could I request that rather than just remove the quotes, could you put a check in to ensure that the $CARootName contains no spaces? If there are no spaces then removing the quotes is OK, if not, they need to be left in.

It might also be worth moving the $ca = "$CAServerFQDN\$CARootName" into a separate function in the CertificateCommon module. But that isn't a requirement.

Does that make sense?

And good job BTW! 😁

svenvanrijen · 2017-02-06T09:52:44Z

Hi @PlagueHO . Thank you for your feedback. Gonna work on that later this week and will keep u posted.

Regards,
Sven

PlagueHO · 2017-02-08T08:30:28Z

Thanks @svenvanrijen - thanks for doing this for the community!

This reverts commit 998e100.

svenvanrijen · 2017-02-09T14:12:27Z

Hi @PlagueHO,

I've been checking on your comments, but it seems like I don't fully get it.
First of all, I don't want to remove ALL the quotes, only the single quotes. I understand that, when using no quotation at all, things go wrong when a string contains spaces.

I'll try to show you what I mean with 3 examples:

the variables are:
$CAServerFQDN = 'ca.test.com'
$CARootName = 'this-is a test'

Example 1: the original config
$ca = "'$CAServerFQDN\$CARootName'"

Output:
'ca.test.com\this-is a test'

Example 2: my suggested change
$ca = "$CAServerFQDN\$CARootName"

Output:
ca.test.com\this-is a test

Example 3: no quotes at all
$ca = $CAServerFQDN\$CARootName

Output (=Error message):
At line:1 char:20 $ca = $CAServerFQDN\$CARootName Unexpected token '\$CARootName' in expression or statement.

So, my point is: removing just the single quotes from the original config is valid as it does not negatively influence the output of the code when spaces are used.

Can you agree on this?

Kind regards,
Sven

This reverts commit a3dddd3.

PlagueHO · 2017-02-10T06:15:29Z

Hi @svenvanrijen - what I meant was to do something like this:

if ($CARootName.Contains(' '))
{
    $ca = "'$CAServerFQDN\$CARootName'"
}
else
{
    $ca = "$CAServerFQDN\$CARootName"
}

Where I work we have CA's with the following naming styles:
CACONTOSO-DV
Contoso.com Issuing CA - DEV 2

The top style CA should be fine without single quotes. The bottom style I think will have problems without single quotes around it. I meant to do some more investigation on my systems to confirm this but I ran out of time today.

What I want to ensure is that any fix works for all naming conventions.

svenvanrijen · 2017-02-10T06:34:56Z

Hi @PlagueHO,

I do understand that, I have the same intentions. I also understood the if/then statement intention, but I think/thought that might not be necessary. But please test this on your systems, and if indeed necessary, I'll be happy to add the if/then statement to the code.

Kind regards,
Sven

PlagueHO · 2017-02-11T07:27:33Z

Hi @svenvanrijen - I'll do a couple of quick tests at the office next week and see if the single quotes are required on all CA's. Thanks for your help and patience!

PlagueHO · 2017-02-13T08:02:05Z

Hi @svenvanrijen - Ok, I have made some progress looking into this.

You can go ahead and keep the change to remove the single quotes completely from line:

$ca = "'$CAServerFQDN\$CARootName'"

There is no need for the check if spaces exist. However, you will need to change this line: https://github.com/PowerShell/xCertificate/blob/dev/DSCResources/MSFT_xCertReq/MSFT_xCertReq.psm1#L407

To:

$arguments = "-Command ""& $ENV:SystemRoot\system32\certreq.exe" + `
    " @('-submit','-q','-config','$ca','$reqPath','$cerPath')" + `
    " | Set-Content -Path '$certReqOutPath'"""

This should allow the resource to work when Credentials are provided and when they are not. The Credential parameter is required when the CA server does not allow "Everyone" to issue certificates (e.g. you need to request the cert using a specific account).

Does this make sense?

svenvanrijen · 2017-02-13T10:02:50Z

Hi @PlagueHO,

No problem, going to work on that.
Are there any other files I need to update, besides the .psm1 and https://github.com/PowerShell/xCertificate/blob/dev/ReadMe.md as @BerheAbrha requested earlier?

Regards,
Sven

PlagueHO · 2017-02-14T08:16:17Z

Hi @svenvanrijen - just add the details of the change to the ## Unreleased section in the Readme.md.

I've managed to identify the problem when using the $credential parameter. To fix it can you please include the following change to this line:
https://github.com/PowerShell/xCertificate/blob/dev/DSCResources/MSFT_xCertReq/MSFT_xCertReq.psm1#L424

Change it from:

$null = Wait-Win32ProcessEnd `
    -Path $command `
    -Arguments $arguments `
    -Credential $Credential

To:

$null = Wait-Win32ProcessStop `
    -Path $command `
    -Arguments $arguments `
    -Credential $Credential

E.g. we're changing the function from Wait-Win32ProcessEnd to Wait-Win32ProcessStop.

One final change (just so the manual integration tests work):
https://github.com/PowerShell/xCertificate/blob/dev/Tests/Integration/MSFT_xCertReq.config.ps1#L22
Change from:

Exportable = $($Exportable.ToString().ToUpper())

Change to:

Exportable = $Exportable

Appreciate your help on this!

Changed all lines as discussed under PR 43 and issue 42

svenvanrijen · 2017-02-15T14:04:03Z

Just all changed lines as discussed under PR 43 and issue 42.

svenvanrijen · 2017-02-15T14:20:10Z

Seems there are still some problems with the CI tests. Looking into that later.

PlagueHO · 2017-02-16T07:27:46Z

@svenvanrijen - the test failures aren't caused by something you've done. We've changed the common DSCResource test frameworks to behave differently which requires some changes to this repo. I've included these changes in my PR #48

Once my PR is merged, your PR will pass and can be merged.

svenvanrijen · 2017-02-16T08:11:49Z

@PlagueHO - Good to know! Thank you for your reply!

PlagueHO · 2017-03-02T07:20:18Z

Hi @svenvanrijen - my PR has gone through now, so your one should begin passing tests. So if you can fix the merge conflicts by rebasing then I should be able to review and merge it. Thanks again for contributing this!

This reverts commit 3494dfa.

# Conflicts: # DSCResources/MSFT_xCertReq/MSFT_xCertReq.psm1 # ReadMe.md

svenvanrijen · 2017-03-02T07:56:00Z

Hi @PlagueHO,

just merged the repo again, but still one check fails. Not sure it's in the module or the test itself. Can you have another look at it or can I help out on this?

Regards,
Sven

PlagueHO · 2017-03-02T08:02:21Z

Hi @svenvanrijen - I think the problem is that my changes from my PR haven't made it into your branch (your PR is undoing a few of my changes). If you have a look at the "Files Changed" you can see a bunch of changes that you didn't actually make 😁 It does look like you've merged my changes though... but perhaps try again?

svenvanrijen · 2017-03-02T08:06:30Z

I'll try again, thnx. I'm quite a newbie on this. I'm sorry....

This reverts commit cbd895a.

This reverts commit 230fce4.

…42"" This reverts commit 97ca5c6.

svenvanrijen · 2017-03-02T09:07:29Z

@PlagueHO I'm affraid I'm not sure how to fix this :(
Any hints or tips?

My thoughts are close this PR, re-fork the original repo and re-apply my changes...

…to dev

ProcessStop instead of ProcessEnd line 560

svenvanrijen · 2017-03-06T12:25:03Z

@PlagueHO
A couple of steps further down the road I guess. Still an error while testing unfortunately:

   Context autorenew is false, credentials passed
      [+] should not throw 352ms
      [-] should call expected mocks 96ms
        You did not declare a mock of the Wait-Win32ProcessStop Command in module MSFT_xCertReq.
        at line: 636 in C:\Program Files\WindowsPowerShell\Modules\Pester\4.0.2\Functions\Mock.ps1

I'm affraid I cannot fix this, can I?

Regards,
Sven

PlagueHO · 2017-03-07T06:52:38Z

Hi @svenvanrijen - I think there is still a number of changes that are being reverted by your PR. If you have a look at the "Files Changed" Tab above you should be able to identify the changes you've made as opposed to the files that are reverted and then bring those changes back.

As for the test failure, I can give you advise on fixing that once the above changes are reverted.

This reverts commit 998e100.

svenvanrijen · 2017-03-12T21:05:22Z

Still problems resolving various merge issues. Closing this PR and opening a new one.

Changed $ca parameter

998e100

Changed $ca parameter from $ca = "'$CAServerFQDN\$CARootName' " to $ca = "$CAServerFQDN\$CARootName". Old parameter gave errors since '$CAServerFQDN (example: 'CA01.domain.com) could not be found when running the script.

msftclas added the cla-not-required label Jan 31, 2017

PlagueHO added the needs review The pull request needs a code review. label Feb 2, 2017

PlagueHO self-assigned this Feb 2, 2017

BerheAbrha suggested changes Feb 2, 2017

View reviewed changes

BerheAbrha added the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Feb 2, 2017

Revert "Changed $ca parameter"

a3dddd3

This reverts commit 998e100.

Revert "Revert "Changed $ca parameter""

92f6117

This reverts commit a3dddd3.

Changed all lines as discussed under PR 43 and issue 42

230fce4

Changed all lines as discussed under PR 43 and issue 42

Chnaged unit test

3494dfa

PlagueHO added in progress The issue is being actively worked on by someone. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Feb 24, 2017

svenvanrijen added 2 commits March 2, 2017 08:26

Revert "Chnaged unit test"

731872f

This reverts commit 3494dfa.

Merge remote-tracking branch 'refs/remotes/PowerShell/dev' into dev

09f59c5

# Conflicts: # DSCResources/MSFT_xCertReq/MSFT_xCertReq.psm1 # ReadMe.md

commit tests

cbd895a

svenvanrijen added 3 commits March 2, 2017 09:12

Revert "commit tests"

8f402ac

This reverts commit cbd895a.

Revert "Changed all lines as discussed under PR 43 and issue 42"

97ca5c6

This reverts commit 230fce4.

Revert "Revert "Changed all lines as discussed under PR 43 and issue …

79ed95f

…42"" This reverts commit 97ca5c6.

svenvanrijen added 4 commits March 6, 2017 13:02

Merge remote-tracking branch 'refs/remotes/PowerShell/dev' into dev

3cda7fc

Merge branch 'dev' of https://github.com/svenvanrijen/xCertificate in…

51527c4

…to dev

Merge tests

5867cde

changed unit test to match actual script

80e8447

ProcessStop instead of ProcessEnd line 560

changed test again

91e8ad6

Revert "Changed $ca parameter"

086f6f3

This reverts commit 998e100.

svenvanrijen closed this Mar 12, 2017

vors removed the needs review The pull request needs a code review. label Mar 12, 2017

PlagueHO removed the in progress The issue is being actively worked on by someone. label Jun 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed $ca parameter #43

Changed $ca parameter #43

svenvanrijen commented Jan 31, 2017 •

edited by joeyaiello

Loading

msftclas commented Jan 31, 2017

BerheAbrha Feb 2, 2017

svenvanrijen Feb 15, 2017

PlagueHO commented Feb 2, 2017

svenvanrijen commented Feb 6, 2017

PlagueHO commented Feb 8, 2017

svenvanrijen commented Feb 9, 2017 •

edited

Loading

PlagueHO commented Feb 10, 2017

svenvanrijen commented Feb 10, 2017

PlagueHO commented Feb 11, 2017

PlagueHO commented Feb 13, 2017

svenvanrijen commented Feb 13, 2017

PlagueHO commented Feb 14, 2017

svenvanrijen commented Feb 15, 2017

svenvanrijen commented Feb 15, 2017

PlagueHO commented Feb 16, 2017

svenvanrijen commented Feb 16, 2017

PlagueHO commented Mar 2, 2017

svenvanrijen commented Mar 2, 2017

PlagueHO commented Mar 2, 2017

svenvanrijen commented Mar 2, 2017

svenvanrijen commented Mar 2, 2017

svenvanrijen commented Mar 6, 2017

PlagueHO commented Mar 7, 2017

svenvanrijen commented Mar 12, 2017

Changed $ca parameter #43

Changed $ca parameter #43

Conversation

svenvanrijen commented Jan 31, 2017 • edited by joeyaiello Loading

msftclas commented Jan 31, 2017

BerheAbrha Feb 2, 2017

Choose a reason for hiding this comment

svenvanrijen Feb 15, 2017

Choose a reason for hiding this comment

PlagueHO commented Feb 2, 2017

svenvanrijen commented Feb 6, 2017

PlagueHO commented Feb 8, 2017

svenvanrijen commented Feb 9, 2017 • edited Loading

PlagueHO commented Feb 10, 2017

svenvanrijen commented Feb 10, 2017

PlagueHO commented Feb 11, 2017

PlagueHO commented Feb 13, 2017

svenvanrijen commented Feb 13, 2017

PlagueHO commented Feb 14, 2017

svenvanrijen commented Feb 15, 2017

svenvanrijen commented Feb 15, 2017

PlagueHO commented Feb 16, 2017

svenvanrijen commented Feb 16, 2017

PlagueHO commented Mar 2, 2017

svenvanrijen commented Mar 2, 2017

PlagueHO commented Mar 2, 2017

svenvanrijen commented Mar 2, 2017

svenvanrijen commented Mar 2, 2017

svenvanrijen commented Mar 6, 2017

PlagueHO commented Mar 7, 2017

svenvanrijen commented Mar 12, 2017

svenvanrijen commented Jan 31, 2017 •

edited by joeyaiello

Loading

svenvanrijen commented Feb 9, 2017 •

edited

Loading