Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modified script for new AZ module #1

Merged
merged 4 commits into from
May 7, 2019
Merged

Modified script for new AZ module #1

merged 4 commits into from
May 7, 2019

Conversation

Daya-Patil
Copy link
Contributor

Modified script for new AZ module

CopyKeys.ps1 Outdated

foreach ($Item in $VmList)
{
if ($Item.StorageProfile.OsDisk.EncryptionSettings.Enabled -eq "True")
#if ($Item.StorageProfile.OsDisk.EncryptionSettings.Enabled -eq "True")
if ((Get-AzVmDiskEncryptionStatus -ResourceGroupName $ResourceGroupName -VMName $item.Name).OsVolumeEncrypted -eq "Encrypted")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this line change. We don't want to make an extra call during the UI as it increases the load time.
Also, some VMs won't show up. These are the single pass encrypted VMs which are not supported by this script as of today.
So, only 2-pass encrypted VMs should be shown.
And even if you want to see both 1 pass and 2 pass encrypted VMs, I would suggest the following

if (($Item.Extensions -ne $null) -and ($Item.Extensions.Id | % {$_.split('/')[-1].tolower().contains('azurediskencryption')}) -contains $true)

CopyKeys.ps1 Outdated
@@ -23,7 +23,8 @@ param(
[switch]$ForceDebug)

### Checking for module versions and assemblies.
#Requires -Modules @{ ModuleName="AzureRM"; ModuleVersion="6.8.1" }
Import-Module AZ
#Requires -Modules @{ ModuleName="Az"; ModuleVersion="1.8.0" }
Copy link
Contributor

@punit-bhatt punit-bhatt Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am unable to run this script on a fresh powershell window due to the following error

The script 'Copy-Keys-Az.ps1' cannot be run because the following modules that are specified by
the "#requires" statements of the script are missing: Az.

This is most likely due to the #requires statement. By default, such statements run before the script execution and so will run before the Import-Module Az statement.
As per the Microsoft docs

Placing a #Requires statement inside a function does NOT limit its scope. All #Requires statements are always applied globally, and must be met, before the script can execute

This error goes away on explicitly importing the module.

CopyKeys.ps1 Outdated
@@ -890,7 +892,7 @@ function Create-Secret(
### <return name="CompletedList">List of VMs for which CopyKeys ran successfully</return>
function Start-CopyKeys
{
$SuppressOutput = Login-AzureRmAccount -ErrorAction Stop
$SuppressOutput = connect-AzAccount -ErrorAction Stop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion as we were planning on adding this change here.
Could you add another check here to prevent Login calls when unnecessary.

$Context = Get-AzContext

if($Context -eq $null)
{
    $SuppressOutput = Login-AzAccount -ErrorAction Stop
}

`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make this change as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please brief which change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the code, the user is asked to login again and again on subsequent script runs which is not required as the context is still retained.
So, instead of the following code line, which helps login,
$SuppressOutput = connect-AzAccount -ErrorAction Stop
we can check for the context and run the login command only when necessary. This can be done as shown below

$Context = Get-AzContext

if($Context -eq $null)
{
    $SuppressOutput = Login-AzAccount -ErrorAction Stop
}

So, please do the needful.

CopyKeys.ps1 Outdated
@@ -910,7 +912,7 @@ function Start-CopyKeys
$TargetBekVault = $UserInputs["TargetBekVault"]
$TargetKekVault = $UserInputs["TargetKekVault"]

$Context = Get-AzureRmContext
$Context = Get-AzContext
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again if the Login changes are made, you could add another check here to prevent a call.

if($Context -eq $null)
{
    $Context = Get-AzContext
}

@punit-bhatt
Copy link
Contributor

The Get-Authentication method is failing with the new AzureAD modules. This is because there is no AcquireToken() method available anymore (Line 543).
You need to replace it with the following method, keeping everything else the same.

### <summary>
### Gets the authentication result to key vaults.
### </summary>
function Get-Authentication
{
    # Vault resources endpoint
    $ArmResource = "https://vault.azure.net"
    # Well known client ID for AzurePowerShell used to authenticate scripts to Azure AD.
    $ClientId = "1950a258-227b-4e31-a9cf-717495945fc2"
    $RedirectUri = "urn:ietf:wg:oauth:2.0:oob"
    $AuthorityUri = "https://login.windows.net/$TenantId"
    $AuthContext = New-Object "Microsoft.IdentityModel.Clients.ActiveDirectory.AuthenticationContext" `
        -ArgumentList $AuthorityUri
    $PlatformParameters = New-Object "Microsoft.IdentityModel.Clients.ActiveDirectory.PlatformParameters" `
        -ArgumentList "Auto", $null
    $AuthResult = $AuthContext.AcquireTokenAsync($ArmResource, $ClientId, $RedirectUri, $PlatformParameters)

    return $AuthResult.Result
}

This uses the method AcquireTokenAsync() which requires an additional parameter - PlatformParameters object.
This has been tested with AzureAD modules - 2.0.1.16 and 2.0.2.16

Copy link
Contributor

@punit-bhatt punit-bhatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested the script for various scenarios and it needed slight modifications to work successfully.
Please update the Get-Authentication method as shown here.
Also, we would like to retain the original script. Since I have gone through the changes, could you add the above change and add this script as a new file, preferably CopyKeys-Az.ps1?

@punit-bhatt punit-bhatt merged commit c01858f into AsrOneSdk:master May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants