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

Classic McEliece Cryptodoc #186

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Classic McEliece Cryptodoc #186

wants to merge 6 commits into from

Conversation

FAlbertDev
Copy link
Collaborator

@FAlbertDev FAlbertDev commented Feb 14, 2024

Crypto documentation for Botan's Classic McEliece implementation.

Truncated Source Links

I experimented with the :srcref: restructuredText role for this documentation. I think the file location lists are unnecessarily bulked with redundant data. I introduced a truncated :srcref view that addresses this problem. In Classic McEliece, I name the Classic McEliece directory once for the reader and then use something like:
:srcref:`[src/lib/pubkey/classic_mceliece]/cmce_types.h` which is displayed as .../cmce_types.h. The link works properly, of course.

Note that the CI fails until randombit/botan#3883 is merged.

TODO Tracker

@FAlbertDev FAlbertDev self-assigned this Feb 15, 2024
@FAlbertDev FAlbertDev added this to the Botan 3.4.0 milestone Feb 15, 2024
@reneme
Copy link
Collaborator

reneme commented Feb 15, 2024

Re:sourceref: I'll have a go at the discussion we had in the FrodoKEM PR. Let's see how we can integrate this. But having an explicit way to shorten the reference links is certainly a good idea.

Edit: I cherry-picked your path elision feature in #188. Let's rebase once that is merged.

@reneme
Copy link
Collaborator

reneme commented Feb 22, 2024

Rebased to main, resolved pending merge conflicts and removed the commit adding the truncation feature to :srcref:. This now uses the unified :srcref: extensions that came in #188.

Copy link
Collaborator

@atreiber94 atreiber94 left a comment

Choose a reason for hiding this comment

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

Very nice, I like it a lot! I added some editorial comments

docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/90_bibliographie.rst Show resolved Hide resolved
docs/cryptodoc/src/90_bibliographie.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/90_bibliographie.rst Show resolved Hide resolved
@FAlbertDev
Copy link
Collaborator Author

Thanks for your comprehensive review, @atreiber94! I addressed your comments.

Copy link
Collaborator

@atreiber94 atreiber94 left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm!

I left open some minor things, most notably I think we should explicitly rule out introducing timing side channels by utilizing the bitvector class when we mention that only parts of the functionality are constant time.

docs/cryptodoc/src/05_10_cmce.rst Show resolved Hide resolved
@atreiber94 atreiber94 marked this pull request as ready for review March 22, 2024 12:36
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Show resolved Hide resolved
docs/cryptodoc/src/05_10_cmce.rst Outdated Show resolved Hide resolved
@atreiber94
Copy link
Collaborator

Addressed all the comments

@atreiber94 atreiber94 requested a review from weitkaemper-bsi May 2, 2024 12:41
@reneme reneme modified the milestones: Botan 3.4.0, Botan 3.6.0 Jun 13, 2024
@reneme reneme modified the milestones: Botan 3.6.0, Botan 3.7.0 Oct 22, 2024
@reneme
Copy link
Collaborator

reneme commented Oct 22, 2024

Postponed once more into Botan 3.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants