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

[Security] Add migrating encoder configuration #34139

Merged
merged 1 commit into from
Nov 9, 2019

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Oct 27, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I don't think the migrating_from option is needed at all: instead, we should always wire a MigratingPasswordEncoder when Sodium/Native encoder variants are selected.

There is one thing that is not documented correctly: when auto is selected, hash_algorithm is used as the digest algo too. But this should be fixed on 4.3, so it's unrelated to this PR.

@teohhanhui
Copy link
Contributor

I don't think the migrating_from option is needed at all: instead, we should always wire a MigratingPasswordEncoder when Sodium/Native encoder variants are selected.

Please don't take control away from the user (again)? 🙏

@nicolas-grekas
Copy link
Member

Please don't take control away from the user (again)? pray

🤦‍♂️ which control would this take away from users?

@chalasr
Copy link
Member Author

chalasr commented Oct 30, 2019

There is one thing that is not documented correctly: when auto is selected, hash_algorithm is used as the digest algo too. But this should be fixed on 4.3, so it's unrelated to this PR.

That's where I disagree. Let's say one have sha1 hashes in the user storage, this is the config required to make migrations work:

App\User:
    algorithm: bcrypt
    hash_algorithm: sha1
    encode_as_base64: false # also required because of the old encoder config

It's really not obvious what this means to me, it just feels like hijacking existing options to achieve what a new clearly-named option would do: migrate from a specific algo.

Hence I think we should rather improve the situation than documenting what currently kinda works by chance.
And I'd even say this PR is not enough yet: we need the ability to pass the required options per legacy algo you want to migrate from, so that things are clear just by looking at the config.

Built-in password migrations are great to make developers able to move forward and stop relying on old algos, I think that the support for this feature should be exhaustive and obvious for anyone reading the config, which is definitely not the case right now.

@chalasr chalasr force-pushed the sec-migrating branch 3 times, most recently from 4993792 to b14b993 Compare October 31, 2019 00:09
@chalasr
Copy link
Member Author

chalasr commented Oct 31, 2019

PR updated, the option now accepts encoder names in addition to algo names:
To migrate from an old encoder which defines some custom values for options, make it a named encoder (if it is not already) and pass its name to the migrate_from list. Same if it's a custom encoder service.

encoders:
    foo:
         id: App\FooEncoder
    digest_sha256:
        algorithm: sha256
        encode_as_base64: false
     App\User:
         algorithm: argon2id
         migrate_from: 
             - foo
             - digest_sha256
             - bcrypt # creates an encoder with default config options

@chalasr
Copy link
Member Author

chalasr commented Nov 9, 2019

@nicolas-grekas as discussed, sodium and native encoders are now always migrating.
PR ready

@chalasr chalasr force-pushed the sec-migrating branch 2 times, most recently from ec235d3 to 6b2df8c Compare November 9, 2019 10:04
@fabpot
Copy link
Member

fabpot commented Nov 9, 2019

Thank you @chalasr.

fabpot added a commit that referenced this pull request Nov 9, 2019
This PR was merged into the 4.4 branch.

Discussion
----------

[Security] Add migrating encoder configuration

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        |

Commits
-------

80955be [Security] Add migrating encoder configuration
@fabpot fabpot merged commit 80955be into symfony:4.4 Nov 9, 2019
@chalasr chalasr deleted the sec-migrating branch November 9, 2019 12:12
This was referenced Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants