fix(scripts): correct and prettify copy kv secrets script

[helenakallekleiv]

Original issue was that when running the script we got the following warning:

WARNING: Set-AzSubscription is not found. The most similar Azure PowerShell commands are:
        Get-AzSubscription
        Select-AzSubscription

This refers to the custom fuction Set-AzSubscription on line 97. I believe the issue was that the custom function name was too similar to an actual built-in Azure PowerShell cmdlet, which does not exist. To fix this I just changed the custom function name from Set-AzSubscription to Set-SubscriptionContext, and updated the references.

Simultaneously I did some formatting, added some comments and updated the parameter names.

Testing

• Copying secrets from one key vault to another in the same subscription works as it should.
• Have not yet tested copying secrets across different subscriptions, but will test this asap.
• Have not tested the script with use of runbook.
• Disabling/enabling firewall works as expected. Output below:

  WARNING: The network rule set has been turned off for this vault.
  WARNING: The network rule set has been turned off for this vault.
  Secret Name 'secret-1' with id 'https://kv-test-hekal.vault.azure.net:443/secrets/secret-1/84a4a19f9d4c47ebaa641ec104ed9560' already exists in destination vault
  Secret Name 'secret-2' with id 'https://kv-test-hekal.vault.azure.net:443/secrets/secret-2/612fa637e8d44b59adce25d798924442' already exists in destination vault
  Secret Name 'secret-3' with id 'https://kv-test-hekal.vault.azure.net:443/secrets/secret-3/7c229dcbd8ff4567b90eb5052d29ffd5' already exists in destination vault
  Reverting firewall settings for Source Vault to 'Deny'.
  Reverting firewall settings for Target Vault to 'Deny'.
  

[helenakallekleiv]

The linter throws the following error messages:

 RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSAvoidUsingConvertToSecureStringWi Error        Copy-AzKey 190   File 'Copy-Az
thPlainText                                      VaultSecre       KeyVaultSecre
                                                 t.ps1            t.ps1' uses C
                                                                  onvertTo-Secu
                                                                  reString with
                                                                   plaintext. T
                                                                  his will expo
                                                                  se secure inf
                                                                  ormation. Enc
                                                                  rypted standa
                                                                  rd strings sh
                                                                  ould be used
                                                                  instead.
PSUseShouldProcessForStateChangingF Warning      Copy-AzKey 96    Function 'Set
unctions                                         VaultSecre       -Subscription
                                                 t.ps1            Context' has
                                                                  verb that cou
                                                                  ld change sys
                                                                  tem state. Th
                                                                  erefore, the
                                                                  function has
                                                                  to support 'S
                                                                  houldProcess'
                                                                  .

Messages formatted for readability:

# RuleName: PSAvoidUsingConvertToSecureStringWithPlainText

File 'Copy-AzKeyVaultSecret.ps1' uses C onvertTo-SecureString with plaintext. 
This will expose secure information. Encrypted standard strings should be used instead.

#RuleName: PSUseShouldProcessForStateChangingFunctions

Function 'Set-SubscriptionContext' hasverb that could change system state.
Therefore, thefunction hasto support 'ShouldProcess'.

[hknutsen]

I think structurally this looks good 👍

Even though the linter throws an error on something that was implemented before your changes, I'd look into if it's something we could implement to stop it from complaining, or worst case ignore it (and add a comment clarifying why we ignore it).

[helenakallekleiv]

The linter simply outputs a warning related to the verb used in this custom function:

#RuleName: PSUseShouldProcessForStateChangingFunctions

Function 'Set-SubscriptionContext' has verb that could change system state.
Therefore, the function has to support 'ShouldProcess'.

To mitigate I will try to use a verb that supports ShouldProcess, AKA "Trial and Error".

[helenakallekleiv]

Used Marshal Class to mitigate the following linter error:

# RuleName: PSAvoidUsingConvertToSecureStringWithPlainText

File 'Copy-AzKeyVaultSecret.ps1' uses ConvertTo-SecureString with plaintext. 
This will expose secure information. Encrypted standard strings should be used instead.

Not entirely familiar with the extended use of this but the linter seems happy.

Code block with the changes in question:

ops-automation/scripts/powershell/Copy-AzKeyVaultSecret.ps1

Lines 138 to 145 in dac702e

	# Convert SecureString to PlainText (Temporarily for comparison purposes)
	function Convert-SecureStringToPlainText {
	param (
	[Parameter(Mandatory = $true)]
	[SecureString]$SecureString
	)
	return [Runtime.InteropServices.Marshal]::PtrToStringAuto([Runtime.InteropServices.Marshal]::SecureStringToBSTR($SecureString))
	}

References

• PtrToStringAuto
• SecureStringToBSTR

[helenakallekleiv]

The linter simply outputs a warning related to the verb used in this custom function:

#RuleName: PSUseShouldProcessForStateChangingFunctions

Function 'Set-SubscriptionContext' has verb that could change system state.
Therefore, the function has to support 'ShouldProcess'.

To mitigate I will try to use a verb that supports ShouldProcess, AKA "Trial and Error".

Changing custom function name from Set-SubscriptionContext to Select-SubscriptionContext mitigated the linter error.

[helenakallekleiv]

Used Marshal Class to mitigate the following linter error:

# RuleName: PSAvoidUsingConvertToSecureStringWithPlainText

File 'Copy-AzKeyVaultSecret.ps1' uses ConvertTo-SecureString with plaintext. 
This will expose secure information. Encrypted standard strings should be used instead.

Not entirely familiar with the extended use of this but the linter seems happy.

Code block with the changes in question:

ops-automation/scripts/powershell/Copy-AzKeyVaultSecret.ps1

Lines 138 to 145 in dac702e

	# Convert SecureString to PlainText (Temporarily for comparison purposes)
	function Convert-SecureStringToPlainText {
	param (
	[Parameter(Mandatory = $true)]
	[SecureString]$SecureString
	)
	return [Runtime.InteropServices.Marshal]::PtrToStringAuto([Runtime.InteropServices.Marshal]::SecureStringToBSTR($SecureString))
	}

References

* [PtrToStringAuto](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.marshal.ptrtostringauto?view=net-8.0)

* [SecureStringToBSTR](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.marshal.securestringtobstr?view=net-8.0)

Reverted these changes.

After testing a million different scenarios, we concluded that for now the workaround for handling key vault secrets as Encrypted Standard Strings (ESS) instead of secure strings converted to plaintext did not work. This is because even though the secret name and value are identical in both source and target key vaults, the ESS value changes after every time the command is being run, and the ESS value for target and source KV secret is not identical.

In conclusion, the linter can keep complaining, as we have not yet found a workaround.

[helenakallekleiv]

Everything, except runbook, have been tested so this PR is ready for review.

[vildeaar]

nice :D