Skip to content

Commit

Permalink
feat: make static certificate pinning optional
Browse files Browse the repository at this point in the history
WHY: I chatted with Threema developers and one of them discouraged me
from implementing static certificate pinning because they have no workflow
to inform developers of upcoming changes to their HTTPS certificates.

So that's why we have at least
* get rid of the hardcoded HTTPS fingerprint, because it's
unmaintainable
* disable static certificate pinning by default

Nevertheless, @rugk writes that static certificate pinning is a useful
feature and does not suffer from the problems of HTTP public key
pinning, see: https://en.wikipedia.org/wiki/HTTP_Public_Key_Pinning

So I changed the source code in such a way that it's still possible to
configure the used HTTP client for certificate pinning as it used to be.

This commit includes a refactoring: DRY the threema client by passing a
reference to `threema` instance. Thus, the client does not have to
remember it's own `private_key`, `api_identity` and `api_secret`. Less
redundancy, less errors.

Fix threemarb#19
  • Loading branch information
roschaefer committed Feb 27, 2021
1 parent 286b223 commit b5d9b3f
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 62 deletions.
2 changes: 1 addition & 1 deletion .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ Metrics/AbcSize:
# Configuration parameters: CountComments, CountAsOne, ExcludedMethods, IgnoredMethods.
# IgnoredMethods: refine
Metrics/BlockLength:
Max: 26
Max: 27

# Offense count: 9
# Configuration parameters: CountComments, CountAsOne, ExcludedMethods, IgnoredMethods.
Expand Down
25 changes: 21 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,31 @@ require 'threema'
threema = Threema.new()
```

By default HTTP Public Key Pinning is enabled to ensure the validity of the Threema Gateway HTTPS certificate. However in those special moments it might be necessary to disable it. Please do this only if you know what you are doing!
If you want, you can configure static certificate pinning like this:

```ruby
require 'threema'

threema = Threema.new(
public_key_pinning: false,
)
threema = Threema.new
threema.client.configure do |config|
# Threema API fingerprint as of 2021-02-27
fingerprint = '42b1038e72f00c8c4dad78a3ebdc6d7a50c5ef288da9019b9171e4d675c08a17'

# See: http://stackoverflow.com/a/22108461
config.use_ssl = true

config.verify_mode = OpenSSL::SSL::VERIFY_PEER

config.verify_callback = lambda do |preverify_ok, cert_store|
return false unless preverify_ok

end_cert = cert_store.chain[0]
return true unless end_cert.to_der == cert_store.current_cert.to_der

remote_fingerprint = OpenSSL::Digest::SHA256.hexdigest(end_cert.to_der)
remote_fingerprint == fingerprint
end
end
```

#### Sending
Expand Down
8 changes: 2 additions & 6 deletions lib/threema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,12 @@
class Threema
attr_reader :client, :private_key, :api_identity, :api_secret

def initialize(api_identity: nil, api_secret: nil, private_key: nil, public_key_pinning: nil)
def initialize(api_identity: nil, api_secret: nil, private_key: nil)
@api_identity = api_identity || ENV['THREEMARB_API_IDENTITY']
@api_secret = api_secret || ENV['THREEMARB_API_SECRET']
@private_key = private_key || ENV['THREEMARB_PRIVATE']

@client = Threema::Client.new(
api_identity: @api_identity,
api_secret: @api_secret,
public_key_pinning: public_key_pinning
)
@client = Threema::Client.new(threema: self)
end

def send(args)
Expand Down
50 changes: 21 additions & 29 deletions lib/threema/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,26 @@

class Threema
class Client
FINGERPRINT = 'a6840a28041a1c43d90c21122ea324272f5c90c82dd64f52701f3a8f1a2b395b'
API_URL = 'https://msgapi.threema.ch'
API_URL = 'https://msgapi.threema.ch'

attr_reader :threema

class << self
def url(path = '')
"#{API_URL}#{path}"
end
end

def initialize(api_identity:, api_secret:, public_key_pinning: true)
@api_identity = api_identity
@api_secret = api_secret
@public_key_pinning = public_key_pinning
def initialize(threema:)
@threema = threema
@configuration = lambda do |config|
config.use_ssl = true
config.verify_mode = OpenSSL::SSL::VERIFY_PEER
end
end

def configure(&block)
@configuration = block
end

def get(url, params = {})
Expand All @@ -40,8 +47,8 @@ def post_form_urlencoded(url, payload)
uri = URI(url)
req = Net::HTTP::Post.new(uri)
req.set_form_data(payload.merge(
from: @api_identity,
secret: @api_secret,
from: threema.api_identity,
secret: threema.api_secret,
))

request_https(uri, req)
Expand Down Expand Up @@ -97,26 +104,11 @@ def not_found_ok(&_block)

private

def request_https(uri, req)
http = Net::HTTP.new(uri.host, uri.port).tap do |config|
# SSL activation and HTTP Public Key Pinning - yay!
# taken and inspired by:
# http://stackoverflow.com/a/22108461
config.use_ssl = true

config.verify_mode = OpenSSL::SSL::VERIFY_PEER

config.verify_callback = lambda do |preverify_ok, cert_store|
return false unless preverify_ok
attr_reader :configuration

end_cert = cert_store.chain[0]
return true unless end_cert.to_der == cert_store.current_cert.to_der
return true unless @public_key_pinning

remote_fingerprint = OpenSSL::Digest::SHA256.hexdigest(end_cert.to_der)
remote_fingerprint == FINGERPRINT
end
end
def request_https(uri, req)
http = Net::HTTP.new(uri.host, uri.port)
http.tap(&configuration) if configuration

# for those special moments...
# http.set_debug_output($stdout)
Expand All @@ -132,8 +124,8 @@ def authed_uri(url, params = {})

def authed(params = {})
params.merge(
from: @api_identity,
secret: @api_secret,
from: threema.api_identity,
secret: threema.api_secret,
)
end
end
Expand Down
3 changes: 1 addition & 2 deletions spec/factories/threema_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@

FactoryBot.define do
factory :threema_client, class: Threema::Client do
api_identity { test_from }
api_secret { test_api_secret }
threema { FactoryBot.build(:threema) }

initialize_with do
new(attributes)
Expand Down
74 changes: 54 additions & 20 deletions spec/threema/client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
expect(described_class).to respond_to(:url)
end

let(:instance) { build(:threema_client) }
let(:instance) { build(:threema_client, threema: threema) }
let(:threema) { build(:threema) }

context '#not_found_ok' do
it 'responds to not_found_ok' do
Expand All @@ -37,30 +38,63 @@
end
end

context 'HTTP Public Key Pinning' do
before(:all) do
WebMock.allow_net_connect!
end
describe '#configure' do
subject { -> { instance.get(url) } }
context 'with static certificate pinning' do
let(:public_key_pinning) { true }

after(:all) do
WebMock.disable_net_connect!
end
before(:all) do
WebMock.allow_net_connect!
end

# this is needed due to internet access restrictions
# in the (travis) CI environment
if !ENV['CI']
it 'checks the HTTP Public Key Pinning' do
expect { instance.get(described_class::API_URL) }.to raise_error(RequestError)
after(:all) do
WebMock.disable_net_connect!
end
end

it "throws OpenSSL::SSL::SSLError exception if HTTP Public Key Pinning doesn't match" do
expect { instance.get('https://requestb.in/zpi5cuzp') }.to raise_error(OpenSSL::SSL::SSLError)
end
before(:each) do
instance.configure do |config|
# Threema API fingerprint as of 2021-02-27
fingerprint = '42b1038e72f00c8c4dad78a3ebdc6d7a50c5ef288da9019b9171e4d675c08a17'

# See: http://stackoverflow.com/a/22108461
config.use_ssl = true

config.verify_mode = OpenSSL::SSL::VERIFY_PEER

config.verify_callback = lambda do |preverify_ok, cert_store|
return false unless preverify_ok

end_cert = cert_store.chain[0]
return true unless end_cert.to_der == cert_store.current_cert.to_der
return true unless public_key_pinning

it 'throws no exception if HTTP Public Key Pinning is disabled' do
instance = build(:threema_client, public_key_pinning: false)
expect { instance.get('https://requestb.in/zpi5cuzp') }.to_not raise_error
remote_fingerprint = OpenSSL::Digest::SHA256.hexdigest(end_cert.to_der)
remote_fingerprint == fingerprint
end
end
end

# this is needed due to internet access restrictions
# in the (travis) CI environment
if !ENV['CI']
context 'given Threema Message API URL with matching certificate' do
let(:url) { described_class::API_URL }
it 'checks HTTP Public Key Pinning' do
should raise_error(RequestError)
# not OpenSSL::SSL::SSLError
end
end
end

context 'given another URL without matching certificate' do
let(:url) { 'https://github.com/thorsteneckel/threema' }
it { should raise_error(OpenSSL::SSL::SSLError) }

context 'but if static certificate pinning is disabled' do
let(:public_key_pinning) { false }
it { should_not raise_error }
end
end
end
end

Expand Down

0 comments on commit b5d9b3f

Please sign in to comment.