Repository navigation
Update cert pinning logic - #100518
Update cert pinning logic#100518staszekscp wants to merge 0 commit into
Conversation
|
|
|
@nyomanjyotisa Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 342d4d6a37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [ ! -s "${tmpdir}/cert_1.pem" ]; then | ||
| printf ' %-34s UNREACHABLE (could not fetch chain)\n' "$host" | ||
| rm -rf "$tmpdir" | ||
| return |
There was a problem hiding this comment.
When --verify cannot fetch a host's certificate chain (for example, due to a DNS, routing, or TLS outage), this path only prints UNREACHABLE and returns without setting VERIFY_FAILED. Consequently, a run where one or every pinned endpoint could not be checked still exits successfully, so CI or a release validation can report that pin verification passed without verifying all live chains.
Useful? React with 👍 / 👎.
| for name in "${needed[@]}"; do | ||
| if [ "$fp" = "$(manifest_fingerprint "$name")" ]; then | ||
| openssl x509 -in "$split" -out "${ROOTS_DIR}/${name}.pem" 2>/dev/null | ||
| fi |
There was a problem hiding this comment.
With --refresh, every manifest root is considered needed, but an existing cached PEM is left in place when its certificate is absent from the newly downloaded bundle. The subsequent cached_root_ok check only compares that stale PEM to the manifest fingerprint, so the command succeeds and regenerates pins from the old cache rather than detecting that Mozilla has removed the root—the distrust condition this refresh flow is meant to surface.
Useful? React with 👍 / 👎.
| local failed=0 | ||
| for name in "${needed[@]}"; do | ||
| if ! cached_root_ok "$name"; then | ||
| echo "ERROR: root '${name}' was not found in the CA bundle (or failed fingerprint" >&2 |
There was a problem hiding this comment.
Could you check the clean-checkout path? It fails for me because the current Mozilla bundle does not contain GTS Root R2.
Also, I think we should add regression tests for clean generation, refresh, and unreachable verification.
| // Try the next authType; fall back to the raw peer chain if all fail. | ||
| } | ||
| } | ||
| return peerCertificates |
There was a problem hiding this comment.
Could you confirm this fallback? If chain rebuilding fails, the raw chain for integrations.expensify.com has no pinned root, so I think it may report a false mismatch.
| "staging.travel.expensify.com" to listOf("sha256/Qb3qmTdRt/xHEN5PVtn+YhKoGqF/lhRX88cSFuSCJqM=", "sha256/kIdp6NNEd8wsugYyyIYFsi1ylMCED3hZbSR8ZFsa/A4="), | ||
| // Group E: CloudFront CDN leaf + Amazon RSA 2048 M01 intermediate | ||
| "d2k5nsl2zxldvw.cloudfront.net" to listOf("sha256/P9HBoLji8YncXSnb0AnAm72fJO/vpmxZrsl4fvUBkxc=", "sha256/DxH4tt40L+eduF6szpY6TONlxhZhBd+pJ9wbHlQ2fuw="), | ||
| // Groups A-D: Cloudflare-fronted expensify.com hosts. Cloudflare can rotate the edge cert |
There was a problem hiding this comment.
Could you confirm whether any of these pinned Cloudflare hostnames can use Sectigo backup certificates? Cloudflare lists Sectigo as a possible backup CA, but it is not included here.
|
Thanks for the review, I've applied the requested changes! |
|
I could not review this diff, so I did not approve the workflow runs. An Expensify employee will review it and approve them manually. |
96bb23a to
93ee237
Compare
|
I could not review this diff, so I did not approve the workflow runs. An Expensify employee will review it and approve them manually. |
|
There is one unsigned commit up there. For simplicity I'll create a new branch and lint this PR there |
|
Closed in favor of #100965 |
2 participants
Explanation of Change
Switches NewDot's certificate pins from leaf + issuing intermediate to root CA only.
Leaf/intermediate pins have already broken twice in production — Cloudflare rotated the www/secure edge cert from Let's Encrypt to Google Trust Services (2026-07-07), and Amazon rotated CloudFront's intermediate from RSA 2048 M01 to M04 (2026-07). Leaves are re-keyed on every renewal and every CA issues from a rotating pool of intermediates, so roots are the only durable pin target. Pinning roots survives leaf rotation, intermediate rotation, and (since all three Cloudflare CAs are pinned) a CA switch, without an emergency release.
Pins are now:
Pinned hosts are unchanged: www, secure, new, integrations, travel (.expensify.com), their staging.* counterparts, and d2k5nsl2zxldvw.cloudfront.net.
Updated across all pin targets — config/certificatePinning/pins.json (canonical), CertificatePinning.kt (OkHttp), CertificatePinning.swift (TrustKit), the react-native-nitro-fetch Cronet patch, and network_security_config_enforce.xml.
Two supporting changes:
anchoredChain() in CertificatePinning.kt — a TLS server never sends its root, so the OkHttp and HttpsURLConnection monitors now rebuild the validated chain up to its trust anchor (X509TrustManagerExtensions.checkServerTrusted) before checking pins. Without it root pins could never match there and every request would report a false mismatch. Android , OkHttp enforce mode, TrustKit and Cronet include the anchor natively.
scripts/generateCertificatePins.sh — for the same reason, roots can't be read off a live handshake, so the script now downloads the Mozilla CA bundle and verifies each root against SHA-256 fingerprints committed in its ROOT_MANIFEST (the fingerprints, not the download, are the source of trust). Adds --verify (confirms each live chain anchors at a pinned root), --refresh and --android, plus ROOTS_BUNDLE= for offline/CI use. The root cache is gitignored.
Still monitor-only (enforcePinning = false everywhere): mismatches are reported to Sentry, connections are not blocked. The root pin-set for Android lives in network_security_config_enforce.xml, ready to copy over when we flip to enforce.
No JS or UI changes.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/633469
PROPOSAL:
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/14106
Tests
Run a release build on Android and iOS (pinning is a no-op in debug). Throughout, check Sentry for certificate_pinning_* events — there should be none.
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2026-09-09.at.14.15.51.mov
Android: mWeb Chrome
iOS: Native
Simulator.Screen.Recording.-.iPhone.17.Pro.-.2026-09-09.at.14.05.34.mov
iOS: mWeb Safari
MacOS: Chrome / Safari