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

Remove manual password input #1237

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JesseAbram
Copy link
Member

Closes #1220

Adds a default password path at .password.txt for ease in testing.

@JesseAbram JesseAbram marked this pull request as ready for review January 6, 2025 16:59
@JesseAbram JesseAbram requested review from HCastano and ameba23 January 6, 2025 16:59
Copy link
Collaborator

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

I would approach this a bit differently.

The behaviour here should be that by default we use PasswordMethod::NoPassword. We may
need some dummy/default password for encrypting the DB for now (but medium term this
shouldn't be needed).

If a --password-file flag is used, use it to encrypt and load the DB. However, don't
have a default password file created or lying around.

I'm kinda approaching this from this mindset where operators shouldn't have to deal with
the encryption in any way. Instead the KVDB should just get encrypted by the fact that it
lives in TEE memory.

@JesseAbram
Copy link
Member Author

I mean that is fair, but are we on the fence still about removing the kvdb cuz if so then #1216 should wait, if we are then the whole kvdb should get removed and I can add onto 1216 and then this would no longer be a thing

Copy link
Contributor

@ameba23 ameba23 left a comment

Choose a reason for hiding this comment

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

Im approving this, just because i think its better than what we currently have as there will be no way to manually enter a password.

But its going to be tricky for operators to add this password file when running on TDX as there will be no ssh access to the virtual machine in which entropy-tss runs. It is possible to make filesystem changes but its not simple and i'm not sure we should encourage people to do it.

Im also not sure this adds anything in terms of security as anyone with access to the filesystem containing the kvdb will also have access to the password file.

I mean that is fair, but are we on the fence still about removing the kvdb cuz if so then #1216 should wait, if we are then the whole kvdb should get removed and I can add onto 1216 and then this would no longer be a thing

I think we probably are going to end up ditching the kvdb. As it looks to me right now our options for storing the keyshare are either SGX sealing, or in-memory only.

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.

Remove manual password input for kvdb
3 participants