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

feat(cli): add development code signing #16845

Merged
merged 1 commit into from Apr 5, 2022

Conversation

wschurman
Copy link
Member

@wschurman wschurman commented Mar 31, 2022

Why

This adds development code signing which will supersede legacy code signing. For now clients will continue to support and request both, but after a while we'll stop requesting the old style and then a while after that we'll stop supporting legacy code signing on this dev server.

The rough strategy for this is outlined in expo/code-signing-certificates#1.

Support for development certificates was added to the server in https://github.com/expo/universe/pull/9232 and https://github.com/expo/universe/pull/9349.

Support for chains (required for this feature) was added to the client in #16375 and #16634.

Depends on #16804.

Closes ENG-4480.

How

Inline docblocks and comments are probably the best way to describe the how, but the general approach is:

  1. Request comes in with keyid=expo-root or keyid=<user keyid>, we handle each one separately.
  2. We sign the manifest with the private key and send the signature back.

Test Plan

Run new and existing tests.

Manual tests:

  1. Load via curl, see correct response including cert chain and signature: curl --request GET \ --url 'http://192.168.0.113:19000/?platform=ios' \ --header 'accept: multipart/mixed' \ --header 'expo-expect-signature: sig, keyid="expo-root", alg="rsa-v1_5-sha256"'
  2. Open in expo go with debugger attached, see code signing works by breakpointing where it validates the signature: nexpo start --force-manifest-type=expo-updates

@wschurman wschurman force-pushed the @wschurman/expo-cli-multipart branch from 4861bf2 to 801051e Compare March 31, 2022 14:57
Base automatically changed from @wschurman/expo-cli-multipart to main March 31, 2022 15:07
@wschurman wschurman force-pushed the @wschurman/expo-cli-codesigning branch from 17c5f47 to 5ded4d7 Compare March 31, 2022 15:08
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Mar 31, 2022
@wschurman wschurman force-pushed the @wschurman/expo-cli-codesigning branch from 5ded4d7 to f6e65a7 Compare March 31, 2022 15:12
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Mar 31, 2022
@wschurman wschurman marked this pull request as ready for review March 31, 2022 15:18
@wschurman wschurman requested a review from ide March 31, 2022 15:19
@linear
Copy link

linear bot commented Mar 31, 2022

Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

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

There are two functions: createCachedFetch and wrapFetchWithOffline you may want to look at. I don't think they quite meet the needs here since createCachedFetch doesn't expire content (I think).

It is also worth looking at the cacache library that is already used by createCachedFetch. It's npm's caching implementation and guards against corruption. This is maybe preferable compared to using "fs" directly.

packages/@expo/cli/src/utils/codesigning.ts Outdated Show resolved Hide resolved
packages/@expo/cli/src/utils/codesigning.ts Outdated Show resolved Hide resolved
packages/@expo/cli/src/utils/codesigning.ts Outdated Show resolved Hide resolved
packages/@expo/cli/src/utils/codesigning.ts Outdated Show resolved Hide resolved
packages/@expo/cli/src/utils/codesigning.ts Outdated Show resolved Hide resolved
@wschurman wschurman requested a review from ide April 4, 2022 17:39
@@ -77,6 +78,7 @@ export const expoStart: Command = async (argv) => {

--dev-client Experimental: Starts the bundler for use with the expo-development-client
--force-manifest-type <manifest-type> Override auto detection of manifest type
--private-key-path <path> Path to private key for code signing. Default: "private-key.pem" in the same directory as the certificate specified by the expo-updates configuration in app.json.
Copy link
Contributor

Choose a reason for hiding this comment

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

The start command does a lot of stuff, maybe signing-key would be a more useful name? This would be akin to serve CLI which uses --ssl-key.

Suggested change
--private-key-path <path> Path to private key for code signing. Default: "private-key.pem" in the same directory as the certificate specified by the expo-updates configuration in app.json.
--signing-key <path> Path to private key for code signing. Default: "private-key.pem" in the same directory as the certificate specified by the expo-updates configuration in app.json.

Copy link
Member Author

Choose a reason for hiding this comment

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

eas-cli uses --private-key-path so I'm tempted to keep it for consistency. That being said I don't feel too strongly about naming so I'm happy to change this if you think the name is clearer and the benefit outweighs the cost of inconsistency (I guess we could rename the eas-cli one as well though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do like the consistency aspect so I'm not in love with changing the arg.

I just checked the help of eas update and noticed a bit of a naming inconsistency there as well which leads me to think it doesn't matter much (dir, path)

--input-dir=<value>
--private-key-path=<value> 

Personally I like the shorter argument names because I'm lazy.

@wschurman wschurman requested a review from EvanBacon April 4, 2022 22:17
Copy link
Contributor

@EvanBacon EvanBacon left a comment

Choose a reason for hiding this comment

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

Added some notes about formatting (my bad for not updating the contributing). Thanks for the test coverage!

Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

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

Approving (+ Evan's feedback)

@wschurman wschurman force-pushed the @wschurman/expo-cli-codesigning branch from 3bebe72 to a570071 Compare April 5, 2022 17:40
@wschurman wschurman merged commit e377ff8 into main Apr 5, 2022
@wschurman wschurman deleted the @wschurman/expo-cli-codesigning branch April 5, 2022 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants