Use secure_string for more secure Password and SecretKey #328

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

All modified and coverable lines are covered by tests ✅

Flag	Coverage Δ
rust	`88.22% <100.00%> (+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	`85.60% <100.00%> (+1.67%)`	⬆️
cot/src/config.rs	`91.66% <100.00%> (+0.31%)`	⬆️

🚀 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.

cot/src/common_types.rs

cot/src/config.rs

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!

cot/src/common_types.rs

seqre

LGTM

Kijewski · 2025-06-02T02:31:04Z

cot/src/common_types.rs

+#[cfg(miri)]
+impl Debug for Password {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "Password(***SECRET***)")


Using non-rustic outputs like trailing * can make things more complicated for downstream users. I would rather use write!(f, "Password(_)"), write!(f, "Password(..)"), or (my preference, because that's what you are supposed to used, if I'm not mistaken) f.debug_struct("Password").finish_non_exhaustive().

Thank you for pointing that out, TIL about it! @eibrahim95 could you change it, please?

I made it like this so it is the same as secure-string, if you notice this one is for miri, the other implementation derives Debug, and depends on SecureString, which implements Debug similarly to the above If that is important, I could change it for both, miri and not(miri)

Kijewski · 2025-06-02T02:35:11Z

Cargo.toml

 mockall = "0.13"
 multer = "3"
 password-auth = { version = "1", default-features = false }
+secure-string = "0.3.0"


secure-string looks kinda dead. Why not use zeroize directly?

zeroize is great at what it does, but it doesn't do enough. secure-string, apart from zeroing the memory, provides constant time equality (although as we looked before, it might just be claim not backed by code, I'll have to double check it) and protections against leaking into swap and core dumps. I believe that PR is good enough to merge it as is and we can always do some additional research and improve that part of the code later on, what do you think?

provides constant time equality

@seqre this is untrue, as you (and I) pointed out. The PR uses subtle for this functionality.

I somewhat agree with @Kijewski that secure-string is not very well maintained. zeroize provides a good portion of the functionality, but mlocking the memory would be nice. We could implement this ourselves (which shouldn't be very difficult, looking at the code), so perhaps it would be good to get this right from the beginning (or just ignore the mlocking part for now). WDYT?

I see a few solutions:

As I mentioned above, we can merge it as is and improve in the future.

We can fork secure-string, update it, use that version, and try to upstream the improvements - it ain't perfect, but something like this: ISibboI/secure-string@main...seqre-contrib:secure-string:main

Its codebase is small, and it's licensed with UNLICENSE, so we can just pull that code into Cot.

The 2nd might be the best, we get what we want, and we might even improve the ecosystem by upstreaming improvements. This way, we can fairly quickly get what we want, and the only change required here is to change the source of the dependency. Would you agree @m4tx?

@seqre ah fair, I guess we could use a fork. Do you mind transferring it to the cot-rs organization, so that it's clear for people that we definitely have the will to maintain it? Similarly to https://github.com/cot-rs/openapi-guis

If we want to use the fork, we'll probably want to publish it on crates.io - at this point I don't think we should use any git dependencies. Of course, ideally, as you say, we should upstream the changes, but if we don't get a response for say, a week or two, we should probably stick to our fork.

The repository has been transferred: https://github.com/cot-rs/secure-string

However, I think that using a git dependency, especially for a repo in our control, is fine for now, @m4tx. If we can't upstream the changes promptly, then sure, we can publish it to crates.io. Similarly to what we did before with other crates that we had to fix ourselves.

Issues were disabled on the forked repo, so I have copy comment here:

<SecureVec<T> as PartialEq>::eq is not sane

I don't think the change in cot-rs/secure-string@004f3da is sane. For a T which contains any internal or trailing padding, e.g. #[repr(C)] struct T { x: u8, y: u16 }, the bytes in the padding cannot be relied on, and should not be read. Also, generic Ts might not have the same order as its transmuted [u8] slice.

The simplest solution would be to remove the generic SecureVec<T>, and invariantly set T = u8. Then you could concentrate on secure strings, without having to care about any and all types there could be.

However, I think that using a git dependency, especially for a repo in our control, is fine for now, @m4tx.

@seqre we can't release the crate to crates.io with a git dependency. When published, the git dependency will be replaced with a reference to a crates.io-published version, so it just won't work.

@Kijewski yeah, I agree that the implementation is not correct, thanks for vigilance! I think we can just use [T]::ct_eq (it's a Vec<T> underneath anyway) and remove the overcomplicated implementation and the unsafe code. Setting T = u8 would also work. @seqre is there a reason you opted for this solution?

I also re-enabled Issues on the repo.

It was the quickest thing I could do to show a proof of concept in a few minutes; it wasn't intended to be the final solution. If we want to go forward with that fork, then we should spend more time on it.

Ah sure, that makes sense!

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

cot/src/common_types.rs Outdated Show resolved Hide resolved

cot/src/config.rs Outdated Show resolved Hide resolved

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

cot/src/common_types.rs Outdated Show resolved Hide resolved

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

Uh oh!

Use secure_string for more secure Password and SecretKey #328

Are you sure you want to change the base?

Use secure_string for more secure Password and SecretKey #328

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m4tx Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seqre Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m4tx Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

<SecureVec<T> as PartialEq>::eq is not sane

Uh oh!

m4tx Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented May 15, 2025 •

edited

Loading

m4tx Jun 2, 2025 •

edited

Loading

seqre Jun 2, 2025 •

edited

Loading

m4tx Jun 2, 2025 •

edited

Loading

`<SecureVec<T> as PartialEq>::eq` is not sane

m4tx Jun 4, 2025 •

edited

Loading