ADFA-4633: Remove BouncyCastle#1542
Conversation
…h pure-Java DER encoder Replaces both usages of BouncyCastle (ToolsManager.generateKeystore and AdbKey certificate creation) with a minimal self-signed X.509 DER encoder written in standard Java/Kotlin APIs, eliminating the bcprov-jdk18on and bcpkix-jdk18on runtime dependencies entirely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSelf-signed X.509 certificate creation now uses a shared standard-API DER utility. Keystore and ADB key flows call it, tests cover encoding behavior, and BouncyCastle certificate dependencies and configuration are removed. ChangesCertificate generation migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ToolsManager
participant SelfSignedCertUtils
participant CertificateFactory
ToolsManager->>SelfSignedCertUtils: Generate certificate from key pair and validity
SelfSignedCertUtils->>SelfSignedCertUtils: Encode and sign DER certificate
SelfSignedCertUtils->>CertificateFactory: Parse certificate bytes
CertificateFactory-->>ToolsManager: Return X509Certificate
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt (2)
14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace non-ASCII characters with standard ASCII equivalents.
As per coding guidelines: "Use ASCII in code and code comments where an equivalent ASCII character is available; prefer
->,-, and straight quotes over non-ASCII equivalents."
common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt#L14-L16: Replace the non-ASCII em dash (—) on line 15 with a standard hyphen (-).common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt#L109-L112: Replace the non-ASCII em dash (—) on line 111 with a standard hyphen (-).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt` around lines 14 - 16, Replace the em dash with an ASCII hyphen in the documentation comment near SelfSignedCert.kt lines 14-16 and in the comment near lines 109-112; make no other changes.Source: Coding guidelines
38-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove decorative separator comments.
As per coding guidelines: "Do not add separator or decorative comments such as banner bars,
// ====rules, or ASCII-art dividers."
common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt#L38-L40: Remove the// -----separator lines, keeping only the descriptive text// TBSCertificate.common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt#L60-L63: Remove the// -----separator lines.common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt#L91-L93: Remove the// -----separator lines.common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt#L100-L102: Remove the// -----separator lines.common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt#L136-L138: Remove the// -----separator lines.common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt#L167-L169: Remove the// -----separator lines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt` around lines 38 - 40, Remove the decorative separator lines from all six listed ranges in common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt: 38-40, 60-63, 91-93, 100-102, 136-138, and 167-169. At the first site, retain only the descriptive comment “TBSCertificate”; remove the separator lines at the remaining sites.Source: Coding guidelines
subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbKey.kt (1)
311-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding the
Lsuffix to the large integer literal.Although Kotlin safely infers
2461449600as aLongbecause it exceedsInt.MAX_VALUE, explicitly appending theLsuffix (e.g.,2461449600L * 1000) clarifies the intent and removes ambiguity about potential integer overflow for future maintainers.💡 Proposed optional refactor
this.certificate = generateSelfSignedCert( KeyPair(publicKey, privateKey), "CN=00", BigInteger.ONE, Date(0), - Date(2461449600 * 1000), + Date(2461449600L * 1000), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbKey.kt` around lines 311 - 317, Update the expiration-date calculation in the certificate assignment to mark the large literal as a Long by adding the L suffix to 2461449600 before multiplying by 1000. Keep the existing generateSelfSignedCert call and date values unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt`:
- Around line 14-16: Replace the em dash with an ASCII hyphen in the
documentation comment near SelfSignedCert.kt lines 14-16 and in the comment near
lines 109-112; make no other changes.
- Around line 38-40: Remove the decorative separator lines from all six listed
ranges in common/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.kt:
38-40, 60-63, 91-93, 100-102, 136-138, and 167-169. At the first site, retain
only the descriptive comment “TBSCertificate”; remove the separator lines at the
remaining sites.
In `@subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbKey.kt`:
- Around line 311-317: Update the expiration-date calculation in the certificate
assignment to mark the large literal as a Long by adding the L suffix to
2461449600 before multiplying by 1000. Keep the existing generateSelfSignedCert
call and date values unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7d0deae-2b7f-4afc-82d7-0f0874077620
📒 Files selected for processing (7)
common/build.gradle.ktscommon/src/main/java/com/itsaky/androidide/managers/ToolsManager.javacommon/src/main/java/com/itsaky/androidide/utils/SelfSignedCert.ktgradle/libs.versions.tomlsettings.gradle.ktssubprojects/shizuku-manager/build.gradle.ktssubprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbKey.kt
💤 Files with no reviewable changes (4)
- common/build.gradle.kts
- settings.gradle.kts
- gradle/libs.versions.toml
- subprojects/shizuku-manager/build.gradle.kts
|
@claude review |
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, comment @claude review on this pull request to trigger a review.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace em dashes with ASCII hyphens in two comments - Remove decorative separator lines from section headers - Add L suffix to long literal in AdbKey to prevent integer overflow Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
hal-eisen-adfa
left a comment
hal-eisen-adfa
left a comment
There was a problem hiding this comment.
Code review (xhigh) of the hand-rolled DER certificate encoder that replaces BouncyCastle. The ASN.1 structure is byte-correct (independently verified against RFC 5280 and strict parsers), but there are two runtime-breaking bugs plus lower-severity issues. Inline comments below. The BouncyCastle removal itself is clean: the settings.gradle bcutil classpath and the Security/BouncyCastleProvider imports were effectively dead, nothing else in the app uses the BC provider, and keystore-generator-plugin keeps its own BC dep and is unaffected.
Note: this new security-critical encoder ships with no unit tests. Worth a round-trip test (generate -> CertificateFactory parse -> assert DN/validity/signature) plus a Locale.setDefault(Locale("ar")) regression test that would have caught finding #2.
- BigInteger.TWO is API 33+ (NoSuchFieldError on Android 9-12); replace with BigInteger.valueOf(2) (as AdbKey.kt already did). - String.format with default locale corrupts UTCTime/GeneralizedTime bytes on Arabic/Persian/Bengali locales; pass Locale.ROOT explicitly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Encode countryName as PrintableString (tag 0x13) instead of UTF8String, per RFC 5280 App. A. - Generalize tlv() long-form length encoding to support content >= 64KiB (was hardcoded to 2 length octets, silently truncating longer content). - Split DN parts on unescaped commas only (RFC 4514-style backslash escaping), so values like "O=Foo\, Inc." don't get split into two RDNs. - Add round-trip unit tests: signature verification, a non-default-locale regression test, and an escaped-comma DN test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed the remaining review feedback on
The two runtime-breaking bugs ( |
Fixes found by an xhigh code-review pass (4 independent finder angles converged on the Calendar bug): - derTime called Calendar.getInstance(TimeZone) without a Locale, so on a device whose default locale uses a non-Gregorian calendar (e.g. Thai th-TH) it silently returned a BuddhistCalendar with YEAR = Gregorian + 543, corrupting cert validity dates for both the release keystore and ADB pairing. Pass Locale.ROOT explicitly. - derTime now also emits GeneralizedTime for years < 1950 (previously only checked the upper 2050 bound), matching BouncyCastle's Time class, and the UTCTime string is now derived from the GeneralizedTime one instead of duplicating the format/cal.get() calls. - derPrintableString validated nothing and encoded with plain US_ASCII, which silently replaces any non-ASCII character with '?' instead of failing; it now validates against PrintableString's legal charset. - encodeDn silently dropped empty-valued RDNs and unrecognized attribute keys, diverging from BouncyCastle's X500Name (which kept empty RDNs and threw on genuinely unrecognized ones). It now preserves empty values and throws IllegalArgumentException for malformed components or unsupported attribute types instead of silently dropping them. - Removed four one-line decorative section comments per CLAUDE.md's no-separator-comments rule. Test suite: the existing locale-regression test used "ar", which never changes the JVM's Calendar implementation and so never exercised the bug above; switched it to "th"-TH and added assertions on the decoded notBefore/notAfter (the prior test only checked cert.verify(), which validates the signature but not the date values, so it couldn't have caught this class of bug even with the right locale). Added coverage for the pre-1950 boundary, preserved empty DN values, and the three new IllegalArgumentException cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Ran an xhigh-effort multi-angle review of this PR and fixed everything it surfaced (8c4f986):
Deliberately left alone (suggestion-level findings, not confirmed bugs): swapping the hand-rolled DN encoder for |
2 participants
Summary
ToolsManager.generateKeystore()andAdbKeywith a newSelfSignedCert.ktutility that encodes X.509 DER manually using only standard Java SE / Android APIsbcprov-jdk18onandbcpkix-jdk18onruntime dependencies fromcommonandshizuku-managerbcutil-jdk18onfrom the build-script classpath insettings.gradle.ktsbcprovJdk18onversion and both library entries fromlibs.versions.tomlTest plan
:common:compileV7DebugKotlin— passes:subprojects:shizuku-manager:compileV7DebugKotlin— passes.p12file is created atKEYSTORE_RELEASEandkeystore.propertiesis written alongside it🤖 Generated with Claude Code