CIP-0083 | confirm openssl behaviour, fix command line example

[paweljakubas]

A. There is one under-specification that I found worth adding.
AES-256-CBC cipher as implemented in OpenSSL requires messages that are a multiple of 16-bytes. If the message does not fit then padding is used. In the current PR the padding technique is explicitly exhibited. It is very important information for those that implement this CIP - as I was in the context of cardano-wallet.

B. The second thing worth mentioning is the example specified, ie.

openssl enc -e -aes-256-cbc -pbkdf2 -iter 10000 -a -k "cardano" <<< '["Invoice-No: 123456789","Order-No: 7654321","Email: john@doe.com"]'

It is unfortunate as <<< is adding new line!

Perform shell expansion on word and pass the result to standard input. This is known as a here-string. Compare the use of word in here-documents above, where word does not undergo shell expansion. The result will have a trailing newline after it.

Further proof:

$ openssl enc -e -aes-256-cbc -pbkdf2 -iter 10000 -a -k "cardano" --nosalt -v <<< '["Invoice-No: 123456789","Order-No: 7654321","Email: john@doe.com"]'
bufsize=8192
IBcjjGQ7akr/CV2Zb0HtCvEPQNndZujCZ7iaFGMjOX3q3PJg5aRUvHgO3gPnDzYE
7jFsGUK1bCdwsrn8kqI92EYRbU2O7jl0EVo2sKPw92Q=
bytes read   :       68
bytes written:      110

$ echo -n '["Invoice-No: 123456789","Order-No: 7654321","Email: john@doe.com"]' | openssl enc -e -aes-256-cbc -pbkdf2 -iter 10000 -a -k "cardano" -nosalt -v
bufsize=8192
IBcjjGQ7akr/CV2Zb0HtCvEPQNndZujCZ7iaFGMjOX3q3PJg5aRUvHgO3gPnDzYE
7jFsGUK1bCdwsrn8kqI92NccbG8oAtPJUktZTTcO/bg=
bytes read   :       67
bytes written:      110

Hence, I allowed myself to rewrite example using echo -n

C. jq powered encryption example.
-j option is missing (it instructs jq to not add newline). Illustrative example below (I used nosalt in order to be easily reproducible).
It should be like that (not adding newline)

$ echo -n '["Invoice-No: 123456789","Order-No: 7654321","Email: john@doe.com"]' | openssl enc -e -aes-256-cbc -pbkdf2 -iter 10000 -a -k "cardano" -nosalt
IBcjjGQ7akr/CV2Zb0HtCvEPQNndZujCZ7iaFGMjOX3q3PJg5aRUvHgO3gPnDzYE
7jFsGUK1bCdwsrn8kqI92NccbG8oAtPJUktZTTcO/bg=

It is now :

$ jq ".\"674\".msg = [ $(jq -crM .\"674\".msg normal-message-metadata.json | openssl enc -e -aes-256-cbc -pbkdf2 -iter 10000 -a -k "cardano" -nosalt | awk {'print "\""$1"\","'} | sed '$ s/.$//') ]" <<< '{"674":{"enc":"basic"}}' | tee encrypted-message-metadata.json | jq
{
  "674": {
    "enc": "basic",
    "msg": [
      "IBcjjGQ7akr/CV2Zb0HtCvEPQNndZujCZ7iaFGMjOX3q3PJg5aRUvHgO3gPnDzYE",
      "7jFsGUK1bCdwsrn8kqI92EYRbU2O7jl0EVo2sKPw92Q="
    ]
  }
}

After last commit and using -j we have perfect alignment with echo -n :

$ jq ".\"674\".msg = [ $(jq -cjrM .\"674\".msg normal-message-metadata.json | openssl enc -e -aes-256-cbc -pbkdf2 -iter 10000 -a -k "cardano" -nosalt | awk {'print "\""$1"\","'} | sed '$ s/.$//') ]" <<< '{"674":{"enc":"basic"}}' | tee encrypted-message-metadata.json | jq
{
  "674": {
    "enc": "basic",
    "msg": [
      "IBcjjGQ7akr/CV2Zb0HtCvEPQNndZujCZ7iaFGMjOX3q3PJg5aRUvHgO3gPnDzYE",
      "7jFsGUK1bCdwsrn8kqI92NccbG8oAtPJUktZTTcO/bg="
    ]
  }
}

[rphair]

@paweljakubas thanks; we'll discuss with & tag the CIP-0083 authors & reviewers as per #409 - but first please remove the superfluous white space changes (probably from your code editor) so we can properly focus on the changes you are proposing. The code diff should look like this when done 🙏 https://github.com/cardano-foundation/CIPs/pull/795/files?w=1

[paweljakubas]

thanks @rphair . I had automatic trailing whitespace removal. Now it should be ok. I will re-check jq command tomorrow if they work after using echo -n and if not will also adjust them.

[rphair]

@paweljakubas we have to be 100% sure about changing an Active CIP especially given the number of wallets supporting this standard.

Also: if these wallets have been padding to 16-byte blocks in practice, without CIP-0083 telling them to, certainly CIP-0083 should be updated to mention such a practice... which in turn suggests maybe a V2 of this protocol if some implementations are not padding the blocks. 😬

@gitmachtl @AndrewWestberg @Crypto2099 where does the trade practice stand on this issue? (The second issue of shell syntax seems absolutely straightforward and should be ready to change regardless: unless any objections about the command phrasing.)

[Crypto2099]

The changes seem minor and "default behavior" as defined, this is just explicitly stating what expected default behavior is. I'm good with adding this change as-written currently personally.

[HeinrichApfelmus]

Also: if these wallets have been padding to 16-byte blocks in practice, without CIP-0083 telling them to,

Do the codesamples that accompany CIP-0083 add padding?

[paweljakubas]

@rphair @Crypto2099 @HeinrichApfelmus Last commit adjust jq encryption example. I added rationale in point C in the description of this PR. Note: jq decryption example is fine as it is now! Cheers!

[gitmachtl]

Also: if these wallets have been padding to 16-byte blocks in practice, without CIP-0083 telling them to,

Do the codesamples that accompany CIP-0083 add padding?

No special things are taken into account about padding in the codesamples.

[gitmachtl]

@paweljakubas
The newline did slip true the checks, because only the json content itself was later on used for further processing. And so an extra linebreak/newline or so did not have an impact on that. The CIP is based on the CIP-0020 transaction messages. Only the content of the strings itself is relevant, not if the json format does include any newlines chars outside of those strings.

So not sure if this is really an issue, i guess we can edit it but it will not have an impact on the current implementation in the wallets, explorers, etc. IMO

[gitmachtl]

thanks @rphair . I had automatic trailing whitespace removal. Now it should be ok. I will re-check jq command tomorrow if they work after using echo -n and if not will also adjust them.

Whitespaces are valid data, before or after some text. So we have to be careful about that. But again, the content is json. So if openssl is automatically adding zero bytes to fill up the 16byte blocks, it does not impact the overall outcome of the json itself for further processing.

[paweljakubas]

@gitmachtl thanks for the comments! Indeed, when handling json objects extra newline might be not a problem as usually it undergoes further json parsing and it should end up with the same end content. Nevertheless, it is always good to have complete openssl interoperability and be surprise the least, especially if one works on the level of bytes. As I was when implementing this CIP and finding openSSL inconsistency as I was not appending newline. Only through debugging on byte level I realized the nature of the inconsistency. IMHO it would be also worth adding something about padding that is implicit in openSSL in this context. I am advocate of adding golden tests when implementing something according to standard/CIP/.. and here those two details are important to be in line. Of course, it is great you anticipate no problems due to that 👍

[rphair]

I'm happy with this after @Crypto2099's confirmation (#795 (comment)) that it reflects trade practice, including:

• padding included & defined exactly as exhaustively documented and linked on the IETF web page;
• a more portable shell example (without the dubious <<<);
• though newline likely "not a problem", not adding it does make the process seem more portable.

[gitmachtl]

While we are updating this README.md, we could also update the fact, that cardanoscan has also included this feature since the last update:

[rphair]

@gitmachtl it's already listed under Implementors: in the header: currently 7 implementors there, including CardanoScan, but only the original 4 of them (used as the basis for Active status) are listed & illustrated in the Path to Active section.

So I think it's understood that further implementors don't have to be added to the text content or provide evidence for them in screenshots (cc @Crypto2099 @Ryun1 in case they disagree).

[rphair]

@paweljakubas I've put this on the CIP Editors Meeting agenda in 5 days' time (https://hackmd.io/@cip-editors/87) since I think as soon as we can get the other editors to run through this, and also get whatever devs present to double-check, this could be merged at the next meeting or shortly afterward (if not already done in the meantime).

[Ryun1]

Seems to be low risk of breaking existing implementations, so this change is probably good.

[rphair]

Well received at this evening's CIP Editor meeting so just waiting for @Crypto2099 to also endorse this before merging.

[rphair]

p.s. marking Last Check & have put on agenda for meeting # 88 in case this final review gets missed somehow in the biweekly period.