[PM-27533] Add cryptography usage guide
๐๏ธ Tracking
https://bitwarden.atlassian.net/browse/PM-27533
๐ Objective
There is a fair bit of change within the cryptographic APIs making it difficult for teams to know what they should be using. Further, teams don't know what tools are available for what type of issue.
This PR aims to solve this by providing a high-level guide, aimed at non-cryptography teams. This can be used to pick the APIs and constructs they should use for their features.
NOTE: The added page is a transitional document. As soon as KM moves ownership of more low-level cryptography usage into it's ownership and other teams don't need to interact with it, we will start removing those pieces from the contributing docs.
Cryptographic primitive documentation will live in the SDK.
โฐ Reminders before review
- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team
๐ฆฎ Reviewer guidelines
- ๐ (
:+1:) or similar for great changes - ๐ (
:memo:) or โน๏ธ (:information_source:) for notes or general info - โ (
:question:) for questions - ๐ค (
:thinking:) or ๐ญ (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - ๐จ (
:art:) for suggestions / improvements - โ (
:x:) or โ ๏ธ (:warning:) for more significant problems or concerns needing attention - ๐ฑ (
:seedling:) or โป๏ธ (:recycle:) for future improvements or indications of technical debt - โ (
:pick:) for minor or nitpick changes
Deploying contributing-docs with ย
ย Cloudflare Pages
| Latest commit: |
5b338ef
|
| Status: | ย โ ย Deploy successful! |
| Preview URL: | https://39d574a2.contributing-docs.pages.dev |
| Branch Preview URL: | https://km-crypto-docs-part-1.contributing-docs.pages.dev |
Checkmarx One โ Scan Summary & Details โ 82f3b79e-e6fd-4f14-8545-2f0db69bfec7
Great job! No new security vulnerabilities introduced in this pull request
๐ This is great! Thank you for taking the time to write this up.
One quick suggestion I had was to put all the structs/classes/methods in code so they stand out at first glance (e.g. MasterPasswordAuthenticationData).
@trmartin4 I linked to the rust impl's of the structs.
Claude finished @quexten's task โโ View job
Review Complete
Summary
I've conducted a thorough review of the cryptography guide documentation. Overall, the document is well-written and provides clear guidance for non-cryptography developers. I found one grammar issue requiring attention and several minor suggestions for improved clarity and consistency.
Findings
Finding 1: Grammar error - missing possessive apostrophe (line 21) โ ๏ธ
Finding 2: Style inconsistency - hyphenation of "content-encryption-key" varies throughout the document (line 94) ๐ญ
Finding 3: Terminology - MAC/HMAC usage is consistent, but first usage could benefit from expansion for clarity (line 102) ๐ญ
Finding 4: Clarity improvement - article usage with "salt" (line 162) ๐ญ
Finding 5: Clarity improvement - "input parameters" specification (line 163) ๐ญ
Previous Review Issues
Regarding my previous review comment on line 175: You were absolutely correct, @quexten - the text already contains "the" before "data" in "encapsulates the data". That finding was an error on my part, and I apologize for the confusion.
The other findings from previous reviews appear to have been addressed or were similarly incorrect. The document is in good shape.
Recommendation
The only change I'd recommend as necessary is Finding 1 (the possessive apostrophe). The other findings are minor style/clarity suggestions that could improve readability but aren't critical.
Some of this content as well as most if not all of what is in #703 seems to be better-served via our "close to code" efforts; see more details at https://bitwarden.atlassian.net/wiki/spaces/EN/pages/1774977070/Documentation+Patterns. With thesec changes in the repos where it's built it will be better-ingested, especially by AI. I would suggest keeping this lighter and covering the what in sdk and clients; this site is for the how.
Hey @withinfocus, KM does keep documentation pretty close to code, for example: https://github.com/bitwarden/sdk-internal/blob/5052afde96ab02e275a94e7975d696715e34c843/crates/bitwarden-crypto/src/safe/password_protected_key_envelope.rs#L1 https://github.com/bitwarden/sdk-internal/blob/main/crates/bitwarden-crypto/src/safe/README.md
However, the changes introduced in this PR at least don't serve the purpose of documenting code, but instead aiding both discoverability and setting rules / process.
For example, if mobile were to introduce an IPC protocol, they should not write their own protocol, but use something standard (like noise). There is no good way to document this close to the code, and the contributing docs are a good central place to discover these rules.
For discoverability, we have gotten feedback by other teams that discoverability is currently poor with regards to cryptography. We want a central place to guide teams to the right tools to use across all repos. We also want engineers onboarding to find this documentation as part of reading the contributing docs, so that these are understood, and not only discovered upon specific search.
On the follow-up work however, I do agree, we should consider moving it to the SDK's crypto repository.
We want a central place to guide teams to the right tools to use across all repos. We also want engineers onboarding to find this documentation as part of reading the contributing docs, so that these are understood, and not only discovered upon specific search.
I definitely understand your intentions with that, but as someone on the outside looking in so to speak this reads rather heavily on concepts vs. truly how something is accomplished. If we say for this PR "this is OK, because these are contributing docs for an SDK or KM engineer" then I could be persuaded, but if it's for all of engineering then I think it doesn't work -- it's far too specific. My belief is that few people here will work on cryptography, deliberately, and that the audience is a specific team doing that kind of work, therefore folder-specific SDK documentation would be its home.
I'd like to get others' opinions of this. If it were in sdk-internal however it could be ingested easily by an AI tool to aid in development there ... the only place crypto development is supposed to occur.
Claude's feedback seems valid too by the way.