[KeyVault] adding soft-delete support for KeyVault certificates #4400

dragav · 2017-08-02T00:59:03Z

Description

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

…into preview

enabling soft-delete for KeyVault certificates: updating tests, help adding missing config file including Name parameter in ByDeletedCertificate parameter set picking up latest KV .net SDK

dragav · 2017-08-02T22:06:11Z

The build appears to be failing because of static analysis; there is a large number of warnings, none of which come from the KeyVault namespace.

The only references to KeyVault listed in the build warnings file are several skipped management test cases.

cormacpayne

@dragav a few comments that need to be resolved

cormacpayne · 2017-08-02T22:50:23Z

src/ResourceManager/KeyVault/Commands.KeyVault.Test/Commands.KeyVault.Test.csproj

@@ -196,6 +196,7 @@
    <Compile Include="UnitTests\SetKeyVaultSecretTests.cs" />
  </ItemGroup>
  <ItemGroup>
+    <None Include="app.config" />


@dragav please remove this

cormacpayne · 2017-08-03T18:00:09Z

src/ResourceManager/KeyVault/Commands.KeyVault/app.config

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="utf-8"?>


@dragav please remove this file

cormacpayne · 2017-08-03T18:01:03Z

src/ResourceManager/KeyVault/Commands.KeyVault/help/Add-AzureKeyVaultKey.md

@@ -12,176 +12,36 @@ Creates a key in a key vault or imports a key into a key vault.

 ## SYNTAX

-### Create (Default)


@dragav what happened to this file?

@cormacpayne help generation seems to output incorrect content for a few commands, I haven't figured out why. I've manually updated some of them, and this one slipped through; will revert the change.

cormacpayne · 2017-08-03T18:02:59Z

src/ResourceManager/KeyVault/Commands.KeyVault/packages.config

@@ -1,7 +1,11 @@
 <?xml version="1.0" encoding="utf-8"?>
 <packages>
  <package id="Microsoft.Azure.ActiveDirectory.GraphClient" version="2.1.0" targetFramework="net45" />
-  <package id="Microsoft.Azure.KeyVault" version="2.3.0-preview" targetFramework="net452" />
-  <package id="Microsoft.Azure.KeyVault.WebKey" version="2.0.6" targetFramework="net452" />
+  <package id="Microsoft.Azure.Common" version="2.1.0" targetFramework="net45" />


@dragav please remove the extra dependencies that were added outside of what previously resided in this file. We recently made some changes to move common dependencies into a separate targets file that is referenced by all projects, so anything defined in that target should not exist in your csproj file or packages.config

cormacpayne · 2017-08-04T20:14:18Z

src/ResourceManager/KeyVault/Commands.KeyVault.Test/Scripts/VaultCertificateTests.ps1

+    Assert-NotNull $createdCert2
+
+    Get-AzureKeyVaultCertificate $keyVault |  Where-Object {$_.CertificateName -like $rootCertName + '*'}  | Remove-AzureKeyVaultCertificate -Force -Confirm:$false
+    Wait-Seconds 30


@dragav why do we need to wait 30 seconds?

Soft-deletion is an asynchronous operation, performed in the background by a different role.

cormacpayne · 2017-08-04T20:25:11Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/UndoAzureKeyVaultCertificateRemoval.cs

+{
+    [Cmdlet( VerbsCommon.Undo, "AzureKeyVaultCertificateRemoval",
+    SupportsShouldProcess = true,
+    ConfirmImpact = ConfirmImpact.Low,


@dragav why change the ConfirmImpact level?

cormacpayne · 2017-08-04T20:29:03Z

src/ResourceManager/KeyVault/Commands.KeyVault/help/AzureRM.KeyVault.md

@@ -20,9 +20,6 @@ Adds a contact for certificate notifications.
 ### [Add-AzureKeyVaultKey](Add-AzureKeyVaultKey.md)
 Creates a key in a key vault or imports a key into a key vault.

-### [Add-AzureKeyVaultManagedStorageAccount](Add-AzureKeyVaultManagedStorageAccount.md)


@dragav it seems like this file did not get refreshed properly

cormacpayne · 2017-08-04T20:34:47Z

src/ResourceManager/KeyVault/Commands.KeyVault/Models/KeyAttributes.cs

@@ -65,7 +65,7 @@ public KeyAttributes()

        public DateTime? Updated { get; private set; }

-        public bool PurgeDisabled { get; private set; }
+        public string RecoveryLevel { get; private set; }


@dragav removing the PurgeDisabled property is a breaking change. Users who run the following will be broken:

$recoveredKey = Undo-AzureKeyVaultKeyRemoval -VaultName $vault -Name $keyName $recoveredKey.Attributes.PurgeDisabled

Our recommendation is to keep supporting this property, making it obsolete until the next breaking change release (this fall) when you can then remove it since users will have been warned ahead of time of the change.

Normally I'd heed your recommendation; in this case, however, the presence of this field has security implications. The server protocol does not include this field, and a default here conveys the wrong information. I would rather have the client be broken (and make a quick script change) than use incorrect information.

Furthermore, the previous PSH release was based on a non-public KV SDK; that is, this field should have been considered "pre-release" via PSH as well, but we did not find a way to mark it as such. This PSH release uses a public KV SDK, and so it is in line with the REST API spec and the service itself.

I can add this as an entry to the breaking change log..

We don't have the option of the client being broken, we will not ship any breaking changes until November, except in the case of critical security issues or other critical bugs.

This can't be a breaking change, since PurgeDisabled was never part of the service REST API spec. Yes, there is a publicly-released PowerShell out there which includes PurgeDisabled, but nobody should have taken a dependency on that since the feature was in preview.

Unfortunately, no breaking changes are a hard requirement

Understood; reverted the breaking change.

cormacpayne · 2017-08-04T20:35:20Z

src/ResourceManager/KeyVault/Commands.KeyVault/Models/KeyIdentityItem.cs

@@ -36,7 +36,7 @@ internal KeyIdentityItem(Azure.KeyVault.Models.KeyItem keyItem, VaultUriHelper v
            NotBefore = keyItem.Attributes.NotBefore;
            Created = keyItem.Attributes.Created;
            Updated = keyItem.Attributes.Updated;
-            PurgeDisabled = keyItem.Attributes.PurgeDisabled;


@dragav same comment throughout this file

cormacpayne · 2017-08-04T21:03:45Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/RemoveAzureKeyVaultCertificate.cs

@@ -27,7 +27,7 @@ namespace Microsoft.Azure.Commands.KeyVault
        SupportsShouldProcess = true,
        ConfirmImpact = ConfirmImpact.High,
        HelpUri = Constants.KeyVaultHelpUri)]
-    [OutputType(typeof(KeyVaultCertificate))]
+    [OutputType(typeof(DeletedCertificateBundle))]


@dragav nothing has changed about the output of this cmdlet, right? It is still returning a KeyVaultCertificate below the block of code you added. The block you added does not write any object to the output stream, either.

Not sure what you mean.. The response of this cmdlet is instantiated on line 93:

DeletedCertificateBundle certBundle = null;

The DataServiceClient (invoked to delete the cert) returns:

DeletedCertificateBundle DeleteCertificate(string vaultName, string certName);

@dragav the object being written to the output stream is still a KeyVaultCertificate on line 111, so the output type has not changed

…e/updates4aug

…ng-change release

…feature/updates4aug

cormacpayne · 2017-08-08T00:07:48Z

On-demand: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1826/

markcowl · 2017-08-22T17:36:53Z

@azuresdkci retest this please

…e/updates4aug

…-powershell into feature/updates4aug

…et Remove-AzureKeyVaultCertificate (a non-breaking change)

cormacpayne

@dragav please open a new PR that fixes the error in the change log

cormacpayne · 2017-08-28T23:25:15Z

src/ResourceManager/KeyVault/ChangeLog.md

@@ -24,6 +24,10 @@
 ## Version 3.3.0

 ## Version 3.2.1
+* New/updated Cmdlets to support soft-delete for KeyVault certificates


@dragav please move this to under Current Release; the versions that are already listed are the notes from previous releases

Dragos Avadanei added 5 commits May 5, 2017 11:27

Merge remote-tracking branch 'refs/remotes/Azure/preview' into preview

7c3f73f

Merge remote-tracking branch 'refs/remotes/Azure/preview' into preview

de7528f

Merge remote-tracking branch 'refs/remotes/Azure/preview' into preview

5fdedbf

Merge branch 'preview' of https://github.com/dragav/azure-powershell …

e0d808a

…into preview

adding support for soft deletion of certificates

de26802

enabling soft-delete for KeyVault certificates: updating tests, help adding missing config file including Name parameter in ByDeletedCertificate parameter set picking up latest KV .net SDK

dragav requested a review from RandalliLama August 2, 2017 00:59

msftclas added the cla-not-required label Aug 2, 2017

dragav requested a review from schaabs August 2, 2017 00:59

azuresdkci added the needs-review label Aug 2, 2017

azuresdkci assigned shahabhijeet Aug 2, 2017

cormacpayne assigned cormacpayne and unassigned shahabhijeet Aug 2, 2017

schaabs previously approved these changes Aug 2, 2017

View reviewed changes

cormacpayne changed the base branch from preview to release-4.3.0 August 4, 2017 16:04

cormacpayne suggested changes Aug 4, 2017

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Aug 4, 2017

cormacpayne assigned dragav and unassigned cormacpayne Aug 4, 2017

addressing review feedback

4cb144c

dragav dismissed schaabs’s stale review via 4cb144c August 5, 2017 00:09

Dragos Avadanei added 4 commits August 4, 2017 18:35

more review feedback - writing the deleted certificate on passthrough

b892bcc

Merge remote-tracking branch 'refs/remotes/Azure/preview' into featur…

6d44d10

…e/updates4aug

restoring the PurgeDisabled flag, to prevent this from being a breaki…

0fdd3cc

…ng-change release

Merge remote-tracking branch 'refs/remotes/Azure/release-4.3.0' into …

468d2e9

…feature/updates4aug

dragav and others added 2 commits August 7, 2017 23:10

Merge branch 'release-4.3.0' into feature/updates4aug

ee24b97

addressing breaking change issues uncovered by the official build

c083c5f

Dragos Avadanei added 6 commits August 24, 2017 18:38

Reverting Certificate cmdlets to use PSH-defined types

a8b3c4c

Merge remote-tracking branch 'refs/remotes/Azure/preview' into featur…

6d4a184

…e/updates4aug

Merge remote-tracking branch 'refs/remotes/Azure/preview' into preview

54f758e

KeyVault: adding soft-delete support for certificates

10ea130

Merge branch 'feature/updates4aug' of https://github.com/dragav/azure…

1c69b0f

…-powershell into feature/updates4aug

reverting inadvertent change picked up from merge

8ac520a

cormacpayne changed the base branch from release-4.3.0 to preview August 25, 2017 22:42

Dragos Avadanei added 2 commits August 28, 2017 13:40

removing project references to Newtonsoft.Json v9

d26cb1b

[KeyVault] Suppressing exception for changing the return type of cmdl…

b71b908

…et Remove-AzureKeyVaultCertificate (a non-breaking change)

cormacpayne approved these changes Aug 28, 2017

View reviewed changes

cormacpayne merged commit 7d70753 into Azure:preview Aug 28, 2017

dragav deleted the feature/updates4aug branch August 29, 2017 01:14

		@@ -12,176 +12,36 @@ Creates a key in a key vault or imports a key into a key vault.

		## SYNTAX

		### Create (Default)

[KeyVault] adding soft-delete support for KeyVault certificates #4400

[KeyVault] adding soft-delete support for KeyVault certificates #4400

Uh oh!

Conversation

dragav commented Aug 2, 2017 • edited by hiobay Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

dragav commented Aug 2, 2017

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne commented Aug 8, 2017

Uh oh!

markcowl commented Aug 22, 2017

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dragav commented Aug 2, 2017 •

edited by hiobay

Loading