Skip to content
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ internal object SignerNative {
*/
external fun signWithPrivateKey(privateKey: ByteArray, network: Int, data: ByteArray): ByteArray?

/**
* Whether Rust signs an identity key of DPP `KeyType` discriminant
* [keyType] (`dash_sdk_resolver_supports_key_type`, the predicate
* `KeychainSigner.swift` asks). Pure; a value outside `0..=255` is
* `false`.
*/
external fun resolverSupportsKeyType(keyType: Int): Boolean

/**
* One-shot derive-then-sign for platform-address keys (the
* `keyType == 0xFF` branch). Derives an ECDSA secp256k1 key from
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ class KeystoreSigner(
* Room rows and re-seed `pendingIdentityKeys`.
*/
private val onSigningKeyInvalidated: (suspend (pubkeyHex: String) -> Unit)? = null,
/**
* Whether Rust signs an identity key of a DPP `KeyType` discriminant.
* Rust owns that set; the parameter exists so a JVM unit test can stub
* it without the native library.
*/
private val supportsIdentityKeyType: (Int) -> Boolean = SignerNative::resolverSupportsKeyType,
Comment on lines 60 to +66

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Suggestion: Preserve the existing KeystoreSigner constructor ABI when adding the capability hook

Appending supportsIdentityKeyType to the public primary constructor changes the JVM constructor from five arguments to six. Kotlin's default-argument machinery does not emit the previous five-argument constructor, so already-compiled consumers can fail with NoSuchMethodError; the default value is not binary compatibility. The change also makes the former last onSigningKeyInvalidated parameter no longer the trailing lambda target, so source callers using KeystoreSigner(...){ ... } can bind the lambda to the new (Int) -> Boolean parameter and fail to compile. Add an explicit five-argument compatibility constructor and keep the capability injection behind an internal or test-only entry point.

source: gpt-6.1-sol (phase2-reviewer: general)

Comment on lines +61 to +66

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Suggestion: Do not expose an unchecked signer-capability override in the public constructor

supportsIdentityKeyType is a public primary-constructor argument even though the stored property is private. A caller can provide a callback that returns true for BLS or EdDSA, causing canSignWith to report that key as usable after the storage check. The subsequent signWithStoredKey path ignores keyType and always calls SignerNative.signWithPrivateKey, whose path produces secp256k1 ECDSA signatures, so the Rust wallet can select a key type that this signer cannot actually sign. Keep the Rust predicate authoritative in the production constructor and expose any override only through an internal or test-only factory.

source: gpt-6.1-sol (phase2-reviewer: architecture-layering)

) : NativeSignerBridge(), AutoCloseable {

private val handleRef =
Expand Down Expand Up @@ -303,21 +309,23 @@ class KeystoreSigner(
}

override fun canSignWith(pubkeyBytes: ByteArray, keyType: Int): Boolean = try {
if (keyType == PLATFORM_ADDRESS_HASH_KEY_TYPE) {
// Prerequisites for the derive-and-sign path: a row with a
// non-empty derivation path plus a stored wallet mnemonic.
// Existence-only — hasMnemonic never decrypts, so no plaintext
// is materialized for a mere capability check.
runBlocking {
platformAddressDao.getAllByAddressHash(pubkeyBytes).any { row ->
row.derivationPath.isNotEmpty() && storage.hasMnemonic(row.walletId)
when {
keyType == PLATFORM_ADDRESS_HASH_KEY_TYPE ->
// Prerequisites for the derive-and-sign path: a row with a
// non-empty derivation path plus a stored wallet mnemonic.
// Existence-only — hasMnemonic never decrypts, so no plaintext
// is materialized for a mere capability check.
runBlocking {
platformAddressDao.getAllByAddressHash(pubkeyBytes).any { row ->
row.derivationPath.isNotEmpty() && storage.hasMnemonic(row.walletId)
}
}
else -> identityKeySignable(keyType, supportsIdentityKeyType) {
// Ciphertext presence is not signing capability after Android
// replaces KEYS_ALIAS: the stale blob remains in DataStore but
// can never be decrypted by the replacement private key.
runBlocking { storage.isPrivateKeyDecryptable(storageKeyFor(pubkeyBytes)) }
}
} else {
// Ciphertext presence is not signing capability after Android
// replaces KEYS_ALIAS: the stale blob remains in DataStore but
// can never be decrypted by the replacement private key.
runBlocking { storage.isPrivateKeyDecryptable(storageKeyFor(pubkeyBytes)) }
}
} catch (_: Exception) {
false
Expand Down Expand Up @@ -372,6 +380,21 @@ class KeystoreSigner(
*/
private const val PLATFORM_ADDRESS_HASH_KEY_TYPE: Int = 0xFF

/**
* Whether [canSignWith] reports an identity key of DPP `KeyType`
* discriminant [keyType] signable. Rust picks signing keys by this
* answer: claiming a key the secp256k1 sign path cannot sign (BLS,
* EdDSA) would get it chosen and then signed with the wrong
* algorithm, so [supportsKeyType] (Rust's predicate) is asked first
* and [privateKeyDecryptable] only for a type it accepts. Factored
* pure so it is unit-testable without the native signer handle.
*/
internal fun identityKeySignable(
keyType: Int,
supportsKeyType: (Int) -> Boolean,
privateKeyDecryptable: () -> Boolean,
): Boolean = supportsKeyType(keyType) && privateKeyDecryptable()

/**
* Structured completion code for a sign-path failure [t]:
* [KeyPermanentlyInvalidatedException] is a "key unavailable until
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package org.dashfoundation.dashsdk.security

import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test

/**
* Pins how [KeystoreSigner.canSignWith] answers for an identity key. Rust
* chooses the signing key by that answer, so a key type Rust's predicate
* refuses (BLS, EdDSA) must not be reported signable even with a stored
* private key. Which types Rust signs is pinned by the Rust tests of
* `dash_sdk_resolver_supports_key_type`; this JVM test cannot load the
* native library, so it stubs the predicate.
*/
class KeystoreSignerKeyTypeTest {

@Test
fun refusesAKeyTypeRustDoesNotSignWithoutConsultingStorage() {
var storageConsulted = false
val signable = KeystoreSigner.identityKeySignable(
keyType = 1,
supportsKeyType = { false },
) {
storageConsulted = true
true
}
assertFalse(signable)
assertFalse(storageConsulted)
}

@Test
fun asksRustAboutTheKeyTypeItWasGiven() {
var askedFor: Int? = null
KeystoreSigner.identityKeySignable(
keyType = 2,
supportsKeyType = {
askedFor = it
true
},
) { true }
assertEquals(2, askedFor)
}

@Test
fun reportsAKeyTypeRustSignsSignableOnlyWhenItsKeyDecrypts() {
assertTrue(KeystoreSigner.identityKeySignable(0, { true }) { true })
assertFalse(KeystoreSigner.identityKeySignable(0, { true }) { false })
}
}
34 changes: 34 additions & 0 deletions packages/rs-dpp/src/identity/identity_public_key/key_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,22 @@ impl KeyType {
}
}

/// Can a key of this type sign an identity-signed state transition?
///
/// This is the set Drive's identity-signed validation verifies a signature by; a key of
/// any other type is refused there with `InvalidIdentityPublicKeyTypeError`. Clients use
/// it to choose or check a signing key before building a transition. A Drive test pins the
/// two to agree for every key type.
pub fn can_sign_identity_signed_transition(&self) -> bool {
match self {
KeyType::ECDSA_SECP256K1 => true,
KeyType::BLS12_381 => true,
KeyType::ECDSA_HASH160 => true,
KeyType::BIP13_SCRIPT_HASH => false,
KeyType::EDDSA_25519_HASH160 => false,
}
}

pub fn signature_verify_cost(
&self,
platform_version: &PlatformVersion,
Expand Down Expand Up @@ -413,6 +429,24 @@ mod tests {
);
}

// -- can_sign_identity_signed_transition() --

#[test]
fn should_sign_identity_signed_transitions_only_with_ecdsa_bls_and_hash160_keys() {
let signing: Vec<KeyType> = KeyType::all_key_types()
.into_iter()
.filter(KeyType::can_sign_identity_signed_transition)
.collect();
assert_eq!(
signing,
[
KeyType::ECDSA_SECP256K1,
KeyType::BLS12_381,
KeyType::ECDSA_HASH160
]
);
}

// -- is_unique_key_type() --

#[test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,19 @@ mod tests {
use dpp::state_transition::errors::PublicKeySecurityLevelNotMetError;
use dpp::state_transition::errors::WrongPublicKeyPurposeError;

/// Clients choose and check signing keys by the dpp predicate; it must name exactly the key
/// types this validation verifies a signature by.
#[test]
fn should_agree_with_the_dpp_identity_signing_key_type_predicate_for_every_key_type() {
for key_type in KeyType::all_key_types() {
assert_eq!(
key_type.can_sign_identity_signed_transition(),
SUPPORTED_KEY_TYPES.contains(&key_type),
"{key_type:?}"
);
}
}

mod convert_to_consensus_signature_error_tests {
use super::*;

Expand Down
2 changes: 0 additions & 2 deletions packages/rs-platform-wallet-ffi/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1380,7 +1380,6 @@ mod tests {
identity_id,
purpose: dpp::identity::Purpose::AUTHENTICATION,
security_levels: vec![dpp::identity::SecurityLevel::CRITICAL],
key_types: vec![dpp::identity::KeyType::ECDSA_SECP256K1],
transition: "an identity update".to_string(),
});
assert_eq!(result.code, PlatformWalletFFIResultCode::ErrorUnknown);
Expand All @@ -1393,7 +1392,6 @@ mod tests {
identity.as_str(),
"AUTHENTICATION",
"CRITICAL",
"ECDSA_SECP256K1",
"an identity update",
] {
assert!(message.contains(part), "{message}");
Expand Down
11 changes: 5 additions & 6 deletions packages/rs-platform-wallet/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use dpp::consensus::state::address_funds::AddressInvalidNonceError;
use dpp::consensus::ConsensusError;
use dpp::fee::Credits;
use dpp::identifier::Identifier;
use dpp::identity::{KeyType, Purpose, SecurityLevel};
use dpp::identity::{Purpose, SecurityLevel};
use dpp::prelude::{AddressNonce, CoreBlockHeight};
use key_wallet::account::StandardAccountType;
use key_wallet::wallet::managed_wallet_info::asset_lock_builder::AssetLockFundingType;
Expand Down Expand Up @@ -87,19 +87,18 @@ pub enum PlatformWalletError {
InvalidIdentityData(String),

/// No key of the identity can sign `transition`: none has `purpose`, one of
/// `security_levels` and one of `key_types` while also being enabled, unexpired and inside
/// its contract bounds.
/// `security_levels` and a type Platform verifies an identity signature by while also being
/// enabled, unexpired and inside its contract bounds.
/// It stays so until an identity update adds such a key. A key that qualifies but that the
/// signer cannot use is the signer-unavailable error instead.
#[error(
"identity {identity_id} has no usable {purpose} key of type {key_types:?} at security \
level {security_levels:?} to sign {transition}"
"identity {identity_id} has no usable {purpose} key at security level \
{security_levels:?} to sign {transition}"
)]
NoUsableSigningKey {
identity_id: Identifier,
purpose: Purpose,
security_levels: Vec<SecurityLevel>,
key_types: Vec<KeyType>,
transition: String,
},

Expand Down
1 change: 0 additions & 1 deletion packages/rs-platform-wallet/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ pub use wallet::identity::network::{
check_signing_key, derive_identity_auth_keypair, usable_signing_key, AutoAcceptProofSource,
ContactCryptoProvider, ContactInfoOpened, ContactInfoPublishOutcome, ContactInfoSealed,
SeedBindingVerification, SigningScope, IDENTITY_GAP_LIMIT, MASTER_KEY_INDEX,
PLATFORM_SIGNING_KEY_TYPES, WALLET_SIGNING_KEY_TYPES,
};
pub use wallet::identity::{
calculate_account_reference, derive_auto_accept_private_key, derive_contact_payment_address,
Expand Down
Loading
Loading