Public key pinning is nil and disabled by default - contrary to documentation

[roschaefer]

Hey @thorsteneckel I'm trying to fix tests locally for this gem. Tests for HTTP Public Key Pinning all fail.

Documentation says:

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!

But looking into the source code:

1. https://github.com/thorsteneckel/threema/blob/master/lib/threema.rb#L20
2. https://github.com/thorsteneckel/threema/blob/master/lib/threema/client.rb#L20
3. https://github.com/thorsteneckel/threema/blob/master/lib/threema/client.rb#L108

Makes me believe, it's nil by default.

When I bin/rails c in our app that installed threema and do the following:

t = Threema.new
t.client

gives me

=> #<Threema::Client:0x00007f55293336c0
  #...
 @public_key_pinning=nil>

I think it's because of a misconception of named arguments:

def some_method(named_argument: true)
  p named_argument
end
some_method(named_argument: nil)
=> nil

I was using ruby 2.7.1p83

Could we maybe remove HTTP key pinning? Or do you prefer fixing HTTP key pinning?

[thorsteneckel]

Hi @roschaefer - HTTP pinning is where I stopped the development back in the days. Unfortunately I can't remember what stage it is in right now. I am in the Threema Group of API developers and we had some chats about it. See the created issues by @rugk from this time. If you're interested in being invited in this group let me know and I ask the admin(s).

The easy fix would be to remove it. My preferred way would be to have it properly implemented also covering the issues linked above. However, I won't find the time over the next 4 weeks as least. Therefore I can't make any demands.

I do agree that it is broken in its current implementation and should get fixed.

[roschaefer]

Hi @roschaefer - HTTP pinning is where I stopped the development back in the days. Unfortunately I can't remember what stage it is in right now. I am in the Threema Group of API developers and we had some chats about it. See the created issues by @rugk from this time. If you're interested in being invited in this group let me know and I ask the admin(s).

Of course I am interested.

@mattwr18 @tillprochaska @phoeinx you?

The easy fix would be to remove it. My preferred way would be to have it properly implemented also covering the issues linked above. However, I won't find the time over the next 4 weeks as least. Therefore I can't make any demands.

That is fine. It's called voluntary unpaid open-source maintenance.

I do agree that it is broken in its current implementation and should get fixed.

I have a follow-up question. Does HTTP public key pinning means that this gem needs to be updated as soon as possible when the public key of Threema Gateway API changes? In the meantime, applications wouldn't be able to receive messages anymore? In our use-case that would not be acceptable. For context, I haven't looked into public key pinning in detail but saw the public key in this gem is outdated.

Compare: https://github.com/thorsteneckel/threema/blob/master/lib/threema/client.rb#L11
(a6840a28041a1c43d90c21122ea324272f5c90c82dd64f52701f3a8f1a2b395b)

With:

     95: def request_https(uri, req)
     96:   http = Net::HTTP.new(uri.host, uri.port).tap do |config|
     97:     # SSL activation and HTTP Public Key Pinning - yay!
     98:     # taken and inspired by:
     99:     # http://stackoverflow.com/a/22108461
    100:     config.use_ssl = true
    101:
    102:     config.verify_mode = OpenSSL::SSL::VERIFY_PEER
    103:
    104:     config.verify_callback = lambda do |preverify_ok, cert_store|
    105:       return false unless preverify_ok
    106:       end_cert = cert_store.chain[0]
    107:       return true unless end_cert.to_der == cert_store.current_cert.to_der
    108:       return true unless @public_key_pinning
    109:
    110:       remote_fingerprint = OpenSSL::Digest::SHA256.hexdigest(end_cert.to_der)
 => 111:       binding.pry
    112:       remote_fingerprint == FINGERPRINT
    113:     end
    114:   end
    115:
    116:   # for those special moments...
    117:   # http.set_debug_output($stdout)
    118:
    119:   http.request(req)
    120: end

[1] pry(#<Threema::Client>)> remote_fingerprint
=> "42b1038e72f00c8c4dad78a3ebdc6d7a50c5ef288da9019b9171e4d675c08a17"

The article on Wikipedia starts with:

HTTP Public Key Pinning (HPKP) is a now-deprecated Internet security mechanism...

[rugk]

Note HPKP has nothing to do with static key pinning, which we use here. Static key pinning does not have the problems of HPKP, so that's not an issue.

As for the wrong key, yeah, likely also a maintenance problem. One just needs to update it to the correct one. Some time ago they have notified the API consumers via mail about that change too: rugk/threema-msgapi-sdk-php#59

[thorsteneckel]

@mattwr18 @tillprochaska @phoeinx @roschaefer please send me your Threema IDs to support at zammad.com, refer to Thorsten and this issue. I'll forward your IDs to the admins.