fix(x509): explorePrivateKey no longer crashes on EC keys - #83
Merged
Merged
Conversation
explorePrivateKey() assumed a PKCS#1 RSA structure unconditionally; on an EC key it indexed into ASN.1 blocks that don't exist there and threw a bare "Cannot read properties of undefined (reading 'position')" (#12). It now checks the key type up front and fails with an actionable error instead. A new exploreEcPrivateKey() actually parses the SEC1 ECPrivateKey structure (version, private scalar, named curve, optional public point), for both raw SEC1 and PKCS#8-wrapped EC keys.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #12.
Problem
explorePrivateKey()unconditionally assumed a PKCS#1 RSA structure. On an EC private key it took a fallback path that indexes into ASN.1 blocks that simply aren't there for a SEC1ECPrivateKey, and crashed with a bare, unactionable error:PR #10 (referenced in the issue) added EC support to certificate exploring, but never to private-key exploring — this issue's gap.
Fix
explorePrivateKey()now checks the key'sasymmetricKeyTypeup front and throws a clear, actionable error for anything other than RSA, instead of crashing on an out-of-bounds ASN.1 read.exploreEcPrivateKey()actually parses the SEC1ECPrivateKeystructure (version, private scalard, named curve from the[0] ECParametersOID, and the optional[1] publicKeybit string), for both raw SEC1 (-----BEGIN EC PRIVATE KEY-----) and PKCS#8-wrapped EC keys — normalized to the same SEC1 DER via Node's ownKeyObject.export(), so both encodings are handled uniformly.explorePrivateKey()is unchanged (same fields, same values) — verified against the existing RSA fixtures/tests, no regressions in the 283-test suite.Why a new function instead of widening
PrivateKeyInternalsexplorePrivateKey()'s return shape (modulus,publicExponent, ...) is RSA-specific, and two existing callers (certificateMatchesPrivateKey,publicKeyAndPrivateKeyMatches) rely on.modulusdirectly — a concept that doesn't exist for EC keys. Turning the return type into a discriminated union would force those callers (and everyone else's TypeScript) to add narrowing they don't need. A separateexploreEcPrivateKey()keeps the RSA API and its callers untouched while giving EC keys their own, correctly-typed result.Testing
test_explore_private_key.ts: EC key exploring (SEC1 and PKCS#8-wrapped), and the two "wrong key type" error paths for both functions.masterbefore the fix, confirmed it no longer reproduces after.node-opcua-crypto-testsuite: 283 passed, 6 skipped, 0 regressions.