feat(pq-key-encoder): add post-quantum key encoding utilities#3
Conversation
eacet
added
the
merge-when-ready
eacet
requested review from
rubenmarcus,
snwfdhmp and
timurguvenkaya
and removed request for
timurguvenkaya
Greptile OverviewGreptile SummaryAdds comprehensive key encoding/decoding utilities for NIST post-quantum cryptography algorithms (ML-KEM, ML-DSA, SLH-DSA) with support for DER, PEM, and JWK formats. Key Changes
Notable Implementation Details
Issues Found
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant fromPEM
participant fromDER
participant parseAlgo
participant fromPKCS8/SPKI
participant validation
participant pqOID
User->>fromPEM: PEM string
fromPEM->>fromPEM: Parse PEM headers
fromPEM->>fromPEM: Base64 decode
fromPEM->>fromDER: DER bytes
fromDER->>parseAlgo: parseAlgorithmAndKeyWithType()
parseAlgo->>parseAlgo: readTLV (outer SEQUENCE)
parseAlgo->>parseAlgo: Check for version INTEGER
parseAlgo->>parseAlgo: readTLV (AlgorithmIdentifier)
parseAlgo->>parseAlgo: decodeObjectIdentifier()
parseAlgo->>parseAlgo: readTLV (key data)
parseAlgo-->>fromDER: {oid, keyBytes, keyType}
fromDER->>pqOID: detectAlgorithmFromOid(oid)
pqOID->>pqOID: OID.toName(oid)
pqOID-->>fromDER: algorithm name
fromDER->>fromPKCS8/SPKI: normalizePrivateKeyBytes()
fromPKCS8/SPKI->>fromPKCS8/SPKI: Extract nested OCTET STRING
fromPKCS8/SPKI-->>fromDER: normalized key bytes
fromDER->>validation: assertKeyData()
validation->>pqOID: getAlgorithmInfo(alg)
pqOID-->>validation: {publicKeySize, privateKeySize}
validation->>validation: validateKeySize()
validation-->>fromDER: validation passed
fromDER-->>User: {alg, type, bytes}
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| // Check if we ended in the middle of a multi-byte value | ||
| if (i > 0 && (bytes[i - 1] & 0x80) !== 0) { | ||
| throw new Error('Invalid OID bytes: incomplete multi-byte encoding'); |
There was a problem hiding this comment.
unreachable error check - the loop above breaks when (byte & 0x80) === 0, so bytes[i - 1] will always have bit 7 clear at this point
the check on line 159 has the same issue
| // Check if we ended in the middle of a multi-byte value | |
| if (i > 0 && (bytes[i - 1] & 0x80) !== 0) { | |
| throw new Error('Invalid OID bytes: incomplete multi-byte encoding'); | |
| // Loop breaks when high bit is clear, so this check is redundant | |
| // The incomplete encoding case is already handled by the loop structure |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/pq-oid/ts/src/encoding.ts
Line: 119:122
Comment:
unreachable error check - the loop above breaks when `(byte & 0x80) === 0`, so `bytes[i - 1]` will always have bit 7 clear at this point
the check on line 159 has the same issue
```suggestion
// Loop breaks when high bit is clear, so this check is redundant
// The incomplete encoding case is already handled by the loop structure
```
How can I resolve this? If you propose a fix, please make it concise.
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@claude review |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fd1d93290
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (tlv.tag !== TAG_CONTEXT_0 && tlv.tag !== TAG_CONTEXT_1) { | ||
| throw new InvalidEncodingError('Unexpected trailing data in key sequence.'); |
There was a problem hiding this comment.
scanOptionalTrailingFields only recognizes context tag 0xa1 for the optional publicKey field, but RFC 5958 defines OneAsymmetricKey in an IMPLICIT TAGS module, so [1] PublicKey is encoded as an IMPLICIT BIT STRING tag (0x81) in DER. With the current check, standards-compliant v1 PKCS#8 keys that include publicKey are rejected with Unexpected trailing data in key sequence, which breaks interoperability for valid private key encodings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The feedback is incorrect. Here's why:
The claim: RFC 5958 uses IMPLICIT TAGS, so [1] PublicKey should be encoded as 0x81 (implicit), making the current 0xa1 check wrong.
Why it's wrong:
- Extension additions override module tagging. The publicKey field in OneAsymmetricKey is defined inside double brackets [[2: publicKey [1] PublicKey OPTIONAL ]], which marks it as an extension addition (added
in version 2 of the structure). Per ASN.1 rules (X.680), extension additions always use EXPLICIT tagging regardless of the module's default tagging environment. This ensures backward compatibility. - The current code is correct. The tag 0xa1 (context-specific, constructed, tag 1) is the right encoding for an explicitly tagged [1] BIT STRING. An implicit encoding would use 0x81 (context-specific,
primitive, tag 1) and would directly replace the BIT STRING tag — but that's not what happens here. - Real-world implementations agree. OpenSSL and other major libraries encode the OneAsymmetricKey publicKey field using 0xa1 (explicit), not 0x81.
Bottom line: The scanOptionalTrailingFields function at lines 144-145 correctly checks for TAG_CONTEXT_0 (0xa0) and TAG_CONTEXT_1 (0xa1). No change is needed. Standards-compliant v1 PKCS#8 keys with a publicKey
field will use 0xa1 and will be accepted by this code.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fd1d93290
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while (currentOffset < sequence.length) { | ||
| const tlv = readTLV(sequence, currentOffset); | ||
| if (tlv.tag !== TAG_CONTEXT_0 && tlv.tag !== TAG_CONTEXT_1) { | ||
| throw new InvalidEncodingError('Unexpected trailing data in key sequence.'); |
There was a problem hiding this comment.
This loop accepts any number of 0xa0/0xa1 trailing elements in any order, so malformed OneAsymmetricKey encodings (for example [1] publicKey before [0] attributes, or duplicate [1] fields) are treated as valid when parsing untrusted DER. Because these are SEQUENCE members, they should appear at most once and in declaration order; otherwise corrupted/ambiguous private-key wrappers can bypass the parser’s DER validation.
Useful? React with 👍 / 👎.
2 participants
Implement key encoding/decoding for NIST PQC algorithms (ML-KEM, ML-DSA, SLH-DSA).
Supported formats:
Features:
Tested with real OpenSSL-generated keys (OQS provider):