Exchange script adjustments #702

LBegnaud · 2018-01-15T15:54:18Z

Supports the ability to import cert from filesystem, add option to remove old exchange certificates, and cleans up logic

Might be best to commit and squash.

johlju

Great work! Just giving my thoughts.

johlju · 2018-01-15T17:13:06Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

    [Parameter(Position=1,Mandatory=$true)]
-    [string]$ExchangeServices
+    [string]$ExchangeServices,          # comma-separated list of exchange services to import for


Maybe we should make this a ValidateSet() with the possible values 'IMAP', 'POP', 'UM', 'IIS', 'SMTP', 'Federation', 'UMCallRouter'

I think that might be a bit out of scope as it would require updating with specific knowledge on Exchange. Keep in mind this script should never be run interactively.

johlju · 2018-01-15T17:17:24Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

-    [string]$ExchangeServices
+    [string]$ExchangeServices,          # comma-separated list of exchange services to import for
+    [Parameter(Position=2,Mandatory=$false)]
+    [bool]$LeaveOldExchangeCerts = 1,   # 0 for false, 1 for true. If set to 1, will skip removal of old exchange certs


Could we use $true and $false instead of 0 and 1?

$true and $false are a pain to pass through from cmd. passing 0 and 1 are dead simple.

Since it's a bool, powershell will interpret all those the same if you really want to.

johlju · 2018-01-15T17:21:18Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

 ## Documentation referenced from https://technet.microsoft.com/en-us/library/aa997231(v=exchg.160).aspx

 # Should work with exchange 2007 and higher
-Get-PSSnapin -registered | ? {$_.Name -match "Microsoft.Exchange.Management.PowerShell" -and ($_.Name -match "Admin" -or $_.Name -match "E2010" -or $_.Name -match "SnapIn")} | Add-PSSnapin -ErrorAction SilentlyContinue
+Get-PSSnapin -registered | Where-Object {$_.Name -match "Microsoft.Exchange.Management.PowerShell" -and ($_.Name -match "Admin" -or $_.Name -match "E2010" -or $_.Name -match "SnapIn")} | Add-PSSnapin -ErrorAction SilentlyContinue

 #$OldThumbprint = (Get-Item -Path RDS:\GatewayServer\SSLCertificate\Thumbprint).CurrentValue


We can remove this comment.

johlju · 2018-01-15T17:21:20Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

 ## Documentation referenced from https://technet.microsoft.com/en-us/library/aa997231(v=exchg.160).aspx

 # Should work with exchange 2007 and higher
-Get-PSSnapin -registered | ? {$_.Name -match "Microsoft.Exchange.Management.PowerShell" -and ($_.Name -match "Admin" -or $_.Name -match "E2010" -or $_.Name -match "SnapIn")} | Add-PSSnapin -ErrorAction SilentlyContinue
+Get-PSSnapin -registered | Where-Object {$_.Name -match "Microsoft.Exchange.Management.PowerShell" -and ($_.Name -match "Admin" -or $_.Name -match "E2010" -or $_.Name -match "SnapIn")} | Add-PSSnapin -ErrorAction SilentlyContinue

 #$OldThumbprint = (Get-Item -Path RDS:\GatewayServer\SSLCertificate\Thumbprint).CurrentValue
 $CertInStore = Get-ChildItem -Path Cert:\LocalMachine -Recurse | Where-Object {$_.thumbprint -eq $NewCertThumbprint} | Sort-Object -Descending | Select-Object -f 1


Does Sort-Object -Descending by default sort on the correct date field, so that the newest are on top?

I'm honestly not sure why i kept using sort, since I'm not specifying the field to sort on and, more importantly, because we are filtering by thumbprint so we will never get certs from different days.

johlju · 2018-01-15T17:21:25Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

@@ -1,56 +1,95 @@
 param(


Could we add comment-based help with a .SYNOPSIS, and an .EXAMPLE in the comment-based help how to call this using Let's encrypt? If there are more than one option to call we should add more than one .EXAMPLE.

I'm a little torn on whether to put it there or just in the wiki. I've never done that so I can see how difficult it is. probably cleaner than the way i have remarks currently

Better? That's pretty cool.

Looks good to me.

johlju · 2018-01-15T17:25:08Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1


-            $cert = $SourceStore.Certificates | Where-Object {$_.thumbprint -eq $CertInStore.Thumbprint}
+        # Now try to find it again...
+        $CertInStore = Get-ChildItem -Path Cert:\LocalMachine -Recurse | Where-Object {$_.thumbprint -eq $NewCertThumbprint} | Sort-Object -Descending | Select-Object -f 1


We should not need to do this, Import-ExchangeCertificate returns the certificate if you add $CertInStore = Import-ExchangeCertificate @importExchangeCertificateParameters -ErrorAction Stop or throws an error (stops) if it can't find the certificate.

If you keep this row, the same comment about -Descending as above.

Seems cleaner to do this so we are sure to get the right type of object. I don't have an exchange install to know if the passthrough from import-exchangecertificate returns the same type of object as get-childitem. I like to play it safe in that regard.

johlju · 2018-01-15T17:30:44Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

+    # Make sure variable is defined
+    $null = Get-ChildItem $CertInStore.PSPath -ErrorAction Stop
+    # Cert must exist in the personal store of machine to bind to exchange services
+    if($CertInStore.PSPath -notlike "*LocalMachine\My\*"){


Question: Are we using this code path as a fail safe if the Import-ExchangeCertificate did not add the certificate to the LocalMachine\My store? Is there a scenario when Import-ExchangeCertificate does not import the certificate to the LocalMachine\My store?

Yes, keep in mind the script's original intent was not to be used with centralssl (the fqdn and storepath aren't required parameters)

johlju · 2018-01-15T17:32:47Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

+
+    if(-not $LeaveOldExchangeCerts){
+        "Old Exchange certificates being cleaned up"
+        Get-ExchangeCertificate -DomainName $CertInStore.Subject.split("=")[1] | Where-Object -FilterScript {


$CertInStore.Subject.split("=")[1]

Could we use $TargetHost instead of this?

TargetHost isn't guaranteed to be passed. According to everything I'm reading in these issue threads, the primary CN of the cert should be the same as TargetHost

Okay. Didn't thought about that. Looks good to me.

johlju · 2018-01-15T17:35:13Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

+
+}catch{
+    "Cert thumbprint was not set successfully"
+    "Error: $($Error[0])"


"Error: $($Error[0])"

Could we use throw $_ instead, to throw a real error? Or after this row maybe?

That would be a question for @WouterTinus . I don't see the difference really. Either way it'll be logged.

Disclaimer: I'm not a proper developer, so I'm not really well-versed error handling. Outputting them always works fine enough for my needs.

Okay. Your way works as well if outputting is the only requirement and not stopping execution telling the user something failed with red error text. :)
Looks good to me.

LBegnaud · 2018-01-15T21:11:49Z

Updated per recommendations from @johlju

johlju

Awesome job adding comment based help! Added a few more suggestive comments around the code.

johlju · 2018-01-16T06:52:32Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

@@ -1,56 +1,95 @@
 param(


Looks good to me.

johlju · 2018-01-16T07:19:19Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

+$CertInStore = Get-ChildItem -Path Cert:\LocalMachine -Recurse | Where-Object {$_.thumbprint -eq $NewCertThumbprint} | Select-Object -f 1
+try{
+    if($CertInStore.PSPath -notlike "*LocalMachine\My\*"){
+        "Cert thumbprint not found in the cert store... which means we should load it. This means TargetHost and StorePath must be specified"


Since it is not certain that $TargetHost and $StorePath is passed, then if they are not, we should not go down this code path since Import-ExchangeCertificate will fail.
So we should add something like

if ($TargetHost -and $StorePath) { ... $null = Import-ExchangeCertificate @importExchangeCertificateParameters -ErrorAction Stop ... }

it'll fail into the catch block. I see no functional difference since both would stop the script from continuing.

johlju · 2018-01-16T07:35:15Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

-Get-PSSnapin -registered | ? {$_.Name -match "Microsoft.Exchange.Management.PowerShell" -and ($_.Name -match "Admin" -or $_.Name -match "E2010" -or $_.Name -match "SnapIn")} | Add-PSSnapin -ErrorAction SilentlyContinue
+Get-PSSnapin -registered | Where-Object {$_.Name -match "Microsoft.Exchange.Management.PowerShell" -and ($_.Name -match "Admin" -or $_.Name -match "E2010" -or $_.Name -match "SnapIn")} | Add-PSSnapin -ErrorAction SilentlyContinue
+
+$CertInStore = Get-ChildItem -Path Cert:\LocalMachine -Recurse | Where-Object {$_.thumbprint -eq $NewCertThumbprint} | Select-Object -f 1


We expect the certificate to me in LocalMachine\My. Why don't we just look for it there?

$CertInStore = Get-ChildItem -Path Cert:\LocalMachine\My | Where-Object {$_.thumbprint -eq $NewCertThumbprint} try { if (-not $CertInStore) { ... $null = Import-ExchangeCertificate @importExchangeCertificateParameters -ErrorAction Stop ... } }

johlju · 2018-01-16T07:37:11Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1


-            $cert = $SourceStore.Certificates | Where-Object {$_.thumbprint -eq $CertInStore.Thumbprint}
+    # Make sure variable is defined
+    $null = Get-ChildItem $CertInStore.PSPath -ErrorAction Stop


If we do the change above, and below, then this row is not needed.

johlju · 2018-01-16T07:39:02Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1


+    # Cert must exist in the personal store of machine to bind to exchange services
+    if($CertInStore.PSPath -notlike "*LocalMachine\My\*"){


We could use the same check as above here.

try { if (-not $CertInStore) { ... $SourceStoreScope = 'LocalMachine' $SourceStoreName = 'My' ... } }

johlju · 2018-01-16T07:40:42Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

+        PrivateKeyExportable = $true
+        }
+
+        $null = Import-ExchangeCertificate @importExchangeCertificateParameters -ErrorAction Stop


We could just do $CertInStore = Import-ExchangeCertificate ... here and skip the row below.

according to the docs, there are no outputs of this cmdlet

Also the method of getting the cert after the import is a bit of a check to make sure $NewCertThumbprint is set properly.

johlju · 2018-01-16T07:42:33Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

+
+    if(-not $LeaveOldExchangeCerts){
+        "Old Exchange certificates being cleaned up"
+        Get-ExchangeCertificate -DomainName $CertInStore.Subject.split("=")[1] | Where-Object -FilterScript {


Okay. Didn't thought about that. Looks good to me.

johlju · 2018-01-16T07:44:30Z

letsencrypt-win-simple/Scripts/ImportExchange.ps1

+
+}catch{
+    "Cert thumbprint was not set successfully"
+    "Error: $($Error[0])"


Okay. Your way works as well if outputting is the only requirement and not stopping execution telling the user something failed with red error text. :)
Looks good to me.

WouterTinus · 2018-02-22T15:36:34Z

@LBegnaud - How are you feeling about this? My gut says that the script is much better already than the once that's currently being distributed in our releases. Would fixing the parameter type issue at a later date cause problems for people upgrading? I guess what I mean to say is don't let perfect get in the way of good :).

nemchik · 2018-02-22T15:59:09Z

I have reviewed the existing script and the proposed script and I'm confident that what they are both trying to do is going to work fine, but it does seem like the new changes to the proposed script might need a little touching up before merging.

LBegnaud · 2018-02-22T16:03:52Z

I honestly had forgotten about this. I think that it's fine in its current state. I'm going to make a quick fix to the parameter to be an int that gets passed a 0 or 1 as I don't have time to mess with it too much right now. I'll push a commit then you're good to merge.

LBegnaud · 2018-02-22T16:14:21Z

K I think it's ready. Thanks for the reminder

WouterTinus · 2018-02-22T17:48:34Z

No problem, thanks for contributing!

LBegnaud added 4 commits January 15, 2018 09:31

More adjustments to exchange import script

88f6e54

exchange script comment tweaks

e192072

error handling logic cleanup

3595395

More logic cleanup

dadeca2

LBegnaud mentioned this pull request Jan 15, 2018

Exchange wiki page (Steps for install) #700

Closed

johlju reviewed Jan 15, 2018

View reviewed changes

Documentation updates

456b35d

johlju reviewed Jan 16, 2018

View reviewed changes

Tweaks to Exchange importer

dd9f118

WouterTinus merged commit 44ec90e into win-acme:master Feb 22, 2018


		# Cert must exist in the personal store of machine to bind to exchange services
		if($CertInStore.PSPath -notlike "LocalMachine\My\"){

Exchange script adjustments #702

Exchange script adjustments #702

Conversation

LBegnaud commented Jan 15, 2018

johlju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LBegnaud commented Jan 15, 2018

johlju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WouterTinus commented Feb 22, 2018

nemchik commented Feb 22, 2018

LBegnaud commented Feb 22, 2018

LBegnaud commented Feb 22, 2018

WouterTinus commented Feb 22, 2018