feat: wrap secrets in custom types to prevent them from leaking #925

AlphaKeks · 2023-05-19T14:09:24Z

Description of change

This aims to close #890. I looked into secrecy and tried to integrate it into the code base and found some odd trait bound issues that wouldn't allow using them in structs that implement Serialize or Deserialize. The crate provides a serde feature that should enable exactly this, but this still would lead to weird issues of standard library types not implementing traits from the secrecy crate. This file seems to be the issue. They require the wrapped type to implement SerializableSecret which they never implement for standard library types anywhere?

Anyway, instead I decided to replicate the functionality of the crate myself by creating a wrapper type that makes sure to not expose the inner value unless by calling the expose function explicitly. The underlying data will also be zeroed out in memory when the type is dropped to ensure it won't leak afterwards.

I have looked through the code base searching for places where this type could be useful and made some changes. If you know other places where secrets are being used that I missed, please let me know.

iulianbarbu

What have you thought about common/src/lib.rs::DatabaseReadyInfo (mainly its password field)? I think we usually need it when db resource information is requested, but we should defend against logging it accidentally.

common/src/secrets.rs

AlphaKeks · 2023-05-24T14:29:38Z

I wrapped the password in Secret, must've overlooked that the first time, thanks.
I am unsure what address_private field is, should we wrap that in Secret too?

iulianbarbu · 2023-05-25T05:28:25Z

I am unsure what address_private field is, should we wrap that in Secret too?

I don't think so. I think it's the address that can be used internally by the backend. It might save some external network calls when compared to accessing the DB through the public URL.

oddgrd

Hey @AlphaKeks, thanks for this! I left a couple of small comments, and clippy also has a suggestion in the CI failure

oddgrd · 2023-06-01T12:05:51Z

common/src/secrets.rs

+
+    #[test]
+    fn secret_struct() {
+        #[allow(dead_code)]


Nit: is this needed?

Initially I didn't have tests for it as I figured it's unnecessary (since everything we could test is basically already enforced by the compiler), but there was a point about having them to prevent regressions in the future. I wasn't entirely sure what to do with that, so if you think we should remove them, I will remove them again.

Ah, sorry! I just meant the allow(dead_code) annotation.

Ah, yeah. The compiler was complaining about the struct being unused.

common/src/lib.rs

AlphaKeks · 2023-06-08T13:02:24Z

A user in our discord brought up that using SharedDB will print out the database connection string when running or deploying a project, which in my opinion shouldn't be the default. What do you guys think about adding a CLI flag to both run and deploy to only show secrets like connections strings on demand?

iulianbarbu · 2023-06-09T06:56:03Z

A user in our discord brought up that using SharedDB will print out the database connection string when running or deploying a project, which in my opinion shouldn't be the default. What do you guys think about adding a CLI flag to both run and deploy to only show secrets like connections strings on demand?

I would include the secrets exposing flagging for the DB connections as well. At the same time, maybe a global flag will do better? It's gonna be accounted for both secrets and db connection strings. What do you think?

AlphaKeks · 2023-06-09T13:43:23Z

maybe a global flag will do better?

Are you talking about a flag for cargo-shuttle itself (not subcommands)? I think since this will only apply to 3 of the 15 or so subcommands, I think it wouldn't be a good fit for a global flag.

I will start implementing the functionality for run and deploy and if you have any objections, I can make it a global flag afterwards.

iulianbarbu · 2023-06-20T09:53:09Z

resources/static-folder/Cargo.toml

@@ -11,6 +11,7 @@ async-trait = "0.1.56"
 dunce = "1.0.3"
 fs_extra = "1.3.0"
 serde = { version = "1.0.148", features = ["derive"] }
+shuttle-common = { path = "../../common", version = "0.18.0", default-features = false }


Why do we need this as a static-folder dep?

Tests were failing because of a type mismatch on this function.

iulianbarbu · 2023-06-28T09:25:47Z

cargo-shuttle/src/args.rs

+    Secrets {
+        #[arg(long)]
+        /// Reveal the values of the secrets
+        show: bool,
+    },


We considered lately that we would prefer not to show secrets at all. It's best to remain hidden after deploying, having the user deploy again if they want to change them. The idea is simple: if the user wants to display the previous secrets used for the deployment, shouldn't use Shuttle for it.

Alright, I will remove the flags.

jonaro00 · 2023-08-17T16:03:38Z

@AlphaKeks Do you want more feedback or help with finishing this? I can take a look if needed.

AlphaKeks · 2023-08-17T18:33:17Z

Oh yeah, kinda forgot about this, my bad! Will take a look tomorrow and get this up to date, then ask for review. I'm pretty sure this was basically finished.

iulianbarbu · 2023-08-18T06:07:19Z

Oh yeah, kinda forgot about this, my bad! Will take a look tomorrow and get this up to date, then ask for review. I'm pretty sure this was basically finished.

We've been discussing polishing this a bit before merging but it is almost there. Please take a look at my last review.

Kazy · 2023-10-19T14:09:24Z

Hey @AlphaKeks, I hope you're doing alright :)

Sorry for the delay and back and forth with the review, we're still interested in getting this merged, if you're also still interested in working on it and getting this through. After discussing it with the team, there are still a few changes we would like to make:

Don't hide the connection string, only the password in it. It could be replaced by ******* (a fixed number).
Add a --show-secrets option but only to the resource list command, that would reveal the password in the connection string.
Rebase on top of main.

If you don't want to or don't have the time to continue working on it that's perfectly fine, just let us know and we'll take over. Thank you very much !

AlphaKeks · 2023-10-21T10:20:08Z

Not really interested anymore, sorry.

Kazy · 2023-10-23T07:00:15Z

No worries, thank you for your work !

iulianbarbu

LGTM, just that we'll need to investigate why tests are failing.

common/src/lib.rs

iulianbarbu

Looks like we can not remove the .collect() call for the into_iter() that easily. LGTM now, just that we need to fix the conflicts.

Related to shuttle-hq#890. This adds a custom wrapper type for secrets, which will be used to prevent them from leaking anywhere. To ensure that API Keys in the CLI never get leaked we wrap them in the new `Secret` type.

…hange

AlphaKeks added the Shuttle Batch label May 19, 2023

AlphaKeks self-assigned this May 19, 2023

iulianbarbu added the M label May 19, 2023

iulianbarbu reviewed May 23, 2023

View reviewed changes

common/src/secrets.rs Outdated Show resolved Hide resolved

oddgrd reviewed Jun 1, 2023

View reviewed changes

iulianbarbu reviewed Jul 6, 2023

View reviewed changes

jonaro00 removed E-M labels Aug 17, 2023

AlphaKeks force-pushed the feat/secrets branch from 9d83696 to c4138a7 Compare August 18, 2023 12:30

jonaro00 requested a review from iulianbarbu September 14, 2023 18:09

Kazy force-pushed the feat/secrets branch from c4138a7 to c3f8742 Compare October 23, 2023 08:52

Kazy assigned Kazy and unassigned AlphaKeks Oct 23, 2023

Kazy force-pushed the feat/secrets branch 4 times, most recently from 5e66e58 to afba827 Compare October 23, 2023 09:38

iulianbarbu reviewed Oct 23, 2023

View reviewed changes

common/src/lib.rs Outdated Show resolved Hide resolved

jonaro00 changed the title ~~Feature: wrap secrets in custom types to prevent them from leaking~~ feat: wrap secrets in custom types to prevent them from leaking Oct 23, 2023

Kazy force-pushed the feat/secrets branch 6 times, most recently from c26649f to 8ca577e Compare October 23, 2023 14:23

iulianbarbu approved these changes Oct 24, 2023

View reviewed changes

AlphaKeks and others added 5 commits October 24, 2023 10:45

feat: add --show-secrets for resources and only show secrets when given

2a8192b

fix(secret-store): use Secret instead of String

58d3fac

fix(secret-store): into_iter should output String, avoid a breaking c…

6b40c44

…hange

fix(cargo-shuttle): issues with rebase

9fe9826

Kazy force-pushed the feat/secrets branch from 8ca577e to 8d2ed5c Compare October 24, 2023 12:05

Kazy requested a review from iulianbarbu October 24, 2023 12:05

feat(cargo): add hint for secrets

452d20e

Kazy force-pushed the feat/secrets branch from 8d2ed5c to 452d20e Compare October 24, 2023 12:10

Merge branch 'main' into feat/secrets

325aa97

iulianbarbu approved these changes Oct 24, 2023

View reviewed changes

Merge branch 'main' into feat/secrets

86ceb9d

iulianbarbu merged commit bf6161c into shuttle-hq:main Oct 25, 2023
34 checks passed

Kazy deleted the feat/secrets branch October 25, 2023 09:14

chesedo mentioned this pull request Oct 27, 2023

bug: compile fails with secrets in resource configs #1353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: wrap secrets in custom types to prevent them from leaking #925

feat: wrap secrets in custom types to prevent them from leaking #925

AlphaKeks commented May 19, 2023

iulianbarbu left a comment •

edited

Loading

AlphaKeks commented May 24, 2023

iulianbarbu commented May 25, 2023

oddgrd left a comment

oddgrd Jun 1, 2023

AlphaKeks Jun 1, 2023

oddgrd Jun 1, 2023

AlphaKeks Jun 2, 2023

AlphaKeks commented Jun 8, 2023

iulianbarbu commented Jun 9, 2023 •

edited

Loading

AlphaKeks commented Jun 9, 2023

iulianbarbu Jun 20, 2023

AlphaKeks Aug 18, 2023

iulianbarbu Jun 28, 2023 •

edited

Loading

AlphaKeks Aug 18, 2023

jonaro00 commented Aug 17, 2023

AlphaKeks commented Aug 17, 2023

iulianbarbu commented Aug 18, 2023

Kazy commented Oct 19, 2023

AlphaKeks commented Oct 21, 2023

Kazy commented Oct 23, 2023

iulianbarbu left a comment

iulianbarbu left a comment

feat: wrap secrets in custom types to prevent them from leaking #925

feat: wrap secrets in custom types to prevent them from leaking #925

Conversation

AlphaKeks commented May 19, 2023

Description of change

iulianbarbu left a comment • edited Loading

Choose a reason for hiding this comment

AlphaKeks commented May 24, 2023

iulianbarbu commented May 25, 2023

oddgrd left a comment

Choose a reason for hiding this comment

oddgrd Jun 1, 2023

Choose a reason for hiding this comment

AlphaKeks Jun 1, 2023

Choose a reason for hiding this comment

oddgrd Jun 1, 2023

Choose a reason for hiding this comment

AlphaKeks Jun 2, 2023

Choose a reason for hiding this comment

AlphaKeks commented Jun 8, 2023

iulianbarbu commented Jun 9, 2023 • edited Loading

AlphaKeks commented Jun 9, 2023

iulianbarbu Jun 20, 2023

Choose a reason for hiding this comment

AlphaKeks Aug 18, 2023

Choose a reason for hiding this comment

iulianbarbu Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

AlphaKeks Aug 18, 2023

Choose a reason for hiding this comment

jonaro00 commented Aug 17, 2023

AlphaKeks commented Aug 17, 2023

iulianbarbu commented Aug 18, 2023

Kazy commented Oct 19, 2023

AlphaKeks commented Oct 21, 2023

Kazy commented Oct 23, 2023

iulianbarbu left a comment

Choose a reason for hiding this comment

iulianbarbu left a comment

Choose a reason for hiding this comment

iulianbarbu left a comment •

edited

Loading

iulianbarbu commented Jun 9, 2023 •

edited

Loading

iulianbarbu Jun 28, 2023 •

edited

Loading