Merge rust-bitcoin/rust-bitcoin#6346: Make `Keypair::as_inner` private
What the developer wrote
Authored by Andrew Poelstra
Merge rust-bitcoin/rust-bitcoin#6346: Make `Keypair::as_inner` private
4cc9cda8bbb82982529aed62afe15a85aaffbd8c Make Keypair::as_inner private (Mitchell Bagot)
Pull request description:
Keypair::as_inner is currently only used to implement TapTweak. Since TapTweak exists in taproot-primitives and cannot be in crypto, it must be modified to manually serialize/deserialize to get the secp type rather than using as_inner.
Replace Keypair::as_inner use in TapTweak with Keypair::from_key_parts and make as_inner pub(super).
ACKs for top commit:
tcharding:
ACK 4cc9cda8bbb82982529aed62afe15a85aaffbd8c
apoelstra:
ACK 4cc9cda8bbb82982529aed62afe15a85aaffbd8c; successfully ran local tests
Tree-SHA512: 0674b3e11a731757a170cc3e6c158df81162d001b19a10ccb14c5bf5b630c2a25c8bcd855448b434d666853a7e6fe9cd9465d36c8878dfb0ee23ffc64b38721c
What changed, and why it matters
This change narrows the visibility of an internal helper method (`Keypair::as_inner`) from public to crate-only, and rewrites the Taproot key tweaking code to build a low-level secp256k1 keypair from serialized parts instead of relying on that helper. It is a defensive API-hardening patch: it reduces the public API surface so external callers cannot directly obtain the underlying secp256k1 keypair, which could make future misuse or information-leak bugs less likely. The rewritten path uses an `unsafe` block, but the surrounding code argues it is safe because the parts come from an already-valid keypair and parity is checked. There is no direct vulnerability being fixed here, and no disclosed security incident.
Review the `unsafe` block in `tap_tweak` to confirm the parity check and key derivation invariants are sufficient and documented; consider adding a unit test or invariant comment. Treat as routine defensive maintenance, not an urgent security patch.
Security signals we found
Reduced public API surface for sensitive key material (defensive hardening)
Introduction of `unsafe` block justified by invariant that reconstructed keypair parts are derived from a valid existing keypair
Manual secret-key serialization/deserialization and parity handling in cryptographic path
No CVE, advisory, or vendor security disclosure present in commit or references
Evidence from the diff
The commit makes crypto::key::Keypair::as_inner pub(super) instead of pub, removing public access to the wrapped secp256k1::Keypair. The only previous in-crate consumer was TapTweak::tap_tweak in taproot_primitives, which used self.as_inner().add_xonly_tweak(&tweak). Because taproot_primitives cannot depend on crypto internals, the implementation now manually reconstructs a secp256k1::Keypair via SecretKey::from_secret_bytes(self.to_secret_bytes()), XOnlyPublicKey::from_byte_array(self.to_x_only_public_key().serialize()), parity-aware negation of the secret key, and unsafe { secp256k1::Keypair::from_key_parts(...) }. The change is API-cleanup/hardening rather than a fix for an exploitable bug.
Changed components
crypto/src/key.rs: `Keypair::as_inner` visibilitytaproot_primitives/src/lib.rs: `UntweakedKeypair::tap_tweak` implementationInspect captured patch +12 / −2
### crypto/src/key.rs
@@ -112,7 +112,7 @@ mod keypair {
/// Returns a reference to the inner [`secp256k1::Keypair`].
#[inline]
- pub fn as_inner(&self) -> &secp256k1::Keypair { &self.0 }
+ pub(super) fn as_inner(&self) -> &secp256k1::Keypair { &self.0 }
}
impl Drop for Keypair {
### taproot_primitives/src/lib.rs
@@ -430,7 +430,17 @@ impl TapTweak for UntweakedKeypair {
fn tap_tweak(&self, merkle_root: Option<TapNodeHash>) -> TweakedKeypair {
let pubkey = XOnlyPublicKey::from_keypair(self);
let tweak = TapTweakHash::from_key_and_merkle_root(pubkey, merkle_root).to_scalar();
- let tweaked = self.as_inner().add_xonly_tweak(&tweak).expect("Tap tweak failed");
+
+ let secp_sk = secp256k1::SecretKey::from_secret_bytes(self.to_secret_bytes())
+ .expect("secret key parsed from serialized secret key");
+ let (xonly, parity) = self.to_x_only_public_key().serialize();
+ let secp_pk = secp256k1::XOnlyPublicKey::from_byte_array(xonly)
+ .expect("x-only public key parsed from serialized point");
+ let secp_sk = if parity == secp256k1::Parity::Odd { secp_sk.negate() } else { secp_sk };
+ // SAFETY: secp_pk and secp_sk are derived from an existing valid keypair, and parity is checked above.
+ let secp_keypair = unsafe { secp256k1::Keypair::from_key_parts(secp_pk, &secp_sk) };
+
+ let tweaked = secp_keypair.add_xonly_tweak(&tweak).expect("Tap tweak failed");
TweakedKeypair::dangerous_assume_tweaked(Self::from(tweaked))
}
Why this scored 18/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.