Disallow Orchard ivk = 0 on IncomingViewingKey::from & SpendingKey generation #3962

dconnolly · 2022-03-25T05:09:29Z

Motivation

A change to the protocol spec for Orchard to reject scalar values in the Pallas scalar field of 0 for incoming viewing keys.

Specifications

if ivk ∈ {0, ⊥}, discard this key and repeat with a new sk.

https://zips.z.cash/protocol/protocol.pdf#orchardkeycomponents

Solution

Update impl From<FullViewingKey> for IncomingViewingKey to become a TryFrom, and return Result::Err if we get ⊥ or the possible pallas::Scalar value is 0 in the scalar field.

Update SpendingKey::new() to also check that a prospective sk doesn't lead to an invalid IncomingViewingKey.

Also implement TryFrom<[u8; 64]> for IncomingViewingKey to enforce that parsing of the raw encoding checks for and rejects the same values.

Closes #3869

Review

This is low priority because it is not an NU5 consensus rule, but an Orchard protocol check for keygen and ingesting of keys/addresses.

Reviewer Checklist

Code implements Specs and Designs
Tests for Expected Behaviour
Tests for Errors

Follow Up Work

teor2345 · 2022-03-25T06:29:39Z

I'm marking this as "do not merge", until we work out how risky it is.
(Or until we've tagged the NU5 testnet rollback code.)

jvff

Looks good. I added some ideas, but they are optional.

zebra-chain/src/orchard/keys.rs

zebra-chain/src/orchard/arbitrary.rs

zebra-chain/src/orchard/keys.rs

dconnolly · 2022-03-25T22:04:54Z

I'm marking this as "do not merge", until we work out how risky it is. (Or until we've tagged the NU5 testnet rollback code.)

All of our shielded key code beyond what is serialized/deserialized on chain/in P2P network messages is currently not being used beyond testing and conformance, until we use it for wallet functionality or whatever. That includes Orchard IncomingViewingKey and SpendingKey derivation. I would consider this a low-risk change.

dconnolly · 2022-03-25T22:10:28Z

I'm marking this as "do not merge", until we work out how risky it is. (Or until we've tagged the NU5 testnet rollback code.)

Definitely willing to hold as :do-not-merge: until we tag as it is low priority

zebra-network/proptest-regressions/peer_set/set/tests/prop.txt

zebra-chain/src/orchard/keys.rs

teor2345 · 2022-04-26T23:21:56Z

@Mergifyio update

mergify · 2022-04-26T23:22:30Z

update

✅ Branch has been successfully updated

dconnolly · 2022-05-11T00:38:55Z

@Mergifyio update

mergify · 2022-05-11T00:39:18Z

update

✅ Branch has been successfully updated

dconnolly · 2022-05-21T18:50:02Z

@mergify rebase

mergify · 2022-05-21T18:50:38Z

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (1/3)
error: could not apply 8a7c96d1... Disallow Orchard ivk = 0 on IncomingViewingKey::from and SpendingKey generation
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 8a7c96d1... Disallow Orchard ivk = 0 on IncomingViewingKey::from and SpendingKey generation
Auto-merging zebra-chain/src/orchard/keys.rs
CONFLICT (content): Merge conflict in zebra-chain/src/orchard/keys.rs
Auto-merging zebra-chain/src/orchard/arbitrary.rs

err-code: E0F31

teor2345 · 2022-05-25T03:09:49Z

@dconnolly this PR hasn't changed in 2 months, can we consider turning it into a ticket as part of the Cryptography triage?

…generation

conradoplg

Looks good

dconnolly · 2022-06-16T01:08:13Z

https://github.com/Mergifyio update

Co-authored-by: Janito Vaqueiro Ferreira Filho <[email protected]>

mergify · 2022-06-16T01:08:46Z

update

✅ Branch has been successfully updated

dconnolly · 2022-06-23T17:39:55Z

Mergifyio update

teor2345 · 2022-06-28T03:30:45Z

@Mergifyio update

mergify · 2022-06-28T03:31:20Z

update

✅ Branch has been successfully updated

teor2345

Re-approving based on previous approvals

daira

Untested ACK.

teor2345

This PR needs clippy::unwrap-in-result fixes:
https://github.com/ZcashFoundation/zebra/runs/7086134368?check_suite_focus=true#step:7:543

daira

re-untested ACK

teor2345

Thanks for these clippy fixes, sorry no-one reviewed this PR until now!

teor2345 · 2022-08-01T02:35:47Z

@Mergifyio update

mergify · 2022-08-01T02:35:53Z

update

✅ Branch has been successfully updated

dconnolly added A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks P-Low ❄️ labels Mar 25, 2022

dconnolly self-assigned this Mar 25, 2022

dconnolly requested review from a team as code owners March 25, 2022 05:09

dconnolly requested review from upbqdn and jvff and removed request for a team March 25, 2022 05:09

teor2345 added the do-not-merge Tells Mergify not to merge this PR label Mar 25, 2022

jvff previously approved these changes Mar 25, 2022

View reviewed changes

zebra-chain/src/orchard/keys.rs Outdated Show resolved Hide resolved

zebra-chain/src/orchard/arbitrary.rs Show resolved Hide resolved

zebra-chain/src/orchard/keys.rs Outdated Show resolved Hide resolved

teor2345 reviewed Mar 28, 2022

View reviewed changes

zebra-network/proptest-regressions/peer_set/set/tests/prop.txt Outdated Show resolved Hide resolved

daira reviewed Mar 28, 2022

View reviewed changes

zebra-chain/src/orchard/keys.rs Outdated Show resolved Hide resolved

dconnolly dismissed jvff’s stale review via ee006e2 March 28, 2022 16:32

teor2345 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Apr 4, 2022

dconnolly added 2 commits June 4, 2022 08:40

Disallow Orchard ivk = 0 on IncomingViewingKey::from and SpendingKey …

fddacf7

…generation

Check that ivk scalar values parsed from bytes are never 0

c4e46b7

conradoplg previously approved these changes Jun 9, 2022

View reviewed changes

Update zebra-chain/src/orchard/keys.rs

81299ee

Co-authored-by: Janito Vaqueiro Ferreira Filho <[email protected]>

dconnolly dismissed conradoplg’s stale review via 81299ee June 16, 2022 01:08

Merge branch 'main' into check-commit-ivk-result

e048efa

Merge branch 'main' into check-commit-ivk-result

786d2c2

teor2345 approved these changes Jun 28, 2022

View reviewed changes

daira previously approved these changes Jun 28, 2022

View reviewed changes

mergify bot added a commit that referenced this pull request Jun 28, 2022

Merge of #3962

06ab590

mergify bot mentioned this pull request Jun 28, 2022

merge-queue: embarking main (54efbe9) and [#4667 + #3962] together #4711

Closed

teor2345 suggested changes Jun 28, 2022

View reviewed changes

teor2345 assigned oxarbitrage and unassigned dconnolly Jun 28, 2022

oxarbitrage added 2 commits June 29, 2022 12:06

Merge remote-tracking branch 'origin/main' into check-commit-ivk-result

e4a3d7b

allow unwrap_in_result lint in added code

5a12adb

oxarbitrage dismissed daira’s stale review via 5a12adb July 2, 2022 23:46

Merge remote-tracking branch 'origin/main' into check-commit-ivk-result

891d121

daira approved these changes Jul 3, 2022

View reviewed changes

teor2345 approved these changes Jul 25, 2022

View reviewed changes

Merge branch 'main' into check-commit-ivk-result

b972fb5

mergify bot added a commit that referenced this pull request Aug 1, 2022

Merge of #3962

66e28c8

mergify bot mentioned this pull request Aug 1, 2022

merge-queue: embarking main (cdd846e) and [#4727 + #4829 + #3962 + #4755 + #4785] together #4852

Closed

mergify bot merged commit bdd2808 into main Aug 1, 2022

mergify bot deleted the check-commit-ivk-result branch August 1, 2022 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow Orchard ivk = 0 on IncomingViewingKey::from & SpendingKey generation #3962

Disallow Orchard ivk = 0 on IncomingViewingKey::from & SpendingKey generation #3962

dconnolly commented Mar 25, 2022 •

edited

Loading

teor2345 commented Mar 25, 2022

jvff left a comment

dconnolly commented Mar 25, 2022 •

edited

Loading

dconnolly commented Mar 25, 2022

teor2345 commented Apr 26, 2022

mergify bot commented Apr 26, 2022

dconnolly commented May 11, 2022

mergify bot commented May 11, 2022

dconnolly commented May 21, 2022

mergify bot commented May 21, 2022

teor2345 commented May 25, 2022

conradoplg left a comment

dconnolly commented Jun 16, 2022

mergify bot commented Jun 16, 2022

dconnolly commented Jun 23, 2022

teor2345 commented Jun 28, 2022

mergify bot commented Jun 28, 2022

teor2345 left a comment

daira left a comment

teor2345 left a comment

daira left a comment

teor2345 left a comment

teor2345 commented Aug 1, 2022

mergify bot commented Aug 1, 2022

Disallow Orchard ivk = 0 on IncomingViewingKey::from & SpendingKey generation #3962

Disallow Orchard ivk = 0 on IncomingViewingKey::from & SpendingKey generation #3962

Conversation

dconnolly commented Mar 25, 2022 • edited Loading

Motivation

Specifications

Solution

Review

Reviewer Checklist

Follow Up Work

teor2345 commented Mar 25, 2022

jvff left a comment

Choose a reason for hiding this comment

dconnolly commented Mar 25, 2022 • edited Loading

dconnolly commented Mar 25, 2022

teor2345 commented Apr 26, 2022

mergify bot commented Apr 26, 2022

✅ Branch has been successfully updated

dconnolly commented May 11, 2022

mergify bot commented May 11, 2022

✅ Branch has been successfully updated

dconnolly commented May 21, 2022

mergify bot commented May 21, 2022

❌ Base branch update has failed

teor2345 commented May 25, 2022

conradoplg left a comment

Choose a reason for hiding this comment

dconnolly commented Jun 16, 2022

mergify bot commented Jun 16, 2022

✅ Branch has been successfully updated

dconnolly commented Jun 23, 2022

teor2345 commented Jun 28, 2022

mergify bot commented Jun 28, 2022

✅ Branch has been successfully updated

teor2345 left a comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 commented Aug 1, 2022

mergify bot commented Aug 1, 2022

✅ Branch has been successfully updated

dconnolly commented Mar 25, 2022 •

edited

Loading

dconnolly commented Mar 25, 2022 •

edited

Loading