From 16af792c78932e8d0d2871420b686104028167e6 Mon Sep 17 00:00:00 2001 From: msJinLei Date: Fri, 19 Nov 2021 16:22:43 +0800 Subject: [PATCH 1/3] Remove secrets of PSAzurerRmAccount from display https://github.com/Azure/azure-powershell/issues/15427 --- .../Accounts.Test/ContextCmdletTests.cs | 37 +++ ...SubscriptionsAfterContextRenameAndSet.json | 248 ++++++++++++++++++ .../Accounts.Test/SubscriptionCmdletTests.cs | 7 + .../Accounts.Test/SubscriptionCmdletTests.ps1 | 21 ++ src/Accounts/Accounts/Accounts.format.ps1xml | 2 +- src/Accounts/Accounts/ChangeLog.md | 1 + .../AzureRmProfile.cs | 21 ++ .../Models/PSAzureRmAccount.cs | 17 ++ 8 files changed, 353 insertions(+), 1 deletion(-) create mode 100644 src/Accounts/Accounts.Test/SessionRecords/Microsoft.Azure.Commands.Profile.Test.SubscriptionCmdletTests/GetSubscriptionsAfterContextRenameAndSet.json diff --git a/src/Accounts/Accounts.Test/ContextCmdletTests.cs b/src/Accounts/Accounts.Test/ContextCmdletTests.cs index edbda72376e1..ae1ba4f8eb5c 100644 --- a/src/Accounts/Accounts.Test/ContextCmdletTests.cs +++ b/src/Accounts/Accounts.Test/ContextCmdletTests.cs @@ -792,6 +792,43 @@ public void ImportContextNoDefaultKey() } } + [Fact] + [Trait(Category.AcceptanceType, Category.CheckIn)] + public void CheckHidenServicePrincipalSecret() + { + var cmdlet = new GetAzureRMContextCommand(); + + // Setup + cmdlet.CommandRuntime = commandRuntimeMock; + var profile = new AzureRmProfile(); + string subscriptionName = "Contoso Subscription 1"; + string accountId = "7a5db92d-499a-46be-8d6e-6666eeee0000"; + string contextName; + var contextTemp = (new AzureContext { Environment = AzureEnvironment.PublicEnvironments[EnvironmentName.AzureCloud] }) + .WithAccount(new AzureAccount { Id = accountId, Type = "ServicePrincipal" }) + .WithTenant(new AzureTenant { Id = Guid.NewGuid().ToString(), Directory = "contoso.com" }) + .WithSubscription(new AzureSubscription { Id = Guid.NewGuid().ToString(), Name = subscriptionName }); + contextTemp.Account.SetProperty(AzureAccount.Property.ServicePrincipalSecret, "5P6******************"); + contextTemp.Account.SetProperty(AzureAccount.Property.Subscriptions, contextTemp.Subscription.Id); + contextTemp.Account.SetProperty(AzureAccount.Property.Tenants, contextTemp.Tenant.Id); + profile.TryAddContext(contextTemp, out contextName); + cmdlet.DefaultProfile = profile; + + // Act + cmdlet.InvokeBeginProcessing(); + cmdlet.ExecuteCmdlet(); + cmdlet.InvokeEndProcessing(); + + // Verify + Assert.True(commandRuntimeMock.OutputPipeline.Count == 1); + var context = (PSAzureContext)commandRuntimeMock.OutputPipeline[0]; + Assert.Equal(subscriptionName, context.Subscription.Name); + Assert.Equal(accountId, context.Account.Id); + var accountExtendedProperties = context.Account.ExtendedProperties; + Assert.Equal(2, accountExtendedProperties.Count()); + Assert.False(accountExtendedProperties.ContainsKey(AzureAccount.Property.ServicePrincipalSecret)); + } + AzureRmProfile CreateMultipleContextProfile() { var profile = new AzureRmProfile(); diff --git a/src/Accounts/Accounts.Test/SessionRecords/Microsoft.Azure.Commands.Profile.Test.SubscriptionCmdletTests/GetSubscriptionsAfterContextRenameAndSet.json b/src/Accounts/Accounts.Test/SessionRecords/Microsoft.Azure.Commands.Profile.Test.SubscriptionCmdletTests/GetSubscriptionsAfterContextRenameAndSet.json new file mode 100644 index 000000000000..4686aa091f2f --- /dev/null +++ b/src/Accounts/Accounts.Test/SessionRecords/Microsoft.Azure.Commands.Profile.Test.SubscriptionCmdletTests/GetSubscriptionsAfterContextRenameAndSet.json @@ -0,0 +1,248 @@ +{ + "Entries": [ + { + "RequestUri": "/tenants?api-version=2021-01-01", + "EncodedRequestUri": "L3RlbmFudHM/YXBpLXZlcnNpb249MjAyMS0wMS0wMQ==", + "RequestMethod": "GET", + "RequestBody": "", + "RequestHeaders": { + "x-ms-client-request-id": [ + "8d43ac0a-3efd-447f-bc3e-c55c24b26d05" + ], + "Accept-Language": [ + "en-US" + ], + "User-Agent": [ + "FxVersion/4.6.28207.03", + "OSName/Windows", + "OSVersion/Microsoft.Windows.10.0.22000.", + "Microsoft.Azure.Management.ResourceManager.Version2021.01.01.SubscriptionClient/1.3.53" + ] + }, + "ResponseHeaders": { + "Cache-Control": [ + "no-cache" + ], + "Pragma": [ + "no-cache" + ], + "x-ms-ratelimit-remaining-tenant-reads": [ + "11998" + ], + "x-ms-request-id": [ + "ba026bf5-402f-4498-9473-04638da0cd01" + ], + "x-ms-correlation-request-id": [ + "ba026bf5-402f-4498-9473-04638da0cd01" + ], + "x-ms-routing-request-id": [ + "SOUTHEASTASIA:20211124T091556Z:ba026bf5-402f-4498-9473-04638da0cd01" + ], + "Strict-Transport-Security": [ + "max-age=31536000; includeSubDomains" + ], + "X-Content-Type-Options": [ + "nosniff" + ], + "Date": [ + "Wed, 24 Nov 2021 09:15:55 GMT" + ], + "Content-Type": [ + "application/json; charset=utf-8" + ], + "Expires": [ + "-1" + ], + "Content-Length": [ + "140" + ] + }, + "ResponseBody": "{\r\n \"value\": [\r\n {\r\n \"id\": \"/tenants/54826b22-38d6-4fb2-bad9-b7b93a3e9c5a\",\r\n \"tenantId\": \"54826b22-38d6-4fb2-bad9-b7b93a3e9c5a\",\r\n \"tenantCategory\": \"Home\"\r\n }\r\n ]\r\n}", + "StatusCode": 200 + }, + { + "RequestUri": "/tenants?api-version=2021-01-01", + "EncodedRequestUri": "L3RlbmFudHM/YXBpLXZlcnNpb249MjAyMS0wMS0wMQ==", + "RequestMethod": "GET", + "RequestBody": "", + "RequestHeaders": { + "x-ms-client-request-id": [ + "10c2752e-0e68-4ad2-ba4c-cd295d89d3d7" + ], + "Accept-Language": [ + "en-US" + ], + "User-Agent": [ + "FxVersion/4.6.28207.03", + "OSName/Windows", + "OSVersion/Microsoft.Windows.10.0.22000.", + "Microsoft.Azure.Management.ResourceManager.Version2021.01.01.SubscriptionClient/1.3.53" + ] + }, + "ResponseHeaders": { + "Cache-Control": [ + "no-cache" + ], + "Pragma": [ + "no-cache" + ], + "x-ms-ratelimit-remaining-tenant-reads": [ + "11998" + ], + "x-ms-request-id": [ + "06c9f7c2-d452-4d99-841a-2ad4a7cf4948" + ], + "x-ms-correlation-request-id": [ + "06c9f7c2-d452-4d99-841a-2ad4a7cf4948" + ], + "x-ms-routing-request-id": [ + "SOUTHEASTASIA:20211124T091557Z:06c9f7c2-d452-4d99-841a-2ad4a7cf4948" + ], + "Strict-Transport-Security": [ + "max-age=31536000; includeSubDomains" + ], + "X-Content-Type-Options": [ + "nosniff" + ], + "Date": [ + "Wed, 24 Nov 2021 09:15:56 GMT" + ], + "Content-Type": [ + "application/json; charset=utf-8" + ], + "Expires": [ + "-1" + ], + "Content-Length": [ + "140" + ] + }, + "ResponseBody": "{\r\n \"value\": [\r\n {\r\n \"id\": \"/tenants/54826b22-38d6-4fb2-bad9-b7b93a3e9c5a\",\r\n \"tenantId\": \"54826b22-38d6-4fb2-bad9-b7b93a3e9c5a\",\r\n \"tenantCategory\": \"Home\"\r\n }\r\n ]\r\n}", + "StatusCode": 200 + }, + { + "RequestUri": "/subscriptions?api-version=2021-01-01", + "EncodedRequestUri": "L3N1YnNjcmlwdGlvbnM/YXBpLXZlcnNpb249MjAyMS0wMS0wMQ==", + "RequestMethod": "GET", + "RequestBody": "", + "RequestHeaders": { + "x-ms-client-request-id": [ + "8d43ac0a-3efd-447f-bc3e-c55c24b26d05" + ], + "Accept-Language": [ + "en-US" + ], + "User-Agent": [ + "FxVersion/4.6.28207.03", + "OSName/Windows", + "OSVersion/Microsoft.Windows.10.0.22000.", + "Microsoft.Azure.Management.ResourceManager.Version2021.01.01.SubscriptionClient/1.3.53" + ] + }, + "ResponseHeaders": { + "Cache-Control": [ + "no-cache" + ], + "Pragma": [ + "no-cache" + ], + "x-ms-ratelimit-remaining-tenant-reads": [ + "11999" + ], + "x-ms-request-id": [ + "3f4e2063-6e65-4009-b5cf-5b29d0f85cca" + ], + "x-ms-correlation-request-id": [ + "3f4e2063-6e65-4009-b5cf-5b29d0f85cca" + ], + "x-ms-routing-request-id": [ + "SOUTHEASTASIA:20211124T091556Z:3f4e2063-6e65-4009-b5cf-5b29d0f85cca" + ], + "Strict-Transport-Security": [ + "max-age=31536000; includeSubDomains" + ], + "X-Content-Type-Options": [ + "nosniff" + ], + "Date": [ + "Wed, 24 Nov 2021 09:15:56 GMT" + ], + "Content-Type": [ + "application/json; charset=utf-8" + ], + "Expires": [ + "-1" + ], + "Content-Length": [ + "500" + ] + }, + "ResponseBody": "{\r\n \"value\": [\r\n {\r\n \"id\": \"/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590\",\r\n \"authorizationSource\": \"RoleBased\",\r\n \"managedByTenants\": [\r\n {\r\n \"tenantId\": \"2f4a9838-26b7-47ee-be60-ccc1fdec5953\"\r\n }\r\n ],\r\n \"tags\": {},\r\n \"subscriptionId\": \"0b1f6471-1bf0-4dda-aec3-cb9272f09590\",\r\n \"tenantId\": \"54826b22-38d6-4fb2-bad9-b7b93a3e9c5a\",\r\n \"displayName\": \"AzureSDKTest\",\r\n \"state\": \"Enabled\",\r\n \"subscriptionPolicies\": {\r\n \"locationPlacementId\": \"Internal_2014-09-01\",\r\n \"quotaId\": \"Internal_2014-09-01\",\r\n \"spendingLimit\": \"Off\"\r\n }\r\n }\r\n ],\r\n \"count\": {\r\n \"type\": \"Total\",\r\n \"value\": 1\r\n }\r\n}", + "StatusCode": 200 + }, + { + "RequestUri": "/subscriptions?api-version=2021-01-01", + "EncodedRequestUri": "L3N1YnNjcmlwdGlvbnM/YXBpLXZlcnNpb249MjAyMS0wMS0wMQ==", + "RequestMethod": "GET", + "RequestBody": "", + "RequestHeaders": { + "x-ms-client-request-id": [ + "10c2752e-0e68-4ad2-ba4c-cd295d89d3d7" + ], + "Accept-Language": [ + "en-US" + ], + "User-Agent": [ + "FxVersion/4.6.28207.03", + "OSName/Windows", + "OSVersion/Microsoft.Windows.10.0.22000.", + "Microsoft.Azure.Management.ResourceManager.Version2021.01.01.SubscriptionClient/1.3.53" + ] + }, + "ResponseHeaders": { + "Cache-Control": [ + "no-cache" + ], + "Pragma": [ + "no-cache" + ], + "x-ms-ratelimit-remaining-tenant-reads": [ + "11998" + ], + "x-ms-request-id": [ + "e64b3b70-a911-42d0-85d3-55cb35a791fd" + ], + "x-ms-correlation-request-id": [ + "e64b3b70-a911-42d0-85d3-55cb35a791fd" + ], + "x-ms-routing-request-id": [ + "SOUTHEASTASIA:20211124T091557Z:e64b3b70-a911-42d0-85d3-55cb35a791fd" + ], + "Strict-Transport-Security": [ + "max-age=31536000; includeSubDomains" + ], + "X-Content-Type-Options": [ + "nosniff" + ], + "Date": [ + "Wed, 24 Nov 2021 09:15:57 GMT" + ], + "Content-Type": [ + "application/json; charset=utf-8" + ], + "Expires": [ + "-1" + ], + "Content-Length": [ + "500" + ] + }, + "ResponseBody": "{\r\n \"value\": [\r\n {\r\n \"id\": \"/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590\",\r\n \"authorizationSource\": \"RoleBased\",\r\n \"managedByTenants\": [\r\n {\r\n \"tenantId\": \"2f4a9838-26b7-47ee-be60-ccc1fdec5953\"\r\n }\r\n ],\r\n \"tags\": {},\r\n \"subscriptionId\": \"0b1f6471-1bf0-4dda-aec3-cb9272f09590\",\r\n \"tenantId\": \"54826b22-38d6-4fb2-bad9-b7b93a3e9c5a\",\r\n \"displayName\": \"AzureSDKTest\",\r\n \"state\": \"Enabled\",\r\n \"subscriptionPolicies\": {\r\n \"locationPlacementId\": \"Internal_2014-09-01\",\r\n \"quotaId\": \"Internal_2014-09-01\",\r\n \"spendingLimit\": \"Off\"\r\n }\r\n }\r\n ],\r\n \"count\": {\r\n \"type\": \"Total\",\r\n \"value\": 1\r\n }\r\n}", + "StatusCode": 200 + } + ], + "Names": {}, + "Variables": { + "SubscriptionId": "0b1f6471-1bf0-4dda-aec3-cb9272f09590" + } +} \ No newline at end of file diff --git a/src/Accounts/Accounts.Test/SubscriptionCmdletTests.cs b/src/Accounts/Accounts.Test/SubscriptionCmdletTests.cs index 729c41fa2a95..88fe4b191013 100644 --- a/src/Accounts/Accounts.Test/SubscriptionCmdletTests.cs +++ b/src/Accounts/Accounts.Test/SubscriptionCmdletTests.cs @@ -59,5 +59,12 @@ public void GetSubscriptionsWithTags() { TestRunner.RunTestScript("Test-GetSubscriptionsWithTags"); } + + [Fact] + [Trait(Category.AcceptanceType, Category.CheckIn)] + public void GetSubscriptionsAfterContextRenameAndSet() + { + TestRunner.RunTestScript("Test-GetSubscriptionsAfterContextRenameAndSet"); + } } } diff --git a/src/Accounts/Accounts.Test/SubscriptionCmdletTests.ps1 b/src/Accounts/Accounts.Test/SubscriptionCmdletTests.ps1 index fc897e20666c..706f253e1945 100644 --- a/src/Accounts/Accounts.Test/SubscriptionCmdletTests.ps1 +++ b/src/Accounts/Accounts.Test/SubscriptionCmdletTests.ps1 @@ -133,3 +133,24 @@ function Test-GetSubscriptionsWithTags $allSubscriptions = Get-AzSubscription Assert-True {($allSubscriptions | Where-Object { $_.Tags -ne $null}).Count -gt 0} } + +<# +.SYNOPSIS +Tests whether subscripitions before and after context rename and set are equal. +.DESCRIPTION +SmokeTest +#> +function Test-GetSubscriptionsAfterContextRenameAndSet +{ + $subscriptionExp = Get-AzSubscription + + $contextWithOutSecret = Get-AzContext + Assert-False {$contextWithOutSecret.Account.ExtendedProperties.Keys -Contains 'ServicePrincipalSecret'} + $newContextName = "ContextWithoutSecret" + Set-AzContext -Context $contextWithOutSecret -Name $newContextName + Assert-AreEqual $newContextName (Get-AzContext).Name + Assert-False {(Get-AzContext).Account.ExtendedProperties.Keys -Contains 'ServicePrincipalSecret'} + + $subscriptionActual = Get-AzSubscription + Assert-AreEqualObjectProperties $subscriptionExp $subscriptionActual +} \ No newline at end of file diff --git a/src/Accounts/Accounts/Accounts.format.ps1xml b/src/Accounts/Accounts/Accounts.format.ps1xml index f00217618a96..818c9fc643a1 100644 --- a/src/Accounts/Accounts/Accounts.format.ps1xml +++ b/src/Accounts/Accounts/Accounts.format.ps1xml @@ -269,7 +269,7 @@ - $_.ExtendedProperties.GetEnumerator() | Where-Object { $_.Key -ne "ServicePrincipalSecret" } + $_.ExtendedProperties.GetEnumerator() diff --git a/src/Accounts/Accounts/ChangeLog.md b/src/Accounts/Accounts/ChangeLog.md index 97a0c98c2740..0c367cc08ee5 100644 --- a/src/Accounts/Accounts/ChangeLog.md +++ b/src/Accounts/Accounts/ChangeLog.md @@ -19,6 +19,7 @@ --> ## Upcoming Release +* Removed `ServicePrincipalSecret` and `CertificatePassword` in Account of Context from display [#15427] * Added optional parameter `MicrosoftGraphAccessToken` to `Connect-AzAccount` * Added optional parameters `MicrosoftGraphEndpointResourceId`, `MicrosoftGraphUrl` to `Add-AzEnvironment` and `Set-AzEnvironment` * Added `-AccountId` property to `UserWithSubscriptionId` parameter set of `Connect-AzAccount` which allows a user name to be pre-selected for interactive logins diff --git a/src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs b/src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs index 38068d727e02..cbff8c7ba1c8 100644 --- a/src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs +++ b/src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs @@ -488,11 +488,25 @@ public bool TryRenameContext(string sourceName, string targetName) return result; } + /// + /// Add the input context with the specified name. + /// If the context with the same tenant, subscription, accountId does not exist, add the input into context list. + /// If the context with the same tenant, subscription, accountId already exist, merge 2 contexes and add the merged context to the context list. + /// + /// The specified new name of the context. + /// The new context to set as default. public bool TrySetContext(string name, IAzureContext context) { bool result = false; if (Contexts != null) { + string oldName = null; + if (TryFindContext(context, out oldName)) + { + var oldContext = Contexts[oldName]; + oldContext.Update(context); + context = oldContext; + } Contexts[name] = context; result = true; } @@ -528,6 +542,13 @@ public bool TrySetDefaultContext(string name) return result; } + /// + /// Set the default context with the input context. + /// If the context with the same name does not exist, add the input into context list and set as default. + /// If the context with the same name already exist, update the attributes with the same names and add the missing attributes. + /// + /// The new context to set as default. + public bool TrySetDefaultContext(IAzureContext context) { bool result = false; diff --git a/src/Accounts/Authentication.ResourceManager/Models/PSAzureRmAccount.cs b/src/Accounts/Authentication.ResourceManager/Models/PSAzureRmAccount.cs index b5c25c610be6..d0f079a3b75a 100644 --- a/src/Accounts/Authentication.ResourceManager/Models/PSAzureRmAccount.cs +++ b/src/Accounts/Authentication.ResourceManager/Models/PSAzureRmAccount.cs @@ -69,6 +69,23 @@ public PSAzureRmAccount() public PSAzureRmAccount(IAzureAccount other) { this.CopyFrom(other); + /* We remove the sceret attributes from display as they are stored in plain text. + * At the same time to keep the secrets in AzureRmContext.json. + * It only takes effect when Az.Accounts converts AzureAccount to PSAzureRmAccount. + * When the user stores the PSAzureRmAccount object (usually in a PSAzureContext) and sets it back as the current context, + * The context after setting without the secrets is stored to AzureRmContext.json. + * The operation above is to update the context object with the same name. + * However, the update we perform only adds or replaces attributes but not to remove anything (refer to ModelExtensions::UpdateProperties). + * And so the existing secrets in AzureRmContext.json is not removed. + */ + if (this.ExtendedProperties.ContainsKey(AzureAccount.Property.ServicePrincipalSecret)) + { + this.ExtendedProperties.Remove(AzureAccount.Property.ServicePrincipalSecret); + } + if (this.ExtendedProperties.ContainsKey(AzureAccount.Property.CertificatePassword)) + { + this.ExtendedProperties.Remove(AzureAccount.Property.CertificatePassword); + } } /// From d43b2e6b03b9334928d5d42cbf5ccb9dc96e4645 Mon Sep 17 00:00:00 2001 From: Jin Lei <54836179+msJinLei@users.noreply.github.com> Date: Thu, 25 Nov 2021 13:02:59 +0800 Subject: [PATCH 2/3] Update src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs Co-authored-by: Dingmeng Xue --- src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs b/src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs index cbff8c7ba1c8..146a50e49ffd 100644 --- a/src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs +++ b/src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs @@ -500,8 +500,7 @@ public bool TrySetContext(string name, IAzureContext context) bool result = false; if (Contexts != null) { - string oldName = null; - if (TryFindContext(context, out oldName)) + if (TryFindContext(context, out string oldName)) { var oldContext = Contexts[oldName]; oldContext.Update(context); From c1ae6df12e53a615f49318b332bc9a0a9d05ac59 Mon Sep 17 00:00:00 2001 From: Lei Jin Date: Thu, 25 Nov 2021 13:07:46 +0800 Subject: [PATCH 3/3] Address review comments --- src/Accounts/Accounts/ChangeLog.md | 2 +- src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Accounts/Accounts/ChangeLog.md b/src/Accounts/Accounts/ChangeLog.md index 0c367cc08ee5..e87e80380927 100644 --- a/src/Accounts/Accounts/ChangeLog.md +++ b/src/Accounts/Accounts/ChangeLog.md @@ -19,7 +19,7 @@ --> ## Upcoming Release -* Removed `ServicePrincipalSecret` and `CertificatePassword` in Account of Context from display [#15427] +* Removed `ServicePrincipalSecret` and `CertificatePassword` in `PSAzureRmAccount` [#15427] * Added optional parameter `MicrosoftGraphAccessToken` to `Connect-AzAccount` * Added optional parameters `MicrosoftGraphEndpointResourceId`, `MicrosoftGraphUrl` to `Add-AzEnvironment` and `Set-AzEnvironment` * Added `-AccountId` property to `UserWithSubscriptionId` parameter set of `Connect-AzAccount` which allows a user name to be pre-selected for interactive logins diff --git a/src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs b/src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs index 146a50e49ffd..e32f0484de89 100644 --- a/src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs +++ b/src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs @@ -502,7 +502,7 @@ public bool TrySetContext(string name, IAzureContext context) { if (TryFindContext(context, out string oldName)) { - var oldContext = Contexts[oldName]; + var oldContext = Contexts[oldName].DeepCopy(); oldContext.Update(context); context = oldContext; }