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

Improve cert handling for Azure deployments #30

Open
damienbod opened this issue Oct 12, 2018 · 16 comments

Comments

@damienbod
Copy link
Owner

commented Oct 12, 2018

@Eneuman Would you be interested in adding your code here?

Saw this issue: SigningKey Azure Key Vault Provider

I have some helpers for this, maybe your solution is better. I use this as a template or quick starter for creating STS servers, which can be deployed easily to Azure App Services or IIS

Greetings Damien

@Eneuman

This comment has been minimized.

Copy link

commented Oct 12, 2018

Of course :)

I'm just gonna add the code in this post and you can pick the parts you like.

The idea is like this:
When you create a new certificate in Azure Key Vault you can set it to automaticly renew (create a new version) at a certain time. The extension will add all "Enabled" versions if the certificate to the ValidationKey set, and add the currect active version as the signing certificate.
The extension caches the keys for 1 day because of performance reasons. When you decide that your key rollover periode is over, you go into Azure Key Vault and disable the old version.

Note: MSI (Managed Service Identities) does not work out of the box on a local dev machine so its better to check environment and use the methods where you add client id and secret.

MSI should work on Azure Scale Sets and Azure App Services (but not in deployment slot scenarios yet, as far as I know, long post on github about it)

The code needs the following nuget packages to work:

    <PackageReference Include="IdentityServer4" Version="2.2.0" />
    <PackageReference Include="Microsoft.Azure.KeyVault" Version="3.0.0" />
    <PackageReference Include="Microsoft.Azure.Services.AppAuthentication" Version="1.0.3" />

DISCLAIMER: I have not fully tested the code yet so there might be bugs left ;)

using IdentityServer4.Stores;
using Microsoft.Azure.KeyVault;
using Microsoft.Azure.Services.AppAuthentication;
using Microsoft.Extensions.Caching.Memory;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.IdentityModel.Clients.ActiveDirectory;
using Microsoft.IdentityModel.Tokens;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Security.Cryptography.X509Certificates;
using System.Threading.Tasks;

namespace Authentication.Identity.Service.Extensions
{

  /// <summary>
  /// Extension methods for using Azure Key Vault with <see cref="IIdentityServerBuilder"/>.
  /// </summary>
  public static class IdentityServerAzureKeyVaultConfigurationExtensions
  {
    /// <summary>
    /// Adds a SigningCredentialStore and a ValidationKeysStore that reads the signing certificate from the Azure KeyVault.
    /// </summary>
    /// <param name="identityServerbuilder">The <see cref="IIdentityServerBuilder"/> to add to.</param>
    /// <param name="vault">The Azure KeyVault uri.</param>
    /// <param name="clientId">The application client id.</param>
    /// <param name="clientSecret">The client secret to use for authentication.</param>
    /// <param name="certificateName">The name of the certificate to use as the signing certificate.</param>
    /// <returns>The <see cref="IIdentityServerBuilder"/>.</returns>
    public static IIdentityServerBuilder AddSigningCredentialFromAzureKeyVault(this IIdentityServerBuilder identityServerbuilder, string vault, string clientId, string clientSecret, string certificateName, int signingKeyRolloverTimeInHours)
    {
      KeyVaultClient.AuthenticationCallback authenticationCallback = (authority, resource, scope) => GetTokenFromClientSecret(authority, resource, clientId, clientSecret);
      var keyVaultClient = new KeyVaultClient(authenticationCallback);

      identityServerbuilder.Services.AddMemoryCache();

      var sp = identityServerbuilder.Services.BuildServiceProvider();
      identityServerbuilder.Services.AddSingleton<ISigningCredentialStore>(new AzureKeyVaultSigningCredentialStore(sp.GetService<IMemoryCache>(), keyVaultClient, vault, certificateName, signingKeyRolloverTimeInHours));
      identityServerbuilder.Services.AddSingleton<IValidationKeysStore>(new AzureKeyVaultValidationKeysStore(sp.GetService<IMemoryCache>(), keyVaultClient, vault, certificateName));

      return identityServerbuilder;
    }

    /// <summary>
    /// Adds a SigningCredentialStore and a ValidationKeysStore that reads the signing certificate from the Azure KeyVault.
    /// </summary>
    /// <param name="identityServerbuilder">The <see cref="IIdentityServerBuilder"/> to add to.</param>
    /// <param name="vault">The Azure KeyVault uri.</param>
    /// <param name="certificateName">The name of the certificate to use as the signing certificate.</param>
    /// <remarks>Use this if you are using MSI (Managed Service Identity)</remarks>
    /// <returns>The <see cref="IIdentityServerBuilder"/>.</returns>
    public static IIdentityServerBuilder AddSigningCredentialFromAzureKeyVault(this IIdentityServerBuilder builder, string vault, string certificateName, int signingKeyRolloverTimeInHours)
    {
      var azureServiceTokenProvider = new AzureServiceTokenProvider();
      var authenticationCallback = new KeyVaultClient.AuthenticationCallback(azureServiceTokenProvider.KeyVaultTokenCallback);
      var keyVaultClient = new KeyVaultClient(authenticationCallback);

      builder.Services.AddMemoryCache();

      var sp = builder.Services.BuildServiceProvider();
      builder.Services.AddSingleton<ISigningCredentialStore>(new AzureKeyVaultSigningCredentialStore(sp.GetService<IMemoryCache>(), keyVaultClient, vault, certificateName, signingKeyRolloverTimeInHours));
      builder.Services.AddSingleton<IValidationKeysStore>(new AzureKeyVaultValidationKeysStore(sp.GetService<IMemoryCache>(), keyVaultClient, vault, certificateName));

      return builder;
    }

    private static async Task<string> GetTokenFromClientSecret(string authority, string resource, string clientId, string clientSecret)
    {
      var authContext = new AuthenticationContext(authority);
      var clientCred = new ClientCredential(clientId, clientSecret);
      var result = await authContext.AcquireTokenAsync(resource, clientCred);
      return result.AccessToken;
    }
  }

  public class AzureKeyVaultSigningCredentialStore : KeyStore, ISigningCredentialStore
  {
    private readonly IMemoryCache _cache;
    private readonly KeyVaultClient _keyVaultClient;
    private readonly string _vault;
    private readonly string _certificateName;
    private readonly int _signingKeyRolloverTimeInHours;

    public AzureKeyVaultSigningCredentialStore(IMemoryCache memoryCache, KeyVaultClient keyVaultClient, string vault, string certificateName, int signingKeyRolloverTimeInHours) : base(keyVaultClient, vault)
    {
      _cache = memoryCache;
      _keyVaultClient = keyVaultClient;
      _vault = vault;
      _certificateName = certificateName;
      _signingKeyRolloverTimeInHours = signingKeyRolloverTimeInHours;
    }

    public async Task<SigningCredentials> GetSigningCredentialsAsync()
    {
      // Try get the signing credentials from the cache
      if (_cache.TryGetValue("SigningCredentials", out SigningCredentials signingCredentials))
        return signingCredentials;

      signingCredentials = await GetFirstValidSigningCredentials();

      if (signingCredentials == null)
        return null;

      // Cache it
      var options = new MemoryCacheEntryOptions();
      options.AbsoluteExpiration = DateTime.Now.AddDays(1);
      _cache.Set("SigningCredentials", signingCredentials, options);

      return signingCredentials;
    }

    private async Task<SigningCredentials> GetFirstValidSigningCredentials()
    {
      // Find all enabled versions of the certificate
      var enabledCertificateVersions = await GetAllEnabledCertificateVersionsAsync(_certificateName);

      if (!enabledCertificateVersions.Any())
      {
        return null;
      }

      // Find the first certificate version that has a passed rollover time
      var certificateVersionWithPassedRolloverTime = enabledCertificateVersions
        .FirstOrDefault(certVersion => certVersion.Attributes.Created.HasValue && certVersion.Attributes.Created.Value < DateTime.UtcNow.AddHours(-_signingKeyRolloverTimeInHours));

      // If no certificate with passed rollovertime was found, pick the first enabled version of the certificate (This can happen if it's a newly created certificate)
      if (certificateVersionWithPassedRolloverTime == null)
      {
        return await GetSigningCredentialsFromCertificateAsync(enabledCertificateVersions.First());
      }
      else
      {
        return await GetSigningCredentialsFromCertificateAsync(certificateVersionWithPassedRolloverTime);
      }
    }
  }

  public class AzureKeyVaultValidationKeysStore : KeyStore, IValidationKeysStore
  {
    private readonly IMemoryCache _cache;
    private readonly KeyVaultClient _keyVaultClient;
    private readonly string _vault;
    private readonly string _certificateName;

    public AzureKeyVaultValidationKeysStore(IMemoryCache memoryCache, KeyVaultClient keyVaultClient, string vault, string certificateName) : base(keyVaultClient, vault)
    {
      _cache = memoryCache;
      _keyVaultClient = keyVaultClient;
      _vault = vault;
      _certificateName = certificateName;
    }

    public async Task<IEnumerable<SecurityKey>> GetValidationKeysAsync()
    {
      // Try get the signing credentials from the cache
      if (_cache.TryGetValue("ValidationKeys", out List<SecurityKey> validationKeys))
        return validationKeys;

      validationKeys = new List<SecurityKey>();

      // Get all the certificate versions (this will also get the currect active version)
      var enabledCertificateVersions = await GetAllEnabledCertificateVersionsAsync(_certificateName);
      foreach (var certificateItem in enabledCertificateVersions)
      {
        // Add the security key to validation keys so any JWT tokens signed with a older version of the signing certificate
        validationKeys.Add(await GetSecurityKeyFromCertificateAsync(certificateItem));
      }

      // Add the validation keys to the cache
      var options = new MemoryCacheEntryOptions();
      options.AbsoluteExpiration = DateTime.Now.AddDays(1);
      _cache.Set("ValidationKeys", validationKeys, options);

      return validationKeys;
    }
  }

  public abstract class KeyStore
  {
    private readonly KeyVaultClient _keyVaultClient;
    private readonly string _vault;

    public KeyStore(KeyVaultClient keyVaultClient, string vault)
    {
      _keyVaultClient = keyVaultClient;
      _vault = vault;
    }

    internal async Task<List<Microsoft.Azure.KeyVault.Models.CertificateItem>> GetAllEnabledCertificateVersionsAsync(string certificateName)
    {
      // Get all the certificate versions (this will also get the currect active version)
      var certificateVersions = await _keyVaultClient.GetCertificateVersionsAsync(_vault, certificateName);

      // Find all enabled versions of the certificate and sort them by creation date in decending order 
      return certificateVersions
        .Where(certVersion => certVersion.Attributes.Enabled.HasValue && certVersion.Attributes.Enabled.Value)
        .OrderByDescending(certVersion => certVersion.Attributes.Created)
        .ToList();
    }
    internal async Task<SigningCredentials> GetSigningCredentialsFromCertificateAsync(Microsoft.Azure.KeyVault.Models.CertificateItem certificateItem)
    {
      var certificateVersionSecurityKey = await GetSecurityKeyFromCertificateAsync(certificateItem);
      return new SigningCredentials(certificateVersionSecurityKey, SecurityAlgorithms.RsaSha256);
    }
    internal async Task<SecurityKey> GetSecurityKeyFromCertificateAsync(Microsoft.Azure.KeyVault.Models.CertificateItem certificateItem)
    {
      var certificateVersionBundle = await _keyVaultClient.GetCertificateAsync(certificateItem.Identifier.Identifier);
      var certificatePrivateKeySecretBundle = await _keyVaultClient.GetSecretAsync(certificateVersionBundle.SecretIdentifier.Identifier);
      var privateKeyBytes = Convert.FromBase64String(certificatePrivateKeySecretBundle.Value);
      var certificateWithPrivateKey = new X509Certificate2(privateKeyBytes, (string)null, X509KeyStorageFlags.MachineKeySet);
      return new X509SecurityKey(certificateWithPrivateKey);
    }
  }
}

You use the code by calling one of the public methods above from "ConfigureServices" inside your startup.cs like this

var identityServerBuilder = services.AddIdentityServer();
identityServerBuilder.AddSigningCredentialFromAzureKeyVault(Configuration["AzureKeyVault:Url"], "<My Key vault client id>", "<My key vault secret>", "<My Cert Name>", <Signing Key Rollover period in hours>);

or if you are using MSI, you can do it like this:

var identityServerBuilder = services.AddIdentityServer();
identityServerBuilder.AddSigningCredentialFromAzureKeyVault(Configuration["AzureKeyVault:Url"], "<My Cert Name>", <Signing Key Rollover period in hours>);
@kroymann

This comment has been minimized.

Copy link

commented Jan 8, 2019

When Azure auto-renews a certificate (or if you manually create a new version of the cert), won't it immediately become the "current" version of the certificate, which according to this code would cause it to immediately become the new signing credential without first being exposed for a while as a validation key? Without that delay, remote clients won't have a chance to learn about the new key via the discovery document before they start receiving tokens that have been signed with the new key.

A reasonable solution would be to update this code so that it prefers to not use a new key for signing until it is at least X days old. If no alternative key is available (ie: if the new key is the only version of the key), then it will use the new key, but if an older version of the key is still available, then it will use the older key for signing until the new one has been around for a configurable number of days.

@Eneuman

This comment has been minimized.

Copy link

commented Jan 8, 2019

Yeah, I think you are right.
I’ll take a look at this and make the necessary adjustments to the code.

Thanks for pointing this out :)

@damienbod

This comment has been minimized.

Copy link
Owner Author

commented Jan 9, 2019

@Eneuman @kroymann Thanks for your help!

@Eneuman

This comment has been minimized.

Copy link

commented Jan 24, 2019

I have updated the code above and added the abillity to specify a singing key rollover period.
It will search the different versions of the certificate and preferable use the one where the rollover time has passed. If not found, it falls back to any enabled version.

@kroymann Please check and see if the code looks okey to you.

@kroymann

This comment has been minimized.

Copy link

commented Jan 24, 2019

That's reasonably close to what I have implemented in my codebase. There are two improvements that could be made:

  1. You're doing identical work to retrieve the complete list of certs and construct X509SecurityKeys from all of them in both the ValidationKeyStore and SigningCredentialStore classes. You could combine them into a single class that implements both interfaces, and which does all that work just once.
  2. Your logic for picking the best signing key credential should be explicitly trying to find the newest version of the cert that matches the required conditions, rather than simply the first version of the cert that matches them. That way, you ensure that new versions of the cert go into effect asap after the SigningKeyRollover period has elapsed. I don't know if there is any guarantee on the order in which the cert versions are returned by the key vault. If they come back sorted oldest to newest, then your current code will choose the oldest version of the cert (that could stop working at any moment) rather than the new version.

Here's what I've written for use in our project (based in part on what you originally wrote): https://gist.github.com/kroymann/72952c079dc46dad774b32d6f154404c

@Eneuman

This comment has been minimized.

Copy link

commented Jan 24, 2019

Thank you for the feedback. I have made the suggested changes to the code above.
Sorting is now handled by the function retrieving the certificate versions instead of relying on MS Api.
I cleanup the code and moved some functions into a abstract base class. I also choose to use LINQ for readabilty in some places. Performance isn't a issue here since it all beeing cached.

@abergs

This comment has been minimized.

Copy link

commented Jan 28, 2019

What's should i consider when picking a SigningKeyRollover length? Any recommended value?

@kroymann

This comment has been minimized.

Copy link

commented Jan 28, 2019

If the downstream services that are receiving and validating your signed tokens are AspNetCore applications using the built-in authentication library, then I believe the default rate at which they will refresh the discovery document is 24 hours. This means that you will need to guarantee that the new key has been exposed as a validation key in the discovery document for at least 24 hours before you start using it to sign anything. Next, if you are using @Eneuman's code from above, you'll see that the values pulled from the AzureKeyVault are cached in memory and are only refreshed every 24 hours, which means the new cert could be 24 hours old before it is ever exposed via the discovery document. Combine those two together and you get a minimum SigningKeyRollover of 48 hours.

@Eneuman

This comment has been minimized.

Copy link

commented Jan 28, 2019

Also note that if you using the code above, a good practice would be to go into azure key vault and mark the old certificate version as disabled when you are sure all the downstream services have started using the new certificate (rollover time has passed + a day or two).
But this will invalidate any access token signed with the old version of the certificate, so if you are using long lived access token, you need your old certificate to be valid and enabled until the access token time to live + SigningKeyRollover has passed.

@abergs

This comment has been minimized.

Copy link

commented Jan 28, 2019

Thank you, that was a good explanation.

@gcbenjamin

This comment has been minimized.

Copy link

commented Mar 1, 2019

Thanks for the code. So what is the recommended time to set in Azure KeyVault to set the certificate to auto-renew and based off that time, what is the recommended SigningKeyRollover?

@kroymann I see you've used a transient lifetime for the ISigningCredentialStore and IValidationKeysStore whereas @Eneuman has used a singleton. In my mind singleton would be more appropriate for this?

@sguryev

This comment has been minimized.

Copy link

commented Mar 18, 2019

I have also noticed that @Eneuman version doesn't enumerate pages via GetCertificateVersionsNextAsync()

@Eneuman

This comment has been minimized.

Copy link

commented Mar 18, 2019

@sguryev GetCertificateVersionsAsync will retrieve up to 25 versions of the certificate. When using this extension, a best practice would be to remove the expired version from azure key vault after it’s no longer needed (see expire time in previous post).
I choose to use GetCertificateVersionsAsync since in my case, I would never reach 25 versions of the same certificate so enumeration was not needed.

@Eneuman

This comment has been minimized.

Copy link

commented Mar 18, 2019

@gcbenjamin It really depends on your security requirements and how much you are willing to spend on certificates :)
As a base I would say:
1 year renewal for certificate.
Automatically renew it after 11 months.
Set rollover time to 3 weeks.
If you are using long lived access tokens you might need to adjust this.

Also make sure to add a health check that validates that the certificate you are using is not going to expire in 3 weeks. Expired CCs is no fun ;)

@kroymann

This comment has been minimized.

Copy link

commented Apr 28, 2019

@kroymann I see you've used a transient lifetime for the ISigningCredentialStore and IValidationKeysStore whereas @Eneuman has used a singleton. In my mind singleton would be more appropriate for this?

@gcbenjamin Sorry for the delayed response (the notification from GitHub ended up in my spam folder for some reason). The lifetime of those services as implemented in my gist isn't super important as there's no stored state. Singleton would also be fine, but it doesn't really matter too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.