feat: make static certificate pinning optional

[roschaefer]

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 #19

Merge after #22 and #23

I have questions: Why are Threema and Threema::Client two different classes? They seem very entangled.

[codecov[bot]]

Codecov Report

Merging #24 (a24183d) into master (ba3dbb8) will increase coverage by 0.99%. The diff coverage is 40.00%.

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage   35.63%   36.62%   +0.99%     
==========================================
  Files          13       13              
  Lines         348      344       -4     
==========================================
+ Hits          124      126       +2     
+ Misses        224      218       -6     

Impacted Files	Coverage Δ
lib/threema.rb	34.48% <0.00%> (ø)
lib/threema/client.rb	36.98% <46.15%> (+4.51%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ba3dbb8...a332c9e. Read the comment docs.

[thorsteneckel]

Hi @roschaefer - thanks! Following up on the Threema discussions with the other API maintainers: How should we proceed here?

[roschaefer]

@thorsteneckel I haven't found time to update this PR, but I will either a) try to make verify_callback part of the API of this gem (to allow for static certificate pinning, as optional feature) or b) if that's too complex/hard-to-maintain - remove it.

You could however summarize the discussion we had on the API maintainers chat. It adds transparency to the Github repository and why things have been decided etc.

I like community chats for real-time communication but I prefer Github as a paper trail of discussions.

[rugk]

Well… the discussion just circled around the usefulness of key pinning. We've noted it's a big risk if people have old implementations that break when Threema updates their cert. Even if announced a whole lot earlier, there is still a risk of breaking. I'd say that static key pinning is of course less-problematic than HPKP and say that an opt-in approach with a big warning that explains what an opt-in means and what developers/users of the Threema API then should pay attention to, is the way to go and still allows those who want to use this additional security feature, to use it.

Yes, and I generally like to keep discussions on GitHub for said reasons. :wink:

[roschaefer]

This is now ready for review, but merge #23 first and rebase this PR to make the diff more readable.

[thorsteneckel]

Beautiful!

[roschaefer]

@thorsteneckel could you tell me if we have master branch-protected in this repo? I'm a little afraid that I might make an accidental git push origin -u HEAD --force in the wrong repo (now after I'm contributor).

[thorsteneckel]

Now it's protected and PRs need at least one reviewer.