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

Mention keyring package in Wrapping APIs vignette? #346

Open
maelle opened this issue Oct 13, 2023 · 11 comments · May be fixed by #620
Open

Mention keyring package in Wrapping APIs vignette? #346

maelle opened this issue Oct 13, 2023 · 11 comments · May be fixed by #620
Milestone

Comments

@maelle
Copy link
Contributor

maelle commented Oct 13, 2023

More precisely, should this workflow be added to that vignette: In an API package doc, recommend the user store their API key with keyring and retrieve it with Sys.getenv(API_KEY=keyring::key_get("API-NAME")?

@hadley
Copy link
Member

hadley commented Oct 13, 2023

What’s the advantage of doing that that you’re thinking of?

@maelle
Copy link
Contributor Author

maelle commented Oct 13, 2023

Storing the secrets in the OS' credentials store is supposed to be safer than storing them in .Renviron.

@asadow
Copy link

asadow commented Oct 13, 2023

I too noticed keyring is not in the httr2 docs, whereas httr has this vignette: Managing secrets.

Interested in this discussion as e.g. qualtRics wraps an API and depends on the user having secrets stored in .Renviron.

@hadley
Copy link
Member

hadley commented Oct 13, 2023

Hmmm, I guess since I wrote that httr vignette I'm no longer sure that the keyring is that much more secure. I think it's theoretically more secure, but in practice it's unlikely to make much difference because for an attacker to have access to your .Renviron then that implies they have access to your computer, and are thus can already access your OS keychain.

OTOH maybe people are using project specific .Renviron files which are much more dangerous because they're more likely to end up accidentally committed into git?

@maelle
Copy link
Contributor Author

maelle commented Oct 16, 2023

Right, maybe it just feels more secure but your point about git seems important.

More niche and less important:

  • I like that the OS keychain really feels more different from writing a secret in a script (compared to .Renviron vs secret in script, in both cases you write things in clear in a file which seems to be a bad habit like having a docx with all your passwords).
  • I know screen-sharing Renviron by mistake is not a big risk but I also like that this couldn't happen with the OS keychain.

@hadley
Copy link
Member

hadley commented Oct 16, 2023

Hmmmm, this all feels like it should maybe be a separate vignette that's env vars vs keychain, but then httr2 doesn't feel like quite the right place for it. Maybe it would be better as a keyring vignette?

@maelle
Copy link
Contributor Author

maelle commented Oct 16, 2023

but how would httr2 users even think of reading keyring docs, could there be a pointer, even small? 🙂

@hadley
Copy link
Member

hadley commented Oct 16, 2023

Right, we'd point to it everywhere we mention env vars.

@hadley
Copy link
Member

hadley commented Oct 19, 2023

I updated the keyring readme: https://keyring.r-lib.org/dev/. Do you think that's sufficient for selling it as an alternative to env vars or do I need to write more?

@maelle
Copy link
Contributor Author

maelle commented Oct 20, 2023

Nice, thank you! I added comments in r-lib/keyring#139 😸

@hadley
Copy link
Member

hadley commented Oct 26, 2023

I think this requires more than just a change to the docs: for it to be a useful (and meaningfully more secure workflow), secret_encrypt() and friends need to know to look in the keychain. I think that's probably still worth doing, but I won't get to it in this release.

@hadley hadley removed this from the v0.3.0 milestone Oct 26, 2023
@hadley hadley added this to the v1.1.0 milestone Dec 24, 2024
hadley added a commit that referenced this issue Dec 24, 2024
* [ ] Update vignette

Fixes #346
@hadley hadley linked a pull request Dec 24, 2024 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants