Cot updates v2#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates secure-string to better integrate with cot-rs by switching equality to subtle::ConstantTimeEq, removing the previous pre-based contracts, and simplifying dependency/test setup.
Changes:
- Implement constant-time equality for
SecureVec,SecureArray, andSecureBoxusingsubtle::ConstantTimeEq, and adjust tests accordingly. - Remove
preandserde_cbordev-dependencies/tests; keep JSON-only serde tests. - Clean up CI and introduce shared test helpers (
src/test_utils.rs) for padding/packed equality cases.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/test_utils.rs |
Adds test-only helper types/constants for constant-time equality and padding/packed layout tests. |
src/serde.rs |
Removes CBOR tests; keeps JSON test coverage for serde support. |
src/secure_utils.rs |
Adds SAFETY comments around mlock/munlock calls. |
src/secure_types/vec.rs |
Adds ConstantTimeEq + conditional PartialEq/Eq for SecureVec; updates tests and removes pre annotations. |
src/secure_types/string.rs |
Replaces pre contracts with inline SAFETY comments; restores Eq impl explicitly. |
src/secure_types/boxed.rs |
Adds ConstantTimeEq + conditional PartialEq/Eq; refactors internal accessors and expands tests for padding/packed structs. |
src/secure_types/array.rs |
Adds ConstantTimeEq + conditional PartialEq/Eq; updates multibyte-related test. |
src/lib.rs |
Adds test-only module export and re-exports subtle::ConstantTimeEq. |
Cargo.toml |
Adds subtle, loosens dependency versions, and removes pre/serde_cbor dev-deps. |
Cargo.lock |
Updates lockfile to reflect dependency changes. |
.woodpecker.yml |
Removes Woodpecker CI configuration. |
.vscode/settings.json |
Removes editor-specific feature configuration. |
.github/workflows/ci.yml |
Minor formatting/cleanup and keeps GitHub Actions CI as the primary pipeline. |
Comments suppressed due to low confidence (2)
src/secure_types/vec.rs:257
- The SAFETY comment says an all-zero bit pattern is valid for
u32, but this test is operating onSecureVec<char>. Please update the comment to match the actual element type / invariant being relied on (that zeroed bytes correspond to validcharvalues, i.e.,\0).
// `zero_out` sets the `len` to 0, here we reset it to check that the bytes were zeroed
// SAFETY: capacity is still >= 8 (zero_out does not shrink), and all bytes were
// zeroed, which is a valid bit pattern for `u32`.
unsafe {
src/secure_types/vec.rs:105
- These trait impls mean
SecureVec<T>only supports==/!=whenT: subtle::ConstantTimeEq(and the earlier derivedHash/Ordimpls were removed). That’s a public API/semver change for genericSecureVec<T>users (e.g., types likecharwill no longer be comparable, andSecureVec<T>can’t be used as a map/set key). If this is intended, it would help to document it prominently (and/or consider providing specialized impls for common byte/key types so typical uses don’t need wrapper newtypes).
impl<T: Copy + Zeroize + ConstantTimeEq> PartialEq for SecureVec<T> {
fn eq(&self, other: &Self) -> bool {
self.ct_eq(other).into()
}
}
impl<T: Copy + Zeroize + ConstantTimeEq> Eq for SecureVec<T> {}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
m4tx
left a comment
There was a problem hiding this comment.
In addition to the comments, I think we should also adopt more config from our other repos, such as the CI pipeline, pre-commit config, rustfmt, etc.
Otherwise, it looks like a step in a good direction.
| reason = "they were initialized to `0` by the call to `zero_out`" | ||
| ) | ||
| )] | ||
| // SAFETY: capacity is still >= 5 (zero_out does not shrink), and all 5 bytes were |
There was a problem hiding this comment.
A safety comment inside a unit test is arguably not very much needed, but it's fine to keep it, too.
Another approach to #1