feat(cli): Initial secrets implementation

[PhracturedBlue]

This is an implementation of a secrets framework to allow various ways to specify and store secrets. It is eventually meant to implement #3312 when complete.

The implementation creates a Secret object which stores any secret. it can be prefixed with various options to allow reading the secret from an env variable, cmdline, vault, etc. Or the secret can be passed in directly. If a secret is provided directly on the cmdline, it will be encrypted using a signing key and will be stored in the config file. If the secret is provided indirectly (cmdline, vault, env-variable) it is not stored.

The signing key is a random 32-byte string, and is stored in the config file after encrypting it with a key derived from the repo password. This makes it easy to change the repo password without needing to update any stored secrets. Currently the code is using fernet for the symmetric-key encryption because it was easy to get working, but likely this should reuse the same algorithms used for file encryption. The code has been updated to share the Aes encryption code between the repo and the secrets.

The signing key is currently stored in a context, as I needed it to be globally available with the initial implementation. However, I think I have mostly eliminated that need, so I am going to try to eliminate the Context stuff.
I have eliminated the need for the context, though there is still some code reformatting needed after the fact.

I am using reflection to walk the local-config struct to find Secrets. This provides a clean interface to the rest of the code in that variables do not need to be individually decoded after the password is available, but I'm not sure it is the ideal solution.

I am posting to see if I can get any feedback on whether this is a reasonable solution, or if something else is desired.

At the moment, only sftp is supported.

Usage:

• --sftp-password --> password is stored encrypted in config file
• --sftp-password envvar:ENVIRONMENT_VARIABLE --> password will be read from $ENVIRONMENT_VARIABLE
• --sftp-password command:"command to execute" --> "command to execute" will be executed and password will be read from stdout
• --sftp-password file: --> password will be read from the contents of

This implementation does mean that there are some illegal password strings (anything starting with one of the keywords followed by a colon). There may be alternatives if this is undesirable.

To Do:

• Remove context manager
• Fix existing tests
• Implement other repo types
  • Implement Azure
  • Implement B2
  • Implement GCS
  • Implement GDrive
  • Implement Rclone
  • Implement S3
  • Implement Webdav
• Implement upgrading from current password-store
• Implement keyring
• Write tests
• Improve code organization now that the manager is gone
• Use a secret for the --password and --server-password cmdline argument
• Figure out if we should remove the gnome-keyring options in favor of the Secret equivalents
• Handle password change
• Share the Key management between the repo and the secrets?

[Scrumplex]

This implementation does mean that there are some illegal password strings (anything starting with one of the keywords followed by a colon). There may be alternatives if this is undesirable.

What if you also add a text: or plaintext: prefix too?

[PhracturedBlue]

What if you also add a text: or plaintext: prefix too?

That is an option. It changes the behavior of kopia, but it is not likely that users are crating/connecting new repos every day, so it probably wouldn't break user's workflows. The one case I am concerned with is whether this can/should be extended for --password While clearly we wouldn't want to store password in the config file, using a secret argument would allow using all the same methods of retrieval which could be nice. If we want to do that, than I don't think I want to change the semantics since that would be a breaking change.

[Scrumplex]

Oh I should clarify that text: / plaintext: should be in addition to just specifying a plaintext string

Examples:

• password -> password
• envvar:SECRET_TOKEN -> value of $SECRET_TOKEN
• file:/run/secrets/token -> contents of /run/secrets/token
• plaintext:file:123 -> file:123

[PhracturedBlue]

Oh I should clarify that text: / plaintext: should be in addition to just specifying a plaintext string

Yeah, I realized that is what you meant after I thought about it some more. I implemented exactly that, and converted --password and --server-password to use secrets to ensure it worked as intended

[jkowalski]

this is looking great already, may I suggest separating non-functional changes (code movement and renames) into separate PR that can be trivially merged so that this one becomes smaller and easier to review?

[PhracturedBlue]

@jkowalski I implemented the changes based on your code review. I will start cleaning up the broken tests and implementing tests for the new functionality next.

[PhracturedBlue]

One thing I noticed is that because we call DeriveKeyFromPassword twice (once for secrets, and once for the repo) and this function is designed to be slow, it adds several seconds to kopia startup time. Currently the salt is different for both cases, so the result is different, and they can't be shared.
I don't have access to the repo when decrypting secrets, so if we wanted to share the salt, we'd need to store it it the local config, which I don't think is ideal.. Not sure how to proceed.

Edit: I guess one option is to default to a less-expensive key-derivation algorithm for secrets. It would still be better than the plain-text we have now

[PhracturedBlue]

I believe I have all functionality implemented now, and covered all storage options. Still need feedback on if the performance degradation is ok, and if not how to remedy it.

All tests pass on my machine, but I don't have the ability to run the other architectures, so need to wait for CI runs

[codecov]

Codecov Report

Attention: 144 lines in your changes are missing coverage. Please review.

Comparison is base (b018116) 75.82% compared to head (97299c1) 75.81%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3345      +/-   ##
==========================================
- Coverage   75.82%   75.81%   -0.01%     
==========================================
  Files         463      468       +5     
  Lines       37100    37587     +487     
==========================================
+ Hits        28132    28498     +366     
- Misses       7036     7122      +86     
- Partials     1932     1967      +35     

Files	Coverage Δ
cli/app.go	76.94% <100.00%> (ø)
cli/command_repository_connect.go	96.51% <100.00%> (+0.12%)	⬆️
cli/command_repository_status.go	79.90% <100.00%> (+0.38%)	⬆️
cli/command_server.go	93.33% <100.00%> (+0.31%)	⬆️
cli/command_server_start.go	88.75% <100.00%> (+0.09%)	⬆️
cli/custom_types.go	100.00% <100.00%> (ø)
cli/password.go	20.27% <100.00%> (+2.21%)	⬆️
cli/storage_azure.go	70.58% <100.00%> (ø)
cli/storage_sftp.go	84.00% <100.00%> (ø)
repo/blob/sftp/sftp_options.go	28.57% <ø> (ø)
... and 16 more

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jkowalski]

this is losing the Required() aspect of some flag which will fail at startup if a value is not provided.

[PhracturedBlue]

Yes, I added the check below instead. I don't see a good way to support the required field along with using a custom flag. I can try to rethink how the env variable case is handled, as this is the primary reason required doesn't work.

Edit: looks like i did for the token but missed the access key.

[jkowalski]

I'm not sure I understand the need to have yet another encryption scheme for embedding encryopted literals in a config file when we have external secret storage systems for fetching those dynamically.

I was hoping we can simplify the solution by separating 2 concerns - one being access to all these runtime providers of secrets and the other - the problem of storing localconfig unencrypted.

1. I was hoping secrets.Secret could simply be a subtype of string (not encrypted at all), sometimes prefixed with a source:

Wen can then have something like:

type Provider interface {
  Fetch(ctx context.Context, param string) (string, error)
}

var Providers = map[string]Provider{}

func (s Secret) Evaluate(ctx context.Context) (string, error) {
   for prefix, impl := range Providers {
     if strings.HasPrefix(string(s), prefix + ":") {
       return impl.Fetch(ctx, s[len(prefix)+1:])
    }
  }

   return string(s), nil
}

This would check if the string starts with any of supported providers and invioke the appropriate provider to get the value, or return the literal value if no prefix matches.

2. The above would be dramatically simpler at the expense of sometimes having raw secrets in the config file, but this could be handled in a completely orthogonal way, namely by encrypting the entire LocalConfig prior to storing it. For encrypting local config we can derive the whole-file encryption key from password using some random salt and encryption parameters that will be embedded in JSON in addition to ciphertext. This would be similar to KopiaRepositoryJSON.EncryptedFormatBytes.

[PhracturedBlue]

That's the kind of feedback that I'd have preferred before doing a full implementation. I don't have the time to rework this completly right now. I'll just leave it until I have time to come back to it.

[jkowalski]

That's the kind of feedback that I'd have preferred before doing a full implementation. I don't have the time to rework this completly right now. I'll just leave it until I have time to come back to it.

Yeah, sorry about that. I've been traveling the whole last week and did not have a chance to look into that properly. I think 85% of this PR is still exactly what we need, we just need to simplify the secrets package and perhaps remove a few things.