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

Implemented newer message fetching mechanism; minor code refactoring; major documentation refactoring #17

Merged
merged 15 commits into from
Oct 26, 2023

Conversation

lsd-cat
Copy link
Member

@lsd-cat lsd-cat commented Oct 18, 2023

See #16

Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

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

@lsd-cat, per https://freedomofpress.slack.com/archives/C123KPJ1X/p1697658988775359 I've performed a documentation-focused "review" as follows:

  • readme
    • text
    • notation
  • diagram
  • code
    • skimmed
    • read exhaustively
    • played with locally

Along with the suggestions I've left inline, I'm "approving" this pull request (a) because this is great work! and (b) so that you can merge it when you're ready. As always, please take what's useful to you and leave the rest! And let me know if you'd like me to do more of the above or follow up with any concrete changes.

Copy link
Member

Choose a reason for hiding this comment

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

Still refers to challenges, where the readme and code now uses gdh. I know we're still finalizing terminology—just noting the discrepancy for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I did a full review of the schema and had to do significant changes to bring the terminology more consistent and fix a few errors especially related to for cycles superscripts.

Comment on lines +273 to +276
* *JC<sub>SK</sub>*: Long term Journalist message-fetching private key
* *JC<sub>PK</sub>*: Long term Journalist message-fetching public key
* *JE<sub>SK</sub>*: Ephemeral per-message key-agreement private key
* *JE<sub>PK</sub>*: Ephemeral per-message key-agreement public key
Copy link
Member

Choose a reason for hiding this comment

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

Seeing these keys listed out this way, for message-fetching and group key agreement, is clarifying. It helps me understand that where before we were talking about a two-step interactive protocol that achieves the desired mechanism (challenge/response), we're now talking about a composition of two cryptographic negotiations that achieve the desired properties (three-party enumeration, two-party retrieval).

README.md Outdated Show resolved Hide resolved
4. *Source* generates the unique passphrase randomly *PW = G()* (the only state that identify the specific *Source*)
5. *Source* derives *S<sub>PK</sub>, S<sub>SK</sub> = G(KDF(encryption_salt + PW))*
6. *Source* derives *SC<sub>PK</sub>, SC<sub>SK</sub> = G(KDF(challenge_salt + PW))*
Only a source can initiate a conversation; there are no other choices as sources are effectively unknown until they initiate contact first.
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth making explicit the other places where the same operation (sending/replying, fetching, reading) are not symmetrical? Right now that requires carefully reading (e.g.) the Journalist message id fetching protocol and Source message id fetching protocol sections and comparing them line by line.

(I can propose text if helpful!)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing, feel free to suggest the text where you think it is more appropriate. I have added colors to the diagram so that it is more clear that the server side is symmetrical, while the client functions are not.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/freedomofpress/securedrop-poc/blob/8776722eab78a6fc446f97e2d0d147644c59b54b/imgs/sd_schema.png is perfect, because it can be read comparatively. I've proposed making that comparison explicitly in #18.

@lsd-cat lsd-cat merged commit c8a2e0f into main Oct 26, 2023
@lsd-cat
Copy link
Member Author

lsd-cat commented Oct 26, 2023

Note that there are still some discrepancies in subscripts between the chart and the text description. It is not super relavant, as i/j/n are just placeholders, but it would be nice to match them everywhere.

cfm added a commit that referenced this pull request Oct 26, 2023
@lsd-cat lsd-cat deleted the improved-challenge-response branch May 2, 2024 10:38
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