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

Read from config file to hide sensitive info from process list #474

Closed
eric-brechemier opened this issue Jan 27, 2016 · 27 comments
Closed
Milestone

Comments

@eric-brechemier
Copy link
Contributor

The method suggested in the documentation, Hiding Password from Process List by setting an environment variable, leaves the value visible in ps listing, since the parameter $PASS is expanded and replaced with its value before running the command.

JWT_SECRET='abc123'
postgrest postgres://postgrest@localhost:5432/db (...) --jwt-secret "$JWT_SECRET"
ps aux | grep jwt-secret
(...) postgrest postgres://postgrest@localhost:5432/egull (...) --jwt-secret abc123
@eric-brechemier
Copy link
Contributor Author

This issue is similar to #277 and calls for the same solution: reading the secret from a configuration file instead.

@begriffs
Copy link
Member

Ah that's too bad, thanks for the heads up. Which config file format would you like? I could do json or yaml or a plain key = value style (see this)

@eric-brechemier
Copy link
Contributor Author

I would go with the latter (key = value) because it is the easiest to generate or read from a shell script.

Also, it is the format used in postgresql.conf.

@begriffs
Copy link
Member

Makes sense. Also should we add a special command line flag to pass the path of a configuration file, or expect the file to live in ~/.postgrest.conf or some other convention?

@eric-brechemier
Copy link
Contributor Author

We should do both:

  1. try reading the file from /etc/postgrestql/postgresql.conf, if available (following Unix convention)
  2. provide a command line flag to load the configuration file from a different path (this is mandatory to boot more than one instance of postgrest)

@ruslantalpa
Copy link
Contributor

hardcoding the path in the binary is not a good idea, it should be a flag.
It's the job of the init script (for each particular OS) to know the default location of the config file and feed that to the binary as a command line argument.
That will give the install scripts (in rpm/deb) the freedom to install the executable/config files where the user choose to.

@eric-brechemier
Copy link
Contributor Author

@ruslantalpa reading the config file from a known location is just a convenience; I agree that the most important is to add support for the command line argument.

@eric-brechemier
Copy link
Contributor Author

Also, the minimum needed to solve this issue is to allow reading the JWT secret from a file.

Since providing the secret as a text parameter is unsafe, we could deprecate or replace the current parameter and define a new parameter which reads the secret from a file instead.

@ruslantalpa
Copy link
Contributor

@eric-brechemier
Copy link
Contributor Author

@ruslantalpa Cool trick, thanks!

This is a good workaround before fixing it properly with a configuration file.

@begriffs begriffs changed the title JWT secret is visible in ps listing Read from config file to hide sensitive info from process list Apr 3, 2016
@begriffs
Copy link
Member

begriffs commented Apr 3, 2016

I just confirmed that --jwt-secret "$(cat jwt.secret)" also exposes the secret. Looks like we do need that config file.

@drawks
Copy link

drawks commented Jun 16, 2016

Any news on when this might be resolved?

FWIW: I'm a fan of the curl style syntax for providing a string param from a file; which is to parse the string passed to the arg and if it starts with a "@" then the arg is read as the file name containing the true value.

for example:

       -F, --form <name=content>
              (HTTP)  This  lets curl emulate a filled-in form in which a user has pressed the submit button. This causes curl to POST data using the Con‐
              tent-Type multipart/form-data according to RFC 2388. This enables uploading of binary files etc. To force the 'content' part to be  a  file,
              prefix  the  file  name  with  an  @  sign. To just get the content part from a file, prefix the file name with the symbol <. The difference
              between @ and < is then that @ makes a file get attached in the post as a file upload, while the < makes a text field and just get the  con‐
              tents for that text field from a file.

              Example, to send your password file to the server, where 'password' is the name of the form-field to which /etc/passwd will be the input:

              curl -F password=@/etc/passwd www.mypasswords.com

p.s. I once again find myself hamstrung by the obscure choice of language for this project. I would happily contribute were postgrest written in something not requiring learning a new language.

@ruslantalpa
Copy link
Contributor

the more i think about this, the more i don't understand how this is a security issue.
like if a person that should not know that password/config ... why does he have access to a shell on the production server?
I think this is "fear" from the age of shared hosting that has no relevance today in the age of AWS/docker

@drawks
Copy link

drawks commented Jun 16, 2016

It is ridiculously presumptuous to say that every person who has shell access to a machine should have access to secret material. There is a reason why filesystems have permissions and systems support multiple users... Because not every user is entitled to all information.

@ruslantalpa
Copy link
Contributor

And there is a reason today the talk in tech is around aws/containers/unikernels for production and not the multiuser os paradigm from the 70's

@begriffs
Copy link
Member

Currently the only values we need to hide from the process list are the postgresql password and the jwt secret. The password can be removed from the command line by using the .pgpass file. This leaves the jwt secret unprotected.

In #495 we found that passing a jwt secret at the command line can mangle it when using binary values and concluded that we need to allow the jwt secret to be loaded from a file. This is another argument for getting it out of the command line arguments. @drawks I like your idea about the @ convention to indicate loading from files. It allows us to keep the same command line argument name.

More generally I looked into just reading all parameters from a postgrest config file, but I think it requires removing the current optparse-applicative code which handles our command line arguments. That's because some of the postgrest arguments are marked as required, and I don't know how to tell the library that we got those arguments from another source (the config file) and that it doesn't have to complain if they are missing from the command line.

So how about extending the jwt secret param to interpret the @filename convention rather than having postgrest read from a config file?

@eric-brechemier
Copy link
Contributor Author

So how about extending the jwt secret param to interpret the @filename convention rather than having postgrest read from a config file?

It works for me. It would break secrets starting with the '@' character however.

begriffs added a commit that referenced this issue Sep 4, 2016
@begriffs
Copy link
Member

begriffs commented Oct 2, 2016

I have implemented the @filename convention for the jwt secret in a PR that is about ready to merge, but I have another thought. Since the 0.4 release is coming up and allows us to make backward incompatible changes, what if we got rid of command line arguments entirely and read everything from the configuration file? This would allow hot reloading and more detailed configuration in the future if necessary.

In particular postgrest would have a single (optional) parameter for its configuration file path. As mentioned above it could have a default location if the parameter is not specified.

Thoughts?

@begriffs begriffs added this to the v0.4 milestone Oct 2, 2016
@begriffs
Copy link
Member

begriffs commented Oct 4, 2016

(This issue got auto-closed by a comment in the PR which implemented @filename, but I'm reopening it for discussion of a full-blown configuration file)

@begriffs begriffs reopened this Oct 4, 2016
@eric-brechemier
Copy link
Contributor Author

what if we got rid of command line arguments entirely and read everything from the configuration file?

That completely makes sense.

In particular postgrest would have a single (optional) parameter for its configuration file path. As mentioned above it could have a default location if the parameter is not specified.

That sounds great.

@begriffs
Copy link
Member

begriffs commented Oct 5, 2016

There are quite a few packages to evaluate.

From my brief inspection the configurator package looks most interesting.

There is even a package to help me honor the XDG Base Directory spec for configuration file location. https://hackage.haskell.org/package/xdg-basedir

@ruslantalpa
Copy link
Contributor

when in doubt always go with BOS :) (but i don't think there is doubt)
i.e. configurator
ppl won't even need to send HUP, they just update the config file ... how cool is that :)

@shofetim
Copy link

shofetim commented Jan 24, 2017

I just tried v0.4 and was surprised by the move to requiring a configuration file. It goes against the 12factorapp best practices, complicates the deployment via docker/kubernetes etc. I've also always liked it that the postgrest was a self contained, relocatable binary with no setup.

Could we get back the ability to configure postgrest via the command line?

@begriffs
Copy link
Member

You're right that it's less convenient for Docker. You have to map the config file into the container. It's also more complicated in the Heroku buildpack which I still need to update for the new version.

Let me explain the reasons leading up to the switch and then we can figure out what makes sense to do.

Originally, as this issue testifies, people wanted to hide secrets from the process list. Even passing in $ENV_VAR on the command line expands in the shell and reveals the secret. (I realize that you're suggesting the program read directly from an env var rather than passing it as an argument though.)

The next complicating factor is that the library we use to parse command line arguments considers each argument as either required or not. However when arguments are read from both the command line and a config file then they are only required on the command line when absent in the config.

The parsing thing is fixable with some work, but I also considered that most other servers like Apache or Nginx use a config file so it felt normal to do things that way. I'm open to questioning the convention of course, but it is a common practice.

The last point is that the jury seems to be out about this particular part of the twelve-factor app design.

Do those articles affect your opinion about the config file?

@russelldavies
Copy link
Contributor

I think there is some conflict between two sides: traditional deployment approaches and containers. For the former, it makes sense to have a config file for the deployment, security, and configuration update benefits. However, when deploying via containers, none of these arguments are relevant nor provide any benefit; the opposite, in fact.

So, I think that the config file should be kept and supported for traditional deployments. But command line flags should also be an option for flexibility. As for containers, postgrest should try to read the uppercase configuration parameter name from the environment variables so a shell entrypoint script doesn't need to be written.

@begriffs
Copy link
Member

begriffs commented Apr 2, 2017

In #826 I'm working on a scheme where the Docker container uses a config file that interpolates from environment variables, so you have an option to either map a custom config file into the container, or just to set environment vars as usual. Ran into a problem about parsing interpolated integers, but got a response about that today in the associated configurator-ng library.

@russelldavies
Copy link
Contributor

Ah, I missed that issue so I'll follow the progress there. Thanks.

krakrjak pushed a commit to krakrjak/postgrest that referenced this issue Apr 5, 2017
eric-brechemier pushed a commit to eGullGolf/postgrest that referenced this issue May 19, 2017
When the secret value starts with '@', the rest of the value is
treated like the path to a file which contains the secret.

This change allows to hide the secret from the command line,
which solves issue PostgREST#474.

Based on:
commit 449480b
Author: Joe Nelson <[email protected]>
Date:   Sat Sep 3 23:47:56 2016 -0700

    Require JWT secret, remove default, optionally read from file

    Fixes PostgREST#474, fixes PostgREST#495 when file is used
monacoremo pushed a commit to monacoremo/postgrest that referenced this issue Jul 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants