nhosoya / omniauth-apple

OmniAuth strategy for Sign In with Apple
MIT License
260 stars 99 forks source link

nonce is optional in callback #102

Open btalbot opened 1 year ago

btalbot commented 1 year ago

In releases before 1.3, the nonce appearing in the id_token was optional and if present was required to be equal to the nonce in the session['omniauth.nonce'] value. That changed in the 1.3 release where the nonce is now required in the id_token and is an error when missing.

My reading of the apple developer docs is that the nonce is an optional feature and may not appear in the id_token. Their wording makes it sounds like the nonce might be useful for locating a user session if the session cookie wasn't received for some reason (like maybe with non-lax cookies and their POST from a different origin).

https://developer.apple.com/documentation/sign_in_with_apple/sign_in_with_apple_rest_api/authenticating_users_with_sign_in_with_apple

nonce
A string for associating a client session with the identity token. This value mitigates replay attacks and is present only if you pass it in the authorization request.

nonce_supported
A Boolean value that indicates whether the transaction is on a nonce-supported platform. If you send a nonce in the authorization request, but don’t see the nonce claim in the identity token, check this claim to determine how to proceed. If this claim returns true, treat nonce as mandatory and fail the transaction; otherwise, you can proceed treating the nonce as optional.

The nonce can be made to be optional and still enforce the expected value by simply not first requiring that the id_token[:nonce] value exists.

diff --git a/lib/omniauth/strategies/apple.rb b/lib/omniauth/strategies/apple.rb
index 5ad3a40..faf8e0e 100644
--- a/lib/omniauth/strategies/apple.rb
+++ b/lib/omniauth/strategies/apple.rb
@@ -125,7 +125,7 @@ module OmniAuth
       end

       def verify_nonce!(id_token)
-        invalid_claim! :nonce unless id_token[:nonce] && id_token[:nonce] == stored_nonce
+        invalid_claim! :nonce unless id_token[:nonce] == stored_nonce
       end

       def invalid_claim!(claim)

I have a spec test for this path coded up but it doesn't fit very nicely into the current test descriptions and contexts as most of those all force a nonce to be present. But here is a spec test that fails without the patch above and passes with it.

diff --git a/spec/omniauth/strategies/apple_spec.rb b/spec/omniauth/strategies/apple_spec.rb
index 2709d1b..10e56ec 100644
--- a/spec/omniauth/strategies/apple_spec.rb
+++ b/spec/omniauth/strategies/apple_spec.rb
@@ -79,6 +79,16 @@ describe OmniAuth::Strategies::Apple do
     WebMock.reset!
   end

+  it 'allows optional nonce' do
+    subject.authorize_params # initializes env, session (for test_mode) and populates 'nonce', 'state'
+    subject.session.delete('omniauth.nonce')  # if no nonce in session => nonce in id_token must also be missing
+    request.params.merge!('id_token' => id_token)
+    expect(valid_id_token_payload).not_to have_key(:nonce)
+    expect { subject.info }.not_to raise_error
+    expect(subject.extra.dig(:raw_info, :id_info, 'nonce_supported')).to eq true
+    expect(subject.extra.dig(:raw_info, :id_info, 'nonce')).to be nil
+  end
+
   describe '#client_options' do
     it 'has correct site' do
       expect(subject.client.site).to eq('https://appleid.apple.com')
btalbot commented 1 year ago

In our use case, the iOS developers choose to use the hybrid client-server flow where the client never makes the 'authorize' phase request but instead makes the request using iOS SDK libraries. The app then submits the code to the callback directly. In this flow there is no state and no nonce in the id_token.

nov commented 1 year ago

when nonce is not supported, it shouldn't be verified. when nonce is supported, and it's included in the original request, it should be veerified. it's current implementation

nov commented 1 year ago

maybe your iOS app is just missing to pass the nonce in the request.

btalbot commented 1 year ago

I'm not the iOS developer but they claim that there is no place to pass one ... and where would they get the nonce from?

btalbot commented 1 year ago

when nonce is not supported, it shouldn't be verified. when nonce is supported, and it's included in the original request, it should be veerified.

The issue is that in 1.3, it was changed so that: when the nonce is not in the session and not in the id-token it's an error

nov commented 1 year ago

when the nonce is not in the session and not in the id-token, this should be true. so, nonce is in the session in your case, I guess.

id_token[:nonce] == stored_nonce
btalbot commented 1 year ago

If that were the only logic then my use case would work and the new spec test would pass; however, that is not what the current implementation is. The current implementation FIRST requires that the id_token[:nonce] value be truthy.

https://github.com/nhosoya/omniauth-apple/blob/master/lib/omniauth/strategies/apple.rb#L127-L129


      def verify_nonce!(id_token)
        invalid_claim! :nonce unless id_token[:nonce] && id_token[:nonce] == stored_nonce
      end
nov commented 1 year ago

ah, then you need nonce.

bvogel commented 1 year ago

I fixed this by instead of setting the whole session to be same_site: :none through a short lived cookie

edit: added a PR for this option as well

santiagendrix commented 8 months ago

According to my research, the nonce is only accessible when authentication occurs through the REST API or Apple JS. However, on other platforms like Native iOS, defining the nonce is not possible.

Complicating matters, the callback attempts to verify the nonce based on the 'nonce_supported: true' sent by the JWT. In reality, though, the 'nonce' is not present.

Unless there is a way to correct the 'nonce_supported' field, I don't believe the nonce should be validated if it is not present.

bvogel commented 8 months ago

Well, the nonce is present in the response but fails the verification from the session if the SameSite session option is set to anything but :none due to Apple returning to the callback as a POST request creating a new session in the context. So nonce is supported and will work if the nonce value is stored differently to the default rails setup, hence the suggested fix by creating a short lived SameSite: :none cookie to store the nonce value.

The probable rejection to that PR is most likely the added :ignore option that will allow to bypass nonce validation, something along the lines what you are suggesting or many, many people are doing in their forks.

btalbot commented 8 months ago

According to my research, the nonce is only accessible when authentication occurs through the REST API or Apple JS. However, on other platforms like Native iOS, defining the nonce is not possible.

I believe this to be correct as well. I'm not an iOS developer, but this is what our frontend devs confirmed to me. I don't have any documentation for this though. My guess is that these maintainers only use web REST API and never use the native ios calls so they can't understand why a nonce might be missing.

So, two issues.

1) nonce not possible in some cases. specifically when using ios native calls to signin with apple 2) confirming nonce (when present or not) currently too difficult as it is hard to properly receive. Possible work-arounds currently include:

santiagendrix commented 8 months ago

Well, the nonce is present in the response but fails the verification from the session if the SameSite session option is set to anything but :none due to Apple returning to the callback as a POST request creating a new session in the context. So nonce is supported and will work if the nonce value is stored differently to the default rails setup, hence the suggested fix by creating a short lived SameSite: :none cookie to store the nonce value.

the suggested fix doesn't work for iOS native apps, the methodnew_nonce is never called.