Closed dunglas closed 1 month ago
Thanks for the PR 😍
Define the SYMFONY_ENDPOINT
environment variable:
# On Unix-like (BSD, Linux and macOS)
export SYMFONY_ENDPOINT=https://raw.githubusercontent.com/symfony/recipes/flex/pull-1314/index.json
# On Windows
SET SYMFONY_ENDPOINT=https://raw.githubusercontent.com/symfony/recipes/flex/pull-1314/index.json
Install the package(s) related to this recipe:
composer req 'symfony/flex:^1.16'
composer req 'symfony/framework-bundle:^7.0'
Don't forget to unset the SYMFONY_ENDPOINT
environment variable when done:
# On Unix-like (BSD, Linux and macOS)
unset SYMFONY_ENDPOINT
# On Windows
SET SYMFONY_ENDPOINT=
In order to help with the review stage, I'm in charge of computing the diff between the various versions of patched recipes. I'm going keep this comment up to date with any updates of the attached patch.
Thanks Kévin, but I'm not sure this is the best solution for the issue mentioned:
NotSecure-...
as the value of the secret; some people will think that Symfony is not safe by default;NotSecure-
prefix or will replace this value entirely but will keep committing it to the repo.In my opinion, we can't properly solve this without a good comment below APP_SECRET
explaining the situation and giving clear indications about what to do.
@javiereguiluz by definition, .env
contains placeholder environment variables that serve as examples of how to configure the app on production. To me, having a NotSecure-
prefix in one of these values doesn't say that Symfony isn't secure but on the contrary, that Symfony takes security seriously and makes it crystal clear that this value must not be used as-is in production.
API Platform has always used a similar placeholder for APP_SECRET
(which, IMHO, is even more clear), and as far as I know, nobody says that API Platform isn't secure: https://github.com/api-platform/api-platform/blob/main/api/.env#L23
I agree with @dunglas.
Besides, I think whatever we do in the env file, we must also document the secret in the getting started docs. I'll propose something tonight/tomorrow!
API Platform has always used a similar placeholder for APP_SECRET (which, IMHO, is even more clear), and as far as I know, nobody says that API Platform isn't secure
Then I'm going to be the one saying this is highly insecure. There are scanners out there that do test for vulnerable Symfony apps that didn't change their APP_SECRET and that's a successful approach to hack many webapps apparently. "not on us" is not a satisfactory answer IMHO. This is related to https://github.com/symfony/symfony/pull/52471
I'm :-1: with this PR because I think we should do better on the topic.
Here are some thoughts:
Ideally, we would remove this APP_SECRET entirely for skeleton apps. And for apps that do need it, we already have another stable secret we can use: SYMFONY_DECRYPTION_SECRET. It'd be great if we could derivate kernel.secret from that var when available.
I also had a look at services that do need this secret, here they are:
uri_signer
- this one is used to generate signed URLs when using ESI fragments. After https://github.com/symfony/symfony/pull/52471, can we consider removing this requirement and encrypt only opportunistically when kernel.secret is populated?security.authenticator.remember_me*
- here, we could use tokens stored in the user DB. Would that make sense?security.authenticator.abstract_login_link_signature_hasher
- for this one, tokens could be used instead?security.login_throttling.%firewallName%.limiter
and here, is the secret really needed when computing the hash? What if we didn't do it?If we think we do need kernel.secret, then an idea could be to turn this into a service instead of a parameter. That service would have one responsibility: throw a nice and actionable error message when no secret is configured, with a __toString method to pass the secret to consumers when its set.
@nicolas-grekas I agree with your long-term approach. However, this patch has the benefit of improving the security and the DX right now, and can easily be reverted when your approach is implemented.
Big +1 for the service, so we could also improve the support for Docker and Kubernetes secrets, HashiCorp Vault, etc. by providing ad-hoc implementations.
Then I'm going to be the one saying this is highly insecure. There are scanners out there that do test for vulnerable Symfony apps that didn't change their APP_SECRET and that's a successful approach to hack many webapps apparently.
Most apps also need API tokens, SMTP passwords etc. We need to find a way to make it clear that values in .env
are only placeholders that sometimes need to be changed in production.
Maybe could we do something like throwing if we detect at runtime that an env var suffixed by _SECRET
has not been overrode?
What about renaming .env to .env.placeholders or .env.template or .env.whatever ?
I like your idea @nicolas-grekas 👍🏻
@OskarStark We follow the same convention as most ecosystems (Angular, Ruby etc) and it's widely accepted that .env
is the place for placeholders. Let's not bring our own convention while there is already a one that is popular.
Just to be clear: using this generated value in production is mostly fine.
Here, with the services I listed, committing the secret means anyone who had access to this token (aka people in the dev team) could fake a remember me token or a login link. This "Not-Secure" prefix is way more scary than needed. Saying "this is not secure" is FUD if the threat model is not clear.
To me, this is mostly secure, and we can and should document this threat model and how one should guard against it when they care.
Note that this threat model analysis makes me realize we don't have any way to rotate the secret, so that people leaving the company who had access to the secret couldn't get long term access to such capabilities. This is most concerning to me: not having a way to properly guard against a threat you want to alleviate.
Note that this threat model analysis makes me realize we don't have any way to rotate the secret, so that people leaving the company who had access to the secret couldn't get long term access to such capabilities. This is most concerning to me: not having a way to properly guard against a threat you want to alleviate.
I noticed the same problem when facing this topic. These are the ways of generating a new APP_SECRET
value I found so far:
rm symfony.lock; composer recipes:install --force
. This generates a new value for the APP_SECRET
variable, but also resets all your configuration to the default.bin/console secrets:set APP_SECRET --random
to generate a new random value in the vault. This generates a shorter value (16 chars) and not a hex number, but a random string (dunno, if that actually matters)The last one is the most sensible, but for me as a developer, it's confusing, that secret:set --random
creates a structurally different value then the one generated in the .env
file (it's not a hex string and way shorter).
So yes, an easy way to generate a new value for APP_SECRET
, that is well documented, would already help. Maybe extending secret:set --random
would be the way to go, since secrets belong into the vault anyway.
Closing in favor of https://github.com/symfony/recipes/pull/1317 :crossed_fingers: Thanks for shaking this topic :)
While it's ok to commit the
APP_SECRET
used for dev and test environments, in production, theAPP_SECRET
environment variable must be overridden by a local value that is not committed (using a secret vault, using Symfony Secrets, creating a local environment variable, or creating an unversioned.env.prod
file are acceptable options).With this patch, Flex will prefix the generated value with
NotSecure-
to make it clear that the committed value must not be used in production.Brought to our attention by @ramsey's https://phpc.social/@ramsey/112437517679689885. See also https://github.com/symfony/symfony/issues/38021.
We'll also have to document that
APP_SECRET
is used as the value for https://symfony.com/doc/current/reference/configuration/framework.html#secret