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.

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

Issue #433: Fretboard: Certificate pinning #446

Conversation

Projects
None yet
2 participants
@fercarcedo
Copy link
Contributor

commented Jul 18, 2018

This pull request introduces certificate pinning. It adds a pinCertificates method to ExperimentSource with a set of base64-encoded SHA-256 of the certificate subject public key info.

The implementation first gets the list of trusted certificates as detailed, cleaning the certificate chain using X509TrustManagerExtensions as described here and here.

I also discovered there are libraries available such as TrustKit which replicate the behavior of Android N+ Network Security Configuration, but I decided against using it because it adds extra bloat to the library and also the user has to create an XML file and register it on the Manifest, I don't know if you have different opinion.

Closes #433

@fercarcedo fercarcedo force-pushed the fercarcedo:fretboard-certificate-pinning branch from c4b3180 to 0a99d36 Jul 18, 2018

@codecov

This comment has been minimized.

Copy link

commented Jul 18, 2018

Codecov Report

Merging #446 into master will decrease coverage by 0.02%.
The diff coverage is 76.47%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #446      +/-   ##
============================================
- Coverage     77.33%   77.31%   -0.03%     
- Complexity      955      966      +11     
============================================
  Files           146      147       +1     
  Lines          3446     3478      +32     
  Branches        487      490       +3     
============================================
+ Hits           2665     2689      +24     
- Misses          534      540       +6     
- Partials        247      249       +2
Impacted Files Coverage Δ Complexity Δ
...ce/fretboard/source/kinto/KintoExperimentSource.kt 90% <100%> (+0.34%) 13 <2> (+1) ⬆️
...tboard/source/kinto/HttpURLConnectionHttpClient.kt 79.16% <75%> (-3.19%) 9 <3> (+2)
...ervice/fretboard/source/kinto/CertificatePinner.kt 75% <75%> (ø) 8 <8> (?)

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 40d000b...cdf123a. Read the comment docs.

@pocmo pocmo added the 🛑 blocked label Jul 26, 2018

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

Blocked:

  • Pending security review
@pocmo

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

@fercarcedo Can you rebase this branch? There are some conflicts here. :)

@fercarcedo fercarcedo force-pushed the fercarcedo:fretboard-certificate-pinning branch from 0a99d36 to f8fc311 Jul 26, 2018

@fercarcedo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

Rebased

@fercarcedo fercarcedo force-pushed the fercarcedo:fretboard-certificate-pinning branch from f8fc311 to f11af34 Jul 26, 2018

@fercarcedo fercarcedo force-pushed the fercarcedo:fretboard-certificate-pinning branch 4 times, most recently from f7bf463 to cdf123a Aug 7, 2018

Issue #433: Fretboard: Certificate pinning
Closes #433: Fretboard: Certificate pinning
@pocmo

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

Given the feedback of the security team I think we should move forward without certificate pinning. Signature validation is a stronger signal and does not have all the problems that come with cert pinning.

@pocmo pocmo closed this Aug 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.