Skip to content

Plumbing for ECC#87

Open
chrysn wants to merge 14 commits into
mainfrom
ec-plumbing
Open

Plumbing for ECC#87
chrysn wants to merge 14 commits into
mainfrom
ec-plumbing

Conversation

@chrysn

@chrysn chrysn commented Jun 24, 2026

Copy link
Copy Markdown
Member

No description provided.

@chrysn
chrysn force-pushed the ec-plumbing branch 2 times, most recently from 2536423 to 0cd72c4 Compare July 2, 2026 13:46
@chrysn

chrysn commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

First stage is approaching review readiness; still open:

  1. Plumbing API for accelerated scalar operations (that's why the plumbing doesn't have a SUPPORTED but HAS_MULTIPLY_SCALAR_POINT; those will be per-function, effectively) – not needed for MVP though.
  2. Loading and storing from bytes representations is missing (that's why the nRF is not a software-only high-level shim yet) – basics are there, but cutting that cord best works after the refactoring in 3.
  3. Some refactoring is better done later as it is about moving between files (and that's hard to review unless it's in stand-alone commits or even PRs).

@chrysn
chrysn force-pushed the ec-plumbing branch 2 times, most recently from 02051fb to 1b46bd3 Compare July 2, 2026 15:52
@chrysn

chrysn commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

I think this is now ready for a first round of review; @WilliamTakeshi, I'd appreciate a test on your hardware as a first step.

@franziskuskiefer / @jschneider-bensch, is that a minimal workable interface for you?

@chrysn
chrysn marked this pull request as ready for review July 3, 2026 13:51
@WilliamTakeshi

Copy link
Copy Markdown
Collaborator

Checked against hardware on 9259a875d, everything pass. I will take a deeper look later

(1/9) running `test_hash_algorithm_sha256`...
(2/9) running `test_hmac_sha256`...
(3/9) running `test_hkdf_sha256`...
(4/9) running `test_tryrng`...
(5/9) running `test_aead_aesccm_16_64_128`...
(6/9) running `test_aead_aesccm_16_64_256`...
(7/9) running `test_dh_ecdh_p256`...
(8/9) running `test_dh_x25519`...
(9/9) running `test_dh_x448`...
all tests passed!
Firmware exited successfully

@jschneider-bensch jschneider-bensch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we can try to implement an abstraction layer based on this in libcrux-p256. We'd need to implement the high-level operations in libcrux-p256 generically over an implementor of ec::plumbing::EcPrimitives<P256> and also implement ec::plumbing::EcPrimitives<P256> using the existing libcrux implementation of the functionality in the trait, right?

I'm not sure I understand at the moment how we'd extend embedded-cal-libcrux based on that, though.

Comment thread embedded-cal/src/error.rs
}
}

impl core::error::Error for ImportError {}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we have any requirements regarding the minimum supported Rust version? E.g. I think core::error::Error is stable since 1.81.

(I don't know what the current MSRV is of embedded-cal, or of its intended primary users, and I don't know if we have to think too much about it. Just wondering if there are some thoughts around it, when I saw the impl here again.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

See #98

/// compilation error), but that will need some testing w/rt ergonomics.
fn import_scalar_bytes(&mut self, scalar: &[u8]) -> Result<Self::Scalar, crate::ImportError>;

fn point(&mut self, x: Self::Scalar, y: Self::Scalar) -> Self::Point;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As I understand the signature, this assembles a Point from an affine representation as two scalars. Should it do any kind of validation of this affine representation, i.e. be fallible?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The way it is used here so far is with the caller's assertion that this is a point on the curve – as the input comes from point reconstruction.

So at this stage I'll fix this in the docs to say that the point is required to be on the curve (and it's the caller's responsibility to check that – after all, this is the plumbing and it's there to just give access to the accelerator).

(Also, now I remember why that function is not documented – because that question was not answered.)

chrysn added 14 commits July 18, 2026 17:45
The outer layer remains because the EC plumbing layer does not yet have
import and export functions -- otherwise, it could become the
software-demo layer.

A future refactoring step should move the CRACEN implementation into
the plumbing module (but that might be moot if the current dh module
is refactored into software-demo).
The high-level layer does not yet use those new types.
As common with plumbing, they are welcome to be empty (with the
supported consts false), and can easily be implemented that way.
… as DH plumbing may have it too.
This helps higher layers store exporeted secrets.
Recent history has shown that all users of EmptyCal<false> eventually
use EmptyCal<true> and just forward or not forward implementations they
need or don't need; let's simplify things.
@chrysn

chrysn commented Jul 18, 2026

Copy link
Copy Markdown
Member Author

Had to rebase because changes to the libcrux AEAD parts conflicted with a change (and it'd have taken a rebase anyway due to the fixups).

@WilliamTakeshi WilliamTakeshi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

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.

3 participants