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

Storing Plaintext in Config Files is Insecure #267

Open
Standing-Man opened this issue Nov 27, 2024 · 7 comments · May be fixed by #275
Open

Storing Plaintext in Config Files is Insecure #267

Standing-Man opened this issue Nov 27, 2024 · 7 comments · May be fixed by #275

Comments

@Standing-Man
Copy link
Contributor

Standing-Man commented Nov 27, 2024

image
We might consider using secure method to store passwords in the config file, and it would be helpful to include instructions on how to use --config with the login command in the README and CLI manual.

Next, could we enable logging in through the config file instead of manually entering credentials each time?

@qcserestipy
Copy link
Contributor

@Standing-Man I looked at skipping the login view in case credentials already exist in the config file. In the PR I started working on the login view is skipped in case credentials are present.

I think the number of times a user might invoke login after creating a config file is limited. However, the additional login view might be annoying this is true.

Concerning storing the password in the config file, do you think obfuscation to base64 would be enough or would be plans for storing an encrypted pwd more suitable?

@Standing-Man
Copy link
Contributor Author

Standing-Man commented Dec 1, 2024

Hi @qcserestipy, Thank you very much for your contributions. Since we are considering encrypting the password in plaintext and then decrypting it during login, we have made a trade-off between security and convenience. Additionally, it would be great if you could add some examples of how to use the config flag in the README. At the same time, this is just a suggestion with some gaps on my part. If you have a better implementation in mind, feel free to bring your ideas to life!

@qcserestipy
Copy link
Contributor

Hi @Standing-Man, I added some info to the readme and also to the docs, feel free to have a look at it. Converning the encryption I totally agree with you. Does it make sense to discuss for a while what mechanism would be desirable? I think there are different solutions with different level of complexity and finding out which is good and which might be overkill is maybe good. Shall we start such kind of a discussion somewhere?

@Standing-Man
Copy link
Contributor Author

The command documentation in the "doc" folder could potentially be automatically generated using Dagger.

@Vad1mo
Copy link
Member

Vad1mo commented Dec 2, 2024

maybe there are some option to use TPM?

Last but not least, just using kyrings might be, exactly. what we need.

@qcserestipy
Copy link
Contributor

@Vad1mo Thank you for your comments on that! I was playing around with the zalando/go-keyring plugin and it seems quite easy to use. In the latest commit to the PR i added an encryption.go that uses this library. In the login password are encrypted and stored in the file, whereas on client creation the password is decrypted for the harbor api client. The config file then looks similar to this:

Screenshot 2024-12-02 at 14 04 29

I am on a darwin arm64 mac, in case people use another os, please feel free to test this code.

@qcserestipy
Copy link
Contributor

@Vad1mo, @Standing-Man I removed the WIP tag from the PR and would be happy about any of your feedback.
I added a mockKeyRing provider, an interface for the zalando keyring, a test for the encryption functionality and adapted the present tests to work with the mockKeyRing in the dagger container.

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 a pull request may close this issue.

3 participants