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

opentf login store token in plain text #386

Closed
eranelbaz opened this issue Sep 12, 2023 · 12 comments
Closed

opentf login store token in plain text #386

eranelbaz opened this issue Sep 12, 2023 · 12 comments
Labels
enhancement New feature or request pending-decision This issue has not been accepted for implementation nor rejected. It's still open to discussion.

Comments

@eranelbaz
Copy link
Member

OpenTF Version

OpenTF v1.6.0-dev
on darwin_amd64

Use Cases

Using opentf login to connect to a remote backend store the token as plain text in /Users/<username>/.terraform.d/credentials.tfrc.json

❯ opentf login <URL>
OpenTF will request an API token for <URL> using your browser.

If login is successful, OpenTF will store the token in plain text in
the following file for use by subsequent commands:
    /Users/eranelbaz/.terraform.d/credentials.tfrc.json

Attempted Solutions

Proposal

Add an option to encrypt the token with some password
Either after prompting for the token or by using an environment variable

References

No response

@eranelbaz eranelbaz added the enhancement New feature or request label Sep 12, 2023
@marcinwyszynski
Copy link
Contributor

Meh.

This is not a regression of any kind, and legacy Terraform always behaved that way.
Overall it's not unusual in general to have these credentials files in your home directory, AWS CLI does something similar IIRC.

@marcinwyszynski
Copy link
Contributor

Presumably the most "native" way would be to use some keyring abstraction like this library but it's potentially a major inconvenience for unclear gain especially that these tokens are meant to expire.

@eranelbaz
Copy link
Member Author

It is indeed not a regression of any kind, I just find it as a security issue when you use OpenTF with login feature,
those meant to expire but it's not must, depends on the token issuer

@marcinwyszynski
Copy link
Contributor

What's the attack vector here, though? An attacker would need to have read access to your home folder, right?

@eranelbaz
Copy link
Member Author

What's the attack vector here, though? An attacker would need to have read access to your home folder, right?

🤔
Kind of yes, it's not that important now that I'm thinking about it, but it can be nice addition for security + I think that we can alter the credentials.tfrc.json folder and if you store it in a different place it might be more important
but again, it's not that important

@marcinwyszynski
Copy link
Contributor

TBH I'm not a professional security expert, so I will let @WSpacelifT jump in.

@cube2222 cube2222 added the pending-decision This issue has not been accepted for implementation nor rejected. It's still open to discussion. label Sep 14, 2023
@mik3h0 mik3h0 mentioned this issue Oct 4, 2023
@cube2222
Copy link
Collaborator

cube2222 commented Oct 4, 2023

I'd be all for using the OS's credential helper abstraction (or keychain, how you called it) for this, longer-term.

git does that too.

Why would it be a major inconvenience @marcinwyszynski? I imagine we could make it configurable, so you don't have to use a keychain abstraction if there is none available. But if it's present, then the user-side experience should be basically the same, other than the token file not existing.

@eranelbaz
Copy link
Member Author

Also we have another suggestion regarding the same security issue, but to resolve it using sso
#643

@marcinwyszynski
Copy link
Contributor

Why would it be a major inconvenience @marcinwyszynski?

Some implementations (like the one I saw with Apple Keychain) would have you type in your admin password every time you want to use the CLI.

@serdardalgic
Copy link
Contributor

Some implementations (like the one I saw with Apple Keychain) would have you type in your admin password every time you want to use the CLI.

Touch ID or Face ID would be an alternative to typing the password every time, but currently the PR to support that has not been merged into the keyring library.

@cube2222
Copy link
Collaborator

Both AWS and Kubernetes store their local credentials this way, so it seems like most view it as a good-enough solution.

Using an os-specific secret management agent would require a bunch of platform-specific work. I'll be closing this for now, but if you have a good proposal on how to approach this without too much complexity added (both in code and build-time), feel free to reopen this issue and make your case.

@cube2222 cube2222 closed this as not planned Won't fix, can't repro, duplicate, stale Jan 22, 2024
@janosdebugs
Copy link
Contributor

For added context, I did a little bit of digging on how to integrate this, and it would seem that we need to implement quite a bit of os-specific calls, for example here for Windows. This can become very tricky if we want to support linking or dynamically loading OS-specific libraries that may or may not be compiled with the flags we expect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pending-decision This issue has not been accepted for implementation nor rejected. It's still open to discussion.
Projects
None yet
Development

No branches or pull requests

5 participants