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

"Sign in With Apple" provider #318

Merged
merged 26 commits into from Sep 20, 2019

Conversation

@martincostello
Copy link
Member

@martincostello martincostello commented Jun 6, 2019

Adds a Sign In with Apple provider based on currently available information.

Reference sources were:

Relates to #314.

All the provider does at present is set the NameIdentifier claim in the claims principal to the value of the subject of the JWT returned from the token endpoint.

No information about a user information endpoint is yet available, but the docs suggest this will come at a later point:

access_token string
(Reserved for future use) A token used to access allowed data. Currently, no data set has been defined for access.

@martincostello
Copy link
Member Author

@martincostello martincostello commented Jun 8, 2019

Working demo now available at https://signinwithapple.azurewebsites.net/

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Jun 9, 2019

Note that in Azure App Service, you have to set WEBSITE_LOAD_USER_PROFILE=1 to be able to load the private key from the .p8 file.

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Jun 9, 2019

Works on Windows but fails on macOS and Linux. Needs refactoring to account for CngKey.Import() being Windows-specific: https://github.com/dotnet/corefx/issues/18733#issuecomment-296723615.

Loading

@kinosang
Copy link
Contributor

@kinosang kinosang commented Jun 9, 2019

Have you try with X509Certificate2? It can parse pkcs8 or pkcs12, and provide RsaPrivateKey and RsaPublicKey.

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Jun 9, 2019

Yeah, I found a comment that lead me to X509Certificate2 for if the private key is in .pfx format.

private ECDsa CreateAlgorithm(byte[] keyBlob, string password)
{
// This becomes xplat in .NET Core 3.0: https://github.com/dotnet/corefx/pull/30271
return RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ?
CreateAlgorithmWindows(keyBlob) :
CreateAlgorithmLinuxOrMac(keyBlob, password);
}
private ECDsa CreateAlgorithmLinuxOrMac(byte[] keyBlob, string password)
{
// Does not support .p8 files in .NET Core 2.x as-per https://github.com/dotnet/corefx/issues/18733#issuecomment-296723615
// Unlike Linux, macOS does not support empty passwords for .pfx files.
using (var cert = new X509Certificate2(keyBlob, password))
{
return cert.GetECDsaPrivateKey();
}
}
private ECDsa CreateAlgorithmWindows(byte[] keyBlob)
{
// Only Windows supports .p8 files in .NET Core 2.0 as-per https://github.com/dotnet/corefx/issues/18733
using (var privateKey = CngKey.Import(keyBlob, CngKeyBlobFormat.Pkcs8PrivateBlob))
{
return new ECDsaCng(privateKey) { HashAlgorithm = CngAlgorithm.Sha256 };
}
}

For now I've forked the implementation so Windows supports .p8 (as that's the format Apple provide the user with so is the least-work to integrate), but Linux and macOS only support .pfx so the user needs to extract the .cer from the .p8 and then combine them into a .pfx using OpenSSL.

Now the tests pass: https://travis-ci.com/martincostello/AspNet.Security.OAuth.Providers/builds/114862873

.NET Core 3.0 supports .p8 on all three platforms, so the implementation can be simplified for 3.0 and the #280 branch, but I imagine if the community want to use this when Apple release the product, they'd rather have the option of 2.x or 3.0, rather than having to port their app to 3.0 just to be able to use Sign In with Apple.

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Jun 9, 2019

ASP.NET Core 3.0 branch of the Apple provider is here.

The code is simpler and supports .p8 private keys on all platforms: ee4a2f7

Tests: https://travis-ci.com/martincostello/AspNet.Security.OAuth.Providers/builds/114865903

Loading

@martincostello martincostello self-assigned this Jun 9, 2019
@martincostello
Copy link
Member Author

@martincostello martincostello commented Jun 10, 2019

I've done a blog post about the implementation here: https://blog.martincostello.com/sign-in-with-apple-prototype-for-aspnet-core/

Loading

@epoyraz
Copy link

@epoyraz epoyraz commented Aug 5, 2019

@martincostello any update on this issue?

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Aug 5, 2019

@epoyraz None as of yet, unless Apple have updated anything since the initial announcement?

Loading

@epoyraz
Copy link

@epoyraz epoyraz commented Aug 5, 2019

@martincostello ah, i see. Thanks for the info! 👍

Loading

@dylanbevandotnet
Copy link

@dylanbevandotnet dylanbevandotnet commented Aug 12, 2019

Hey @martincostello I'm not sure on how the non-windows flow works with the pfx. It looks like setting the key name to a key.pfx instead of key.p8 will still attempt to read the cert content as a base64 string which fails. Could you please give some guidance on how it's intended to work? If I manually read the cert via 509Certificate2 passing in the password and using that rawBytes value as the base64 string I get a crypto failure later on

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Aug 13, 2019

Hi @dylanbevandotnet - does the Cross-platform Support section of my blog post explain things?

Essentially, the underlying cryptographic APIs in .NET Core 2.2 behave differently, so there's a different approach per-platform.

This won't be a problem in 3.0 as the APIs have feature parity in this area.

Loading

@dylanbevandotnet
Copy link

@dylanbevandotnet dylanbevandotnet commented Aug 13, 2019

@martincostello not really I'm afraid. I don't see how a password protected pfx is read in as a base64 string. Following your blog I would expect that I replace the UsePrivateKey delegate to point to the pfx instead of the p8, and supply the certificate password. However doing that will cause the exception to be thrush in the extension method as it tries to convert that pfx content to a base 64 string.

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Aug 13, 2019

Is it possible you've generated the pfx file in an incorrect format?

Does your pfx look like a similar file format to the one in the tests? https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/99dadcdba64b3a8b8ed1e477c54e052abfb68e9f/test/AspNet.Security.OAuth.Providers.Tests/Apple/test.pfx

Loading

@dylanbevandotnet
Copy link

@dylanbevandotnet dylanbevandotnet commented Aug 13, 2019

@martincostello so the root issue is that if you use a non Windows os you have to set the PrivateKeyBytes delegate because if you just set the UsePrivateKey delegate it'll automatically assume that you're not using a pfx because it contains a call to ConvertFromBase64String. If the code in UsePrivateKey were to catch a System.FormatException it could then fall back to just returning the raw bytes from the file. From an end user's view point it seems nicer that I can point to either a p8 or a pfx and the code will figure out the right thing to do.

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Aug 13, 2019

Catching an exception at runtime seems a bit bleurgh, and ultimately is a hack to workaround the different API surfaces.

For 2.2, the caller is the owner of the key material and knows their own runtime environment, so should make the appropriate decision on how to configure things.

There's an equally-hacky way to do this here: https://github.com/martincostello/SignInWithAppleSample/blob/50d0c6eb04dca53d0cd69f16d3005bf631291af0/src/SignInWithApple/Apple/AppleAuthenticationOptionsExtensions.cs#L39-L57

For 3.0 the need to do that becomes redundant as all platforms can natively use the .p8 file Apple provide with no extra complexity. See here for the changes: ee4a2f7

That said, I can always re-review things once I do further work on this whenever Apple provide the capability to get the user claims for the token and this code can progress further towards being feature-complete.

Loading

@dylanbevandotnet
Copy link

@dylanbevandotnet dylanbevandotnet commented Aug 13, 2019

Sounds reasonable. Thanks @martincostello

Loading

@kevinchalet
Copy link
Member

@kevinchalet kevinchalet commented Aug 13, 2019

@martincostello is there any x-plat issue preventing you from using X509Certificate2 everywhere?

Importing the private key bytes certainly works, but you won't be able to use HSM-backed EC certificates using this approach.

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Aug 13, 2019

@PinpointTownes There was an issue with using a pfx cert with no password on it (admittedly, not a secure practice, my test cert found it), so it wasn't 100% compatible. That's what lead to me forking the setup code for 2.2 after I hit a brick wall trying to get .p8 to work in the first place.

The 3.0 version can be nicer as it can use the .p8 file Apple provide from the developer portal that doesn't have a password set on it on Windows, macOS and Linux, so is simpler to configure.

Relevant excerpt from my blog post:

Once the Client Secret was being generated successfully from the .p8 file as-needed, I figured the bulk of the implementation was pretty much done.

As it turned out, I'd made some assumptions in the implementation from working on my Windows 10 laptop that meant that the provider only worked on Windows and not on Linux or macOS, which is a bit embarrassing for integrating with an Apple product, let alone on cross-platform .NET Core.

First this required changing the code so it could use more generic ECDA APIs to load the .p8 private key from Linux and macOS. A bit of Google-fu later lead to me finding this issue in Core CLR. It turns out that PKCS #8 keys aren't supported in .NET Core 2.x on non-Windows platforms. This is fixed in .NET Core 3.0, but that doesn't do us any good right now.

To fix this I followed these instructions to generate a .pfx file (PKCS #12) from the .p8 file and then use the X509Certificate2 class to load the key instead (commit) on non-Windows platforms. It's a bit bleurgh to have to branch the code based on the operating system and require a different key format, but without pulling in a lot of extra code I didn't think it was worth it.

Unfortunately, that didn't fix everything either. It fixed Linux but macOS was still broken. This time it was because of this issue where macOS cannot open private keys with no password set.

This required me to add a further option to support specifying a password for the certificate, which on reflection I should have done anyway. I was just being lazy in my tests.

With that change done, finally everything was working as expected on both Windows, Linux and macOS!

Loading

@dylanbevandotnet
Copy link

@dylanbevandotnet dylanbevandotnet commented Sep 6, 2019

Looks like Apple have changed something. I now see response_mode must be form_post when name or email scope is requested.

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Sep 6, 2019

Yeah, my test site has stopped working too. I’ll have to investigate when I get the time over the weekend.

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Sep 8, 2019

So the response_mode error is resolved by adding this to AppleAuthenticationHandler, but then it fails after sign-in with: "The oauth state was missing or invalid.".

protected override string BuildChallengeUrl(AuthenticationProperties properties, string redirectUri)
{
    string challengeUrl = base.BuildChallengeUrl(properties, redirectUri);
    return QueryHelpers.AddQueryString(challengeUrl, "response_mode", "form_post");
}

Loading

Fix the build by enabling the latest version of C#.
Get the user's name and email address, if available, as claims after signing in with an Apple ID. These details are only available the first time the user signs in; if they are not persisted they cannot currently be obtained again.
@martincostello martincostello force-pushed the Sign-In-With-Apple branch from a5df29a to 4a93eb6 Sep 8, 2019
@dylanbevandotnet
Copy link

@dylanbevandotnet dylanbevandotnet commented Sep 8, 2019

Thanks @martincostello you're a star. This whole thing reminds me of the xkcd 'one more standard' comic

Loading

@YuriiNskyi
Copy link

@YuriiNskyi YuriiNskyi commented Sep 9, 2019

This pull request is great and completely fulfills requirements to use Apple Sign In, but are there any plans to finally merge it, due to upcoming Apple Event WWDC 19?

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Sep 9, 2019

@YuriiNskyi There are no plans to merge this PR or release the provider until after Apple officially launch the service and the API surface is definitely stable.

Loading

@YuriiNskyi
Copy link

@YuriiNskyi YuriiNskyi commented Sep 9, 2019

@martincostello That's very disappointing fact, checking the last paragraph in this news, we want to be prepared with already implemented Apple Sign In, when it goes to be commercially available.

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Sep 9, 2019

@YuriiNskyi You're welcome to build the provider from source if you want to ship early, but this won't be merged until the Apple API surface is declared stable.

As you can see, I've already had to make breaking changes to the provider code over the weekend, and Apple's documentation is quite lacking and hasn't been updated since Sign In with Apple was first announced.

Loading

@martincostello martincostello marked this pull request as ready for review Sep 9, 2019
Use "Sign in with Apple" instead of "Sign In with Apple".
Use the same approach as the other OAuth handlers and access the Events property via the Options property.
Remove TODO comment.
Check whether Trace logging is enabled before logging the Apple token response.
@martincostello martincostello changed the title [WIP] "Sign in With Apple" provider "Sign in With Apple" provider Sep 15, 2019
@martincostello
Copy link
Member Author

@martincostello martincostello commented Sep 15, 2019

With iOS 13 scheduled for release next Thursday (the 19th September), I think this is basically "done" now and the service is probably unlikely to change further, at least as an initial release.

As this one's more involved that the other providers, would you mind giving this a review please @PinpointTownes if you get some time this coming week?

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Sep 20, 2019

I’ll look into merging this either later today or over the weekend so it can also be incorporated into the 3.0.0 branch for Monday.

Loading

Comment out the Apple provider as it causes the application to fail to start if the values aren't set and/or the key file does not exist.
@martincostello martincostello merged commit 15078f4 into aspnet-contrib:dev Sep 20, 2019
2 checks passed
Loading
@martincostello martincostello deleted the Sign-In-With-Apple branch Sep 20, 2019
@martincostello
Copy link
Member Author

@martincostello martincostello commented Sep 20, 2019

A prerelease version of this package (AspNet.Security.OAuth.Apple 2.1.1-preview-0882) is available in MyGet.

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Sep 20, 2019

There's also a version available for ASP.NET Core 3.0 RC1: 3.0.0-rc1.19470.58

Loading

@dylanbevandotnet
Copy link

@dylanbevandotnet dylanbevandotnet commented Oct 11, 2019

Loading

@suencien
Copy link

@suencien suencien commented Apr 19, 2020

Hi Martin, this is awesome! Will you be able to help? I'm using OpenId Connect at ASP.NET Web Forms to integrate with Sign in with Apple?

I've got it to authenticate, but when returning to the Return URI https://iluvrun.com/signin-apple, it hits the 404 error. How do I get the site to handle this?

Loading

@martincostello
Copy link
Member Author

@martincostello martincostello commented Apr 19, 2020

Hi Martin, this is awesome! Will you be able to help? I'm using OpenId Connect at ASP.NET Web Forms to integrate with Sign in with Apple?

I've got it to authenticate, but when returning to the Return URI https://iluvrun.com/signin-apple, it hits the 404 error. How do I get the site to handle this?

The provider is designed for ASP.NET Core, so I'm not sure how much success you'd get trying to get it to work as part of a ASP.NET Web Forms application.

The endpoints are provided as middleware, so I'm not sure how you'd go about trying to run that as part of a Web Forms app. I haven't worked with Web Forms since about 2013, so I don't really have the experience or bandwidth to help you out trying to get it to work with it either I'm afraid!

Loading

@suencien
Copy link

@suencien suencien commented Apr 20, 2020

Hi Martin, it's ok. It's been a great reference. I'll keep searching for the answer :)

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants