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

block-padding: implement ISO 10126 #936

Closed
wants to merge 1 commit into from
Closed

block-padding: implement ISO 10126 #936

wants to merge 1 commit into from

Conversation

tirz
Copy link
Contributor

@tirz tirz commented Aug 20, 2023

This PR adds rand_core as a dependency for block-padding.
Maybe ISO 10126 should be put behind a feature?

@newpavlov
Copy link
Member

We certainly do not want to depend on getrandom/rand_core by default. At the very least it should be behind a disabled-by-default feature.

Have you encountered the need for it in practice or is it done simply for completeness sake? IIRC random padding is usually frowned upon. So if it's the latter, I am not sure it's worth to add this (arguably) peculiar padding scheme.

@tirz
Copy link
Contributor Author

tirz commented Aug 20, 2023

@newpavlov I added the feature iso-10126.
This is just for completeness sake.
I was working on a personal project and I decided to switch the Padding implementation to a generics P (Iso10126 is a possibility)... But really, I just need PKCS#7.

@newpavlov
Copy link
Member

Since ISO 10126 fits somewhat poorly into block-padding, I think it's better to simply omit it. If someone actually will need to use it in practice, then it can be implemented fairly easily outside of block-padding, our trait design allows to do it without much pain. Especially considering that the proposed implementation could be not flexible enough (e.g. it does not allow use of PRNG seeded with a fixed seed).

@newpavlov newpavlov closed this Aug 21, 2023
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.

2 participants