Add more specific error messages to HTTP signature verification (#21617)

* Return specific error on failure to parse Date header

* Add error message when preferredUsername is not set

* Change error report to be JSON and include more details

* Change error report to differentiate unknown account and failed refresh

* Add tests
pull/23156/head
Claire 2023-01-18 16:47:56 +01:00 committed by GitHub
parent 30e895299c
commit 68dcbcb7bf
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 109 additions and 15 deletions

View File

@ -46,11 +46,11 @@ module SignatureVerification
end
def require_account_signature!
render plain: signature_verification_failure_reason, status: signature_verification_failure_code unless signed_request_account
render json: signature_verification_failure_reason, status: signature_verification_failure_code unless signed_request_account
end
def require_actor_signature!
render plain: signature_verification_failure_reason, status: signature_verification_failure_code unless signed_request_actor
render json: signature_verification_failure_reason, status: signature_verification_failure_code unless signed_request_actor
end
def signed_request?
@ -97,11 +97,11 @@ module SignatureVerification
actor = stoplight_wrap_request { actor_refresh_key!(actor) }
raise SignatureVerificationError, "Public key not found for key #{signature_params['keyId']}" if actor.nil?
raise SignatureVerificationError, "Could not refresh public key #{signature_params['keyId']}" if actor.nil?
return actor unless verify_signature(actor, signature, compare_signed_string).nil?
fail_with! "Verification failed for #{actor.to_log_human_identifier} #{actor.uri} using rsa-sha256 (RSASSA-PKCS1-v1_5 with SHA-256)"
fail_with! "Verification failed for #{actor.to_log_human_identifier} #{actor.uri} using rsa-sha256 (RSASSA-PKCS1-v1_5 with SHA-256)", signed_string: compare_signed_string, signature: signature_params['signature']
rescue SignatureVerificationError => e
fail_with! e.message
rescue HTTP::Error, OpenSSL::SSL::SSLError => e
@ -118,8 +118,8 @@ module SignatureVerification
private
def fail_with!(message)
@signature_verification_failure_reason = message
def fail_with!(message, **options)
@signature_verification_failure_reason = { error: message }.merge(options)
@signed_request_actor = nil
end
@ -209,8 +209,8 @@ module SignatureVerification
end
expires_time = Time.at(signature_params['expires'].to_i).utc if signature_params['expires'].present?
rescue ArgumentError
return false
rescue ArgumentError => e
raise SignatureVerificationError, "Invalid Date header: #{e.message}"
end
expires_time ||= created_time + 5.minutes unless created_time.nil?

View File

@ -28,6 +28,7 @@ class ActivityPub::FetchRemoteActorService < BaseService
raise Error, "Unsupported JSON-LD context for document #{uri}" unless supported_context?
raise Error, "Unexpected object type for actor #{uri} (expected any of: #{SUPPORTED_TYPES})" unless expected_type?
raise Error, "Actor #{uri} has moved to #{@json['movedTo']}" if break_on_redirect && @json['movedTo'].present?
raise Error, "Actor #{uri} has no 'preferredUsername', which is a requirement for Mastodon compatibility" unless @json['preferredUsername'].present?
@uri = @json['id']
@username = @json['preferredUsername']

View File

@ -16,6 +16,8 @@ describe ApplicationController, type: :controller do
controller do
include SignatureVerification
before_action :require_actor_signature!, only: [:signature_required]
def success
head 200
end
@ -23,10 +25,17 @@ describe ApplicationController, type: :controller do
def alternative_success
head 200
end
def signature_required
head 200
end
end
before do
routes.draw { match via: [:get, :post], 'success' => 'anonymous#success' }
routes.draw do
match via: [:get, :post], 'success' => 'anonymous#success'
match via: [:get, :post], 'signature_required' => 'anonymous#signature_required'
end
end
context 'without signature header' do
@ -118,6 +127,37 @@ describe ApplicationController, type: :controller do
end
end
context 'with request with unparseable Date header' do
before do
get :success
fake_request = Request.new(:get, request.url)
fake_request.add_headers({ 'Date' => 'wrong date' })
fake_request.on_behalf_of(author)
request.headers.merge!(fake_request.headers)
end
describe '#signed_request?' do
it 'returns true' do
expect(controller.signed_request?).to be true
end
end
describe '#signed_request_account' do
it 'returns nil' do
expect(controller.signed_request_account).to be_nil
end
end
describe '#signature_verification_failure_reason' do
it 'contains an error description' do
controller.signed_request_account
expect(controller.signature_verification_failure_reason[:error]).to eq 'Invalid Date header: not RFC 2616 compliant date: "wrong date"'
end
end
end
context 'with request older than a day' do
before do
get :success
@ -140,6 +180,13 @@ describe ApplicationController, type: :controller do
expect(controller.signed_request_account).to be_nil
end
end
describe '#signature_verification_failure_reason' do
it 'contains an error description' do
controller.signed_request_account
expect(controller.signature_verification_failure_reason[:error]).to eq 'Signed request date outside acceptable time window'
end
end
end
context 'with inaccessible key' do
@ -171,6 +218,7 @@ describe ApplicationController, type: :controller do
context 'with body' do
before do
allow(controller).to receive(:actor_refresh_key!).and_return(author)
post :success, body: 'Hello world'
fake_request = Request.new(:post, request.url, body: 'Hello world')
@ -189,22 +237,67 @@ describe ApplicationController, type: :controller do
it 'returns an account' do
expect(controller.signed_request_account).to eq author
end
end
it 'returns nil when path does not match' do
context 'when path does not match' do
before do
request.path = '/alternative-path'
expect(controller.signed_request_account).to be_nil
end
it 'returns nil when method does not match' do
describe '#signed_request_account' do
it 'returns nil' do
expect(controller.signed_request_account).to be_nil
end
end
describe '#signature_verification_failure_reason' do
it 'contains an error description' do
controller.signed_request_account
expect(controller.signature_verification_failure_reason[:error]).to include('using rsa-sha256 (RSASSA-PKCS1-v1_5 with SHA-256)')
expect(controller.signature_verification_failure_reason[:signed_string]).to include("(request-target): post /alternative-path\n")
end
end
end
context 'when method does not match' do
before do
get :success
expect(controller.signed_request_account).to be_nil
end
it 'returns nil when body has been tampered' do
describe '#signed_request_account' do
it 'returns nil' do
expect(controller.signed_request_account).to be_nil
end
end
end
context 'when body has been tampered' do
before do
post :success, body: 'doo doo doo'
expect(controller.signed_request_account).to be_nil
end
describe '#signed_request_account' do
it 'returns nil when body has been tampered' do
expect(controller.signed_request_account).to be_nil
end
end
end
end
end
context 'when a signature is required' do
before do
get :signature_required
end
context 'without signature header' do
it 'returns HTTP 401' do
expect(response).to have_http_status(401)
end
it 'returns an error' do
expect(Oj.load(response.body)['error']).to eq 'Request not signed'
end
end
end
end