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

[lexik/jwt-authentication-bundle] gitignore JWT keys #89

Closed
wants to merge 1 commit into from

Conversation

teohhanhui
Copy link
Contributor

Q A
License MIT

Fixes #84

symfony-bot
symfony-bot previously approved these changes May 31, 2017
@javiereguiluz
Copy link
Member

Even if this proposal is "the right thing" to avoid committing sensitive data to the repo, I think the fix is not correct. The etc/ dir (or config/ if we finally rename it) should be committed entirely. Now that we're getting rid of the parameters.yml + parameters.yml.dist files, it's time to simplify things committing the etc/ dir entirely and putting all the sensitive credentials in the .env file.

@teohhanhui
Copy link
Contributor Author

teohhanhui commented May 31, 2017

putting all the sensitive credentials in the .env file

From moby/moby#13490:

  • Environment Variables. Probably the most used, because it's part of the "12 factor app". Environment variables are discouraged, because they are;
    • Accessible by any proces in the container, thus easily "leaked"
    • Preserved in intermediate layers of an image, and visible in docker inspect
    • Shared with any container linked to the container

Docker now has a "secrets" feature (Swarm Mode only) which exposes each piece of sensitive data as files under the /run/secrets directory.

dunglas
dunglas previously approved these changes Jun 21, 2017
@dunglas
Copy link
Member

dunglas commented Jun 21, 2017

Maybe can we just move the JWT keys in var/jwt?

@sroze
Copy link
Contributor

sroze commented Jun 23, 2017

Yes, I think that this should definitely live in var if they are committed to the code repository.

@sstok
Copy link
Contributor

sstok commented Jun 23, 2017

If the JWT keys are generated for each application separate (like secret), then 👍

@Pierstoval Pierstoval mentioned this pull request Jun 23, 2017
@chalasr
Copy link
Member

chalasr commented Jun 27, 2017

Not sure about var. I use it myself and not so long ago someone told me "But var/ is about storage, temporary data, cache, logs... Removing keys just breaks the setup", and I think he is right.

@dunglas
Copy link
Member

dunglas commented Jun 27, 2017

But the keys should be different on each instance of an app. So they shouldn't be in etc/. Imo we should have a directory to store such sensitive informations (maybe secret/?).

Btw, I'm implementing a mechanism similar to env() but to read files containing secret data at runtime and inject them in the container as parameters (for instance, to use Docker secrets). It would be nice to have a directory to store such data (for those not using Docker).

@teohhanhui
Copy link
Contributor Author

I disagree about using var. Please see the relevant FHS section: http://www.pathname.com/fhs/pub/fhs-2.3.html#PURPOSE31

etc is the correct location.

@teohhanhui
Copy link
Contributor Author

Since etc has been renamed to config, I agree with @dunglas that we should put it under secrets/jwt.

Will update the PR.

@teohhanhui teohhanhui changed the title [lexik/jwt-authentication-bundle] gitignore contents of etc/jwt [lexik/jwt-authentication-bundle] gitignore JWT keys Jul 5, 2017
@dunglas
Copy link
Member

dunglas commented Jul 20, 2017

I've almost finished secrets support for the DI component. It just needs some polish (probably when I'll be back from vacation).

"JWT_PRIVATE_KEY_PATH": "%CONFIG_DIR%/jwt/private.pem",
"JWT_PUBLIC_KEY_PATH": "%CONFIG_DIR%/jwt/public.pem",
"JWT_PRIVATE_KEY_PATH": "secrets/jwt/private.pem",
"JWT_PUBLIC_KEY_PATH": "secrets/jwt/public.pem",
Copy link
Member

Choose a reason for hiding this comment

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

Shoudn't it be %PROJECT_DIR%/secrets/jwt/...

Copy link
Contributor Author

@teohhanhui teohhanhui Sep 11, 2017

Choose a reason for hiding this comment

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

No, unless we want to support storing the key pair outside of the project directory? (Out of scope for this PR anyway...)

Copy link
Contributor Author

@teohhanhui teohhanhui Sep 11, 2017

Choose a reason for hiding this comment

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

But if we go ahead with this, we should probably add a secrets-dir option in Flex?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Fabien. This looks really strange. It should be %CONFIG_DIR%/secrets/jwt/... or %PROJECT_DIR%/secrets/jwt/...

Copy link
Contributor Author

@teohhanhui teohhanhui Sep 11, 2017

Choose a reason for hiding this comment

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

%CONFIG_DIR% resolves to config.
%PROJECT_DIR% resolves to the full path of the project directory.

What we might want to add is a %SECRETS_DIR% which resolves to secrets. But I still find it weird to see a %SECRETS_DIR% in an environment variable... I think the resolving should happen when Flex updates the .env (and .env.dist) files. oh wait it already does...

Copy link
Contributor Author

@teohhanhui teohhanhui Sep 11, 2017

Choose a reason for hiding this comment

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

symfony/symfony#23901 introduced the file prefix for %env(...)% processing.

Copy link
Member

@dunglas dunglas Sep 11, 2017

Choose a reason for hiding this comment

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

@javiereguiluz managing secret's encryption at this level looks like a bad idea to me. There are already plenty dedicated tools to do it like Docker and Kubernetes. IMO the best thing to to is to make it easy to use them in Symfony (and it's achieved with symfony/symfony#23901), not to reinvent the wheel.

I'm 👍 to introduce a new secrets directory and store things like JWT keys and Doctrine passwords in it (and retrieve them with symfony/symfony#23901).

Copy link
Contributor

Choose a reason for hiding this comment

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

The risk of the secrets directory is that people would be tempted to directly commit their credentials in the code repository... You use the example of Docker/Kubernetes and the secrets management is exactly meant to prevent people to add their secrets within the Docker image directly. As long as we give a very clear documentation that these secrets have to be development secrets and overwritten by production secrets while deploying production, then I'm 👍 with such secrets directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding an empty directory with a .gitignore which ignores everything else should help.

Copy link
Member

Choose a reason for hiding this comment

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

We can add this directory to the .gitignore file.

@sroze
Copy link
Contributor

sroze commented Sep 12, 2017

Not convinced about that the DX with this directory in the .gitignore as it means you don't have a working development environment directly after cloning a project anymore. Even if our official recipes could generate development secrets, I'm not convinced about user secrets.

@dunglas
Copy link
Member

dunglas commented Sep 12, 2017

You can ignore the content of the directory while keeping it in the version system.

@teohhanhui
Copy link
Contributor Author

Secrets should never be checked in.

@sroze
Copy link
Contributor

sroze commented Sep 12, 2017

Secrets should never be checked in.

This is not true. Production secrets should not be checked in. Development secrets should be checked in, so as a developer, I don't have to pull secrets from anywhere else to have a development environment working.

@teohhanhui
Copy link
Contributor Author

Development secrets should be checked in, so as a developer, I don't have to pull secrets from anywhere else to have a development environment working.

I disagree. But we can already see this is a never-ending discussion.

VolCh pushed a commit to VolCh/recipes that referenced this pull request Nov 30, 2017
@nicolas-grekas
Copy link
Member

I'm closing as the PR is old, and Makefile have been removed.
Please resubmit if you think we still need to improve.

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.

9 participants