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

feat(dpapi): implement encryption and key derivation functions #350

Merged
merged 11 commits into from
Jan 17, 2025

Conversation

TheBestTvarynka
Copy link
Collaborator

@TheBestTvarynka TheBestTvarynka commented Jan 17, 2025

Hi,
I implemented encryption/decryption and key derivation functions in this PR.

The crypto module contains many magic numbers. I took them from the Python DPAPI implementation: https://github.com/jborean93/dpapi-ng/blob/main/src/dpapi_ng/_crypto.py and https://github.com/jborean93/dpapi-ng/blob/main/src/dpapi_ng/_gkdi.py.

Docs & references:

@TheBestTvarynka TheBestTvarynka self-assigned this Jan 17, 2025
rand.workspace = true
hmac.workspace = true

rust-kbkdf = { version = "1.1", git = "https://gitlab.com/TheBestTvarynka/rust-kbkdf.git", branch = "fix-key-generation" }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it awful? Absolutely yes.
Do we have any alternative? Not really. See this: RustCrypto/KDFs#75 (comment)

I think the best solution is to contribute to RustCrypto. Or we can fork the rust-kbkdf crate

Copy link
Member

Choose a reason for hiding this comment

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

The best approach is typically to contribute to the upstream project, because this gives back to the open source community while removing maintenance burden from our shoulders. We definitely can’t release a crate with a git dependency. As you said, either we contribute back, either we go the fork route with a crate we maintain and push to crates.io. I’ll link this to Marc-André, and we’ll see how we prioritize this work.

Copy link
Member

@CBenoit CBenoit Jan 17, 2025

Choose a reason for hiding this comment

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

@TheBestTvarynka UPD: We agreed with Marc that the best course of action was to contribute KBKDF to RustCrypto.

  1. Take over the work in progress PR. Collaborate with the author and RustCrypto maintainers to implement the missing pieces so that PR can be merged. Check with them if you can open a new PR from your fork, or work from theirs.
  2. Let RustCrypto publish the crate and manage it.
  3. Remove the git dependency from dpdpi in favor of the proper crates.io dependency.
  4. Integrate and publish our dpdpi crate.

Your priority should be to move this forward so we’re not stuck with a git dependency when it’s time to integrate dpdpi into sspi.
It’s fine to merge work on dpdpi even with the git dependency, because dpdpi is not depended on by our other crates that we are publishing to crates.io yet.
Feel free to rotate back on the dpdpi work when you wait for their feedback.
RustCrypto maintainers are pretty responsive, so I believe things should move forward quickly 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great!

@TheBestTvarynka TheBestTvarynka marked this pull request as ready for review January 17, 2025 15:26
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

LGTM!

@CBenoit CBenoit merged commit c2122e6 into dev/dpapi Jan 17, 2025
42 checks passed
@CBenoit CBenoit deleted the feat/dpapi-encryption-and-key-derivation branch January 17, 2025 16:25
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