The ProtectKeysWithAzureKeyVault section of this page suggests a series of steps that result in a serious security issue.

[hypermoose]

The ProtectKeysWithAzureKeyVault section of this page suggests that the reader run the sample code twice. Once with the ProtectKeysWithAzureKeyVault call commented out to create the initial blob and then a second time with the protect call left in. The problem with that is that not only is the block file created for the keyring but an initial key is created that is valid for 3 months from the time you ran it. When you add the ProtectKeysWithAzureKeyVault it actually does NOTHING!. Your key is now free and clear in blob storage for anyone to read UNTIL that key expires and a new one is generated which is keyvault encrypted. The instructions should state that the block file should be edited to remove the key or the sample empty file is so small you should just include it in the instructions.

The other thing that is wrong is the permissions need for the web app in keyvault, You must have the get permissions as well as wrap and unwrap.

---

Document Details

⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

• ID: b63ab082-ad30-b00d-ecd1-ca88eee1659e
• Version Independent ID: 69818122-e47a-3fed-ea37-8009f66e2a5c
• Content: Configure ASP.NET Core Data Protection
• Content Source: aspnetcore/security/data-protection/configuration/overview.md
• Product: aspnet-core
• Technology: aspnetcore-security
• GitHub Login: @Rick-Anderson
• Microsoft Alias: riande

[Rick-Anderson]

@markmcgookin please review

[markmcgookin]

Yeah, this is a super confusing topic. We do state here that ideally you should just upgrade to the new libraries as that will create things properly, but it might be made clearer that this doesn't require running the code twice with the line removed.

We recommended upgrading to the Azure.Extensions.AspNetCore.DataProtection.Blobs and Azure.Extensions.AspNetCore.DataProtection.Keys packages because the API provided automatically creates the blob if it doesn't exist.

Maybe we should remove the workaround and just insist people use the new packages?

Also... re: keyvault config, yeah we should probably mention that those specific permissions are needed. Even though this isn't necessarily about configuring keyvault itself, it would make sense to at least give users a heads up for what is required.

[Rick-Anderson]

Maybe we should remove the workaround and just insist people use the new packages?

Yes, let's do that. We want to encourage folks to use the latest bits.

we should probably mention that those specific permissions are needed. Even though this isn't necessarily about configuring keyvault itself, it would make sense to at least give users a heads up for what is required.

Can you PR this?

[hypermoose]

I did use the new libraries and I believe it threw an exception until I commented out the second line that the blob did not exist. Not 100% sure though. Gary Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for Windows 10 From: Mark ***@***.***> Sent: Friday, June 18, 2021 7:07 AM To: ***@***.***> Cc: Gary ***@***.***>; ***@***.***> Subject: Re: [dotnet/AspNetCore.Docs] The ProtectKeysWithAzureKeyVault section of this page suggests a series of steps that result in a serious security issue. (#22546) Yeah, this is a super confusing topic. We do state here<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2FAspNetCore.Docs%2Fblob%2Fmain%2Faspnetcore%2Fsecurity%2Fdata-protection%2Fconfiguration%2Foverview.md%23protectkeyswithazurekeyvault&data=04%7C01%7C%7C9dd50f09dcb9459cf57608d932626ccd%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637596220600594856%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=AtcOdkcdN3S%2B9nIxIxvcyW7JFs4thh%2BDyJ3s32ju3do%3D&reserved=0> that ideally you should just upgrade to the new libraries as that will create things properly, but it might be made clearer that this doesn't require running the code twice with the line removed. We recommended upgrading to the Azure.Extensions.AspNetCore.DataProtection.Blobs and Azure.Extensions.AspNetCore.DataProtection.Keys packages because the API provided automatically creates the blob if it doesn't exist. Maybe we should remove the workaround and just insist people use the new packages? Also... re: keyvault config, yeah we should probably mention that those specific permissions are needed. Even though this isn't necessarily about configuring keyvault itself, it would make sense to at least give users a heads up for what is required. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2FAspNetCore.Docs%2Fissues%2F22546%23issuecomment-864065192&data=04%7C01%7C%7C9dd50f09dcb9459cf57608d932626ccd%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637596220600594856%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BI2f4cuTqxeTBdZF%2Fm2tVXi2RUgZ31hJ8fpGOSko1gk%3D&reserved=0>, or unsubscribe<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FADSNZJWEQCA5JMRUUMJPUV3TTNHKPANCNFSM46YFANYQ&data=04%7C01%7C%7C9dd50f09dcb9459cf57608d932626ccd%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637596220600604814%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=MEUVwYPg%2BYkWcZbEe0gfYLyVVqnu1W%2FGhTJRzvnwN8A%3D&reserved=0>.

[markmcgookin]

I am pretty sure the new libraries work fine without the second line commented out. I had this discussion here and here

I'll have a look at re-doing this bit as it's pretty confusing at the minute.

[Rick-Anderson]

@markmcgookin can you take a look at this and #24096

[markmcgookin]

@markmcgookin can you take a look at this and #24096

PR #24184 should cover both of these, not sure if it's automatically picked up this one though. Might need to manually close if/when it's merged.