Skip to content

[Security][5.2.0 only bug] Default entry_point selection does not work for 2nd firewall #39249

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
romaricdrigon opened this issue Nov 30, 2020 · 4 comments · Fixed by #39261
Closed

[Security][5.2.0 only bug] Default entry_point selection does not work for 2nd firewall #39249

romaricdrigon opened this issue Nov 30, 2020 · 4 comments · Fixed by #39261
Labels
Bug Security Status: Needs Review

Comments

@romaricdrigon
Copy link
Contributor

romaricdrigon commented Nov 30, 2020 •
edited
Loading

Symfony version(s) affected: 5.2.0 with new authenticator system
5.2.0-RC2 was not affected

Description

Using security configuration below, I will get a 401 error. HttpBasicAuthenticator::start() is not called, so it won't have the correct WWW-Authenticate header. And then authentication will never work (in browsers).

How to reproduce

Updated, see reproducer below: https://github.com/romaricdrigon/reproducer-for-39249

With this security.yaml: 

security:
    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false
        snapshot_internal_api:
            http_basic: ~
            pattern: /api/snapshot
            provider: internal_provider
            stateless: true
        main: # ...

Workaround for other users affected

A temporary workaround is to make sure to define entry_point - though there's a small catch about which key to use, you should use service name:

security:
    firewalls:
        # ...
        snapshot_internal_api:
            entry_point: security.authenticator.http_basic.snapshot_internal_api # The line to add
            http_basic: ~
        # ...

Possible Solution

I believe this issue was introduced by #39153
I suspect that http_basic authenticator is not part of those. I will have a look at a possible fix asap, but I'm not sure to fully grasp all details yet.
@derrabus
Copy link
Member

@wouterj Can you have a look?

@romaricdrigon
Copy link
Contributor Author

romaricdrigon commented Nov 30, 2020 •
edited
Loading

Fiddling a little bit with the container pass, it looks the issue is more complex. It appear when there are multiple firewalls using http_basic; then I have the impression the wrong ExceptionListener is modified, or something similar.
I will try to work on a reproducer tomorrow or the day after, because my YAML snippet is clearly not enough.

@romaricdrigon
Copy link
Contributor Author

This issue is provoked when there is a firewall with custom_authenticators before http_basic. I'm editing issue title.

Here's a repository with the minimal code to reproduce it: https://github.com/romaricdrigon/reproducer-for-39249

@romaricdrigon romaricdrigon changed the title [Security][5.2.0 only bug] Default entry_point selection does not work with "http_basic" authenticator [Security][5.2.0 only bug] Default entry_point selection does not work for 2nd firewall Dec 1, 2020
@romaricdrigon
Copy link
Contributor Author

Found it, the issue is that there is a return instead of continue, hence aborting too early the compiler pass.

I'm opening a PR in a few minutes.

romaricdrigon pushed a commit to romaricdrigon/symfony that referenced this issue Dec 1, 2020
[Security] fix symfony#39249, default entry_point compiler pass was r…
307572d 
…eturning too early
@romaricdrigon romaricdrigon mentioned this issue Dec 1, 2020
Merged
romaricdrigon pushed a commit to romaricdrigon/symfony that referenced this issue Dec 1, 2020
[Security] fix symfony#39249, default entry_point compiler pass was r…
c377805 
…eturning too early
chalasr added a commit that referenced this issue Dec 1, 2020
@chalasr
bug #39261 [Security] fix #39249, default entry_point compiler pass w…
0b48115 
…as returning too early (romaricdrigon)

This PR was merged into the 5.2 branch.

Discussion
----------

[Security] fix #39249, default entry_point compiler pass was returning too early

| Q             | A
| ------------- | ---
| Branch?       | 5.2 (bug introduced in 5.2.0, after RC2)
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #39249
| License       | MIT
| Doc PR        | N/A

A `return` instead of `continue` was making compiler pass return after the first firewall. Hence subsequents firewalls never had a default entrypoint set.
This issue would occur with all firewalls, with any type of authenticator, though I saw it first with `http_basic` - because it is a bit more opaque and harder to debug.

Commits
-------

c377805 [Security] fix #39249, default entry_point compiler pass was returning too early
@derrabus derrabus closed this as completed Dec 1, 2020
scheb added a commit to scheb/2fa that referenced this issue Dec 1, 2020
@scheb
Workaround for symfony/symfony#39249 until 5.2.1 is released
5bdcc9c
scheb added a commit to scheb/2fa that referenced this issue Dec 1, 2020
@scheb
Workaround for symfony/symfony#39249 until 5.2.1 is released
1822a29
derrabus added a commit that referenced this issue Dec 2, 2020
@derrabus
Merge branch '5.2' into 5.x
e97f9e7 
* 5.2:
  Fix rate limiter documentation
  Fix merge.
  fix lexing mapping values with trailing whitespaces
  [String] Fix Notice when argument is empty string
  [Inflector] Fix Notice when argument is empty string
  [Security] fix #39262, more defensive PasswordMigratingListener
  [Workflow] Fixed case when the marking store is not defined
  [Security] fix #39249, default entry_point compiler pass was returning too early
  Fix small typos
@fabpot fabpot mentioned this issue Dec 18, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Security Status: Needs Review
Projects
None yet
Milestone
No milestone
3 participants
@romaricdrigon @derrabus @carsonbot