8000 [SecurityBundle] Comma separated ips for security.access_control · Issue #40881 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[SecurityBundle] Comma separated ips for security.access_control #40881

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.

Sign up for GitHub

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

Jump to bottom
Closed
fvilpoix opened this issue Apr 20, 2021 · 7 comments
Closed

[SecurityBundle] Comma separated ips for security.access_control #40881

fvilpoix opened this issue Apr 20, 2021 · 7 comments
Labels
Bug SecurityBundle Status: Needs Review

Comments

@fvilpoix
Copy link

Symfony version(s) affected: 5.2.6

Description
Hi,
I tried to use the new feature in 5.2.0 from #38149.
The comma separated value is rejected by the SecurityExtension

How to reproduce

security:
    # ...
    access_control:
         - { path: ^/profile, roles: ROLE_USER, ips: [::1, '%trusted_ips%'] }


parameters:
    trusted_ips: '127.0.0.1,127.0.0.2'

Then, I get a LogicException

image
@fvilpoix fvilpoix added the Bug label Apr 20, 2021
@ro0NL
Copy link
Contributor
ro0NL commented Apr 20, 2021

duplicates #40865

@zidaneus
Copy link
zidaneus commented Apr 20, 2021 •
edited
Loading

It's works, if trusted_ips: "%env(APP_SOME_IP_ADDRESSES)%"). The problem with using environment variables only occurs when the security bundle is validated. Without running validation, the bundle will work using environment variables as a value with comma separated string. See \Symfony\Bundle\SecurityBundle\DependencyInjection\SecurityExtension::createRequestMatcher

@ro0NL
Copy link
Contributor
ro0NL commented Apr 20, 2021

right, does it duplicate #40864? (the example there doesnt use envs either)

from the error The given value "127.0.0.1,127.0.0.2" i reasoned no envs were involved actually :)

with envs the validation should be bypassed as per

symfony/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

Line 739 in 2dd8445

if (!$usedEnvs && !$this->isValidIp($ip)) {

The problem with using environment variables only occurs when the security bundle is validated.

Then there'd be a second issue indeed 👍

@fvilpoix
Copy link
Author

It's exactly the same problem than #40864 and #40865 : in the three cases, the providen parameters are replaced by their values (from ENV or from a parameter), and then the security bundle fails on its validation.

I've just tried with this config, without any parameter:

    access_control:
        - { path: ^/profile, roles: ROLE_USER, ips: [::1, '127.0.0.1,127.0.0.2'] }

And the validation throws the same exception.

@edefimov
Copy link
Contributor
edefimov commented Apr 21, 2021 •
edited
Loading

@ro0NL , @xabbuh the issues #40881, #40864, #40865 are all about the same problem. We have inconsistent behavior when using plain string or environment variable as a configuration value.
Environment variable bypasses the check on line 1039 in SecurityExtension.php. However simple string will fail this check. Moreover if we can process comma-separated list of ip addresses given in environment variable, then there should be no problem if the same list is passed as plain string, right? The source code in RequestMatcher on line 130 proves this idea 8000 .
I think the fix here should be either in removing the check in security extension to make the behavior consistent, or allow validation pass if comma-separated string of valid ip addresses given

@ro0NL
Copy link
Contributor
ro0NL commented Apr 21, 2021

The problem *with using environment variables only occurs when the security bundle is validated.

the providen parameters are replaced by their values (from ENV and then the security bundle fails on its validation.

do we actually have a reproducer config that uses envs and crashes?

@edefimov
Copy link
Contributor
edefimov commented Apr 21, 2021 •
edited
Loading

@ro0NL, Well, we have some internal command which is executed before deployment to target environment and validates all configured environment variables. Under the hood the command builds another container with all environment variables resolved and checks thrown exceptions. (We also can share this code as feature request into framework bundle)

@edefimov edefimov mentioned this issue Apr 22, 2021
Merged
edefimov added a commit to edefimov/symfony that referenced this issue Apr 22, 2021
@edefimov
symfony#40881 [Security] Allow ips parameter in access_control accept…
7a0e899 
… comma-separated string
edefimov< 8000 /a> added a commit to edefimov/symfony that referenced this issue Apr 22, 2021
@edefimov
symfony#40881 [Security] Allow ips parameter in access_control accept…
693afa4 
… comma-separated string
edefimov added a commit to edefimov/symfony that referenced this issue Apr 22, 2021
@edefimov
symfony#40881 [Security] Allow ips parameter in access_control accept…
bb24270 
… comma-separated string
edefimov added a commit to edefimov/symfony that referenced this issue Apr 22, 2021
@edefimov
symfony#40881 [Security] Allow ips parameter in access_control accept…
9b5173f 
… comma-separated string
edefimov added a commit to edefimov/symfony that referenced this issue Apr 22, 2021
@edefimov
symfony#40881 [Security] Allow ips parameter in access_control accept…
cc6fe6f 
… comma-separated string
edefimov added a commit to edefimov/symfony that referenced this issue Apr 25, 2021
@edefimov
symfony#40881 [Security] Allow ips parameter in access_control accept…
93efd05 
… comma-separated string
edefimov added a commit to edefimov/symfony that referenced this issue May 1, 2021
@edefimov
symfony#40881 [Security] Allow ips parameter in access_control accept…
5eb1801 
… comma-separated string
nicolas-grekas added a commit that referenced this issue May 7, 2021
@nicolas-grekas
bug #40902 [Security] Allow ips parameter in access_control to accept…
897f287 
… comma-separated string (edefimov)

This PR was merged into the 5.2 branch.

Discussion
----------

[Security] Allow ips parameter in access_control to accept comma-separated string

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #40881, #40864, #40865
| License       | MIT

PR #38149 introduced a new feature to accept a comma-separated string in ip adresses setting in `access_control` configuration  section of security bundle.

However the feature works in inconsistent manner: comma-separated string can be successfully passed via environment variable, but can not be passed as plain string. This PR changes this inconsistent behavior by allowing validation pass if comma-separated list of ip addresses is given in plain string.

More detailed explanation about the inconsistent behavior can be found [here](#40881 (comment))

Commits
-------

8947482 [SecurityBundle] Allow ips parameter in access_control accept comma-separated string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug SecurityBundle Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ro0NL @fvilpoix @zidaneus @edefimov @carsonbot
0