Auth: add package override support to getTokenWithAccount (#3278)#3468
Auth: add package override support to getTokenWithAccount (#3278)#3468vijay12481248-lab wants to merge 5 commits into
Conversation
Mirrors the override logic from AccountAuthenticator.getAuthToken into AuthManagerServiceImpl.getTokenWithAccount so that apps calling this path directly (e.g. patched YouTube flavors) also benefit from package overrides, fixing UNREGISTERED_ON_API_CONSOLE errors.
Addresses all code review issues from PR microg#3468: Critical Fixes: - Fix overridePackage fallback default that triggered override for all callers - Add bounds check for getCertificates().get(0) to handle empty cert lists - Include KEY_ACCOUNT_NAME and KEY_ACCOUNT_TYPE in NeedPermission result Medium Fixes: - Fix override trigger condition: only check KEY_OVERRIDE_PACKAGE (not OR) - Use 'SHA-1' (with hyphen) instead of 'SHA1' for Java MessageDigest standard - Remove putExtras(extras) that leaked sensitive caller data to activity Minor Improvements: - Clean up variable scoping for override-related variables - Add defensive error handling for edge cases All changes maintain backward compatibility and no API modifications. Package override logic now properly mirrors AccountAuthenticator.getAuthToken behavior. Fixes: microg#3468
…ure block The setPackageSignature block had certs.get(0) inside an isEmpty() guard, but the safety was implicit and fragile. Restructure to: - Fetch overrideCert from extras bytes if present (no cert lookup needed) - Otherwise call getCertificates and use 'certs.isEmpty() ? null : certs.get(0)' - Null-check overrideCert before calling setPackageSignature This makes null safety self-evident without relying on an outer isEmpty() wrapper, addressing code review feedback on PR microg#3468.
…resolution The override cert was computed twice — once in the isOverrideAllowed block and again below for setPackageSignature. Consolidate into a single resolution: - When overridePackage is provided, resolve and store overrideCert once - Pass it down for both the userdata key check and setPackageSignature - Use 'certs.isEmpty() ? null : certs.get(0)' explicitly (no outer guard) - Null-check overrideCert before calling setPackageSignature Addresses follow-up review comment on PR microg#3468.
matteozanettii
left a comment
There was a problem hiding this comment.
Hi @vijay12481248-lab , few observations regarding the implementation:
- Certificate Selection: The logic uses
certs.get(0), assuming the first certificate is the target. Have you considered edge cases with APK Signature Scheme v3 where key rotation might introduce multiple certificates? - SHA-1 Usage: I noticed
SHA-1is used forauthManager.setPackageSignature. While I assume this is a legacy requirement for the Auth servers, can you confirm if this is strictly necessary instead of moving toSHA-256? - Magic Strings: There are several hardcoded strings like
"NeedPermission","1", and specifically"delegatee_user_id". Extracting these into constants would prevent future typos and improve overall maintainability. - Broad Exception Catch: The
catch (Exception e)block when setting the override signature is quite broad. It might be safer to catch specific exceptions likeNoSuchAlgorithmExceptionto prevent swallowing unrelated errors.
|
|
Thanks for the review! 1. Certificate Selection ( 2. SHA-1 in |
| overrideCert = certs.isEmpty() ? null : certs.get(0); | ||
| } | ||
| if (overrideCert != null) { | ||
| authManager.setPackageSignature(PackageManagerUtilsKt.toHexString(PackageManagerUtilsKt.digest(overrideCert, "SHA-1"), "")); |
There was a problem hiding this comment.
| authManager.setPackageSignature(PackageManagerUtilsKt.toHexString(PackageManagerUtilsKt.digest(overrideCert, "SHA-1"), "")); | |
| authManager.setPackageSignature(PackageManagerUtilsKt.toHexString(PackageManagerUtilsKt.digest(overrideCert, "SHA-1"), "")); | |
| if (!effectivePackageName.equals(packageName)) { | |
| // SHA-1 is used here intentionally: Google's Auth servers require the package | |
| // signature in SHA-1 hex form for legacy compatibility. This is not used for | |
| // security decisions — the authorization check above uses SHA-256. | |
| byte[] overrideCertificateBytes = extras.getByteArray(AccountAuthenticator.KEY_OVERRIDE_CERTIFICATE); | |
| CertData overrideCert; | |
| if (overrideCertificateBytes != null) { | |
| overrideCert = new CertData(overrideCertificateBytes); | |
| } else { | |
| // See comment above about certs.get(0) and APK Signature Scheme v3 | |
| List<CertData> certs = PackageManagerUtilsKt.getCertificates(context.getPackageManager(), packageName); | |
| overrideCert = certs.isEmpty() ? null : certs.get(0); | |
| } | |
| if (overrideCert != null) { | |
| authManager.setPackageSignature(PackageManagerUtilsKt.toHexString(PackageManagerUtilsKt.digest(overrideCert, "SHA-1"), "")); | |
| } | |
| } |
matteozanettii
left a comment
There was a problem hiding this comment.
from copilot: Copilot said: I'll help you find a solution for
I'll help you find a solution for this failing job. Let me start by retrieving the job logs.Now let me examine the failing file:## Solution
The build is failing due to a Java compilation error in AuthManagerServiceImpl.java at line 221. The issue is an unreachable exception handler: the code catches NoSuchAlgorithmException, but the only method that could throw it (PackageManagerUtilsKt.digest()) is never actually called in the try block when an override certificate is not present.
Root Cause
Looking at lines 208-223, the try-catch block attempts to handle NoSuchAlgorithmException, but the exception can only be thrown by PackageManagerUtilsKt.digest() at line 219. However, line 219 is only executed if overrideCert != null (line 218). If overrideCert is null, the try block completes without any code that throws NoSuchAlgorithmException, making the catch clause unreachable.
Fix
Remove the try-catch block since the exception handling is not needed. The code is already defensive by checking if overrideCert != null before calling the digest method:
Mirrors the override logic from AccountAuthenticator.getAuthToken into AuthManagerServiceImpl.getTokenWithAccount so that apps calling this path directly (e.g. patched YouTube flavors) also benefit from package overrides, fixing UNREGISTERED_ON_API_CONSOLE errors.