Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve public key pinning #17

Closed
rugk opened this issue Sep 17, 2016 · 10 comments · Fixed by #55
Closed

Improve public key pinning #17

rugk opened this issue Sep 17, 2016 · 10 comments · Fixed by #55
Labels
enhancement help wanted

Comments

@rugk
Copy link
Contributor

rugk commented Sep 17, 2016 •
edited
Loading

Currently the fingerprint of the certificate is pinned.
There are things to improve this pinning:

  1. When cert expires and/or is renewed,. the fingerprint will change, so this breaks clients regularly!
    To prevent this, pin the public key of the key pair instead of the fingerprint.
  2. When the key pair has to be changed (e.g. in case of a server breach) there should be a backup key pinned, which can be used in this case. As Threema uses HPKP, they specify one. More information here.

As this may be an upstream issue with aiohttp, I also opened an issue there: aio-libs/aiohttp#1187
@lgrahl
Copy link
Contributor

lgrahl commented Sep 27, 2016

Also related: aio-libs/aiohttp#1185

@rugk
Copy link
Contributor Author

rugk commented Sep 28, 2016

Yeah, but HPKP is about dynamic pinning. This issue here is about a static (hardcoded) pin.

@rugk rugk mentioned this issue Nov 9, 2016
Closed
@rugk
Copy link
Contributor Author

rugk commented Mar 27, 2017

Another good point made by @thorsteneckel: You still use SHA-1…

@rugk rugk mentioned this issue Mar 27, 2017
Open
@lgrahl
Copy link
Contributor

lgrahl commented Mar 27, 2017 •
edited
Loading

It's not like I have a choice. You should raise that issue towards the aiohttp project. :)
Seems this has been changed at some point. You can make a PR as I currently have zero time I can spend on this project.

@rugk
Copy link
Contributor Author

rugk commented Mar 27, 2017

Maybe @thorsteneckel could look into it? I think you've already looked into the code…

@thorsteneckel
Copy link

I'll take the next free slot of time and create a PR.

lgrahl added a commit that referenced this issue May 12, 2017
@lgrahl
Use SHA256 fingerprint (mentioned in #17)
b3def98
@lgrahl
Copy link
Contributor

lgrahl commented May 12, 2017

@rugk It was actually MD5 but don't tell anyone. 😨
Changed it to SHA256 a6840a28041a1c43d90c21122ea324272f5c90c82dd64f52701f3a8f1a2b395b. However, this of course does not resolve the issue of public key pinning.

@dbrgn
Copy link
Contributor

dbrgn commented May 19, 2017

@rugk is right about that one, pinning the fingerprint is sometimes not an option in production, since it might break the gateway without notice.

Verifying the public key and backup key (via HPKP) would be the better approach.

@thorsteneckel any news on a free timeslot? :)

@lgrahl
Copy link
Contributor

lgrahl commented May 19, 2017

That's why verifying the fingerprint is turned off by default at the moment... which of course isn't ideal.

@rugk
Copy link
Contributor Author

rugk commented Apr 13, 2019

Actually, the aiohttp devs do not really care for that feature here…

Also remember aio-libs/aiohttp#1187 is about static key pinning (what we need here) and aio-libs/aiohttp#1185 about dynamic one (i.e. HPKP). And I would bet I know the argument they'll come up with if you urge them to implement HPKP or implement it yourself in a PR…

lgrahl added a commit that referenced this issue May 17, 2021
@lgrahl
Remove fingerprint and verify_fingerprint from Connection
4a79901 
The fingerprint will change from time to time and hard-coding it in this
library we cannot forcibly deploy (unlike e.g. the Threema apps) is a
surprising footgun since your services may suddenly fail (when Threema changes
the fingerprint). As pointed out in #17, hard-coding the fingerprint (over the
public key) is also undesirable. Furthermore, we want users to use their
custom `aiohttp.ClientSession` instance. Therefore, we have decided to remove
it. If you want to retain this feature, all you have to do is provide your own
`aiohttp.ClientSession` in the following way:

    Connection(session=aiohttp.ClientSession(
        connector=aiohttp.TCPConnector(ssl=<fingerprint>)))

See the aiohttp docs for details.

Closes #17
Resolves #13 (by providing your own `SSLContext`)
@lgrahl lgrahl mentioned this issue May 17, 2021
Merged
lgrahl added a commit that referenced this issue May 17, 2021
@lgrahl
Remove fingerprint and verify_fingerprint from Connection
aaaeb5c 
The fingerprint will change from time to time and hard-coding it in this
library we cannot forcibly deploy (unlike e.g. the Threema apps) is a
surprising footgun since your services may suddenly fail (when Threema changes
the fingerprint). As pointed out in #17, hard-coding the fingerprint (over the
public key) is also undesirable. Furthermore, we want users to use their
custom `aiohttp.ClientSession` instance. Therefore, we have decided to remove
it. If you want to retain this feature, all you have to do is provide your own
`aiohttp.ClientSession` in the following way:

    Connection(session=aiohttp.ClientSession(
        connector=aiohttp.TCPConnector(ssl=<fingerprint>)))

See the aiohttp docs for details.

Closes #17
Resolves #13 (by providing your own `SSLContext`)
lgrahl added a commit that referenced this issue May 17, 2021
@lgrahl
Remove fingerprint and verify_fingerprint from Connection
648e865 
The fingerprint will change from time to time and hard-coding it in this
library we cannot forcibly deploy (unlike e.g. the Threema apps) is a
surprising footgun since your services may suddenly fail (when Threema changes
the fingerprint). As pointed out in #17, hard-coding the fingerprint (over the
public key) is also undesirable. Furthermore, we want users to use their
custom `aiohttp.ClientSession` instance. Therefore, we have decided to remove
it. If you want to retain this feature, all you have to do is provide your own
`aiohttp.ClientSession` in the following way:

    Connection(session=aiohttp.ClientSession(
        connector=aiohttp.TCPConnector(ssl=<fingerprint>)))

See the aiohttp docs for details.

Closes #17
Resolves #13 (by providing your own `SSLContext`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement help wanted
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove hard-coded fingerprint threema-ch/threema-msgapi-sdk-python
4 participants
@dbrgn @lgrahl @thorsteneckel @rugk