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

AeadKx #838

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

AeadKx #838

wants to merge 2 commits into from

Conversation

gonatienza
Copy link
Contributor

Recently thought about building this wrapper to simplify and harden the usage of newer key exchange apis along the newer aead constructs:

crypto_kx_client_session_keys(unsigned char rx[crypto_kx_SESSIONKEYBYTES],
                              unsigned char tx[crypto_kx_SESSIONKEYBYTES],
                              const unsigned char client_pk[crypto_kx_PUBLICKEYBYTES],
                              const unsigned char client_sk[crypto_kx_SECRETKEYBYTES],
                              const unsigned char server_pk[crypto_kx_PUBLICKEYBYTES]);

crypto_kx_server_session_keys(unsigned char rx[crypto_kx_SESSIONKEYBYTES],
                              unsigned char tx[crypto_kx_SESSIONKEYBYTES],
                              const unsigned char server_pk[crypto_kx_PUBLICKEYBYTES],
                              const unsigned char server_sk[crypto_kx_SECRETKEYBYTES],
                              const unsigned char client_pk[crypto_kx_PUBLICKEYBYTES]);

It would provide an xchacha20poly1305_ietf option for nacl.public. Besides the existing xsalsa20poly1305 from crypto_box. Plus the capability of using rx and tx keys (which would help with nonce counters use cases).

Here's a simple test as a quick visual aid:

from nacl.public import PrivateKx, AeadClient, AeadServer

skbob = PrivateKx.generate()
pkbob = skbob.public_key
skalice = PrivateKx.generate()
pkalice = skalice.public_key

bob_aead = AeadServer(skbob, pkalice)

message = b"Kill all humans"

alice_aead = AeadClient(skalice, pkbob)

encrypted = bob_aead.encrypt(message)

plaintext = alice_aead.decrypt(encrypted)

new_plaintext = bob_aead.decrypt_beforetx(encrypted)

encoded = bob_aead.encode()

new_aead = AeadClient.decode(encoded)

last_plaintext = new_aead.decrypt_beforetx(encrypted)

assert (
	bob_aead.rx_key() ==
	alice_aead.tx_key() ==
	new_aead.rx_key()
)

assert (
	alice_aead.rx_key() ==
	bob_aead.tx_key() ==
	new_aead.tx_key()
)

assert (
	plaintext ==
	new_plaintext ==
	last_plaintext
) 

Will do the docs afterwards, if merged.

Let me know what you think. Or if it feels like an overkill I will close the request.

HTH.

@reaperhulk
Copy link
Member

I think wrapping the newer APIs in libsodium is a good idea (we should stay up-to-date!), but I am a bit concerned about how to represent it to callers when we have a lot of partially overlapping concepts. Do you see that as solely a documentation problem?

@gonatienza
Copy link
Contributor Author

gonatienza commented Sep 16, 2024

Hey @reaperhulk, I think pynacl users follow the docs guidance before looking into docstring and trying to figure things out by themselves for sure. I always saw the wrappers and their documentation as an excellent way in which PYCA recommends, simplifies and hardens implementation practices of the bindings.

The reason for these wrappers was simply to provide within nacl.public an xchacha20 alternative. Which I really believe more and more users will tend to go towards to by default.

Are the PrivateKx and PublicKx the classes that provide the overlapping concept feeling? There could be ways to mitigate that, but would involve touching (without affecting its current default behavior) the PrivateKey and PublicKey classes. I originally wrote the wrappers using those classes as they are today. But the fix sizes being mapped to crypto_box_* sizes (although they were the same value) did not feel it was the right way of future proofing. And touching them did not feel like the right way to go either (I tested that too).

@gonatienza
Copy link
Contributor Author

Hey @reaperhulk , just submitted some modifications for readability and to implement the new base class as an abstract base class instead. More consistent with the rest of the code in PyNacl.

@gonatienza
Copy link
Contributor Author

Hey @reaperhulk , should I kill this PR?

@reaperhulk
Copy link
Member

I've been too busy to review this unfortunately. I don't think you should kill it until I've at least had a chance to look more closely. At that point I'll let you know, sorry about the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants