-
Notifications
You must be signed in to change notification settings - Fork 32
feat: android aab signing #593
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
base: main
Are you sure you want to change the base?
feat: android aab signing #593
Conversation
🦋 Changeset detectedLatest commit: 321b308 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@mlisikbf is attempting to deploy a commit to the Callstack Team on Vercel. A member of the Team first needs to authorize it. |
function isAab(filePath: string): boolean { | ||
return path.extname(filePath).toLowerCase() === '.aab'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opted to switch on path extension rather than adding a separate --aab
flag, as that should be enough to distinguish and the flag seemed superfluous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run prettier here and there (this one is missing trailing newline)
It's on my list, will come back to this shortly! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks for the contribution! Just the few comments to be resolved + I think we should decide to go with apksigner
instead of jarsigner
to make it more secure & follow Android docs' recommendations.
packages/platform-android/src/lib/commands/signAndroid/signAndroid.ts
Outdated
Show resolved
Hide resolved
packages/platform-android/src/lib/commands/signAndroid/signAndroid.ts
Outdated
Show resolved
Hide resolved
]; | ||
|
||
try { | ||
await spawn('jarsigner', jarsignerArgs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be better to use apksigner actually. Jarsigner is not recommended in the docs and it was not created for signing APKs, therefore e.g. it:
- does not know that APKs for Android <= 17 cannot be signed with SHA-256 digests ([major] - I also don't see the code that would adjust that), while apktool detects & adjusts that automatically
- [major] there are security concerns that come from jarsigner, which constitues Android's v1 signing scheme: it only signs parts of the ZIP (APK / AAB), e.g. ZIP metadata is not signed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting. I assumed apksigner won't work based on bundletool docs explicitly calling out not to use it. Testing locally, it works and only requires a min-sdk-version
param, and using bundletool
I can get apks out of the resigned bundle that pass apksigner verify
(and install & run fine).
Will need to test it in our pipeline (with play store test track submission), and come back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to hear that! Sure, let's wait for the result - thanks for checking this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the min-sdk-version
is required, you can take default version from the template (currently 24) and add a --min-sdk-version
flag where users will be able to overwrite it when necessary. Btw can you check if the tool fails when sdk mismatches? that would be ideal as we could point users to that flag to avoid confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this indicates we shouldn't use apksigner, or just that it's API is tailored for use with apks.
I've tested a few combinations with --min-sdk-version
, and in each one, I can successfully produce apks from the bundle locally using bundletool
-- and by uploading the bundle to play console and downloading an apk from there. It would seem that the value of --min-sdk-version
is irrelevant when signing an aab:
- project with min sdk 34,
--min-sdk-version
lower - 0, 24 - project with min sdk 24,
--min-sdk-version
higher - 34, 36 - project with min sdk 34,
--min-sdk-version
higher - 36
All tested on a device with sdk 34 / Android 14, using bundletool build-apks --connected-device
and bundletool install-apks
to test locally.
In apksigner docs, there's this note on --min-sdk-version
:
Higher values allow the tool to use stronger security parameters when signing the app but limit the APK's availability to devices running more recent versions of Android
In our case, I can install on lower version when setting --min-sdk-version
high for aab signing, because at a later point - when prepared by play store or built by bundletool, the individual apks will be signed again (and will read sdk versions from manifest)
Updated the PR to remove jarsigner-related bits. This reduces the change set to extension-agnostic naming, different asset path and --min-sdk-version added for aabs.
Added a --min-sdk-version arg to the command. Set it to default to 36 for aabs, just so that stronger security parameters are used for signing. User can alternatively provide an override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking!
I'm looking at apksigner help around this flag:
--max-sdk-version Highest API Level on which this APK's signatures will be
verified. By default, the highest possible value is used.
maybe we don't need to expose it? We can add it later if someone requests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one you link is max
not min
-- but it did cross my mind that it might be better no to expose --min-sdk-version
and just have a default for aabs. Will remove it just to keep the PR focused on aabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, let's do that.
// 6. Align archive after signing if aab | ||
if (isAab(outputPath)) { | ||
await alignArchive() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The post-signing alignment for AABs also overwrites the signed outputPath with the unsigned tempArchivePath, losing the signature
This bot's comment is actually incorrect, we can ignore that (LLM trash as usual). The issue is exactly why apksigner is preferred over jarsigner, because jarsigner means Android v1 signature. apksigner uses v2,3 or 4 signing which is whole-file signing and therefore there is no way to tamper with the file after signing and therefore signing is needed after alignment. However, if we move to apksigner (as I suggested in a comment below), we would then have to keep in mind the (only) right way would be align-then-sign.
also, can you send me some materials on why AAB needs aligning after signing, while APK does it before?
It's not that they need it that way, it results from the tool choice. Jarsigner (legacy, which constitutes the old Android singing scheme v1) only signs parts of the ZIP (APK / AAB), and the signatures don't cover ZIP metadata. Zipalign stores the actual alignment information in the ZIP metdata (not covered by the signatures) and this is why the signing should occur after alignment.
Apksigner signs according to v2+ Android signing schemes which treat the whole file as a blob (including ZIP metadata). Therefore, realigning means invalidating the signature.
Some information on that can be found here: https://source.android.com/docs/security/features/apksigning#v1
packages/platform-android/src/lib/commands/signAndroid/command.ts
Outdated
Show resolved
Hide resolved
packages/platform-android/src/lib/commands/signAndroid/signAndroid.ts
Outdated
Show resolved
Hide resolved
return []; | ||
} | ||
|
||
return ['--min-sdk-version', minSdkVersion || '36']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minSdkVersion
is 24. 36 is the compileSdkVersion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree to keep 36 given it does not appear we need to match actual min sdk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it fail if you don’t have sdk 36 or silently pass? If I read the man correctly it will set the latest available so I wonder if we even need to set that flag.
Minor tweaks and we're good to merge this today |
…roid.ts Co-authored-by: Michał Pierzchała <thymikee@gmail.com>
Summary
Introduces aab signing using the existing
yarn sign:android
command, with the same list of arguments, just provide a path to an .aab file.signAndroid
will check file extension to decide on:Includes naming changes, updates to messages and docs where appropriate.
An alternative would be for end-users to invoke jarsigner themselves, extract and modify the bundle as needed.
Test plan
(cd packages/platform-android && npm link)
npm rock create
(select android, skip plugins)npm link @rock-js/platform-android
npm rock create-keystore:android
- use default values, for simplicity usefake-pass
when prompted for passwordnpx rock build:android --aab --local --variant release
(note: using--local
to opt out of cache as it does not seem to catch js changes for release variants)<WelcomeScreen/>
with custom jsx)npx rock bundle --platform android --entry-file index.js --bundle-output output.bundle
npx rock sign:android ./android/app/build/outputs/bundle/release/app-release.aab --keystore ./android/app/release.keystore --keystore-password fake-pass --key-password fake-pass --key-alias rock-alias --jsbundle ./output.bundle
bundletool
--brew install bundletool
thenbundletool build-apks --bundle=./android/app/build/outputs/bundle/release/app-release.aab --output=output/apks.apks --mode=universal --local-testing --ks=./android/app/release.keystore --ks-key-alias=rock-alias --ks-pass=pass:fake-pass --key-pass=pass:fake-pass
unzip output/apks.apks -d output
and install on a connected device withadb install output/universal.apk
revert changes and verify apk signing:
npx rock build:android --local --variant release
closes #588
Note
Generalizes Android signing to handle both APK and AAB, updating args, signing/align flow, bundle replacement paths, and docs.
@rock-js/platform-android
)sign:android
: Now signs bothAPK
andAAB
based on file extension.apksigner
for APK andjarsigner
for AAB; adjusts zipalign order accordingly.assets
(APK) orbase/assets
(AAB).binaryPath
throughout (apk
->binaryPath
/path
) and updates messages.alignArchiveFile
,signAab
,signApk
,isAab
, and password handling forjarsigner
.website/src/docs/cli.md
forsign:android
to acceptAPK
orAAB
and reflect newbinaryPath
argument and output descriptions.@rock-js/platform-android
.Written by Cursor Bugbot for commit 321b308. This will update automatically on new commits. Configure here.