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

Create certificate support proposal.md #176

Closed
wants to merge 4 commits into from
Closed

Conversation

bgavrilMS
Copy link
Member

No description provided.

@bgavrilMS
Copy link
Member Author

Copied from #165 but made into a PR so that we can add comments

"CertificateFromBase64Encoded": "Base64Encoded"
},
{
"CertificateFromKeyVault": "https://vaultname.vault.azure.net/certificates/certificateid/3fb1c62f74b844b0a2d9f1a3d289648d"
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if the naming here is a bit misleading if you expect to use managed identity here? AFAIK, you can only use this if the code is running in an Azure VM or an Azure Function?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually no: Managed identities also work for App services, plenty of Azure services (increasing) and even locally on the dev box (using the shared token cache)

They can prove their identity to Azure AD by 3 means:
- client secrets
- client certificates
- client assertions
Copy link
Member Author

Choose a reason for hiding this comment

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

Would you want to integrate with Wilson and expose configuring a SigningCredential here? I think this is similar, if not better, than client assertions option and would open up more scenarios (not that these 2 are mutually exclusive)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know if SigningCredential also works with MSI?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not as far as I know, but @GeoK would know best

Copy link
Member

Choose a reason for hiding this comment

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

SigningCredentials works with any SecurityKey, including ManagedKeyVaultSecurityKey that supports MSI.
SigningCredentials object has to be constructed with a KeyVaultCryptoProvider as CustomCryptoProvider, but that can be simplified by introducing a new class, e.g. ManagedKeyVaultSigningCredentials.

This would be a current setup:

var sc = new SigningCredentials(new ManagedKeyVaultSecurityKey([keyId]), [alg])
{
    CryptoProviderFactory = new CryptoProviderFactory
    {
        CustomCryptoProvider = new KeyVaultCryptoProvider()
    }
}

Copy link
Collaborator

@jmprieur jmprieur May 27, 2020

Choose a reason for hiding this comment

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

Thanks @GeoK :

  • Does it use the new Azure SDK TokenCredential?
  • Is MSI working with the developer credentials in the shared token cache (between VS, Azure Rm PowerShell and Azure CLI) ?

also do customers need to provide the algorithm? do the understand what this means? (I don't)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with TokenCredential. ManagedKeyVaultSecurityKey leverages Microsoft.Azure.KeyVault and Microsoft.Azure.Services.AppAuthentication. Would these libraries have the business logic that you are looking for?

SigningCredentials is usually used in token signing scenarios where algorithm needs to be specified.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@GeoK : I believe that the new Azure SDK uses Azure.Security.KeyVault (and Azure.Identity). Underneath, it uses MSAL.NET.


Today, Microsoft.Identity.Web enables developers to provide client secrets.
In addition to Client secret, we'd want Microsoft.Identity.Web to support client certificates. The constraints are the following:
- enable several ways of getting the certificate. You'd provide a description on how to get the certificate.
Copy link
Member Author

Choose a reason for hiding this comment

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

Can the developers programatically configure an X509Certificate object?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's a good idea;

In addition to Client secret, we'd want Microsoft.Identity.Web to support client certificates. The constraints are the following:
- enable several ways of getting the certificate. You'd provide a description on how to get the certificate.
- from the certificate store (Windows) and a thumbprint ("440A5BE6C4BE2FF02A0ADBED1AAA43D6CF12E269")
- from the certificate store (Windows) and a distinguished name ("CN=TestCert")
Copy link
Member Author

Choose a reason for hiding this comment

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

- from a KeyVault address.
- getting the certificate just in time, rather than paying the startup cost. For instance for a web app that signs in a user, do not load the certificate until an access token is needed to call a Web API.
- when the certificate is stored in KeyVault, leverage Managed identity (probably though the Azure SDK for .NET)
- help you rotating your certificates but letting you provide several (2) certificates
Copy link
Member Author

Choose a reason for hiding this comment

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

How to you envision this scenario? I see that you provided support for configuring N certificates, but this scenario is not described. I'm wondering if an array of certificates is what is needed here? Perhaps 2 well known certificates should be used instead (e.g. "current_certificate", "next_certificate") ?

Proposed sequence:
1. Experiment with the KeyVault SDK part of the Azure SDK. This has the advantage of getting the secrets from KeyVault both locally, and when the Web app or Web API is deployed, as it leverages Managed identity. See https://docs.microsoft.com/en-us/azure/key-vault/general/tutorial-net-create-vault-azure-web-app#update-the-code (this one is for client secrets, we need to find the equivalent with certificates, for instance by looking at https://docs.microsoft.com/en-us/dotnet/api/overview/azure/security.keyvault.certificates-readme?view=azure-dotnet#retrieve-a-certificate). We could use, for the client the DefaultAzureCredentials (which also work well in the dev environment)

This means we would now leverage the following NuGet packages: **Azure.Identity** and **Azure.Security.KeyVault.Certificates**. This seems fine but we need to check if there is a negative impact.
Copy link
Member Author

Choose a reason for hiding this comment

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

As part of the experiment, we should also understand if there is significant logic related to MSI in those assemblies. If not, perhaps we can avoid the dependencies.

/// this value is the path to the certificate in the cert store, for instance <c>CurrentUser/My</c></item>
/// </list>
/// </summary>
public string Container { get; set; }
Copy link
Member Author

@bgavrilMS bgavrilMS May 26, 2020

Choose a reason for hiding this comment

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

Personally I don't like this "weakly typed" API, but perhaps I don't understand the ASP.NET Core prior art. Why not use a builder pattern that resembles the json document you described

Certificate.LoadFromFile(string path);
Certificate.LoadFromKeyVault(...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bgavrilMS : so you like the initial json? I think it's a bit confusing.

@jennyf19 jennyf19 marked this pull request as draft May 26, 2020 15:34
@jennyf19
Copy link
Collaborator

this feature has been added. I don't think we need the PR anymore, we will update the docs/wiki accordingly. thanks @bgavrilMS
fyi: @jmprieur

@jennyf19 jennyf19 closed this Jun 10, 2020
@jmprieur jmprieur deleted the bogavril/spec branch June 13, 2020 13:10
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.

4 participants