Use secure_string for more secure Password and SecretKey by eibrahim95 · Pull Request #328 · cot-rs/cot

eibrahim95 · 2025-05-15T19:59:55Z

This implements #47

Updated cot::common_types::Password struct to use SecureString for enhanced security.
Implemented PartialEq and Eq for cot::common_types::Password to be able to compare 2 Password instances
Added tests for equality comparison of instances.
Updated SecretKey struct to use SecureBytes for improved security handling.
Adjusted implementations of PartialEq and Debug to align with SecureBytes.
Implemented serialization for SecretKey since we cannot derive Serialize because SecureBytes doesn't implement it .
Included tests for JSON serialization of SecretKey.

- Updated struct to use SecureString for enhanced security. - Implemented PartialEq and Eq to be able to compare 2 Password instances - Added tests for equality comparison of instances.

…curity - Updated SecretKey struct to use SecureBytes for improved security handling. - Adjusted implementations of PartialEq and Debug to align with SecureBytes. - Implemented serialization for SecretKey since we cannot derive Serialize since SecureBytes doesn't implement it . - Included tests for JSON serialization of SecretKey.

codecov · 2025-05-15T20:22:36Z

Codecov Report

❌ Patch coverage is 94.44444% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cot/src/config.rs	89.47%	2 Missing ⚠️

Flag	Coverage Δ
rust	`90.25% <94.44%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cot/src/common_types.rs	`84.67% <100.00%> (+0.58%)`	⬆️
cot/src/config.rs	`94.58% <89.47%> (+0.05%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

eibrahim95 · 2025-05-16T18:03:17Z

Looks like miri doesn't support mlock, thinking about mocking it for miri only what do you think @m4tx @seqre ?

seqre

@m4tx can you look at my comments, I'd like a second pair of eyes.

eibrahim95 · 2025-05-19T10:46:13Z

@seqre What do you think about the miri thing, because that is a blocker for this PR?

m4tx · 2025-05-27T13:36:56Z

Looks like miri doesn't support mlock, thinking about mocking it for miri only what do you think @m4tx @seqre ?

Hey, thanks for your patience! Can you share more details on mocking a syscall for Miri? I didn't know it was possible.

Otherwise, I think the only option is to ignore all the failing tests. Which would be kind of a sad thing to do, given how many there are.

- Refactored Password and SecretKey structs to provide Miri-compatible versions, using String and Box<[u8]> respectively. - Adjusted methods for creating and accessing passwords and secret keys to ensure compatibility with Miri's limitations.

eibrahim95 · 2025-05-27T20:54:26Z

Hey, thanks for your patience! Can you share more details on mocking a syscall for Miri? I didn't know it was possible.

I realize I didn't fully think through the mock idea - it's beyond my current skill level anyway.

Otherwise, I think the only option is to ignore all the failing tests. Which would be kind of a sad thing to do, given how many there are.

I kept a normal implementation for miri, all should be good now, please have a look.

seqre · 2025-05-31T09:52:13Z

I'm not sure if creating a special implementation just for Miri is the best approach. It forces us to maintain twice as much code. However, getting rid of it would force us to ignore a lot of only partially related tests from Miri testing, which is also not a good end result. I think that for now, we can leave that implementation be, and we can always reassess the situation in the future if it becomes a hassle. Would you agree @m4tx?

@eibrahim95, on a side note, please add Unlicense to cargo-deny's allowed list of licenses to make the CI check pass

seqre

One last request from me and it should be good in my eyes!

seqre

LGTM

eibrahim95 · 2026-03-15T19:04:10Z

Hey Guys, I'm seeing some activity here, although i totally forgot what we we were doing here :D but let me know if i can help.

Thanks

seqre · 2026-03-20T21:19:50Z

Hey @eibrahim95, it's nice to hear from you again! We're going back to all old PRs and trying to get them working and merge them. So far, I've only updated this PR to the newest changes on the master branch. In the background, I'm working on updating secure_string for our use case. Once that's done, we can use it to finish this PR and merge it!

# Conflicts: # cot/src/common_types.rs

seqre · 2026-05-22T18:06:44Z

Hey @eibrahim95, we finished working on https://github.com/cot-rs/securer-string, so I took the liberty of updating the PR's code to use it.

@eibrahim95, @m4tx, and @Kijewski I'd appreciate if you guys could take a peek and see if it's in good shape now!

Kijewski · 2026-05-22T18:39:30Z

That's nitpicking from me, feel free to ignore. :)

In https://github.com/cot-rs/securer-string/blob/1995c9dc687bd6fc24da8a1e0ba4ae5e51381e75/src/serde.rs#L38, I would not print the underlying error, because some deserializing library might print the password string or parts of it. It could be something simple as "string contains an unescaped apostrophe", which would weaken the user's security at least a tiny bit. Same with the other instances of {error}.

In https://github.com/cot-rs/securer-string/blob/1995c9dc687bd6fc24da8a1e0ba4ae5e51381e75/src/secure_utils.rs#L6, no error is reported to the caller, which is mostly okay I guess, but if the mlock failed, the munlock call must be skipped.

This does not look right to me: https://github.com/cot-rs/securer-string/blob/1995c9dc687bd6fc24da8a1e0ba4ae5e51381e75/src/secure_types/array.rs#L34-L35. The compiler is free to move content after the mlock call happened. Maybe drop the SecureArray type entirely?

The #[must_use] annotation is missing in https://github.com/cot-rs/securer-string/blob/1995c9dc687bd6fc24da8a1e0ba4ae5e51381e75/src/secure_types/string.rs#L22. The other methods are annotated.

If the password length is less than page sized (which is somewhat ensured, as macOS uses 16k pages if I'm not mistaken), and two passwords get allocated in the same page, then dropping one password will munlock the other password. Fixing this problem would be difficult, I think. Either allocate pages manually for every password or have some sort of arena that stores all passwords. I hate the latter option, though, because that arena would be treasure chest for any evil-doer .

seqre · 2026-05-25T17:35:33Z

Thanks @Kijewski, I appreciate your review a lot! I've made a PR with the first 4 fixes and left the last point as an issue in the repo to handle at some other time as it's a bit harder to tackle right away.

m4tx

A couple minor things still, but with the securer-string almost in place, we should be able to merge this very soon!

m4tx · 2026-06-15T12:44:45Z

+    }
+}
+
+impl Debug for Password {


Do we still need/want this if SecureString hides the content in the debug impl anyway? Same for other types (such as SecretKey).

I'd say yes, because then instead of Password(..) you'll get Password(SecureString { .. }) if you just do plain #[derive(Debug)].

m4tx · 2026-06-15T12:58:30Z

-///    where the security tradeoff is understood, e.g., when you're creating a
-///    user registration form with the "retype your password" field, where both
-///    passwords come from the same source anyway.
+/// 2. An alternative is to compare 2 instances of the [`Password`] type


Now this sounds like option 2 and 1 are essentially the same. It feels like we should explain in more detail the differences between the two - e.g. the first calculates hashes of passwords and compares that, making it computationally expensive and very difficult to make any timing attacks, vs. just comparing the passwords with constant-time comparison which can still leak the length of the password. I don't know how to put this in words in a short way.

I've updated it, is it okay now?

eibrahim95 added 2 commits May 15, 2025 21:42

feat: add secure-string dependency and integrate into Password struct

2cdfd65

- Updated struct to use SecureString for enhanced security. - Implemented PartialEq and Eq to be able to compare 2 Password instances - Added tests for equality comparison of instances.

github-actions Bot added the C-lib Crate: cot (main library crate) label May 15, 2025

Merge branch 'master' into secure-string

e55a961

seqre reviewed May 19, 2025

View reviewed changes

Comment thread cot/src/common_types.rs Outdated

Comment thread cot/src/config.rs Outdated

eibrahim95 added 3 commits May 27, 2025 19:50

Merge branch 'master' into secure-string

0d29a72

Fix inconsistency in docs about the implementation of PartialEq

e255fd1

Add Unlicense to cargo-deny's allowed list of licenses

c2d06c0

eibrahim95 requested review from m4tx and seqre May 31, 2025 12:26

seqre requested changes Jun 1, 2025

View reviewed changes

Comment thread cot/src/common_types.rs Outdated

Use constant time comparison for Password comparison

767a1f6

eibrahim95 requested a review from seqre June 1, 2025 11:54

seqre approved these changes Jun 1, 2025

View reviewed changes

Kijewski reviewed Jun 2, 2025

View reviewed changes

Comment thread cot/src/common_types.rs Outdated

Kijewski reviewed Jun 2, 2025

View reviewed changes

Comment thread Cargo.toml Outdated

seqre added 2 commits March 9, 2026 19:58

Merge branch 'master' into secure-string

adb211e

fix merge conflicts

8f5aa2c

seqre mentioned this pull request Mar 15, 2026

Cot updates cot-rs/securer-string#1

Closed

Merge branch 'master' into secure-string

5dfdb50

seqre linked an issue Mar 23, 2026 that may be closed by this pull request

Consider using secure_string crate (or similar) for Password #47

Open

seqre added 4 commits May 22, 2026 19:30

Merge remote-tracking branch 'origin/master' into secure-string

6a9ed9d

# Conflicts: # cot/src/common_types.rs

use securer-string

477ee40

clean up password and secret key

3e055c4

update securer-string to 0.1.2

517c689

github-actions Bot added the A-deps Area: Dependencies label May 22, 2026

fix forgotten test

d2d814f

seqre mentioned this pull request May 25, 2026

Memory page size not aligned to password length cot-rs/securer-string#9

Open

Merge branch 'master' into secure-string

2303d97

m4tx requested changes Jun 15, 2026

View reviewed changes

seqre added 3 commits June 22, 2026 23:48

address pr review comments

c2e72c7

Merge branch 'master' into secure-string

de170fb

update securer-string to v0.1.3

d7cc822

seqre requested a review from m4tx June 22, 2026 22:02

cargo fmt

616385f

Uh oh!

Uh oh!

Conversation

eibrahim95 commented May 15, 2025

Uh oh!

codecov Bot commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eibrahim95 commented May 16, 2025

Uh oh!

seqre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eibrahim95 commented May 19, 2025

Uh oh!

m4tx commented May 27, 2025

Uh oh!

eibrahim95 commented May 27, 2025

Uh oh!

seqre commented May 31, 2025

Uh oh!

seqre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seqre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eibrahim95 commented Mar 15, 2026

Uh oh!

seqre commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seqre commented May 22, 2026

Uh oh!

Kijewski commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seqre commented May 25, 2026

Uh oh!

m4tx left a comment

Choose a reason for hiding this comment

Uh oh!

m4tx Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

seqre Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

m4tx Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

seqre Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented May 15, 2025 •

edited

Loading

seqre commented Mar 20, 2026 •

edited

Loading

Kijewski commented May 22, 2026 •

edited

Loading