search

zetkin / lyra

3 stars 3 forks source link

Make accidental publishing of security tokens less likely #123

Open henrycatalinismith opened 4 months ago

henrycatalinismith commented 4 months ago

From https://github.com/zetkin/lyra/issues/114, where I accidentally included the github_token value from my projects.yaml as part of my example.

Security tokens

You are not the first to accidentally publish security tokens. I think we can conclude that Lyra encourages this kind of mistake and it's something we should prioritise fixing. Lyra administrators are not always going to be so lucky that GitHub invalidates their published credentials.

a-jaxell commented 2 months ago

I would like to work on a fix for this.

I was thinking of a precommit hook or script that checks wether a config file exists and a variable of name github_token has a value. If not, this wont allow any commits. If a user submits a faulty token value this will be handled on push.

Perhaps setting an env variable is better?

WULCAN commented 2 months ago

Hmm, requiring a config file to exist will be an obstacle for a developer only looking to commit a small fix, without setting up a local testing environment.

It will also not be effective in stopping the kind of mistake we made in #114 where the token was published without any commit being made.

Reading the secret from an environment variable would indeed be better. It would separate the secret from the other, less sensitive, configuration properties. When the user shares her less sensitive configuration properties, she will be much less likely to also share a separated secret.

Please note however that environment variables are not in general secure places to store secrets. Typically, every process in the operating system will be able to read the environment variables of a process. Files on the other hand can have read restrictions. I'm not sure if environment variable security has improved in more recent operating systems.

It is also quite common for troubleshooting tools to log the entire environment, which would reveal the token unintentionally. I think there's also a mechanism in Next for automatically sharing parts of the server process's environment with the client browser, but I think it only shares variables named according to a certain pattern, specifically to avoid this kind of leak. Other framework tools might not be so careful.

a-jaxell commented 1 month ago

After some time marinating with this issue I am now wondering a bit about the issue.

Since we have functionality to read the config from ./config/ and that this folder is kept out of version control. To me this seems like a "good-enough" guard for this specific case.

This leaves me with two toughts: What other ways would a developer risk submitting their credentials and is it possible and in scope to fool-proof involuntary token submission?

WULCAN commented 1 month ago

Yes, I think you're right that our .gitignore is good enough compared to a precommit hook. However, both .gitignore and precommit hooks will only help against leaking the token via git. If we leak the token via some other channel, these solutions will do nothing.

What happened in #114 is that we copied configuration files into an issue description. By sharing the complete configuration, we also shared a GitHub access token.

Moving the access tokens to a separate file would help a little.

Moving access tokens to in memory-only, with some kind of admin UI to configure them, would help more, but would make restarting instances too cumbersome.

Removing access tokens would be much better and it might be possible. Lyra only uses GitHub access tokens to create pull requests. We do not intend for translators to create pull requests, only Lyra administrators and project developers should create pull requests. These kinds of users are probably already signed in to GitHub with their browser or could sign in if requested. That's sufficient to create pull requests with https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/using-query-parameters-to-create-a-pull-request I think that's what's #17 is about. It would have other benefits too: the PR would be created by the user clicking the button, not the user who set up the Lyra instance.