diff --git a/lib/action_pack/passkey.rb b/lib/action_pack/passkey.rb index ace33176c..085960ec2 100644 --- a/lib/action_pack/passkey.rb +++ b/lib/action_pack/passkey.rb @@ -47,14 +47,13 @@ class ActionPack::Passkey < Rails.configuration.action_pack.passkey.parent_class # Verifies the attestation response from the browser and persists a new passkey record. # The +passkey+ hash should contain +client_data_json+, +attestation_object+, and +transports+ - # as submitted by the registration form. The +challenge+ defaults to - # +ActionPack::WebAuthn::Current.challenge+, which is automatically populated from the session - # by ActionPack::Passkey::Request. Any additional +attributes+ (e.g. +holder+) are passed - # through to +create!+. + # as submitted by the registration form. The challenge is extracted from the authenticator's + # +clientDataJSON+ response and verified server-side. Any additional +attributes+ (e.g. +holder+) + # are passed through to +create!+. # # Raises ActionPack::WebAuthn::InvalidResponseError if the attestation is invalid. - def register(passkey, challenge: ActionPack::WebAuthn::Current.challenge, **attributes) - credential = ActionPack::WebAuthn::PublicKeyCredential.register(passkey, challenge: challenge) + def register(passkey, **attributes) + credential = ActionPack::WebAuthn::PublicKeyCredential.register(passkey) create!(**credential.to_h, **attributes) end @@ -74,17 +73,17 @@ class ActionPack::Passkey < Rails.configuration.action_pack.passkey.parent_class # Looks up a passkey by credential ID and verifies the assertion response from the browser. # Returns the authenticated Passkey record, or +nil+ if the credential is not found or # verification fails. - def authenticate(passkey, challenge: ActionPack::WebAuthn::Current.challenge) - find_by(credential_id: passkey[:id])&.authenticate(passkey, challenge: challenge) + def authenticate(passkey) + find_by(credential_id: passkey[:id])&.authenticate(passkey) end end # Verifies the assertion response against this passkey's stored credential and updates the # +sign_count+ and +backed_up+ attributes. Returns +self+ on success, or +nil+ if the # response is invalid. - def authenticate(passkey, challenge: ActionPack::WebAuthn::Current.challenge) + def authenticate(passkey) credential = to_public_key_credential - credential.authenticate(passkey, challenge: challenge) + credential.authenticate(passkey) update!(sign_count: credential.sign_count, backed_up: credential.backed_up) self rescue ActionPack::WebAuthn::InvalidResponseError diff --git a/lib/action_pack/passkey/challenges_controller.rb b/lib/action_pack/passkey/challenges_controller.rb index 82886266a..3eca28236 100644 --- a/lib/action_pack/passkey/challenges_controller.rb +++ b/lib/action_pack/passkey/challenges_controller.rb @@ -5,9 +5,10 @@ # authentication ceremony so that the challenge is issued just-in-time rather # than embedded in the initial page load. # -# The generated challenge is stored in an encrypted, HTTP-only, same-site -# cookie and simultaneously returned in the JSON response body. The cookie is -# consumed by ActionPack::Passkey::Request on the subsequent form submission. +# The generated challenge is returned in the JSON response body. The challenge +# is a signed, expiring token that the server can verify on the subsequent +# form submission without needing server-side state — the challenge is +# extracted from the authenticator's +clientDataJSON+ response. # # == Route # @@ -15,17 +16,11 @@ # via +config.action_pack.passkey.routes_prefix+). # class ActionPack::Passkey::ChallengesController < ActionController::Base - COOKIE_NAME = :action_pack_passkey_challenge - include ActionPack::Passkey::Request - # Generates a fresh challenge, stores it in an encrypted cookie, and returns - # it as JSON. The cookie is consumed on the next passkey form submission. + # Generates a fresh challenge and returns it as JSON. def create - challenge = create_passkey_challenge - - cookies.encrypted[COOKIE_NAME] = { value: challenge, httponly: true, same_site: :lax, secure: !request.local? && request.ssl? } - render json: { challenge: challenge } + render json: { challenge: create_passkey_challenge } end private diff --git a/lib/action_pack/passkey/request.rb b/lib/action_pack/passkey/request.rb index 6e7832517..265b49bc6 100644 --- a/lib/action_pack/passkey/request.rb +++ b/lib/action_pack/passkey/request.rb @@ -43,9 +43,7 @@ # == Before Action # # Automatically populates +ActionPack::WebAuthn::Current+ with the request -# host, origin, and challenge (read from the encrypted cookie set by -# ChallengesController). The cookie is deleted after being read to prevent -# replay. +# host and origin. # module ActionPack::Passkey::Request extend ActiveSupport::Concern @@ -54,8 +52,6 @@ module ActionPack::Passkey::Request before_action do ActionPack::WebAuthn::Current.host = request.host ActionPack::WebAuthn::Current.origin = request.base_url - ActionPack::WebAuthn::Current.challenge = cookies.encrypted[ActionPack::Passkey::ChallengesController::COOKIE_NAME] - cookies.delete(ActionPack::Passkey::ChallengesController::COOKIE_NAME) end end diff --git a/lib/action_pack/web_authn/authenticator/assertion_response.rb b/lib/action_pack/web_authn/authenticator/assertion_response.rb index 6d4f07cd0..b8f1292ec 100644 --- a/lib/action_pack/web_authn/authenticator/assertion_response.rb +++ b/lib/action_pack/web_authn/authenticator/assertion_response.rb @@ -17,7 +17,6 @@ # authenticator_data: params[:response][:authenticatorData], # signature: params[:response][:signature], # credential: credential.to_public_key_credential, -# challenge: ActionPack::WebAuthn::Current.challenge, # origin: "https://example.com" # ) # diff --git a/lib/action_pack/web_authn/authenticator/attestation_response.rb b/lib/action_pack/web_authn/authenticator/attestation_response.rb index 58fdecacd..42ddda86f 100644 --- a/lib/action_pack/web_authn/authenticator/attestation_response.rb +++ b/lib/action_pack/web_authn/authenticator/attestation_response.rb @@ -9,7 +9,6 @@ # response = ActionPack::WebAuthn::Authenticator::AttestationResponse.new( # client_data_json: params[:response][:clientDataJSON], # attestation_object: params[:response][:attestationObject], -# challenge: ActionPack::WebAuthn::Current.challenge, # origin: "https://example.com" # ) # diff --git a/lib/action_pack/web_authn/authenticator/response.rb b/lib/action_pack/web_authn/authenticator/response.rb index af878405b..fabb07815 100644 --- a/lib/action_pack/web_authn/authenticator/response.rb +++ b/lib/action_pack/web_authn/authenticator/response.rb @@ -23,7 +23,6 @@ # authenticator_data: authenticator_data, # signature: signature, # credential: credential, -# challenge: ActionPack::WebAuthn::Current.challenge, # origin: "https://example.com", # user_verification: :required # ) @@ -34,9 +33,9 @@ class ActionPack::WebAuthn::Authenticator::Response include ActiveModel::Validations attr_reader :client_data_json - attr_accessor :challenge, :origin, :user_verification + attr_accessor :origin, :user_verification - validate :challenge_must_match + validate :challenge_must_be_present validate :challenge_must_not_be_expired validate :origin_must_match validate :must_not_be_cross_origin @@ -45,9 +44,8 @@ class ActionPack::WebAuthn::Authenticator::Response validate :user_must_be_present validate :user_must_be_verified_when_required - def initialize(client_data_json:, challenge: nil, origin: nil, user_verification: :preferred) + def initialize(client_data_json:, origin: nil, user_verification: :preferred) @client_data_json = client_data_json - @challenge = challenge @origin = origin @user_verification = user_verification.to_sym end @@ -76,20 +74,16 @@ class ActionPack::WebAuthn::Authenticator::Response end private - def challenge_must_match - if challenge.blank? + def challenge_must_be_present + if client_data["challenge"].blank? errors.add(:base, "Challenge missing") - elsif client_data["challenge"].blank? - errors.add(:base, "Challenge missing in client data") - elsif !ActiveSupport::SecurityUtils.secure_compare(challenge.to_s, client_data["challenge"].to_s) - errors.add(:base, "Challenge does not match") end end def challenge_must_not_be_expired - return if errors.any? || challenge.blank? + return if errors.any? - signed_message = Base64.urlsafe_decode64(challenge) + signed_message = Base64.urlsafe_decode64(client_data["challenge"]) unless ActionPack::WebAuthn.challenge_verifier.verified(signed_message) errors.add(:base, "Challenge has expired") diff --git a/lib/action_pack/web_authn/current.rb b/lib/action_pack/web_authn/current.rb index 7189a4dd5..77293e83d 100644 --- a/lib/action_pack/web_authn/current.rb +++ b/lib/action_pack/web_authn/current.rb @@ -14,10 +14,6 @@ # The expected origin (typically +request.base_url+). Validated against the # +origin+ field in the authenticator's client data. # -# [+challenge+] -# The Base64URL-encoded challenge read from the encrypted cookie. Validated -# against the +challenge+ field in the authenticator's client data. -# class ActionPack::WebAuthn::Current < ActiveSupport::CurrentAttributes - attribute :host, :origin, :challenge + attribute :host, :origin end diff --git a/lib/action_pack/web_authn/public_key_credential.rb b/lib/action_pack/web_authn/public_key_credential.rb index 647443ef5..5279f29c9 100644 --- a/lib/action_pack/web_authn/public_key_credential.rb +++ b/lib/action_pack/web_authn/public_key_credential.rb @@ -10,7 +10,6 @@ # # credential = ActionPack::WebAuthn::PublicKeyCredential.register( # params[:passkey], -# challenge: ActionPack::WebAuthn::Current.challenge, # origin: ActionPack::WebAuthn::Current.origin # ) # @@ -80,11 +79,10 @@ class ActionPack::WebAuthn::PublicKeyCredential # PublicKeyCredential with the registered credential data. # # Raises +InvalidResponseError+ if the attestation is invalid. - def register(params, challenge: ActionPack::WebAuthn::Current.challenge, origin: ActionPack::WebAuthn::Current.origin) + def register(params, origin: ActionPack::WebAuthn::Current.origin) response = ActionPack::WebAuthn::Authenticator::AttestationResponse.new( client_data_json: params[:client_data_json], attestation_object: params[:attestation_object], - challenge: challenge, origin: origin ) @@ -126,13 +124,12 @@ class ActionPack::WebAuthn::PublicKeyCredential # Updates +sign_count+ and +backed_up+ on success. # # Raises +InvalidResponseError+ if the assertion is invalid. - def authenticate(params, challenge: ActionPack::WebAuthn::Current.challenge, origin: ActionPack::WebAuthn::Current.origin) + def authenticate(params, origin: ActionPack::WebAuthn::Current.origin) response = ActionPack::WebAuthn::Authenticator::AssertionResponse.new( client_data_json: params[:client_data_json], authenticator_data: params[:authenticator_data], signature: params[:signature], credential: self, - challenge: challenge, origin: origin ) diff --git a/test/controllers/my/passkey_challenges_controller_test.rb b/test/controllers/my/passkey_challenges_controller_test.rb index 80f64298c..43e0fb5fb 100644 --- a/test/controllers/my/passkey_challenges_controller_test.rb +++ b/test/controllers/my/passkey_challenges_controller_test.rb @@ -10,15 +10,6 @@ class My::PasskeyChallengesControllerTest < ActionDispatch::IntegrationTest end end - test "stores challenge in cookie" do - untenanted do - post my_passkey_challenge_url - - jar = ActionDispatch::Cookies::CookieJar.build(request, cookies.to_hash) - assert_equal response.parsed_body["challenge"], jar.encrypted[ActionPack::Passkey::ChallengesController::COOKIE_NAME] - end - end - test "returns a different challenge each time" do untenanted do post my_passkey_challenge_url diff --git a/test/lib/action_pack/passkey_test.rb b/test/lib/action_pack/passkey_test.rb index 56dc49095..a2c6ad934 100644 --- a/test/lib/action_pack/passkey_test.rb +++ b/test/lib/action_pack/passkey_test.rb @@ -20,7 +20,7 @@ class ActionPack::PasskeyTest < ActiveSupport::TestCase challenge = ActionPack::Passkey.request_options(credentials: [ @passkey ]).challenge assertion = build_assertion(challenge: challenge) - result = @passkey.authenticate(assertion, challenge: challenge) + result = @passkey.authenticate(assertion) assert_equal @passkey, result end @@ -30,14 +30,14 @@ class ActionPack::PasskeyTest < ActiveSupport::TestCase assertion = build_assertion(challenge: challenge) assertion[:signature] = Base64.urlsafe_encode64("invalid", padding: false) - assert_nil @passkey.authenticate(assertion, challenge: challenge) + assert_nil @passkey.authenticate(assertion) end test "authenticate updates sign count and backed_up" do challenge = ActionPack::Passkey.request_options(credentials: [ @passkey ]).challenge assertion = build_assertion(challenge: challenge, sign_count: 5, backed_up: true) - @passkey.authenticate(assertion, challenge: challenge) + @passkey.authenticate(assertion) assert_equal 5, @passkey.reload.sign_count assert @passkey.backed_up? diff --git a/test/lib/action_pack/web_authn/authenticator/assertion_response_test.rb b/test/lib/action_pack/web_authn/authenticator/assertion_response_test.rb index c0b2a96c4..abdad8122 100644 --- a/test/lib/action_pack/web_authn/authenticator/assertion_response_test.rb +++ b/test/lib/action_pack/web_authn/authenticator/assertion_response_test.rb @@ -27,7 +27,6 @@ class ActionPack::WebAuthn::Authenticator::AssertionResponseTest < ActiveSupport authenticator_data: @authenticator_data, signature: @signature, credential: @credential, - challenge: @challenge, origin: @origin ) end @@ -56,7 +55,6 @@ class ActionPack::WebAuthn::Authenticator::AssertionResponseTest < ActiveSupport authenticator_data: @authenticator_data, signature: sign(@authenticator_data, client_data_json), credential: @credential, - challenge: @challenge, origin: @origin ) @@ -73,7 +71,6 @@ class ActionPack::WebAuthn::Authenticator::AssertionResponseTest < ActiveSupport authenticator_data: @authenticator_data, signature: Base64.urlsafe_encode64("invalid-signature", padding: false), credential: @credential, - challenge: @challenge, origin: @origin ) @@ -84,14 +81,28 @@ class ActionPack::WebAuthn::Authenticator::AssertionResponseTest < ActiveSupport assert_equal "Invalid signature", error.message end - test "validate! raises when challenge does not match" do - @response.challenge = "wrong-challenge" + test "validate! raises when challenge in client data is invalid" do + client_data_json = { + challenge: "not-a-valid-signed-challenge", + origin: @origin, + type: "webauthn.get" + }.to_json + + authenticator_data = build_authenticator_data(user_verified: true) + + response = ActionPack::WebAuthn::Authenticator::AssertionResponse.new( + client_data_json: client_data_json, + authenticator_data: authenticator_data, + signature: sign(authenticator_data, client_data_json), + credential: @credential, + origin: @origin + ) error = assert_raises(ActionPack::WebAuthn::InvalidResponseError) do - @response.validate! + response.validate! end - assert_equal "Challenge does not match", error.message + assert_match /Challenge (is invalid|has expired)/, error.message end test "validate! raises when origin does not match" do @@ -111,7 +122,6 @@ class ActionPack::WebAuthn::Authenticator::AssertionResponseTest < ActiveSupport authenticator_data: authenticator_data, signature: sign(authenticator_data, @client_data_json), credential: @credential, - challenge: @challenge, origin: @origin, user_verification: :preferred ) @@ -128,7 +138,6 @@ class ActionPack::WebAuthn::Authenticator::AssertionResponseTest < ActiveSupport authenticator_data: authenticator_data, signature: sign(authenticator_data, @client_data_json), credential: @credential, - challenge: @challenge, origin: @origin, user_verification: :required ) @@ -145,7 +154,6 @@ class ActionPack::WebAuthn::Authenticator::AssertionResponseTest < ActiveSupport authenticator_data: authenticator_data, signature: sign(authenticator_data, @client_data_json), credential: @credential, - challenge: @challenge, origin: @origin, user_verification: :required ) diff --git a/test/lib/action_pack/web_authn/authenticator/attestation_response_test.rb b/test/lib/action_pack/web_authn/authenticator/attestation_response_test.rb index 7aaca8ae4..727cd9c5e 100644 --- a/test/lib/action_pack/web_authn/authenticator/attestation_response_test.rb +++ b/test/lib/action_pack/web_authn/authenticator/attestation_response_test.rb @@ -48,7 +48,6 @@ class ActionPack::WebAuthn::Authenticator::AttestationResponseTest < ActiveSuppo @response = ActionPack::WebAuthn::Authenticator::AttestationResponse.new( client_data_json: @client_data_json, attestation_object: ATTESTATION_NONE_VERIFIED, - challenge: @challenge, origin: @origin ) end @@ -67,7 +66,6 @@ class ActionPack::WebAuthn::Authenticator::AttestationResponseTest < ActiveSuppo response = ActionPack::WebAuthn::Authenticator::AttestationResponse.new( client_data_json: @client_data_json, attestation_object: ATTESTATION_NONE_NOT_VERIFIED, - challenge: @challenge, origin: @origin, user_verification: :preferred ) @@ -81,7 +79,6 @@ class ActionPack::WebAuthn::Authenticator::AttestationResponseTest < ActiveSuppo response = ActionPack::WebAuthn::Authenticator::AttestationResponse.new( client_data_json: @client_data_json, attestation_object: ATTESTATION_NONE_VERIFIED, - challenge: @challenge, origin: @origin, user_verification: :required ) @@ -95,7 +92,6 @@ class ActionPack::WebAuthn::Authenticator::AttestationResponseTest < ActiveSuppo response = ActionPack::WebAuthn::Authenticator::AttestationResponse.new( client_data_json: @client_data_json, attestation_object: ATTESTATION_NONE_NOT_VERIFIED, - challenge: @challenge, origin: @origin, user_verification: :required ) @@ -117,7 +113,6 @@ class ActionPack::WebAuthn::Authenticator::AttestationResponseTest < ActiveSuppo response = ActionPack::WebAuthn::Authenticator::AttestationResponse.new( client_data_json: client_data_json, attestation_object: ATTESTATION_NONE_VERIFIED, - challenge: @challenge, origin: @origin ) @@ -128,14 +123,24 @@ class ActionPack::WebAuthn::Authenticator::AttestationResponseTest < ActiveSuppo assert_equal "Client data type is not webauthn.create", error.message end - test "validate! raises when challenge does not match" do - @response.challenge = "wrong-challenge" + test "validate! raises when challenge in client data is invalid" do + client_data_json = { + challenge: "not-a-valid-signed-challenge", + origin: @origin, + type: "webauthn.create" + }.to_json + + response = ActionPack::WebAuthn::Authenticator::AttestationResponse.new( + client_data_json: client_data_json, + attestation_object: ATTESTATION_NONE_VERIFIED, + origin: @origin + ) error = assert_raises(ActionPack::WebAuthn::InvalidResponseError) do - @response.validate! + response.validate! end - assert_equal "Challenge does not match", error.message + assert_match /Challenge (is invalid|has expired)/, error.message end test "validate! raises when origin does not match" do @@ -152,7 +157,6 @@ class ActionPack::WebAuthn::Authenticator::AttestationResponseTest < ActiveSuppo response = ActionPack::WebAuthn::Authenticator::AttestationResponse.new( client_data_json: @client_data_json, attestation_object: ATTESTATION_PACKED_VERIFIED, - challenge: @challenge, origin: @origin ) @@ -173,7 +177,6 @@ class ActionPack::WebAuthn::Authenticator::AttestationResponseTest < ActiveSuppo response = ActionPack::WebAuthn::Authenticator::AttestationResponse.new( client_data_json: @client_data_json, attestation_object: ATTESTATION_PACKED_VERIFIED, - challenge: @challenge, origin: @origin ) diff --git a/test/lib/action_pack/web_authn/authenticator/response_test.rb b/test/lib/action_pack/web_authn/authenticator/response_test.rb index ce0b5dc9f..bd1a9b180 100644 --- a/test/lib/action_pack/web_authn/authenticator/response_test.rb +++ b/test/lib/action_pack/web_authn/authenticator/response_test.rb @@ -18,7 +18,6 @@ class ActionPack::WebAuthn::Authenticator::ResponseTest < ActiveSupport::TestCas @response = TestableResponse.new( client_data_json: @client_data_json, authenticator_data: @authenticator_data, - challenge: @challenge, origin: @origin ) end @@ -41,9 +40,20 @@ class ActionPack::WebAuthn::Authenticator::ResponseTest < ActiveSupport::TestCas assert @response.valid? end - test "valid? returns false when challenge does not match" do - @response.challenge = "wrong-challenge" - assert_not @response.valid? + test "valid? returns false when challenge in client data is invalid" do + client_data_json = { + challenge: "not-a-valid-signed-challenge", + origin: @origin, + type: "webauthn.create" + }.to_json + + response = TestableResponse.new( + client_data_json: client_data_json, + authenticator_data: @authenticator_data, + origin: @origin + ) + + assert_not response.valid? end test "valid? returns false when origin does not match" do @@ -51,14 +61,24 @@ class ActionPack::WebAuthn::Authenticator::ResponseTest < ActiveSupport::TestCas assert_not @response.valid? end - test "validate! raises when challenge does not match" do - @response.challenge = "wrong-challenge" + test "validate! raises when challenge in client data is invalid" do + client_data_json = { + challenge: "not-a-valid-signed-challenge", + origin: @origin, + type: "webauthn.create" + }.to_json + + response = TestableResponse.new( + client_data_json: client_data_json, + authenticator_data: @authenticator_data, + origin: @origin + ) error = assert_raises(ActionPack::WebAuthn::InvalidResponseError) do - @response.validate! + response.validate! end - assert_equal "Challenge does not match", error.message + assert_match /Challenge (is invalid|has expired)/, error.message end test "validate! raises when origin does not match" do @@ -82,7 +102,6 @@ class ActionPack::WebAuthn::Authenticator::ResponseTest < ActiveSupport::TestCas response = TestableResponse.new( client_data_json: client_data_json, authenticator_data: @authenticator_data, - challenge: @challenge, origin: @origin ) @@ -108,7 +127,6 @@ class ActionPack::WebAuthn::Authenticator::ResponseTest < ActiveSupport::TestCas response = TestableResponse.new( client_data_json: @client_data_json, authenticator_data: wrong_rp_data, - challenge: @challenge, origin: @origin ) @@ -130,7 +148,6 @@ class ActionPack::WebAuthn::Authenticator::ResponseTest < ActiveSupport::TestCas response = TestableResponse.new( client_data_json: client_data_json, authenticator_data: @authenticator_data, - challenge: @challenge, origin: @origin )