Skip to content

feat: import pkcs8 keys#721

Open
adamspofford-dfinity wants to merge 2 commits intomainfrom
spofford/import-pkcs8
Open

feat: import pkcs8 keys#721
adamspofford-dfinity wants to merge 2 commits intomainfrom
spofford/import-pkcs8

Conversation

@adamspofford-dfinity
Copy link
Copy Markdown
Contributor

Checks whether keys are pkcs8 or sec1 before importing them.

@adamspofford-dfinity adamspofford-dfinity marked this pull request as ready for review April 7, 2026 21:56
@adamspofford-dfinity adamspofford-dfinity requested a review from a team as a code owner April 7, 2026 21:56
@lwshang lwshang requested a review from Copilot April 8, 2026 16:30
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for importing ECDSA identities from either SEC1 (EC PRIVATE KEY) or PKCS#8 (PRIVATE KEY) PEM encodings, with curve validation to reject keys for the wrong curve.

Changes:

  • Extend Secp256k1Identity::from_pem and Prime256v1Identity::from_pem to accept PKCS#8 keys in addition to SEC1 keys.
  • Introduce a shared parse_ec_pkcs8_key_bytes helper to validate EC algorithm + curve OID and extract inner SEC1 bytes (including a legacy dfx “hatchet surgery” fallback).
  • Add unit tests covering PKCS#8 import success and wrong-curve rejection for both identities.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
ic-agent/src/identity/secp256k1.rs Adds PKCS#8 handling branch in from_pem and new PKCS#8-focused tests.
ic-agent/src/identity/prime256v1.rs Adds PKCS#8 handling branch in from_pem and new PKCS#8-focused tests.
ic-agent/src/identity/mod.rs Adds shared PKCS#8 EC parsing/validation helper used by both identities.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +164 to +168
if curve_oid != expected_curve {
return Err(error::PemError::UnsupportedKeyCurve(
curve_name.to_string(),
curve_oid.as_bytes().to_vec(),
));
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curve_oid.as_bytes().to_vec() returns the OID content bytes (no ASN.1 tag/len). Existing SEC1 parsing reports the DER-encoded OID bytes from the EC PARAMETERS block (includes the 0x06 tag/length). This makes PemError::UnsupportedKeyCurve(_, Vec<u8>) inconsistent depending on input format (SEC1 vs PKCS#8), which can break consumers relying on those bytes. Consider storing a consistently encoded representation (e.g., DER-encode curve_oid the same way EC PARAMETERS does, or store a string form instead).

Copilot uses AI. Check for mistakes.
Comment on lines +61 to +62
if pem.tag() == PrivateKeyInfo::PEM_LABEL {
// PKCS#8 "PRIVATE KEY" format
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PrivateKeyInfo::PEM_LABEL is typically only available when the pkcs8 crate is built with its pem feature (it implements the PemLabel trait). In this repo, pkcs8 appears to be built without that feature (Cargo.lock shows pkcs8 depends only on der and spki, not pem-rfc7468), so this comparison is likely to fail to compile. Consider comparing against the literal tag ("PRIVATE KEY") instead, and drop the PrivateKeyInfo import if it’s only used for PEM_LABEL.

Copilot uses AI. Check for mistakes.
Comment on lines +61 to +62
if pem.tag() == PrivateKeyInfo::PEM_LABEL {
// PKCS#8 "PRIVATE KEY" format
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PrivateKeyInfo::PEM_LABEL is usually gated behind the pkcs8 crate’s pem feature (it needs to implement the PemLabel trait). Here pkcs8 seems to be built without that feature (Cargo.lock shows pkcs8 depends only on der and spki), so this will likely be a compile error. Suggest comparing pem.tag() to the literal "PRIVATE KEY" and removing the PrivateKeyInfo import if it’s only used for PEM_LABEL.

Copilot uses AI. Check for mistakes.
@@ -31,8 +31,11 @@ impl Secp256k1Identity {
}

/// Creates an identity from a PEM certificate.
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc comment says “Creates an identity from a PEM certificate”, but from_pem actually expects a PEM-encoded private key (SEC1 EC PRIVATE KEY or PKCS#8 PRIVATE KEY). Consider updating the wording to avoid implying an X.509 certificate is accepted here.

Suggested change
/// Creates an identity from a PEM certificate.
/// Creates an identity from a PEM-encoded private key.

Copilot uses AI. Check for mistakes.
@@ -31,8 +31,11 @@ impl Prime256v1Identity {
}

/// Creates an identity from a PEM certificate.
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc comment says “Creates an identity from a PEM certificate”, but from_pem is parsing a PEM-encoded private key (SEC1 EC PRIVATE KEY or PKCS#8 PRIVATE KEY). Suggest rewording to avoid suggesting that X.509 certificates are supported here.

Suggested change
/// Creates an identity from a PEM certificate.
/// Creates an identity from a PEM-encoded private key.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants