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

feat: prevent sensitive information (key, password, secrets etc.) from being printed in plain #1501

Merged
merged 5 commits into from May 1, 2023

Conversation

DevilExileSu
Copy link
Contributor

@DevilExileSu DevilExileSu commented Apr 30, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

add secret type, SecretString and SecretBytes.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

close #1487

@DevilExileSu
Copy link
Contributor Author

DevilExileSu commented Apr 30, 2023

because Bytes implements Deref, the len() and is_empty() seem to be redundant.

impl Bytes {
pub fn len(&self) -> usize {
self.0.len()
}
pub fn is_empty(&self) -> bool {
self.0.is_empty()
}
}

impl Deref for Bytes {
type Target = [u8];
fn deref(&self) -> &[u8] {
&self.0
}
}

And we can also implement Deref for BytesString.

@codecov
Copy link

codecov bot commented Apr 30, 2023

Codecov Report

Merging #1501 (f408a57) into develop (7dbac89) will decrease coverage by 0.46%.
The diff coverage is 69.41%.

@@             Coverage Diff             @@
##           develop    #1501      +/-   ##
===========================================
- Coverage    85.89%   85.44%   -0.46%     
===========================================
  Files          550      550              
  Lines        82545    82600      +55     
===========================================
- Hits         70904    70574     -330     
- Misses       11641    12026     +385     

Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

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

I think we could use secstr to do this. @sunng87

src/servers/src/auth.rs Outdated Show resolved Hide resolved
src/common/base/src/lib.rs Outdated Show resolved Hide resolved
@DevilExileSu
Copy link
Contributor Author

I think we could use secstr to do this. @sunng87

You are right, we can use secstr or secrecy, which is more safe.

@sunng87
Copy link
Member

sunng87 commented May 1, 2023

Yes, it will be nice to use existing solutions.

@DevilExileSu DevilExileSu marked this pull request as draft May 1, 2023 08:47
@DevilExileSu
Copy link
Contributor Author

DevilExileSu commented May 1, 2023

The inner struct of serstr::SerStr is Vec<u8>, which requires additional processing when deserializing toml string, and needs to convert Vec<u8> into &str when constructing Instance, so I use the more convenient secrecy::SecretString

@DevilExileSu DevilExileSu marked this pull request as ready for review May 1, 2023 09:01
@DevilExileSu DevilExileSu marked this pull request as draft May 1, 2023 09:11
@DevilExileSu DevilExileSu marked this pull request as ready for review May 1, 2023 09:28
@evenyag
Copy link
Contributor

evenyag commented May 1, 2023

The inner struct of serstr::SerStr is Vec<u8>, which requires additional processing when deserializing toml string, and needs to convert Vec<u8> into &str when constructing Instance

secstr provides a SecUtf8. But using secrecy is fine, these two crates are similar.

Bytes implements Deref, the len() and is_empty() seem to be redundant.

Oh, right, you could open a new PR for this.

we can also implement Deref for BytesString.

I'm not sure, as BytesString might hold a non-UTF-8 string in the future. But we can implement Deref first since we only support UTF-8 strings now. Once it really needs to support non-UTF-8 encoding, we must remove the Deref impl and methods that access it as str directly.

@evenyag evenyag merged commit 6aae5b7 into GreptimeTeam:develop May 1, 2023
19 checks passed
@DevilExileSu
Copy link
Contributor Author

Thx! Got it. I'm ashamed that I didn't check the documentation for secstr carefully earlier.

@DevilExileSu DevilExileSu deleted the issue-1487 branch May 1, 2023 13:11
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
…m being printed in plain (GreptimeTeam#1501)

* feat: add secret type

* chore: replace key, password, secrets with secret type.

* chore: use secrecy

* chore: remove redundant file

* style: taplo fmt
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.

prevent sensitive information (key, password, secrets etc.) from being printed in plain
3 participants