Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite AES code with cryptography (drop pyaes) #1526

Merged
merged 1 commit into from
Oct 8, 2023

Conversation

roshii
Copy link
Contributor

@roshii roshii commented Aug 7, 2023

Fixes #1518

@roshii roshii force-pushed the rewrite-aes branch 2 times, most recently from 8451e1f to 09cd05d Compare August 7, 2023 07:28
@roshii
Copy link
Contributor Author

roshii commented Aug 15, 2023

  • Fixed Padding
  • Added NIST test vectors

Vectors are parsed with parsed with https://gist.github.com/roshii/23f474394381093ba5ee814c1cb4fa6d
Forward block encryption/decryption is not tested as not relevant for joinmarket, please correct me if wrong.

@AdamISZ
Copy link
Member

AdamISZ commented Aug 15, 2023

"Forward block encryption/decryption"

I know about block ciphers, and about forward secrecy, but what is "forward block encryption"? I tried google but didn't see anything using that term?

@AdamISZ
Copy link
Member

AdamISZ commented Aug 15, 2023

The CI failure appears unrelated.

I have done a bit of testing moving wallets between old version of this encryption code and new version, checking for decryption (obviously) working, checking addresses and balances. It works.

I've also read the code and see everything being correct.

tACK 2ae5cca

However I see no rush in merging this one, obviously.

@roshii
Copy link
Contributor Author

roshii commented Aug 16, 2023

"Forward block encryption/decryption"

I know about block ciphers, and about forward secrecy, but what is "forward block encryption"? I tried google but didn't see anything using that term?

Sorry for that, I meant "block chaining" in which the output block is used as input block in the next round of encryption. Now that you mention forward secrecy I realize I did mix up terms having CBC schema in mind...

@AdamISZ
Copy link
Member

AdamISZ commented Aug 16, 2023

"Forward block encryption/decryption"

I know about block ciphers, and about forward secrecy, but what is "forward block encryption"? I tried google but didn't see anything using that term?

Sorry for that, I meant "block chaining" in which the output block is used as input block in the next round of encryption. Now that you mention forward secrecy I realize I did mix up terms having CBC schema in mind...

OK, now the name is clear, I see what you're asking. Indeed we wouldn't need to check block by block (though, we could) that the actual CBC mechanism is working as intended (which seems to be the intention of the NIST vectors). But it'd be wrong to say it doesn't apply to our use case, since our plaintexts are bigger than a single CBC block, so block chaining is occurring.

@roshii
Copy link
Contributor Author

roshii commented Aug 22, 2023

OK, now the name is clear, I see what you're asking. Indeed we wouldn't need to check block by block (though, we could) that the actual CBC mechanism is working as intended (which seems to be the intention of the NIST vectors). But it'd be wrong to say it doesn't apply to our use case, since our plaintexts are bigger than a single CBC block, so block chaining is occurring.

OK, I see my mistake now. I wrongly assumed that AES-CBC encryption of plaintext 2 was depending on plaintext 1 ... I now get that it is in fact plaintext block 2 that depends on plaintext block 1
I shall adjust test data, thanks!

@roshii
Copy link
Contributor Author

roshii commented Aug 23, 2023

  • Rebased on master
  • Adjusted test cases (concatenating blocks into single strings)

@roshii
Copy link
Contributor Author

roshii commented Aug 23, 2023

  • Fixed conflicting test function names

@roshii
Copy link
Contributor Author

roshii commented Sep 20, 2023

rebased

Copy link
Member

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

ACK 6eded6e

@roshii
Copy link
Contributor Author

roshii commented Oct 8, 2023

rebased

Copy link
Member

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

re-ACK 91dacf6

@AdamISZ AdamISZ merged commit c68ab5a into JoinMarket-Org:master Oct 8, 2023
20 checks passed
@roshii roshii deleted the rewrite-aes branch November 3, 2023 17:54
kristapsk added a commit that referenced this pull request Feb 10, 2024
d89dcde Remove convert_old_wallet.py script (Kristaps Kaupe)

Pull request description:

  Old wallet format isn't used for years and script is broken since removal of pyaes dependency in #1526. If somebody still needs it, he can use older JoinMarket version to do conversion.

Top commit has no ACKs.

Tree-SHA512: 26308ca2807ff954ace6308a42e490d61fe8e95ed92b3453233ec72a59a38d975cfb1cf6bdeda93d4c855e6994c593f05b6cc1d2e78bc4ab3d7bde84c02a8982
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite AES code using cryptography and drop pyaes dependency
3 participants