fix(signing): replace SHA-256 hash with real Ed25519 signature (#12)#74
Open
gbengaeben wants to merge 2 commits into
Open
fix(signing): replace SHA-256 hash with real Ed25519 signature (#12)#74gbengaeben wants to merge 2 commits into
gbengaeben wants to merge 2 commits into
Conversation
…ChainLabs#12) The previous `sign_server_side` produced `SHA-256(secret_key || xdr)` — a value that could only be re-derived by callers that already had the secret key, so it was not a signature in any usable sense and could not be verified by a third party. This change replaces that hash with a proper Ed25519 digital signature (RFC 8032) using `ed25519-dalek`. Any holder of the signer's public key can now verify the signature without touching secret material, which is the property the function name always implied. Key points - Adds `ed25519-dalek = "1.3"` (pinned; v1 has the straightforward `Keypair::sign -> Signature` API we need). - `ServerSignedTransaction` gains an explicit `algorithm` (serde-default `"ed25519"`) and `signer_public_key` (serde-default empty) so verifiers and future migrations are unambiguous. Legacy pre-OrbitChainLabs#12 JSON still deserialises. - New `ServerSignedTransaction::verify()` returns `Ok(true)` / `Ok(false)` / `Err` for authentic / forged / malformed payloads and is documented with `# Errors`. - Inline Stellar strkey codec (Crockford Base32 + CRC16-XModem) so the new dependency surface stays at one crate instead of pulling in both `ed25519-dalek` and `stellar-strkey`. - Tests: keep the existing build/validate/round-trip tests, add strkey roundtrip + bad-CRC rejection, signing determinism, signature verification, XDR tamper rejection, single-bit signature flip rejection, wrong-public-key rejection, unknown-algorithm rejection, missing-public-key rejection, JSON roundtrip, and backward-compatible deserialisation of legacy payloads. Refs: OrbitChainLabs#12
Follow-up to OrbitChainLabs#12. The original PR pinned `ed25519-dalek = "1.3"` but the crates.io `max_stable_version` is now `2.2.0` (v3.x is pre-release on crates.io), so the workspace resolver failed in CI: error: failed to select a version for the requirement `ed25519-dalek = "^1.3"` This commit: - Bumps the dep to `ed25519-dalek = "2.2"` so the resolver is happy. - Migrates the call site to the v2.x API: * `Keypair { secret, public }` -> `SigningKey` (single struct). * `SecretKey::from_bytes` -> `SigningKey::from_bytes`. * `PublicKey::from(&secret)` -> `signing_key.verifying_key()`. * `keypair.sign(msg)` -> `signing_key.sign(msg)` (same RFC 8032 deterministic Ed25519 path, no RNG required). * `public.as_bytes()` -> `verifying_key.to_bytes()`. - Drops the now-unused `Keypair`, `PublicKey`, `SecretKey` imports. - Adds a one-line comment on the deterministic-sign property so the next reader does not accidentally swap in a randomized signer. Behavioural surface (test coverage) is unchanged; the new test set added in OrbitChainLabs#12 already exercises the signing, verification, tampering rejection, and JSON round-trip paths the verifier depends on. Refs: OrbitChainLabs#74 (CI feedback) Refs: OrbitChainLabs#12
|
I'd like to work on this. Approach:
Estimated effort: ~1-2 hours. PR incoming shortly. |
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
2 participants
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.
Closes #12.
SigningRequest::sign_server_sidepreviously producedSHA-256(secret_key || xdr), which is not a digital signature — it can only be re-derived by callers who already hold the secret, so third parties (auditors, dashboards, downstream services) cannot verify it. The struct field was namedsignatureand the issue is labelledsecurity.This change replaces the hash with a real RFC 8032 Ed25519 signature via
ed25519-dalek 1.3. Any holder of the signer's public key can now verify the signature without seeing secret material.Changes
ServerSignedTransactiongains an explicitalgorithm(serde-default"ed25519") andsigner_public_key(serde-default empty) so the signature scheme and the verifier anchor are unambiguous. The added fields preserve backward-compatible deserialisation of pre-[LOW] Server-side signing uses SHA-256 hash instead of Ed25519 digital signature #12 JSON.ServerSignedTransaction::verify()returnsOk(true)/Ok(false)/Errwith a documented# Errorssection and rejects unknown algorithms, missing public keys, malformed strkeys (CRC mismatch), non-hex signatures, and wrong-length signatures.ed25519-dalekandstellar-strkey.SecretKeyis zeroized on drop viaed25519_dalek::SecretKey'sZeroizederive.Verification
CI:
cargo fmt --check,cargo clippy,cargo test -p orbitchain-tools. The authoritative check runs on upstream CI; the implementation sandbox did not have a Rust toolchain at coding time, so no local run was possible.