Tighten criteria for accepting Shelley or Byron addresses in REST API #3019

HeinrichApfelmus · 2021-11-15T12:41:36Z

Summary

It has become established usage that Byron addresses are associated with the Base58 encoding while Shelley addresses are associated with the Bech32 encoding.

To catch more user errors, we now enforce this usage more strictly.

Specifically,

Base58 encoded addresses are assumed to be Byron addresses and are additionally deserialized to CBOR to check whether they comply with the Byron address binary format.
We no longer accept Base16 encoded addresses. The swagger documentation has already highlighted that only Base58 and Bech32 are accepted.

To avoid loss of funds, we strongly recommend to use Shelley addresses and the Bech32 encoding, as this encoding includes error detection and is more robust against misspellings and truncations.

Relevant standards: CIP 19 — Cardano addresses.

Comments

(EDIT: correction)

Ideally, lay users would only see a single canonical version of every address. Unfortunately, we are not quite there yet, and this PR is only a partial improvement. For example,
- Bech32-encoded addresses can represent Byron addresses instead of Shelley addresses. These would be shown in Base58 encoding in the blockchain explorer.
- A sequence of bytes which is too long to be a valid Shelley address, but has a header-byte that indicates a Shelley address, may be truncated to the proper size. Put differently, seemingly invalid addresses are mapped to valid ones. Fortunately, the error correction inherent to Bech32 and reserving the Base58 encoding for Byron addresses (implemented in this PR) make this less likely to occur through user mistake.
A general clean up of our address data types would be nice, but is out of scope for this pull request.

Issue Number

ADP-1033, ADP-1280

jonathanknowles

Hi @HeinrichApfelmus

Thank-you for making this PR!

I've had a look through, and it seems very reasonable. Though I have a question and a couple of small suggestions. (See comments.)

Thanks!

lib/shelley/src/Cardano/Wallet/Shelley/Compatibility.hs

jonathanknowles · 2021-11-16T04:10:50Z

lib/shelley/src/Cardano/Wallet/Shelley/Compatibility.hs

-tryBech32 text = do
-    (_, dp) <- either (const Nothing) Just (Bech32.decodeLenient text)
-    dataPartToBytes dp
+tryBech32 = fmap CA.unAddress . CA.fromBech32


By following CA.fromBech32 to its definition, I think I have confirmed that fromBech32 does indeed also use Bech32.decodeLenient. However, I just want to check: do we have regression test coverage to warn us if this ever changes?

(I ask, because it's just inside the realm of possibility that someone might change the definition of CA.fromBech32 to use Bech32.decode, rather than its more lenient variant.)

Hm. I'm not sure if we should check this, for two reasons:

The address format is standardized in CIP-0019 which lists the cardano-addresses package as a reference implementation. So, regardless of how Cardano.Address.fromBech32 is implemented, it must be correct — by virtue of being a reference. 😅

Using Bech32.decode instead of Bech32.decodeLenient would actually not be a regression — as far as the definition and error correcting properties of the Bech32 encoding are concerned. 🙈 (BIP-0173: "A Bech32 string is at most 90 characters long and consists of […]"). Shelley addresses are typically 57 bytes in length (1 byte header + 28 bytes payment part + 28 bytes delegation part), so that would work out. The big exception are pointer addresses which can be arbitrarily long, but of which there are only a handful of instances on mainnet. 🙈

Oh wait. Even the regular Shelley addresses would not quite fit into the 90 character limit, as 57 bytes = 456 bits = 92.5 * (5 bits), and one character encodes 5 bits.

I have added a test that covers Shelly addresses with delegation part; these will start to fail if the 90 character limit is enforced. In this way, we are testing functionality while covering the formalities.

HeinrichApfelmus · 2021-11-16T13:54:35Z

Thanks for your review, Jonathan! 😊

Will merge unless there are further objections.

rvl

Would it be possible to add a note to the swagger API doc, just to make clear the decoding behaviour to users?

HeinrichApfelmus · 2021-11-18T12:31:30Z

bors r+

3019: Tighten criteria for accepting Shelley or Byron addresses in REST API r=HeinrichApfelmus a=HeinrichApfelmus ### Summary It has become established usage that Byron addresses are associated with the Base58 encoding while Shelley addresses are associated with the Bech32 encoding. To catch more user errors, we now enforce this usage more strictly. Specifically, * Base58 encoded addresses are assumed to be Byron addresses and are additionally deserialized to CBOR to check whether they comply with the Byron address binary format. * We no longer accept Base16 encoded addresses. The swagger documentation has already highlighted that only Base58 and Bech32 are accepted. To avoid loss of funds, we strongly recommend to use Shelley addresses and the Bech32 encoding, as this encoding includes error detection and is more robust against misspellings and truncations. Relevant standards: [CIP 19 — Cardano addresses](https://github.com/cardano-foundation/CIPs/tree/master/CIP-0019). ### Comments (EDIT: correction) * Ideally, lay users would only see a single canonical version of every address. Unfortunately, we are not quite there yet, and this PR is only a partial improvement. For example, * Bech32-encoded addresses can represent Byron addresses instead of Shelley addresses. These would be shown in Base58 encoding in the blockchain explorer. * A sequence of bytes which is too long to be a valid Shelley address, but has a header-byte that indicates a Shelley address, may be truncated to the proper size. Put differently, seemingly invalid addresses are mapped to valid ones. Fortunately, the error correction inherent to Bech32 and reserving the Base58 encoding for Byron addresses (implemented in this PR) make this less likely to occur through user mistake. * A general clean up of our address data types would be nice, but is out of scope for this pull request. ### Issue Number ADP-1033, ADP-1280 Co-authored-by: Heinrich Apfelmus <[email protected]>

iohk-bors · 2021-11-18T13:03:14Z

Build failed:

buildkite/cardano-wallet



src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs:238:13:
--
  | 1) CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_CREATE_05 - Invalid addresses, q
  | "Please enter your passphrase: **************\nError in $.payments[0].address: parsing AddressAmount failed, Unrecognized address encoding: must be either bech32 or base58. Perhaps your address is not entirely correct? Please double-check each character within the address and try again.\n" does not contain "Unable to decode address"
  |  
  | To rerun use: --match "/CLI Specifications/SHELLEY_CLI_TRANSACTIONS/TRANS_CREATE_05 - Invalid addresses/q/"
  |  
  | src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs:238:13:
  | 2) CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_CREATE_05 - Invalid addresses, empty
  | "Please enter your passphrase: **************\nError in $.payments[0].address: parsing AddressAmount failed, Unrecognized address encoding: must be either bech32 or base58. Perhaps your address is not entirely correct? Please double-check each character within the address and try again.\n" does not contain "Unable to decode address"
  |  
  | To rerun use: --match "/CLI Specifications/SHELLEY_CLI_TRANSACTIONS/TRANS_CREATE_05 - Invalid addresses/empty/"
  |  
  | src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs:238:13:
  | 3) CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_CREATE_05 - Invalid addresses, no address
  | "Please enter your passphrase: **************\nError in $.payments[0].address: parsing AddressAmount failed, Unrecognized address encoding: must be either bech32 or base58. Perhaps your address is not entirely correct? Please double-check each character within the address and try again.\n" does not contain "Unable to decode address"
  |  
  | To rerun use: --match "/CLI Specifications/SHELLEY_CLI_TRANSACTIONS/TRANS_CREATE_05 - Invalid addresses/no address/"
  |  
  | src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs:243:59:
  | 4) CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_CREATE_06 - Invalid amount, string with wildcards
  | uncaught exception: IOException of type ResourceVanished
  | fd:119: hFlush: resource vanished (Broken pipe)
  |  
  | To rerun use: --match "/CLI Specifications/SHELLEY_CLI_TRANSACTIONS/TRANS_CREATE_06 - Invalid amount/string with wildcards/"
  |  
  | src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs:386:13:
  | 5) CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_ESTIMATE_08 - Invalid addresses, q
  | "Error in $.payments[0].address: parsing AddressAmount failed, Unrecognized address encoding: must be either bech32 or base58. Perhaps your address is not entirely correct? Please double-check each character within the address and try again.\n" does not contain "Unable to decode address"
  |  
  | To rerun use: --match "/CLI Specifications/SHELLEY_CLI_TRANSACTIONS/TRANS_ESTIMATE_08 - Invalid addresses/q/"
  |  
  | src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs:386:13:
  | 6) CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_ESTIMATE_08 - Invalid addresses, empty
  | "Error in $.payments[0].address: parsing AddressAmount failed, Unrecognized address encoding: must be either bech32 or base58. Perhaps your address is not entirely correct? Please double-check each character within the address and try again.\n" does not contain "Unable to decode address"
  |  
  | To rerun use: --match "/CLI Specifications/SHELLEY_CLI_TRANSACTIONS/TRANS_ESTIMATE_08 - Invalid addresses/empty/"
  |  
  | src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs:386:13:
  | 7) CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_ESTIMATE_08 - Invalid addresses, no address
  | "Error in $.payments[0].address: parsing AddressAmount failed, Unrecognized address encoding: must be either bech32 or base58. Perhaps your address is not entirely correct? Please double-check each character within the address and try again.\n" does not contain "Unable to decode address"
  |  
  | To rerun use: --match "/CLI Specifications/SHELLEY_CLI_TRANSACTIONS/TRANS_ESTIMATE_08 - Invalid addresses/no address/"

I believe this is
#expected

piotr-iohk · 2021-11-18T16:27:24Z

lib/core-integration/src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs

          , ( "wildcards", T.unpack wildcardsWalletName, parseErr )
          , ( "arabic", T.unpack arabicWalletName, encodeErr )
          , ( "kanji", T.unpack kanjiWalletName, encodeErr )
          , ( "polish", T.unpack polishWalletName, encodeErr )
          , ( "[]", "[]", encodeErr )
-          , ( "no address", "", encodeErr2 )
+          , ( "no address", "", encodeErr )


Maybe it would be actually good to add addresses from the issue to that list?

For reasons of privacy, I suppose that we do not want to add the addresses from the issue. However, I have added a randomly generated Base58 string that is not a valid Byron address.

Strictly speaking, the Bech32 standard defines that the encoded sequence may not be longer than 90 characters — but Shelley addresses can be longer than that. We can cover this case with a test that looks more at functionality and considers addresses with delegation part. Also changes suggested during review and additional tests. Co-authored-by: Jonathan Knowles <[email protected]>

HeinrichApfelmus · 2021-11-19T12:19:41Z

bors r+

iohk-bors · 2021-11-19T14:15:12Z

Build succeeded:

HeinrichApfelmus self-assigned this Nov 15, 2021

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1033 branch 2 times, most recently from 97da63f to a9bf986 Compare November 15, 2021 15:40

jonathanknowles approved these changes Nov 16, 2021

View reviewed changes

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1033 branch 3 times, most recently from db395be to dace3d1 Compare November 16, 2021 13:54

rvl reviewed Nov 17, 2021

View reviewed changes

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1033 branch from dace3d1 to 543e7a7 Compare November 17, 2021 16:04

Use Cardano.Address for decoding Byron or Shelly addresses

dca1588

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1033 branch from 543e7a7 to 217c3cc Compare November 18, 2021 12:29

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1033 branch 2 times, most recently from 0424900 to 320bc22 Compare November 18, 2021 16:17

piotr-iohk reviewed Nov 18, 2021

View reviewed changes

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1033 branch from 320bc22 to 1461cc3 Compare November 18, 2021 16:57

iohk-bors bot merged commit 09e6383 into master Nov 19, 2021

iohk-bors bot deleted the HeinrichApfelmus/ADP-1033 branch November 19, 2021 14:15

WilliamKingNoel-Bot pushed a commit that referenced this pull request Nov 19, 2021

docs: Merge #3019

a25bfdb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tighten criteria for accepting Shelley or Byron addresses in REST API #3019

Tighten criteria for accepting Shelley or Byron addresses in REST API #3019

HeinrichApfelmus commented Nov 15, 2021 •

edited

Loading

jonathanknowles left a comment

jonathanknowles Nov 16, 2021

HeinrichApfelmus Nov 16, 2021

HeinrichApfelmus Nov 16, 2021

HeinrichApfelmus Nov 16, 2021

HeinrichApfelmus commented Nov 16, 2021

rvl left a comment

HeinrichApfelmus commented Nov 18, 2021

iohk-bors bot commented Nov 18, 2021 •

edited by Anviking

Loading

piotr-iohk Nov 18, 2021

HeinrichApfelmus Nov 18, 2021

HeinrichApfelmus commented Nov 19, 2021

iohk-bors bot commented Nov 19, 2021

Tighten criteria for accepting Shelley or Byron addresses in REST API #3019

Tighten criteria for accepting Shelley or Byron addresses in REST API #3019

Conversation

HeinrichApfelmus commented Nov 15, 2021 • edited Loading

Summary

Comments

Issue Number

jonathanknowles left a comment

Choose a reason for hiding this comment

jonathanknowles Nov 16, 2021

Choose a reason for hiding this comment

HeinrichApfelmus Nov 16, 2021

Choose a reason for hiding this comment

HeinrichApfelmus Nov 16, 2021

Choose a reason for hiding this comment

HeinrichApfelmus Nov 16, 2021

Choose a reason for hiding this comment

HeinrichApfelmus commented Nov 16, 2021

rvl left a comment

Choose a reason for hiding this comment

HeinrichApfelmus commented Nov 18, 2021

iohk-bors bot commented Nov 18, 2021 • edited by Anviking Loading

piotr-iohk Nov 18, 2021

Choose a reason for hiding this comment

HeinrichApfelmus Nov 18, 2021

Choose a reason for hiding this comment

HeinrichApfelmus commented Nov 19, 2021

iohk-bors bot commented Nov 19, 2021

HeinrichApfelmus commented Nov 15, 2021 •

edited

Loading

iohk-bors bot commented Nov 18, 2021 •

edited by Anviking

Loading