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

Decrypt message above 16k #1342

Closed
wants to merge 2 commits into from
Closed

Decrypt message above 16k #1342

wants to merge 2 commits into from

Conversation

Dzordzu
Copy link

@Dzordzu Dzordzu commented Nov 23, 2023

Current behavior

As of now decrypting in crypto.py works only for small messages. This does affect an entire minioadmin module.

Introduced solution

Partition the message into smaller chunks. Decode every chunk separately

Related issues


from argon2.low_level import Type, hash_secret_raw
from Crypto.Cipher import AES, ChaCha20_Poly1305

_NONCE_LEN = 8
# Encrypted message format:
Copy link
Member

Choose a reason for hiding this comment

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

Where do you get this information?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Also it was present in the current code

Copy link
Author

@Dzordzu Dzordzu Nov 23, 2023

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

  1. Please point https://github.com/minio/madmin-go/blob/main/encrypt.go#L43-L49 instead of repeating it here.
  2. I don't see more than 16k implementation in madmin-go. Could you point why do we need that?

Copy link
Author

@Dzordzu Dzordzu Nov 23, 2023

Choose a reason for hiding this comment

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

  1. I don't understand. What are you asking me to do?
  2. It's located in the sio implementation. As of now smaller messages work as intended. Unfortunately, the responses that contain 16k bytes are not decrypted at all. In the official sio-go implementation (by secure-io, that is internally used by madmin-go) the payload is split into many smaller chunks. By default the buffer size (it is chunk size) contains 16K bytes. Not to mention that madmin-go uses the default configuration.

Copy link
Author

@Dzordzu Dzordzu Nov 23, 2023

Choose a reason for hiding this comment

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

If you want to reproduce the issue try to create above 3k buckets with members (with long names). Then try to call an endpoint responsible for listing entities with their affiliation (see an issue, related to this PR)

Copy link
Member

Choose a reason for hiding this comment

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

If you want to reproduce the issue try to create above 3k buckets with members (with long names). Then try to call an endpoint responsible for listing entities with their affiliation

For this scenario, what API have you used to reproduce the issue?

Copy link
Author

Choose a reason for hiding this comment

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

For example (minioadmin, so prefixed with proper path):

idp/ldap/policy-entities
idp/builtin/policy-entities

list-users for bigger instances…

Copy link
Author

@Dzordzu Dzordzu Nov 23, 2023

Choose a reason for hiding this comment

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

Anything with potentially more capacious response. 16k is the limit


from argon2.low_level import Type, hash_secret_raw
from Crypto.Cipher import AES, ChaCha20_Poly1305

_NONCE_LEN = 8
# Encrypted message format:
Copy link
Member

Choose a reason for hiding this comment

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

  1. Please point https://github.com/minio/madmin-go/blob/main/encrypt.go#L43-L49 instead of repeating it here.
  2. I don't see more than 16k implementation in madmin-go. Could you point why do we need that?

@Dzordzu
Copy link
Author

Dzordzu commented Nov 23, 2023

Squashed commits

return ChaCha20Poly1305CipherProvider()
return None


def decrypt(payload: bytes, password: str) -> bytes:
Copy link
Member

Choose a reason for hiding this comment

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

We would need to have reader interface than handling bytes here.

I am working on this including size > 16KiB for encrypt and decrypt.

@balamurugana
Copy link
Member

Please refer #1345. Feel free to test it and close this PR accordingly

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.

3 participants