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

mgm: Generalize TDES logic to enable other algorithms #588

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Jan 4, 2025

Part of #330.

Includes #584 because I don't have write permissions on the repo and thus can't create stacked PRs directly.

@tony-iqlusion
Copy link
Member

@str4d if you can rebase and remove draft we can get this reviewed

@str4d str4d force-pushed the 330-mgm-generalization branch from 622d989 to aaae123 Compare February 13, 2025 00:21
@str4d str4d marked this pull request as ready for review February 13, 2025 00:21
@str4d
Copy link
Contributor Author

str4d commented Feb 13, 2025

Rebased on main. Note that although I've marked it as not draft / ready for review, there are still some TODOs in the code that I want reviewers to comment on.

@str4d str4d force-pushed the 330-mgm-generalization branch from aaae123 to 4c00e0b Compare February 13, 2025 00:23
@str4d
Copy link
Contributor Author

str4d commented Feb 13, 2025

Force-pushed to fix a rebase bug.

@str4d str4d force-pushed the 330-mgm-generalization branch from 4c00e0b to ca197e1 Compare February 13, 2025 00:29
@str4d
Copy link
Contributor Author

str4d commented Feb 13, 2025

Force-pushed to update a new test for the generalization.

Comment on lines +214 to +215
/// TODO: Can we distinguish DES from AES-192? Or do we take `C` as a parameter and
/// require the caller to know the type of the bytes they are parsing?
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure you can take a generic cipher instance since you're trying to map concrete ciphers to an enum. I think you either need some kind of separate algorithm identifier enum you can pass as a parameter, or to have separate constructor functions per algorithm.


/// Derives a management key (MGM) with the given algorithm from a stored salt.
///
/// TODO: Is this supported for AES? Is the algorithm supposed to be dynamic?
Copy link
Member

Choose a reason for hiding this comment

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

No idea here

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