Simplify PEM loading

[martincostello]

Simplify the code to load the PEM file for Sign in with Apple after feedback on dotnet/runtime#53807.

The Apple provider already has some breaking changes for .NET 6, so would be a good time to try and simplify it as well to leverage the new PEM support added in .NET 5.

Leaving as draft for now, as if we make this change it might be worth seeing if it can leverage the forthcoming Secret<T> type as it's a key (dotnet/designs#147 (comment), if this is a good use case) which would make it something like this instead:

public Func<string, CancellationToken, Task<Secret<char>>>? PrivateKey { get; set; }

which would then be used at the last-minute like this:

using var pem = await context.Options.PrivateKey!(context.Options.KeyId!, context.HttpContext.RequestAborted);
// ...
algorithm.ImportFromPem(pem.RevealToString());

Might be overkill as it needs to read off disk as a string anyway to use the ImportFromPem() method.

[martincostello]

Secret<T> has been pushed out into 7.0 now. I'll circle back to the original change soon and add into the 6.0 branch.

[martincostello]

Tested with my sample app in Azure with an RC1 daily build and Azure Key Vault and works as expected. Also makes it neater to configure getting the secret from Azure:

using AspNet.Security.OAuth.Apple;
using Azure;
using Azure.Security.KeyVault.Secrets;

namespace Microsoft.Extensions.DependencyInjection;

internal static class AppleAuthenticationOptionsExtensions
{
    public static AppleAuthenticationOptions UseAzureKeyVaultSecret(
        this AppleAuthenticationOptions options,
        Func<string, CancellationToken, Task<Response<KeyVaultSecret>>> secretProvider)
    {
        options.GenerateClientSecret = true;
        options.PrivateKey = async (keyId, cancellationToken) =>
        {
            var secret = await secretProvider(keyId, cancellationToken);
            return secret.Value.Value;
        };

        return options;
    }
}

[kevinchalet]

Personally, I've never been a huge fan of representing crypto material as "simple" strings (unpopular opinion: I liked SecureString 😄). Should we instead directly return an ECDsa instance here? Or an AsymmetricAlgorithm if we want to future-proof that delegate to support other algorithms (we never know, Apple may switch back to the good ol' RSA 😄).

If none of these options work for you, I'd probably return a ReadOnlyMemory<char> here.

[martincostello]

The idea behind this API is/was to just get the raw key material out of the file you get from Apple, but do it in a non-implementation specific way, so you could get it from disk, from Azure Key Vault, from an env var, etc. etc.

The ECDsa is then created internally from the PEM returned above.

https://github.com/martincostello/AspNet.Security.OAuth.Providers/blob/077853134729f67606d710998c77c8f5ea032e71/src/AspNet.Security.OAuth.Apple/Internal/DefaultAppleClientSecretGenerator.cs#L96-L125

Originally I did it with byte[] as it was the lowest-common-denominator, but that lead to ugly hacks like this to be able to create the algorithm from the certificate file:

AspNet.Security.OAuth.Providers/src/AspNet.Security.OAuth.Apple/AppleAuthenticationOptionsExtensions.cs

Lines 43 to 51 in 6759e8d

	string privateKey = await reader.ReadToEndAsync();
	if (privateKey.StartsWith("-----BEGIN PRIVATE KEY-----", StringComparison.Ordinal))
	{
	string[] lines = privateKey.Split('\n');
	privateKey = string.Join(string.Empty, lines[1..^1]);
	}
	return Convert.FromBase64String(privateKey);

So that's why I wanted to swap it out string so I could just use ImportFromPem(). I did try with ReadOnlySpan<char>, but it wouldn't let me use that. I didn't think of ReadOnlyMemory<char> though, and locally that seems to work OK.

I'd be happy to change it to ReadOnlyMemory<char> if it's not "too exotic" 😄.

The only downside is that it's slightly less intuitive to use in the Key Vault use case (call to AsMemory() at the end):

public static AppleAuthenticationOptions UseAzureKeyVaultSecret(
    this AppleAuthenticationOptions options,
    Func<string, CancellationToken, Task<Response<KeyVaultSecret>>> secretProvider)
{
    options.GenerateClientSecret = true;
    options.PrivateKey = async (keyId, cancellationToken) =>
    {
        var secret = await secretProvider(keyId, cancellationToken);
        return secret.Value.Value.AsMemory();
    };

    return options;
}

[kevinchalet]

The idea behind this API is/was to just get the raw key material out of the file you get from Apple, but do it in a non-implementation specific way, so you could get it from disk, from Azure Key Vault, from an env var, etc. etc.

Wouldn't moving the responsibility to create the ECDsa to the delegate implementer achieve the same goal?

With your example, it would likely look like something like this:

public static AppleAuthenticationOptions UseAzureKeyVaultSecret(
    this AppleAuthenticationOptions options,
    Func<string, CancellationToken, Task<Response<KeyVaultSecret>>> secretProvider)
{
    options.GenerateClientSecret = true;
    options.PrivateKey = async (keyId, cancellationToken) =>
    {
        var secret = await secretProvider(keyId, cancellationToken);
        var algorithm = ECDsa.Create();
        algorithm.ImportFromPem(secret.Value.Value);
        return algorithm;
    };

    return options;
}

[martincostello]

While that would work, that just seems like it's offloading more code onto the client by expecting them to also provide the algorithm, rather than just configure the secret.

Given the Apple provider is already "more complicated" that the other popular ones (e.g. Facebook and Google), I wanted to keep it relatively easy for people to just plug-in and go, rather than expose them to the underlying mechanics of how it works.

Today it's pretty much "tell me where the secret is, I'll do the rest". With the above it turns into "do some crypto stuff you might not be familiar with and then I'll use it". I was aiming to find the sweet spot between ease of use and extensibility, and I think exposing the algorithm is a step too far away from usability for the average dev as it's making it less of a black box to deviate even slightly from the default built-in usage mode of providing the file path.

Maybe we could add more built-in extension methods to turn a string/ROM into the algorithm for you as a building block, but exposing the algorithm on the options feels a bit too lower-level to me, as if you wanted to go super-off-the-beaten-path, you could just implement your own AppleClientSecretGenerator anyway.

[kevinchalet]

Makes sense, let's opt for ReadOnlyMemory<char> then 😄

Since you mentioned Azure Key Vault, it's interesting to note that since we're forced to have the private key material, we can't support scenarios where the key material is stored in a HSM and can't be exported (they call that "hardware keys"). But you're likely right, it's an advanced scenario.

[martincostello]

Updated to ReadOnlyMemory<char> and I made some updates to the documentation too.