'OctopusMasterKey' is not in $currentState, so it triggers a service restart

[jona7777]

After checking the cOctopusServer.psm1 we can find that the current state for the OctopusMasterKey property is null. On the other hand, the default value for OctopusMasterKey is [PSCredential]::Empty. Even if we force an OctopusMasterKey property in our recipe, it will always be a PSCredential which is never null. Therefore, the Test-ReconfigurationRequiresServiceRestart function will always trigger a service restart because current state OctopusMasterKey never matches desired state OctopusMasterKey.

We have confirmed that removing the OctopusMasterKey from the attribute $reconfigurableProperties on the Test-ReconfigurationRequiresServiceRestart function, the service does not restart.

We are happy to submit a pull request but we seek your direction first. Should it be OctopusMasterKey added to the $currentState? Or should it be removed from the properties that trigger a service restart?

[matt-richardson]

Hi @jona7777, thanks for reaching out.

My memory is a tad hazy, but I believe the intent is that the OctopusMasterKey property should always be supplied on an existing instance. Otherwise, it cant setup correctly in some circumstances.

Is that an option in your circumstance?

[jona7777]

Hi @matt-richardson,

Your reply is much appreciated. Unfortunately, that is not an option that could fix this issue. We agree with you that the OctopusMasterKey property should always be supplied on an existing instance. The problem is that, even when that property is included in our desired state, the OctopusMasterKey property in the $currentState PowerShell attribute is not returned. Therefore, the following piece of code forces an Octopus service restart.

function Test-ReconfigurationRequiresServiceRestart($currentState, $desiredState) {
    $reconfigurableProperties = @(
        'ListenPort',
        'WebListenPrefix',
        'ForceSSL',
        'HomeDirectory',
        'SqlDbConnectionString',
        'OctopusBuiltInWorkerCredential',
        'OctopusMasterKey'    ######## HERE IS THE PROPERTY TO EVALUATE
        )
    foreach ($property in $reconfigurableProperties) {
        if ($currentState.Item($property) -is [PSCredential]) {
            if (Test-PSCredentialChanged $currentState.Item($property) $desiredState.Item($property)) {
                write-verbose "Triggering service restart as property '$property' has changed"
                return $true
            }
        }
        elseif ($currentState.Item($property) -ne ($desiredState.Item($property))) {
            ######## HERE IS WHERE THE SERVICE RESTART IS TRIGGERED
            write-verbose "Triggering service restart as property '$property' has changed"
            return $true
        }
    }
    return $false
}

Please note that the $currentState variable is formed by calling the Get-TargetResource procedure which returns the following object missing the OctopusMasterKey property.

        $currentResource = @{
            Name                                      = $Name;
            Ensure                                    = $existingEnsure;
            State                                     = $existingState;
            DownloadUrl                               = $existingDownloadUrl;
            WebListenPrefix                           = $existingWebListenPrefix;
            SqlDbConnectionString                     = $existingSqlDbConnectionString;
            ForceSSL                                  = $existingForceSSL;
            HSTSEnabled                               = $existingHSTSEnabled;
            HSTSMaxAge                                = $existingHSTSMaxAge;
            AllowUpgradeCheck                         = $existingOctopusUpgradesAllowChecking;
            AllowCollectionOfUsageStatistics          = $existingOctopusUpgradesIncludeStatistics;
            ListenPort                                = $existingListenPort;
            OctopusAdminCredential                    = $existingOctopusAdminCredential;
            LegacyWebAuthenticationMode               = $existingLegacyWebAuthenticationMode;
            AutoLoginEnabled                          = $existingAutoLoginEnabled;
            OctopusServiceCredential                  = $existingOctopusServiceCredential;
            HomeDirectory                             = $existingHomeDirectory;
            LicenseKey                                = $existingLicenseKey;
            GrantDatabasePermissions                  = $GrantDatabasePermissions;
            OctopusBuiltInWorkerCredential            = $existingOctopusBuiltInWorkerCredential;
            PackagesDirectory                         = $existingPackagesDirectory;
            ArtifactsDirectory                        = $existingArtifactsDirectory;
            TaskLogsDirectory                         = $existingTaskLogsDirectory;
            LogTaskMetrics                            = $existingLogTaskMetrics;
            LogRequestMetrics                         = $existingLogRequestMetrics;
            TaskCap                                   = $existingTaskCap
        }

Therefore, there is a need to take one of two actions: 1) Add the OctopusMasterKey property to the $currentState object. 2) Remove the OctopusMasterKey property from the service restart evaluation.

Looking forward to your advice. Thanks once again for your support.

[matt-richardson]

Hey @jona7777, thanks for your detailed reply.

My main hesitation here was whether returning the master key from Get-TargetResource could be considered a leak, but I think I'm okay with it on the basis that: a) we already return OctopusBuiltInWorkerCredential b) it appears that the docs say

In the Get-TargetResource function implementation, use the key resource property values that are provided as parameters to check the status of the specified resource instance. This function must return a hash table that lists all the resource properties as keys and the actual values of these properties as the corresponding values.

On this basis, it appears we should move forward with the

1, Add the OctopusMasterKey property to the $currentState object.

approach.

It would be ace if you could add a test in cOctopusServer.Tests.ps1, but feel free to timebox it and bail if it gets too complex (it doesn't appear there's any real tests around Set-TargetResource behaviour)

Thanks for contributing!

[mcasperson]

Hi @jona7777, are you still happy to provide a PR for this issue?

[jona7777]

Hi @matt-richardson & @mcasperson, Sorry for my lack of response. I was checking with my team oversight to determine when we could work on that pull request. Now I see that you have already moved forward with the pull request. Just had a brief look into it and from my limited experience on the code behind it looks like these changes should fix our service restart issue. Looking forward to the final merge and version release. Thank you very much for your excellent support!

[matt-richardson]

This has been resolved in v4.0.929. Let us know how it goes!

[jona7777]

Hi @matt-richardson!

We really appreciate your assistance with this issue. Unfortunately, there is still a need to make a tiny adjustment. The function Get-ServerConfiguration on the OctopusDSCHelpers.ps1 file adds the 'OctopusMasterKey' property as a plain password text into the $config object. Nevertheless, this property is expected to be a PSCredential.

Therefore, line 270 ...

$config | Add-Member -NotePropertyName "OctopusMasterKey" -NotePropertyValue $masterkey

... should be replaced by something like ...

$masterkeycred = New-Object System.Management.Automation.PSCredential ("notused", ($masterkey | ConvertTo-SecureString -AsPlainText -Force))
$config | Add-Member -NotePropertyName "OctopusMasterKey" -NotePropertyValue $masterkeycred

We have successfully tested that change in our kitchen environment. Please let us know if we should open a new issue for that fix.

Thanks so much for your continuous support!

[matt-richardson]

Hi @jona7777 - sorry about that. Kind of frustrating for you, so please accept my apologies.

I've just pushed up a PR - https://github.com/OctopusDeploy/OctopusDSC/pull/261. Can you test that out and confirm it works for you?

[jona7777]

Hi @matt-richardson,

No worries at all. We are aware that programming often involves some back and forth. Moreover, it is much appreciated that you are giving the needed priority to this issue.

Your PR changes look very good. Actually, the way password is encrypted is more secure. Yet, please note that after testing it has been noticed that the function Test-PSCredentialChanged on the cOctopusServer.psm1 requires both UserName and Password to match. So the current code only works when we configure our DSC config object to use 'ignored' as the UserName of the 'OctopusMasterKey' property. Unfortunately, this is not the best way to test the 'OctopusMasterKey' property because the UserName may vary from one configuration to the other.

There are at least two options: 1) Include in the documentation a specific UserName (i.e. 'ignored') that should always be supplied to the PSCredential object on the 'OctopusMasterKey' property. 2) Adjust the logic of the function Test-PSCredentialChanged to handle cases where the UserName check should be skipped.

Most probably option 2) looks better.

Thanks once again! Looking forward to your comments/changes.

[matt-richardson]

I think the testing model is starting to creak here - I keep adding tests, but keep missing the mark on actually fixing the bug. Glad to hear you've got a good test setup going at your end.

If I had all the time in the world, I think I'd re-write a bunch of the tests and probably also the module itself... I can dream.

I've pushed a fix up that ignores the username on the master key - can you try that?

[jona7777]

Good job, @matt-richardson! After testing these changes, we can say that OctopusDSC no longer triggers a service restart when the desired 'OctopusMasterKey' property matches the running 'OctopusMasterKey' value.

Looking forward to the final merge and release.

Thank you very much for this fix!

[matt-richardson]

This has been released in v4.0.932. Sorry for the delay!