contributing-docs icon indicating copy to clipboard operation
contributing-docs copied to clipboard

[PM-27533] Add cryptography usage guide

Open quexten opened this issue 2 months ago โ€ข 9 comments

๐ŸŽŸ๏ธ 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

quexten avatar Nov 06 '25 13:11 quexten

Deploying contributing-docs with ย Cloudflare Pages ย 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

View logs

Logo Checkmarx One โ€“ Scan Summary & Details โ€“ 82f3b79e-e6fd-4f14-8545-2f0db69bfec7

Great job! No new security vulnerabilities introduced in this pull request

github-actions[bot] avatar Nov 06 '25 13:11 github-actions[bot]

๐Ÿ™Œ 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 avatar Nov 06 '25 14:11 trmartin4

@trmartin4 I linked to the rust impl's of the structs.

quexten avatar Nov 07 '25 11:11 quexten

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.


claude[bot] avatar Nov 11 '25 16:11 claude[bot]

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.

withinfocus avatar Nov 11 '25 17:11 withinfocus

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.

quexten avatar Nov 12 '25 00:11 quexten

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.

withinfocus avatar Nov 12 '25 21:11 withinfocus

Claude's feedback seems valid too by the way.

withinfocus avatar Nov 24 '25 13:11 withinfocus