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

[cli] Fix development codesigning certificate validity checks #27361

Merged

Conversation

wschurman
Copy link
Member

@wschurman wschurman commented Feb 29, 2024

Why

We need to validate that all certificates in the chain are valid instead of just the leaf (development) certificate. What we're seeing is that we forgot to rotate the Expo Go intermediate certificate at the required cadence of 1 year, so the intermediate cert being served is not valid. https://github.com/expo/universe/pull/14632 and expo/code-signing-certificates#1 rotate the cert, but since the CLI caches the cert it'll continue to try to use the cached development (only if offline) and intermediate cert for up to 30 days (if the user had just generated one).

This will need to be cherry-picked and released. In the meantime, the recommendation is just to go online once and then go back offline if the user so desires.

How

Check that all certificates in the cached chain are valid, otherwise report cache as null and re-fetch current from server.

Test Plan

  1. create expo project
  2. npx expo start
  3. open in store version of Expo Go
  4. see "Certificate not valid" error
  5. Open codesigning cache file for project (~/.expo/codesigning/<project_id>/development-code-signing-settings-2.json)
  6. start in offline mode
  7. reload the app in Expo Go
  8. see "Offline and no cached development certificate found, unable to sign manifest" since cached stuff is invalidated

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Feb 29, 2024
@wschurman wschurman requested review from ide and quinlanj and removed request for byCedric and EvanBacon February 29, 2024 04:00
if (leafCertificate.validity.notBefore > now || leafCertificate.validity.notAfter < now) {
return null;

// ensure all intermediate certificates are valid (in case we forget to rotate)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not just forgetting to rotate, but if the signing certificate (the leaf) expires after the intermediate certificate, too, right?

Say the leaf expires on March 15 and the intermediate certificate expires on February 28. We rotate the intermediate certificate on February 27, but the client CLI uses the old intermediate certificate while it's in offline mode. You could make the argument that the leaf should not have such an expiration (feature for another PR?) but in this case, validating the intermediate certificate would be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically if we rotate every year this will never be the case, but yeah, I agree. I thought about that (setting the leaf expiration to MIN(+30 days, intemediate expiration)) but in the end the result would be the same (invalid chain).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's still a good idea to have the child's expiration not exceed its parent's - it's too easy (like in this scenario) to look at just the child and see it's unexpired and to think it's valid, but not a necessary constraint to implement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll create a separate PR on expo/code-signing-certificates repo to add the min logic above. Going to wait for the leap day bug to be over though so I don't confuse myself: expo/code-signing-certificates#14 (comment)

@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Mar 1, 2024
@wschurman
Copy link
Member Author

wschurman commented Mar 1, 2024

Probably not going to chery-pick this since the incident was resolved by just not running offline once (if the user is still experiencing symptoms). Still going to land this for correctness for the next version though.

@wschurman wschurman merged commit a92b889 into main Mar 1, 2024
5 of 6 checks passed
@wschurman wschurman deleted the @wschurman/fix-development-codesigning-validity-checks branch March 1, 2024 20:53
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint compatible bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants