oauth: add sign-in with apple support (fixes #110) #243

AlexCuse · 2024-02-22T01:31:57Z

Implements an oauth provider for sign-in with apple. Requires some
additional flexibility in oauth credentials / providers:

Include a map of additional data in credentials.
Allow providers to override secret behavior - configured secret in
apple credentials is a private key used to sign a secret calculated
at runtime, in this case a JWT that includes additional data as claims.
Allow providers to accept returns as HTTP POST instead of GET.
Allow providers to add additional oauth options to authorization
request.

lib/oauth/apple/privkey_test.go

AlexCuse · 2024-02-24T21:46:35Z

One thing I noticed so far trying to test is that Apple doesn't like including the scopes in the auth request unless response_mode=form_post is specified. Should be pretty straightforward but guess we'll need a post handler for oauth return 😢

AlexCuse · 2024-03-22T15:17:11Z

One thing I noticed so far trying to test is that Apple doesn't like including the scopes in the auth request unless response_mode=form_post is specified. Should be pretty straightforward but guess we'll need a post handler for oauth return 😢

We just needed to redirect POSTs to the existing handler and allow providers to override the method for routing.

diegosperes · 2024-03-23T12:56:59Z

lib/oauth/apple.go

+	teamID, keyID, expiresIn, constructErr := apple.ExtractCredentialData(credentials.Additional)
+	if constructErr != nil {
+		return nil, fmt.Errorf("apple credentials: %w", constructErr)
+	}
+
+	signingKey, publicKey, constructErr := apple.ParsePrivateKey(credentials.SigningKey, keyID)
+	if constructErr != nil {
+		return nil, fmt.Errorf("failed to parse signing key: %w", constructErr)
+	}


Would be possible to move apple. ExtractCredentialData and apple.ParsePrivateKey to that scope?

The AppleProvider will not function without the necessary credentials and keys. By moving this logic to a higher scope, it will be clearer what needs to be modified, thereby preventing substantial changes to the API. Those values can be stored in the additional hash map.

Think I prefer leaving it in provider initialization and keeping the app as dumb as possible.

diegosperes · 2024-03-23T13:43:59Z

lib/oauth/apple/idtoken_test.go

+	"golang.org/x/oauth2"
+)
+
+func testSigningKey(t *testing.T) (jose.SigningKey, string, *rsa.PrivateKey) {


This function will be accessible within the apple package. It might be more effective to encapsulate it within TestGetAppleIDTokenClaims by creating a variable of the function type.

Functions in _test files aren't callable like that but the change is easy enough to make.

diegosperes · 2024-03-23T13:46:10Z

lib/oauth/apple/idtoken_test.go

+	})
+}
+
+func Test_validateExp(t *testing.T) {


There is a typo in the test case name here, please update to use MixedCaps.

diegosperes · 2024-03-23T13:53:20Z

lib/oauth/provider.go

+	MethodOverride  string
+	VerificationKey jose.JSONWebKey
+}
+
 // NewProvider returns a properly configured Provider
 func NewProvider(config *oauth2.Config, userInfo UserInfoFetcher, signingKey jose.SigningKey) *Provider {


Instead of creating NewProviderWithOverrides, we could employ the functional options pattern. This approach would centralize the logic in one place, thereby enhancing maintainability.

diegosperes · 2024-03-23T13:56:47Z

lib/oauth/provider_test.go

+	"golang.org/x/oauth2"
+)
+
+func TestProvider_Secret(t *testing.T) {


There is a typo here in the name of test case "_".

diegosperes · 2024-03-23T14:05:57Z

server/public_routes.go

-	for providerName := range app.OauthProviders {
+	for providerName, provider := range app.OauthProviders {
+		var returnRoute *route.Route
+		if provider.ReturnMethod() == http.MethodPost {


For the sake of consistency, I suggest we implement both HTTP methods for all OAuth providers instead of making it conditional.

Think I prefer keeping the surface area as small as possible here.

consistency is one of my favorite words, and POST is more secure than the default GET, but it's hard to argue with minimizing the surface area.

Implements an oauth provider for sign-in with apple. Requires some additional flexibility in oauth credentials / providers: - Include a map of additional data in credentials. - Allow providers to override secret behavior - configured secret in apple credentials is a private key used to sign a secret calculated at runtime, in this case a JWT that includes additional data as claims. - Allow providers to accept returns as HTTP POST instead of GET. - Allow providers to add additional oauth options to authorization request.

cainlevy

I can't reasonably verify the Apple-specific details, so I tried to focus on changes outside of lib/oauth/apple as well as the JWT validation process.

Would you take a look at the comment on extractUserFromClaims specifically? The rest is aesthetic, I think.

docs/config.md

lib/oauth/credentials.go

cainlevy · 2024-03-28T04:47:09Z

server/public_routes.go

-	for providerName := range app.OauthProviders {
+	for providerName, provider := range app.OauthProviders {
+		var returnRoute *route.Route
+		if provider.ReturnMethod() == http.MethodPost {


consistency is one of my favorite words, and POST is more secure than the default GET, but it's hard to argue with minimizing the surface area.

lib/oauth/apple/idtoken.go

cainlevy · 2024-03-28T05:34:27Z

lib/oauth/apple/idtoken.go

+	return claims, nil
+}
+
+func extractUserFromClaims(claims map[string]interface{}, clientID string) (string, string, error) {


can we use https://pkg.go.dev/github.com/go-jose/go-jose/v3/jwt#Claims.Validate for the standard iss, aud, and exp claims? then our code can focus on our unique values like sub and email.

Will see what I can do here. The problem is if we read into jwt.Claims we lose visibility into sub and email - but there might be something we can do with a custom Claims type in the apple package that embeds / implements its own json unmarshaler that lets us leverage it.

i'd normally verify claims like iss, aud, and exp during the decoding step. is it easier to get at sub and email if you move the others?

I kinda like how this turned out let me know what you think 95e1df3

cainlevy · 2024-03-28T05:36:08Z

lib/oauth/apple/pubkey.go

+type mockKeyStore struct {
+	keys map[string]*rsa.PublicKey
+}
+
+func (ks *mockKeyStore) get(keyID string) (*rsa.PublicKey, error) {
+	if key, ok := ks.keys[keyID]; ok {
+		return key, nil
+	}
+	return nil, &keyNotFoundError{keyID: keyID}
+}


should mockKeyStore be in package apple_test?

cainlevy · 2024-03-28T05:37:34Z

lib/oauth/apple/idtoken_test.go

@@ -0,0 +1,291 @@
+package apple


should this be package apple_test?

Its testing unexported functions that I'm not sure we need to export - but I did move the mock keyStore into the _test file.

Moving to apple/test probably works as well.

Actually this got a lot more practical when I implemented that claims suggestion.

Co-authored-by: Lance Ivy <[email protected]>

cainlevy

claims validation looks good to me!

cainlevy · 2024-03-31T14:25:31Z

lib/oauth/apple/claims.go

+		return fmt.Errorf("missing claim 'iat'")
+	}
+
+	// is default 1m leeway OK here?


i'd think so. if there's some network delay when the user returns to authn, this leeway should handle it. if the token is more than 1m too old, then someone's clock is out of sync and they're going to have bigger problems.

cainlevy · 2024-03-31T14:26:40Z

lib/oauth/apple/claims.go

+	return c.Claims.Validate(jwt.Expected{
+		Issuer:   BaseURL,
+		Audience: jwt.Audience{clientID},
+	})


i checked to confirm that it will validate exp and iat against time.Now() by default. 👍

AlexCuse commented Feb 22, 2024

View reviewed changes

lib/oauth/apple/privkey_test.go Outdated Show resolved Hide resolved

AlexCuse force-pushed the apple-oauth branch 2 times, most recently from 0159567 to 893e82d Compare March 22, 2024 09:56

AlexCuse force-pushed the apple-oauth branch from aa637ce to a965eef Compare March 22, 2024 16:39

diegosperes reviewed Mar 23, 2024

View reviewed changes

AlexCuse force-pushed the apple-oauth branch from 2927cfc to d983a19 Compare March 24, 2024 12:42

AlexCuse mentioned this pull request Mar 24, 2024

oauth: allow passing additional credential data to providers #241

Closed

AlexCuse force-pushed the apple-oauth branch from 05cd2e0 to e9c7ff8 Compare March 25, 2024 00:28

AlexCuse marked this pull request as ready for review March 25, 2024 00:44

diegosperes approved these changes Mar 25, 2024

View reviewed changes

AlexCuse requested a review from cainlevy March 26, 2024 00:38

AlexCuse force-pushed the apple-oauth branch from e9c7ff8 to 087b98b Compare March 26, 2024 10:59

AlexCuse force-pushed the apple-oauth branch from 087b98b to 646540d Compare March 27, 2024 00:51

cainlevy requested changes Mar 28, 2024

View reviewed changes

AlexCuse and others added 4 commits March 28, 2024 07:14

Apply suggestions from code review

7b0d5a8

Co-authored-by: Lance Ivy <[email protected]>

move mockKeyStore to test file

75ebde4

fix test + remove credential reference to signing key

5645188

define custom apple claims with validation

619b2e9

AlexCuse force-pushed the apple-oauth branch from 95e1df3 to 619b2e9 Compare March 29, 2024 01:20

AlexCuse requested a review from cainlevy March 29, 2024 01:26

cainlevy approved these changes Mar 31, 2024

View reviewed changes

comment cleanup

6377550

AlexCuse merged commit 3d109a8 into keratin:main Apr 3, 2024
2 checks passed

AlexCuse deleted the apple-oauth branch April 3, 2024 01:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oauth: add sign-in with apple support (fixes #110) #243

oauth: add sign-in with apple support (fixes #110) #243

AlexCuse commented Feb 22, 2024 •

edited

Loading

AlexCuse commented Feb 24, 2024 •

edited

Loading

AlexCuse commented Mar 22, 2024

diegosperes Mar 23, 2024

AlexCuse Mar 24, 2024

diegosperes Mar 23, 2024

AlexCuse Mar 24, 2024

diegosperes Mar 23, 2024

diegosperes Mar 23, 2024

AlexCuse Mar 24, 2024

diegosperes Mar 23, 2024

diegosperes Mar 23, 2024

AlexCuse Mar 24, 2024

cainlevy Mar 28, 2024

cainlevy left a comment

cainlevy Mar 28, 2024

cainlevy Mar 28, 2024

AlexCuse Mar 28, 2024

cainlevy Mar 28, 2024

AlexCuse Mar 29, 2024

cainlevy Mar 28, 2024

cainlevy Mar 28, 2024

AlexCuse Mar 28, 2024

AlexCuse Mar 29, 2024

cainlevy left a comment

cainlevy Mar 31, 2024

cainlevy Mar 31, 2024

oauth: add sign-in with apple support (fixes #110) #243

oauth: add sign-in with apple support (fixes #110) #243

Conversation

AlexCuse commented Feb 22, 2024 • edited Loading

AlexCuse commented Feb 24, 2024 • edited Loading

AlexCuse commented Mar 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cainlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cainlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexCuse commented Feb 22, 2024 •

edited

Loading

AlexCuse commented Feb 24, 2024 •

edited

Loading