-
Notifications
You must be signed in to change notification settings - Fork 41
Update architecture.rst #50
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely correct about the 64 bit chunk number. And the remaining clarifications don't hurt either.
I am deeply sorry that it took me so long to verify this - I must have marked the request for review as read. And if it wasn't for @infeo who made me aware of this again today, I might have never reviewed it. I hope you can forgive me! 😅
# Conflicts: # source/security/architecture.rst
WalkthroughThe changes update documentation related to the security architecture and vault encryption processes. In the architecture documentation, the verification of the vault configuration JWT signature now uses the concatenation of the encryption masterkey and MAC masterkey, rather than just the masterkey. The masterkey file format description is clarified to state that the derived Key Encryption Key (KEK) is 32 bytes long, and both the encryption and MAC masterkeys are encrypted using AES Key Wrap. It is also explicitly noted that the scrypt derivation process is non-parallel. In the vault documentation, the construction of the Additional Authenticated Data (AAD) for AES-GCM encryption of file content chunks is changed: instead of a list containing the chunk number and file header nonce, the AAD is now a single byte sequence formed by concatenating the chunk number (as a 64-bit big-endian integer) and the header nonce. No public or exported entities are altered. Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
source/security/architecture.rst (2)
62-62
: Clarify signature concatenation orderIt’s ambiguous whether the encryption masterkey or the MAC masterkey comes first. To avoid potential interoperability issues, specify the exact order—for example: “concatenate the 32-byte encryption masterkey followed by the 32-byte MAC masterkey before verifying the JWT.”
128-128
: Hyphenate byte-sized terms and explicitly list scrypt paramsFor consistency and clarity, hyphenate “32-byte” and spell out all scrypt parameters (including
p=1
anddkLen=32
). For example:- Alternatively, for normal password-protected vaults, Cryptomator will derive a 32byte long :abbr:`KEK (Key-encryption key)` via `scrypt <https://tools.ietf.org/html/rfc7914>`_ (non-parallel), encrypt both masterkeys using `AES Key Wrap (RFC 3394)`_, + Alternatively, for normal password-protected vaults, Cryptomator derives a 32-byte KEK via scrypt (N=32768, r=8, p=1, dkLen=32), then encrypts both masterkeys using AES Key Wrap (RFC 3394),source/security/vault.rst (1)
59-59
: Disambiguate big-endian width in pseudocodeThe call
bigEndian(i)
doesn’t make the byte length explicit. Since AAD requires an 8-byte (64-bit) integer, consider using a more descriptive function or signature:-aad := bigEndian(i) . headerNonce +aad := bigEndian64(i) . headerNonce /* encode i as 8-byte big-endian */
@ho-oto Since the PR was left unattended for so long, some things were already fixed. I merged the main branch into yours and applied your changes again. Thank you for your contribution! |
Hello!
I've recently undertaken the challenge of decrypting a Cryptomator Vault, relying exclusively on the existing documentation, which can be found here: https://github.com/ho-oto/cryptomator-extractor-rs
In the course of this endeavor, I noticed several areas where the documentation is lacking or incomplete. This PR is intended to address and amend these shortcomings: