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

[RFC] Remove the mandatory secret from fw-bundle #27724

Open
Taluu opened this Issue Jun 26, 2018 · 11 comments

Comments

Projects
None yet
7 participants
@Taluu
Contributor

Taluu commented Jun 26, 2018

Description
This is a kind of continuation of #14026.

Currently, on the framework bundle, if the parameter secret is not given, it just does not set the kernel.secret parameter. But without this parameter, the uri_signer service cannot be declared, or otherwise we get an error from the container (something along the lines of "no parameter 'kernel.secret' found").

This service (uri_signer) is currently only used in the fragments, which are activated only if the relevant config is enabled (here is the search : https://github.com/symfony/symfony/search?l=XML&q=secret if you exclude the tests, you can see it's only used on the uri_signer, which is only used on the fragments related services).

My proposal is to declare the uri_signer service only if the secret parameter is given ; this would allow to bypass the mandatory state of the secret parameter. WDYT ?

@Taluu Taluu changed the title from Remove the mandatory secret from fw-bundle to [RFC] Remove the mandatory secret from fw-bundle Jun 26, 2018

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Jun 26, 2018

IIRC this parameter was used in the past for things like the RememberMe Token, the CSRF token, etc. It looks like we no longer use it. @nicolas-grekas could container.build_hash be a viable alternative for using it in uri_signer and therefore remove the kernel.secret param? Thanks!

@javiereguiluz javiereguiluz added the RFC label Jun 26, 2018

@stof

This comment has been minimized.

Member

stof commented Jun 26, 2018

could container.build_hash be a viable alternative for using it in uri_signer

that would not be a good idea: a new deployment could have a different container build hash, and make all existing signed URIs invalid (even if they were generated 3s before the deployment).
but we could indeed make the kernel.secret optional if you don't rely on the uri_signer (we might need to expose a config setting though, as the service might be used for something else than fragments, so we cannot register it only when fragments are enabled)

@Taluu

This comment has been minimized.

Contributor

Taluu commented Jun 26, 2018

Then how about registering it only if the secret parameter is set ?

@xabbuh

This comment has been minimized.

Member

xabbuh commented Jun 26, 2018

We need to be careful though. There might be other bundles (scheb/two-factor-bundle being one of them) out there which rely on the parameter being set.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 26, 2018

What's the issue with having this parameter? It's a feature of the framework to me, like the log folder. Having a stable secret has many use cases.
Since this is generated by Flex, most people just don't notice it exists. And if you're not, you know what and how to wire and configure things and that's one of the many other steps needed to setup Symfony, don't you think?

@Taluu

This comment has been minimized.

Contributor

Taluu commented Jun 26, 2018

I don't think the suggestion was understood.

in here https://github.com/symfony/framework-bundle/blob/master/DependencyInjection/FrameworkExtension.php#L165-L167

We can see that the kernel.secret parameter is registered only if the secret config is given. That's fine.. But, the uri_signer service, which is declared in https://github.com/symfony/framework-bundle/blob/master/Resources/config/services.xml#L58-L60 needs that kernel.secret parameter... and that file is always loaded, regardless of the secret config state (as we can see here : https://github.com/symfony/framework-bundle/blob/master/DependencyInjection/FrameworkExtension.php#L123)

So my main proposal was to move the uri_signer and declare it only if the secret is defined. Because currently, if you do not give a secret, the application can't work. This is flirting with a bug actually IMO.

WDYT ?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 26, 2018

The current logic allows kernel.secret to be defined outside of semantic configuration - nothing else AFAIK.

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Jun 26, 2018

The service "uri_signer" has a dependency on a non-existent parameter "kernel.secret". Did you mean this: "kernel.charset"?`

by declaring uri_signer service conditionally we only move the dependency error, unless we cleanup the entire chain or make the config/param required. I can live with the current behavior, allthough the error message / hint can be improved.

@derrabus

This comment has been minimized.

Contributor

derrabus commented Jul 4, 2018

kernel.secret just sounds way too important, given what that parameter is being used for nowerdays. I currently see two scenarios when looking into projects:

A: The framework.secret setting is set to some dummy value because the container won't compile otherwise.

B: A fancy deployment mechanism is built around the kernel.secret parameter to make sure production, staging and development machines have different and really secret values.

The scenario that I don't see at the moment: The project actually uses functionality that depends on a proper secret.

This "secret" just confuses people and the only functionality that still depends on it isn't a commonly used one. This is why I would support @Taluu's proposal.

A possible solution could be to enable/disable the uri_signer together with fragment rendering in the framework bundle configuration and to move the secret to the fragment rendering section.

@Taluu

This comment has been minimized.

Contributor

Taluu commented Jul 4, 2018

I think the solution would be to leave the %kernel.secret% declaration as is, and move the uri_signer declaration on the fragments part. To use fragments, you need a secret, and that's pretty much it.

Once again, the real problem is that despite the fact that the secret parameter is marked as optional (it doesn't matter whether it is used with flex, generated through it, ... not everybody uses flex, and not everybody actually need that secret thingy), it is not actually, because the uri_service needs the kernel.secret parameter, which is built on the secret config.

This leads me to have to declare a symfony-lol value, because otherwise, I have the container compile error. Once again, I'm not proposing to remove the secret (or kernel.secret)... I propose to make it either truly optional or mandatory (which would be a shame IMO). I'll give a reproducer when I'll have some time (probably tomorrow).

@Taluu

This comment has been minimized.

Contributor

Taluu commented Jul 6, 2018

A bit late, but here's the reproducer : https://github.com/Taluu/symfony-reproducer/tree/secret-not-optionnal

I just commented out the secret, and as you can see, even though the secret is marked as optionnal, it is not optional within the fw-bundle.

In ParameterBag.php line 100:

  The service "uri_signer" has a dependency on a non-existent parameter "kernel.secret". Did
  you mean this: "kernel.charset"?

So, here's a bunch of solutions :

  1. we declare the uri_signer service only if secret is declared
  2. move its declaration only when fragments are used
  3. we make the secret not optional (I fail to see a case where %kernel.secret% is declared but not secret...).

AFAIC, The solution I'd to go for would be the first one. Not that it doesn't matter if flex is generating (or not) for the secret.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment