From 0af6c5efdcf4d07031dc018e9e61466b4c577fbe Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Tue, 3 Oct 2023 14:59:28 -0400 Subject: [PATCH] DEV: Refactor webauthn to support passkeys (1/3) (#23586) This is part 1 of 3, split up of PR #23529. This PR refactors the webauthn code to support passkey authentication/registration. Passkeys aren't used yet, that is coming in PRs 2 and 3. Co-authored-by: Alan Guo Xiang Tan --- app/controllers/users_controller.rb | 11 +- app/models/concerns/second_factor_manager.rb | 7 +- app/models/user.rb | 15 +- config/locales/server.en.yml | 2 + lib/discourse_webauthn.rb | 26 +++- .../authentication_service.rb} | 23 ++- .../base_validation_service.rb} | 33 ++++- .../challenge_generator.rb | 0 .../registration_service.rb} | 15 +- .../user_security_key_fabricator.rb | 6 + .../authentication_service_spec.rb} | 131 ++++++++++++++---- .../challenge_generator_spec.rb | 0 .../registration_service_spec.rb} | 113 ++++++++++++--- spec/requests/session_controller_spec.rb | 6 +- spec/requests/users_controller_spec.rb | 11 +- spec/support/webauthn_integration_helpers.rb | 54 ++++++-- .../user_preferences_security_spec.rb | 3 + 17 files changed, 354 insertions(+), 102 deletions(-) rename lib/{webauthn/security_key_authentication_service.rb => discourse_webauthn/authentication_service.rb} (86%) rename lib/{webauthn/security_key_base_validation_service.rb => discourse_webauthn/base_validation_service.rb} (64%) rename lib/{webauthn => discourse_webauthn}/challenge_generator.rb (100%) rename lib/{webauthn/security_key_registration_service.rb => discourse_webauthn/registration_service.rb} (95%) rename spec/lib/{webauthn/security_key_authentication_service_spec.rb => discourse_webauthn/authentication_service_spec.rb} (66%) rename spec/lib/{webauthn => discourse_webauthn}/challenge_generator_spec.rb (100%) rename spec/lib/{webauthn/security_key_registration_service_spec.rb => discourse_webauthn/registration_service_spec.rb} (54%) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c0fd86f3189..9d0789121e3 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1578,13 +1578,12 @@ class UsersController < ApplicationController params.require(:attestation) params.require(:clientData) - ::DiscourseWebauthn::SecurityKeyRegistrationService.new( + ::DiscourseWebauthn::RegistrationService.new( current_user, params, - challenge: DiscourseWebauthn.challenge(current_user, secure_session), - rp_id: DiscourseWebauthn.rp_id, - origin: Discourse.base_url, - ).register_second_factor_security_key + session: secure_session, + factor_type: UserSecurityKey.factor_types[:second_factor], + ).register_security_key render json: success_json rescue ::DiscourseWebauthn::SecurityKeyError => err render json: failed_json.merge(error: err.message) @@ -1631,7 +1630,7 @@ class UsersController < ApplicationController def disable_second_factor # delete all second factors for a user current_user.user_second_factors.destroy_all - current_user.security_keys.destroy_all + current_user.second_factor_security_keys.destroy_all Jobs.enqueue( :critical_user_email, diff --git a/app/models/concerns/second_factor_manager.rb b/app/models/concerns/second_factor_manager.rb index 68b3c48f6e0..e5aca5f2d29 100644 --- a/app/models/concerns/second_factor_manager.rb +++ b/app/models/concerns/second_factor_manager.rb @@ -163,12 +163,11 @@ module SecondFactorManager end def authenticate_security_key(secure_session, security_key_credential) - ::DiscourseWebauthn::SecurityKeyAuthenticationService.new( + ::DiscourseWebauthn::AuthenticationService.new( self, security_key_credential, - challenge: DiscourseWebauthn.challenge(self, secure_session), - rp_id: DiscourseWebauthn.rp_id, - origin: Discourse.base_url, + session: secure_session, + factor_type: UserSecurityKey.factor_types[:second_factor], ).authenticate_security_key end diff --git a/app/models/user.rb b/app/models/user.rb index 3f4c19b68bc..c6d49bfddcd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1722,11 +1722,18 @@ class User < ActiveRecord::Base new_secure_identifier end + def second_factor_security_keys + security_keys.where(factor_type: UserSecurityKey.factor_types[:second_factor]) + end + def second_factor_security_key_credential_ids - security_keys - .select(:credential_id) - .where(factor_type: UserSecurityKey.factor_types[:second_factor]) - .pluck(:credential_id) + second_factor_security_keys.pluck(:credential_id) + end + + def passkey_credential_ids + security_keys.where(factor_type: UserSecurityKey.factor_types[:first_factor]).pluck( + :credential_id, + ) end def encoded_username(lower: false) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 5801cddcb26..92bdd4ade80 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1012,6 +1012,7 @@ en: invalid_origin_error: "The origin of the authentication request does not match the server origin." malformed_attestation_error: "There was an error decoding the attestation data." invalid_relying_party_id_error: "The Relying Party ID of the authentication request does not match the server Relying Party ID." + user_presence_error: "User presence is required." user_verification_error: "User verification is required." unsupported_public_key_algorithm_error: "The provided public key algorithm is not supported by the server." unsupported_attestation_format_error: "The attestation format is not supported by the server." @@ -1020,6 +1021,7 @@ en: ownership_error: "The security key is not owned by the user." not_found_error: "A security key with the provided credential ID could not be found." unknown_cose_algorithm_error: "The algorithm used for the security key is not recognized." + malformed_public_key_credential_error: "The provided public key is invalid." topic_flag_types: spam: diff --git a/lib/discourse_webauthn.rb b/lib/discourse_webauthn.rb index 2742cfd51b1..7756495ec2e 100644 --- a/lib/discourse_webauthn.rb +++ b/lib/discourse_webauthn.rb @@ -1,8 +1,4 @@ # frozen_string_literal: true -require "webauthn/challenge_generator" -require "webauthn/security_key_base_validation_service" -require "webauthn/security_key_registration_service" -require "webauthn/security_key_authentication_service" module DiscourseWebauthn ACCEPTABLE_REGISTRATION_TYPE = "webauthn.create" @@ -22,6 +18,8 @@ module DiscourseWebauthn end class UserVerificationError < SecurityKeyError end + class UserPresenceError < SecurityKeyError + end class ChallengeMismatchError < SecurityKeyError end class InvalidTypeError < SecurityKeyError @@ -34,7 +32,9 @@ module DiscourseWebauthn end class MalformedAttestationError < SecurityKeyError end - class NotFoundError < SecurityKeyError + class KeyNotFoundError < SecurityKeyError + end + class MalformedPublicKeyCredentialError < SecurityKeyError end class OwnershipError < SecurityKeyError end @@ -68,7 +68,21 @@ module DiscourseWebauthn end def self.rp_id - Discourse.current_hostname + Rails.env.production? ? Discourse.current_hostname : "localhost" + end + + def self.origin + case Rails.env + when "development" + # defaults to the Ember CLI local port + # you might need to change this and the rp_id above + # if you are using a non-default port/hostname locally + "http://localhost:4200" + when "test" + "http://localhost:3000" + else + Discourse.base_url + end end def self.rp_name diff --git a/lib/webauthn/security_key_authentication_service.rb b/lib/discourse_webauthn/authentication_service.rb similarity index 86% rename from lib/webauthn/security_key_authentication_service.rb rename to lib/discourse_webauthn/authentication_service.rb index 920b726e1b4..8c18c6f9220 100644 --- a/lib/webauthn/security_key_authentication_service.rb +++ b/lib/discourse_webauthn/authentication_service.rb @@ -2,21 +2,26 @@ require "cose" module DiscourseWebauthn - class SecurityKeyAuthenticationService < SecurityKeyBaseValidationService + class AuthenticationService < BaseValidationService ## # See https://w3c.github.io/webauthn/#sctn-verifying-assertion for # the steps followed here. Memoized methods are called in their # place in the step flow to make the process clearer. def authenticate_security_key if @params.blank? || (!@params.is_a?(Hash) && !@params.is_a?(ActionController::Parameters)) - return false + raise( + MalformedPublicKeyCredentialError, + I18n.t("webauthn.validation.malformed_public_key_credential_error"), + ) end + security_key = UserSecurityKey.find_by(credential_id: @params[:credentialId]) + raise(KeyNotFoundError, I18n.t("webauthn.validation.not_found_error")) if security_key.blank? + # 3. Identify the user being authenticated and verify that this user is the # owner of the public key credential source credentialSource identified by credential.id: - security_key = UserSecurityKey.find_by(credential_id: @params[:credentialId]) - raise(NotFoundError, I18n.t("webauthn.validation.not_found_error")) if security_key.blank? - if security_key.user != @current_user + if @factor_type == UserSecurityKey.factor_types[:second_factor] && + (@current_user == nil || security_key.user == nil || security_key.user != @current_user) raise(OwnershipError, I18n.t("webauthn.validation.ownership_error")) end @@ -49,11 +54,12 @@ module DiscourseWebauthn # 13. Verify that the User Present bit of the flags in authData is set. # https://blog.bigbinary.com/2011/07/20/ruby-pack-unpack.html # - # bit 0 is the least significant bit - LSB first + validate_user_presence + # # 14. If user verification is required for this registration, verify that # the User Verified bit of the flags in authData is set. - validate_user_verification + validate_user_verification if @factor_type == UserSecurityKey.factor_types[:first_factor] # 15. Verify that the values of the client extension outputs in clientExtensionResults and the authenticator # extension outputs in the extensions in authData are as expected, considering the client extension input @@ -86,6 +92,9 @@ module DiscourseWebauthn # Success! Update the last used at time for the key. security_key.update(last_used: Time.zone.now) + + # Return security key record so controller can use it to update the session + security_key rescue OpenSSL::PKey::PKeyError raise(PublicKeyError, I18n.t("webauthn.validation.public_key_error")) end diff --git a/lib/webauthn/security_key_base_validation_service.rb b/lib/discourse_webauthn/base_validation_service.rb similarity index 64% rename from lib/webauthn/security_key_base_validation_service.rb rename to lib/discourse_webauthn/base_validation_service.rb index 6cfc50f3f9c..62df01d5b1d 100644 --- a/lib/webauthn/security_key_base_validation_service.rb +++ b/lib/discourse_webauthn/base_validation_service.rb @@ -1,11 +1,12 @@ # frozen_string_literal: true module DiscourseWebauthn - class SecurityKeyBaseValidationService - def initialize(current_user, params, challenge_params) + class BaseValidationService + def initialize(current_user, params, session:, factor_type:) @current_user = current_user @params = params - @challenge_params = challenge_params + @factor_type = factor_type + @session = session end def validate_webauthn_type(type_to_check) @@ -31,9 +32,26 @@ module DiscourseWebauthn ) end + ## flags per specification + # https://www.w3.org/TR/webauthn-2/#sctn-authenticator-data + # bit 0 - user presence + # bit 1 - reserved for future use + # bit 2 - user verification + # bit 3-5 - reserved for future use + # bit 6 - attested credential data + # bit 7 - extension data + + def validate_user_presence + flags = auth_data[32].unpack("b*")[0].split("") + # bit 0 - user presence + return if flags[0] == "1" + raise(UserPresenceError, I18n.t("webauthn.validation.user_presence_error")) + end + def validate_user_verification flags = auth_data[32].unpack("b*")[0].split("") - return if flags[0] == "1" + # bit 2 - user verification + return if flags[2] == "1" raise(UserVerificationError, I18n.t("webauthn.validation.user_verification_error")) end @@ -52,15 +70,16 @@ module DiscourseWebauthn end def challenge_match? - Base64.decode64(client_data["challenge"]) == @challenge_params[:challenge] + Base64.decode64(client_data["challenge"]) == + DiscourseWebauthn.challenge(@current_user, @session) end def origin_match? - client_data["origin"] == @challenge_params[:origin] + client_data["origin"] == DiscourseWebauthn.origin end def rp_id_hash_match? - auth_data[0..31] == OpenSSL::Digest::SHA256.digest(@challenge_params[:rp_id]) + auth_data[0..31] == OpenSSL::Digest::SHA256.digest(DiscourseWebauthn.rp_id) end def client_data_hash diff --git a/lib/webauthn/challenge_generator.rb b/lib/discourse_webauthn/challenge_generator.rb similarity index 100% rename from lib/webauthn/challenge_generator.rb rename to lib/discourse_webauthn/challenge_generator.rb diff --git a/lib/webauthn/security_key_registration_service.rb b/lib/discourse_webauthn/registration_service.rb similarity index 95% rename from lib/webauthn/security_key_registration_service.rb rename to lib/discourse_webauthn/registration_service.rb index 0cdd4950acc..3c84026bb72 100644 --- a/lib/webauthn/security_key_registration_service.rb +++ b/lib/discourse_webauthn/registration_service.rb @@ -3,12 +3,12 @@ require "cbor" require "cose" module DiscourseWebauthn - class SecurityKeyRegistrationService < SecurityKeyBaseValidationService + class RegistrationService < BaseValidationService ## # See https://w3c.github.io/webauthn/#sctn-registering-a-new-credential for # the registration steps followed here. Memoized methods are called in their # place in the step flow to make the process clearer. - def register_second_factor_security_key + def register_security_key # 4. Verify that the value of C.type is webauthn.create. validate_webauthn_type(::DiscourseWebauthn::ACCEPTABLE_REGISTRATION_TYPE) @@ -38,11 +38,12 @@ module DiscourseWebauthn # 11. Verify that the User Present bit of the flags in authData is set. # https://blog.bigbinary.com/2011/07/20/ruby-pack-unpack.html # - # bit 0 is the least significant bit - LSB first + validate_user_presence + # # 12. If user verification is required for this registration, verify that # the User Verified bit of the flags in authData is set. - validate_user_verification + validate_user_verification if @factor_type == UserSecurityKey.factor_types[:first_factor] # 13. Verify that the "alg" parameter in the credential public key in authData matches the alg # attribute of one of the items in options.pubKeyCredParams. @@ -100,7 +101,7 @@ module DiscourseWebauthn # the Relying Party SHOULD fail this registration ceremony, or it MAY decide to accept # the registration, e.g. while deleting the older registration. encoded_credential_id = Base64.strict_encode64(credential_id) - endcoded_public_key = Base64.strict_encode64(credential_public_key_bytes) + encoded_public_key = Base64.strict_encode64(credential_public_key_bytes) if UserSecurityKey.exists?(credential_id: encoded_credential_id) raise(CredentialIdInUseError, I18n.t("webauthn.validation.credential_id_in_use_error")) end @@ -112,9 +113,9 @@ module DiscourseWebauthn UserSecurityKey.create!( user: @current_user, credential_id: encoded_credential_id, - public_key: endcoded_public_key, + public_key: encoded_public_key, name: @params[:name], - factor_type: UserSecurityKey.factor_types[:second_factor], + factor_type: @factor_type, ) rescue CBOR::UnpackError, CBOR::TypeError, CBOR::MalformedFormatError, CBOR::StackError raise MalformedAttestationError, I18n.t("webauthn.validation.malformed_attestation_error") diff --git a/spec/fabricators/user_security_key_fabricator.rb b/spec/fabricators/user_security_key_fabricator.rb index edc4279f60c..cab4772b5c2 100644 --- a/spec/fabricators/user_security_key_fabricator.rb +++ b/spec/fabricators/user_security_key_fabricator.rb @@ -24,3 +24,9 @@ Fabricator(:user_security_key_with_random_credential, from: :user_security_key) credential_id { SecureRandom.base64(40) } public_key { SecureRandom.base64(40) } end + +Fabricator(:passkey_with_random_credential, from: :user_security_key) do + credential_id { SecureRandom.base64(40) } + public_key { SecureRandom.base64(40) } + factor_type { UserSecurityKey.factor_types[:first_factor] } +end diff --git a/spec/lib/webauthn/security_key_authentication_service_spec.rb b/spec/lib/discourse_webauthn/authentication_service_spec.rb similarity index 66% rename from spec/lib/webauthn/security_key_authentication_service_spec.rb rename to spec/lib/discourse_webauthn/authentication_service_spec.rb index 4cfc11981c7..2f0a58a2862 100644 --- a/spec/lib/webauthn/security_key_authentication_service_spec.rb +++ b/spec/lib/discourse_webauthn/authentication_service_spec.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true require "discourse_webauthn" -require "webauthn/security_key_registration_service" ## # These tests use the following parameters generated on a local discourse @@ -12,7 +11,6 @@ require "webauthn/security_key_registration_service" # - signature # - authenticator_data # - client_data_origin -# - challenge_params_origin # # To create another test (e.g. for a different COSE algorithm) you need to: # @@ -24,21 +22,23 @@ require "webauthn/security_key_registration_service" # you need to add puts debugger statements (or use binding.pry) like so: # # puts client_data -# puts signature -# puts auth_data +# puts @params # -# The auth_data will have the challenge param, but you must Base64.decode64 to -# use it in the let(:challenge) variable. The signature and auth_data params -# can be used as is. +# The client_data will have the challenge param, but you must Base64.decode64 to +# use it in the let(:challenge) variable. +# +# puts Base64.decode64(client_data["challenge"]) # # You also need to make sure that client_data_param has the exact same structure -# and order of keys as auth_data, otherwise even with everything else right the +# and order of keys, otherwise even with everything else right the # public key verification will fail. # -# The origin params just need to be whatever your localhost URL for Discourse is. +# @params will contain authenticatorData and signature which you can use as is. +# +# The origin param needs to be http://localhost:3000 (that's the port tests run on) -RSpec.describe DiscourseWebauthn::SecurityKeyAuthenticationService do - subject(:service) { described_class.new(current_user, params, challenge_params) } +RSpec.describe DiscourseWebauthn::AuthenticationService do + subject(:service) { described_class.new(current_user, params, **options) } let(:security_key_user) { current_user } let!(:security_key) do @@ -47,7 +47,9 @@ RSpec.describe DiscourseWebauthn::SecurityKeyAuthenticationService do credential_id: credential_id, public_key: public_key, user: security_key_user, + factor_type: UserSecurityKey.factor_types[:second_factor], last_used: nil, + name: "Some key", ) end let(:public_key) do @@ -56,6 +58,7 @@ RSpec.describe DiscourseWebauthn::SecurityKeyAuthenticationService do let(:credential_id) do "mJAJ4CznTO0SuLkJbYwpgK75ao4KMNIPlU5KWM92nq39kRbXzI9mSv6GxTcsMYoiPgaouNw7b7zBiS4vsQaO6A==" end + let(:secure_session) { SecureSession.new("tester") } let(:challenge) { "81d4acfbd69eafa8f02bc2ecbec5267be8c9b28c1e0ba306d52b79f0f13d" } let(:client_data_challenge) { Base64.strict_encode64(challenge) } let(:client_data_webauthn_type) { "webauthn.get" } @@ -87,29 +90,39 @@ RSpec.describe DiscourseWebauthn::SecurityKeyAuthenticationService do signature: signature, } end - ## - # The original key was generated in localhost - let(:rp_id) { "localhost" } - let(:challenge_params_origin) { "http://localhost:3000" } - let(:challenge_params) { { challenge: challenge, rp_id: rp_id, origin: challenge_params_origin } } + + let(:options) do + { session: secure_session, factor_type: UserSecurityKey.factor_types[:second_factor] } + end let(:current_user) { Fabricate(:user) } + before do + # we have to stub here because the public key was created using this specific challenge + DiscourseWebauthn.stubs(:challenge).returns(challenge) + end + it "updates last_used when the security key and params are valid" do - expect(service.authenticate_security_key).to eq(true) + expect(service.authenticate_security_key).to eq(security_key) expect(security_key.reload.last_used).not_to eq(nil) end context "when params is blank" do let(:params) { nil } - it "returns false with no validation" do - expect(service.authenticate_security_key).to eq(false) + it "raises a MalformedPublicKeyCredentialError" do + expect { service.authenticate_security_key }.to raise_error( + DiscourseWebauthn::MalformedPublicKeyCredentialError, + I18n.t("webauthn.validation.malformed_public_key_credential_error"), + ) end end context "when params is not blank and not a hash" do let(:params) { "test" } - it "returns false with no validation" do - expect(service.authenticate_security_key).to eq(false) + it "raises a MalformedPublicKeyCredentialError" do + expect { service.authenticate_security_key }.to raise_error( + DiscourseWebauthn::MalformedPublicKeyCredentialError, + I18n.t("webauthn.validation.malformed_public_key_credential_error"), + ) end end @@ -118,7 +131,7 @@ RSpec.describe DiscourseWebauthn::SecurityKeyAuthenticationService do it "raises a NotFoundError" do expect { service.authenticate_security_key }.to raise_error( - DiscourseWebauthn::NotFoundError, + DiscourseWebauthn::KeyNotFoundError, I18n.t("webauthn.validation.not_found_error"), ) end @@ -135,6 +148,18 @@ RSpec.describe DiscourseWebauthn::SecurityKeyAuthenticationService do end end + context "when the second-factor authentication is initiated without a user" do + let(:current_user) { nil } + let(:security_key_user) { Fabricate(:user) } + + it "raises an OwnershipError" do + expect { service.authenticate_security_key }.to raise_error( + DiscourseWebauthn::OwnershipError, + I18n.t("webauthn.validation.ownership_error"), + ) + end + end + context "when the client data webauthn type is not webauthn.get" do let(:client_data_webauthn_type) { "webauthn.explode" } @@ -169,9 +194,9 @@ RSpec.describe DiscourseWebauthn::SecurityKeyAuthenticationService do end context "when the sha256 hash of the relaying party ID does not match the one in attestation.authData" do - let(:rp_id) { "bad_rp_id" } - it "raises a InvalidRelyingPartyIdError" do + DiscourseWebauthn.stubs(:rp_id).returns("bad_rp_id") + expect { service.authenticate_security_key }.to raise_error( DiscourseWebauthn::InvalidRelyingPartyIdError, I18n.t("webauthn.validation.invalid_relying_party_id_error"), @@ -214,7 +239,7 @@ RSpec.describe DiscourseWebauthn::SecurityKeyAuthenticationService do let(:public_key) do "pAEDAzkBACBZAQCqsl50KrR5zVm/QT9vWkeGTGxby32m0QRtCRh2UWseqoG0ZmBhGeWEYvkdoYlB1jObQKEHsAeB+1NBf5q69/88AA5zv4fzrvCydCtL41EUsHYFEbaPGnB61zZmYVLTPI7BYa+fu4F4MzFa924s36tVlU/L7n04peviJVZW2C1YIQfwOGDZJSvUpqJoZMQtw1vGRfrb4cQKlHfrpDZUpa3QLE8phh4ce4nwtX1tUnUGgCy8sOaFVkDNufENGTNr8HdAIHcinUiax3yy/Q8LjSZb8UR2ha6oXSe1vRHhj001B/P/mr5AdVMxSrOT1sUNXWkHv8L8IzS/iTBQpsC8CADZIUMBAAE=" end - let(:challenge_params_origin) { "http://localhost:4200" } + # This key was generated using this specific origin let(:client_data_origin) { "http://localhost:4200" } # This has to be in the exact same order with the same data as it was originally @@ -231,7 +256,9 @@ RSpec.describe DiscourseWebauthn::SecurityKeyAuthenticationService do end it "updates last_used when the security key and params are valid" do - expect(service.authenticate_security_key).to eq(true) + DiscourseWebauthn.stubs(:origin).returns("http://localhost:4200") + + expect(service.authenticate_security_key).to eq(security_key) expect(security_key.reload.last_used).not_to eq(nil) end end @@ -241,4 +268,56 @@ RSpec.describe DiscourseWebauthn::SecurityKeyAuthenticationService do expect(COSE::Algorithm.find(alg)).not_to be_nil end end + + describe "authenticating a valid passkey" do + let(:options) do + { factor_type: UserSecurityKey.factor_types[:first_factor], session: secure_session } + end + + ## + # These are sourced from an actual key, see instructions at the top of this spec for details + # + let(:public_key) { valid_passkey_data[:public_key] } + let(:credential_id) { valid_passkey_data[:credential_id] } + let(:signature) { valid_passkey_auth_data[:signature] } + let(:authenticator_data) { valid_passkey_auth_data[:authenticatorData] } + let(:challenge) { valid_passkey_challenge } + + let(:client_data_param) { passkey_client_data_param("webauthn.get") } + + let!(:security_key) do + Fabricate( + :user_security_key, + credential_id: credential_id, + public_key: public_key, + user: security_key_user, + factor_type: UserSecurityKey.factor_types[:first_factor], + last_used: nil, + name: "A key", + ) + end + + it "works and returns the correct key credential" do + key = service.authenticate_security_key + expect(key).to eq(security_key) + expect(key.factor_type).to eq(UserSecurityKey.factor_types[:first_factor]) + end + + context "when the user verification flag in the key is false" do + it "raises a UserVerificationError" do + # simulate missing user verification in the key data + # by setting third bit to 0 + flags = "10000010" # correct flag sequence is "10100010" + overriden_auth_data = service.send(:auth_data) + overriden_auth_data[32] = [flags].pack("b*") + + service.instance_variable_set(:@auth_data, overriden_auth_data) + + expect { service.authenticate_security_key }.to raise_error( + DiscourseWebauthn::UserVerificationError, + I18n.t("webauthn.validation.user_verification_error"), + ) + end + end + end end diff --git a/spec/lib/webauthn/challenge_generator_spec.rb b/spec/lib/discourse_webauthn/challenge_generator_spec.rb similarity index 100% rename from spec/lib/webauthn/challenge_generator_spec.rb rename to spec/lib/discourse_webauthn/challenge_generator_spec.rb diff --git a/spec/lib/webauthn/security_key_registration_service_spec.rb b/spec/lib/discourse_webauthn/registration_service_spec.rb similarity index 54% rename from spec/lib/webauthn/security_key_registration_service_spec.rb rename to spec/lib/discourse_webauthn/registration_service_spec.rb index e45c1766d2c..077f9d20235 100644 --- a/spec/lib/webauthn/security_key_registration_service_spec.rb +++ b/spec/lib/discourse_webauthn/registration_service_spec.rb @@ -1,10 +1,10 @@ # frozen_string_literal: true require "discourse_webauthn" -require "webauthn/security_key_registration_service" -RSpec.describe DiscourseWebauthn::SecurityKeyRegistrationService do - subject(:service) { described_class.new(current_user, params, challenge_params) } +RSpec.describe DiscourseWebauthn::RegistrationService do + subject(:service) { described_class.new(current_user, params, **options) } + let(:secure_session) { SecureSession.new("tester") } let(:client_data_challenge) { Base64.encode64(challenge) } let(:client_data_webauthn_type) { "webauthn.create" } let(:client_data_origin) { "http://localhost:3000" } @@ -18,7 +18,8 @@ RSpec.describe DiscourseWebauthn::SecurityKeyRegistrationService do ## # This attestation object was sourced by manually registering # a key with `navigator.credentials.create` and capturing the - # results in localhost. + # results in localhost. It does not have a user verification + # flag set (i.e. it is only usable as 2FA). let(:attestation) do "o2NmbXRkbm9uZWdhdHRTdG10oGhhdXRoRGF0YVjESZYN5YgOjGh0NBcPZHZgW4/krrmihjLHmVzzuoMdl2NBAAAAAAAAAAAAAAAAAAAAAAAAAAAAQFmvayWc8OPJ4jj4sevfxBmvUglDMZrFalyokYrdnqOVvudC0lQialaGQv72eBzJM2Qn1GfJI7lpBgFJMprisLSlAQIDJiABIVgg+23/BZux7LK0/KQgCiQGtdr51ar+vfTtHWpRtN17gOwiWCBstV918mugVBexg/rdZjTs0wN/upHFoyBiAJCaGVD8OA==" end @@ -32,16 +33,19 @@ RSpec.describe DiscourseWebauthn::SecurityKeyRegistrationService do ## # The above attestation was generated in localhost; Discourse.current_hostname # returns test.localhost which we do not want - let(:rp_id) { "localhost" } - let(:challenge_params) { { challenge: challenge, rp_id: rp_id, origin: "http://localhost:3000" } } - let(:challenge) { "f1e04530f34a1b6a08d032d8550e23eb8330be04e4166008f26c0e1b42ad" } + let(:options) do + { session: secure_session, factor_type: UserSecurityKey.factor_types[:second_factor] } + end + + let(:challenge) { DiscourseWebauthn.stage_challenge(current_user, secure_session).challenge } + let(:current_user) { Fabricate(:user) } context "when the client data webauthn type is not webauthn.create" do let(:client_data_webauthn_type) { "webauthn.explode" } it "raises an InvalidTypeError" do - expect { service.register_second_factor_security_key }.to raise_error( + expect { service.register_security_key }.to raise_error( DiscourseWebauthn::InvalidTypeError, I18n.t("webauthn.validation.invalid_type_error"), ) @@ -52,7 +56,7 @@ RSpec.describe DiscourseWebauthn::SecurityKeyRegistrationService do let(:client_data_challenge) { Base64.encode64("invalid challenge") } it "raises a ChallengeMismatchError" do - expect { service.register_second_factor_security_key }.to raise_error( + expect { service.register_security_key }.to raise_error( DiscourseWebauthn::ChallengeMismatchError, I18n.t("webauthn.validation.challenge_mismatch_error"), ) @@ -63,7 +67,7 @@ RSpec.describe DiscourseWebauthn::SecurityKeyRegistrationService do let(:client_data_origin) { "https://someothersite.com" } it "raises a InvalidOriginError" do - expect { service.register_second_factor_security_key }.to raise_error( + expect { service.register_security_key }.to raise_error( DiscourseWebauthn::InvalidOriginError, I18n.t("webauthn.validation.invalid_origin_error"), ) @@ -71,10 +75,10 @@ RSpec.describe DiscourseWebauthn::SecurityKeyRegistrationService do end context "when the sha256 hash of the relaying party ID does not match the one in attestation.authData" do - let(:rp_id) { "bad_rp_id" } - it "raises a InvalidRelyingPartyIdError" do - expect { service.register_second_factor_security_key }.to raise_error( + DiscourseWebauthn.stubs(:rp_id).returns("bad_rp_id") + + expect { service.register_security_key }.to raise_error( DiscourseWebauthn::InvalidRelyingPartyIdError, I18n.t("webauthn.validation.invalid_relying_party_id_error"), ) @@ -88,7 +92,7 @@ RSpec.describe DiscourseWebauthn::SecurityKeyRegistrationService do end it "raises a UnsupportedPublicKeyAlgorithmError" do - expect { service.register_second_factor_security_key }.to raise_error( + expect { service.register_security_key }.to raise_error( DiscourseWebauthn::UnsupportedPublicKeyAlgorithmError, I18n.t("webauthn.validation.unsupported_public_key_algorithm_error"), ) @@ -106,7 +110,7 @@ RSpec.describe DiscourseWebauthn::SecurityKeyRegistrationService do end it "raises a UnsupportedAttestationFormatError" do - expect { service.register_second_factor_security_key }.to raise_error( + expect { service.register_security_key }.to raise_error( DiscourseWebauthn::UnsupportedAttestationFormatError, I18n.t("webauthn.validation.unsupported_attestation_format_error"), ) @@ -122,14 +126,14 @@ RSpec.describe DiscourseWebauthn::SecurityKeyRegistrationService do context "when the credential id is already in use for any user" do it "raises a CredentialIdInUseError" do # register the key to the current user - security_key = service.register_second_factor_security_key + security_key = service.register_security_key # update the key to be on a different user other_user = Fabricate(:user) security_key.update(user: other_user) # error! - expect { service.register_second_factor_security_key }.to raise_error( + expect { service.register_security_key }.to raise_error( DiscourseWebauthn::CredentialIdInUseError, I18n.t("webauthn.validation.credential_id_in_use_error"), ) @@ -142,10 +146,83 @@ RSpec.describe DiscourseWebauthn::SecurityKeyRegistrationService do end it "raises a MalformedAttestationError" do - expect { service.register_second_factor_security_key }.to raise_error( + expect { service.register_security_key }.to raise_error( DiscourseWebauthn::MalformedAttestationError, I18n.t("webauthn.validation.malformed_attestation_error"), ) end end + + context "when the user presence flag is false" do + it "raises a UserPresenceError" do + # simulate missing user presence by flipping first bit to 0 + flags = "00000010" + overridenAuthData = service.send(:auth_data) + overridenAuthData[32] = [flags].pack("b*") + + service.instance_variable_set(:@auth_data, overridenAuthData) + + expect { service.register_security_key }.to raise_error( + DiscourseWebauthn::UserPresenceError, + I18n.t("webauthn.validation.user_presence_error"), + ) + end + end + + it "registers a valid second-factor key" do + key = service.register_security_key + expect(key).to be_a(UserSecurityKey) + expect(key.user).to eq(current_user) + expect(key.factor_type).to eq(UserSecurityKey.factor_types[:second_factor]) + end + + describe "registering a second factor key as first factor" do + let(:options) do + { factor_type: UserSecurityKey.factor_types[:first_factor], session: secure_session } + end + + it "does not work since second-factor key does not have the user verification flag" do + expect { service.register_security_key }.to raise_error( + DiscourseWebauthn::UserVerificationError, + I18n.t("webauthn.validation.user_verification_error"), + ) + end + end + + describe "registering a passkey" do + let(:options) do + { factor_type: UserSecurityKey.factor_types[:first_factor], session: secure_session } + end + + ## + # key registered locally using + # - localhost:3000 as the origin (via an origin override in discourse_webauthn.rb) + # - frontend webauthn.create has user verification flag enabled + let(:attestation) do + "o2NmbXRkbm9uZWdhdHRTdG10oGhhdXRoRGF0YVikSZYN5YgOjGh0NBcPZHZgW4/krrmihjLHmVzzuoMdl2NFAAAAAK3OAAI1vMYKZIsLJfHwVQMAICRXq4sFZ9XpWZOzfJ8EguJmoEPMzNVyFMUWQfT5u1QzpQECAyYgASFYILjOiAHAwNrXkCk/tmyYRiE87QyV/15wUvhcXhr1JfwtIlggClQywgQvSxTsqV/FSK0cNHTTmuwfzzREqE6eLDmPxmI=" + end + + it "works with a valid key" do + key = service.register_security_key + expect(key).to be_a(UserSecurityKey) + expect(key.user).to eq(current_user) + expect(key.factor_type).to eq(UserSecurityKey.factor_types[:first_factor]) + end + + context "when the user verification flag in the key is false" do + it "raises a UserVerificationError" do + # simulate missing user verification by flipping third bit to 0 + flags = "10000010" # correct flag sequence is "10100010" + overriden_auth_data = service.send(:auth_data) + overriden_auth_data[32] = [flags].pack("b*") + + service.instance_variable_set(:@auth_data, overriden_auth_data) + + expect { service.register_security_key }.to raise_error( + DiscourseWebauthn::UserVerificationError, + I18n.t("webauthn.validation.user_verification_error"), + ) + end + end + end end diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index b95e4bd6b08..c9056efe849 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -120,7 +120,7 @@ RSpec.describe SessionController do expect(response_body_parsed["challenge"]).to eq( DiscourseWebauthn.challenge(user, secure_session), ) - expect(DiscourseWebauthn.rp_id).to eq(Discourse.current_hostname) + expect(DiscourseWebauthn.rp_id).to eq("localhost") end end end @@ -2041,7 +2041,9 @@ RSpec.describe SessionController do expect(session[:current_user_id]).to eq(nil) response_body = response.parsed_body expect(response_body["failed"]).to eq("FAILED") - expect(response_body["error"]).to eq(I18n.t("login.invalid_security_key")) + expect(response_body["error"]).to eq( + I18n.t("webauthn.validation.malformed_public_key_credential_error"), + ) end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index a272fb710fe..179b897d88b 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -5835,7 +5835,6 @@ RSpec.describe UsersController do context "when the creation parameters are invalid" do it "shows a security key error and does not create a key" do - stub_as_dev_localhost create_second_factor_security_key _response_parsed = response.parsed_body @@ -5872,9 +5871,10 @@ RSpec.describe UsersController do user: user1, factor_type: UserSecurityKey.factor_types[:second_factor], ) + Fabricate(:passkey_with_random_credential, user: user1) end - it "should disable all totp and security keys" do + it "should disable all totp and security keys (but not passkeys)" do expect_enqueued_with( job: :critical_user_email, args: { @@ -5887,7 +5887,12 @@ RSpec.describe UsersController do expect(response.status).to eq(200) expect(user1.reload.user_second_factors).to be_empty - expect(user1.security_keys).to be_empty + expect(user1.second_factor_security_keys).to be_empty + expect(user1.security_keys.length).to eq(1) + expect(user1.security_keys[0].factor_type).to eq( + UserSecurityKey.factor_types[:first_factor], + ) + expect(user1.passkey_credential_ids.length).to eq(1) end end end diff --git a/spec/support/webauthn_integration_helpers.rb b/spec/support/webauthn_integration_helpers.rb index a7a646721ab..3d8ba5d4306 100644 --- a/spec/support/webauthn_integration_helpers.rb +++ b/spec/support/webauthn_integration_helpers.rb @@ -17,6 +17,10 @@ module DiscourseWebauthnIntegrationHelpers # This is because the challenge is embedded # in the post data's authenticatorData and must match up. See # simulate_localhost_webauthn_challenge for a real example. + + # All of the valid security key data is sourced from a localhost + # login (with origin http://localhost:3000). + def valid_security_key_data { credential_id: @@ -55,19 +59,45 @@ module DiscourseWebauthnIntegrationHelpers } end - # all of the valid security key data is sourced from a localhost - # login, if this is not set the specs for webauthn WILL NOT WORK - def stub_as_dev_localhost - Discourse.stubs(:current_hostname).returns("localhost") - Discourse.stubs(:base_url).returns("http://localhost:3000") + def simulate_localhost_webauthn_challenge + DiscourseWebauthn.stubs(:challenge).returns(valid_security_key_challenge_data[:challenge]) end - def simulate_localhost_webauthn_challenge - stub_as_dev_localhost - DiscourseWebauthn::ChallengeGenerator.stubs(:generate).returns( - DiscourseWebauthn::ChallengeGenerator::ChallengeSession.new( - challenge: valid_security_key_challenge_data[:challenge], - ), - ) + # Passkey data sourced from a key generated in a local browser + # with webauthn.create that includes the user verification flag on localhost:3000 + # usin puts statements in the passkeys session controllers + def valid_passkey_challenge + "66b47014ef72937d8320ed893dc797e8a9a6d5098b89b185ca3d439b3656" + end + + def passkey_client_data_param(type) + { + type: type, + challenge: Base64.strict_encode64(valid_passkey_challenge), + origin: "http://localhost:3000", + crossOrigin: false, + } + end + + def valid_passkey_auth_data + { + clientData: Base64.strict_encode64(passkey_client_data_param("webauthn.get").to_json), + credentialId: "JFeriwVn1elZk7N8nwSC4magQ8zM1XIUxRZB9Pm7VDM=", + authenticatorData: "SZYN5YgOjGh0NBcPZHZgW4/krrmihjLHmVzzuoMdl2MFAAAAAA==", + signature: + "MEUCIG5AFaw2Nfy69hHjeRLqm3LzQRMFb+TRbUAz19WJymegAiEAyEEyGdAMB2/NBwRCHM47IwtjKWCLEtabAX2BaK6fD8g=", + } + end + + def valid_passkey_data + { + credential_id: "JFeriwVn1elZk7N8nwSC4magQ8zM1XIUxRZB9Pm7VDM=", + public_key: + "pQECAyYgASFYILjOiAHAwNrXkCk/tmyYRiE87QyV/15wUvhcXhr1JfwtIlggClQywgQvSxTsqV/FSK0cNHTTmuwfzzREqE6eLDmPxmI=", + } + end + + def simulate_localhost_passkey_challenge + DiscourseWebauthn.stubs(:challenge).returns(valid_passkey_challenge) end end diff --git a/spec/system/user_page/user_preferences_security_spec.rb b/spec/system/user_page/user_preferences_security_spec.rb index 3f691416015..619d2415e8b 100644 --- a/spec/system/user_page/user_preferences_security_spec.rb +++ b/spec/system/user_page/user_preferences_security_spec.rb @@ -14,6 +14,9 @@ describe "User preferences for Security", type: :system do describe "Security keys" do it "adds a 2F security key and logs in with it" do + # system specs run on their own host + port + DiscourseWebauthn.stubs(:origin).returns(current_host + ":" + Capybara.server_port.to_s) + # simulate browser credential authorization options = ::Selenium::WebDriver::VirtualAuthenticatorOptions.new page.driver.browser.add_virtual_authenticator(options)