Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[FrameworkBundle] Allow secrets vaults to be used directly outside Symfony#47445

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:6.2fromAndreasA:secrets-not-internal
Sep 12, 2022
Merged

[FrameworkBundle] Allow secrets vaults to be used directly outside Symfony#47445

nicolas-grekas merged 1 commit intosymfony:6.2fromAndreasA:secrets-not-internal
Sep 12, 2022

Conversation

@AndreasA
Copy link
Contributor

@AndreasAAndreasA commentedSep 1, 2022
edited
Loading

QA
Branch?6.2
Bug fix?no
New feature?yes
Deprecations?no
Tickets
LicenseMIT
Doc PR

This PR removes the internal tag from the secrets vaults.

This allows developers to use them directly without the need to use Symfony config etc. They still have to install the whole framework bundle but that just means an overhead during download, they are still able to directly instantiate the vault usingnew SodiumVault(....) for instance.

Furthermore, by removing internal from the AbstractVault, it is possible to add a custom vault and specify it replacing thesecrets.vault service for instance.

See ticket:#44151
and previous PR:#45571

@AndreasA
Copy link
ContributorAuthor

@nicolas-grekas as previously discussed. This should be everything.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

can you add a line in the changelog file of the bundle please?

@AndreasA
Copy link
ContributorAuthor

can you add a line in the changelog file of the bundle please?

Done. Hopefully the changelog entry is fine this way?

@AndreasA
Copy link
ContributorAuthor

@nicolas-grekas Do I need to change anything else because the bot added the "needs work" tag?

@nicolas-grekasnicolas-grekas changed the titleAllow secrets vaults to be used directly outside Symfony (removed internal PHPDoc).Allow secrets vaults to be used directly outside SymfonySep 12, 2022
@carsonbotcarsonbot changed the titleAllow secrets vaults to be used directly outside Symfony[FrameworkBundle] Allow secrets vaults to be used directly outside SymfonySep 12, 2022
@nicolas-grekas
Copy link
Member

Thank you@AndreasA.

@nicolas-grekasnicolas-grekas merged commit022b58f intosymfony:6.2Sep 12, 2022
@chalasr
Copy link
Member

Sorry to comment on a closed ticket but I have to say that I'm personally not very satisfied with the path we took here:

  • A bundle isn't meant to provide logic or any public code, should rather just be glue
  • What if I don't want secrets e.g. because I have my own setup for those? Having it always enabled kinda goes against some of the motivations behind flex
  • The cost of moving these classes from internal to a new component is just the same as opening these classes modulo the cons listed above

All in all, I fail to see what prevents us from opening this API the same way we usually do it for other parts of the framework.
I hope we can reconsider before it gets released.

@nicolas-grekas
Copy link
Member

I can reply for myself: I rejected#45571 because I've no idea which part of which domain those classes would cover. Creating a new component sends a message that this new lib will solve a useful problem. But I honestly don't know the boundaries of a new symfony/secrets component. Those classes look a bit "random" to me from a domain coverage pov. I preferred not opening a pandora box by creating a component that would raise false expectations - thus be deceptive. Also, I don't plan to invest my time into defining+implementing a "secrets" domain.
On the other side, if making those classes non-internal helps some ppl, I've no objection to do so because I know we can deal with the BC promise quite easily on them.

@AndreasA
Copy link
ContributorAuthor

AndreasA commentedSep 15, 2022
edited
Loading

Personally, I would also have preferred a component but this way there are already some possibilities:

  • I know shop system that uses Symfony Framework and the bundle loading for plugins. However, to determine active plugins they initialize the database connection before the kernel is finished loading. They provide an option to provide the db URL through external code. So by the SodiumVault etc. not being internal, I can still use them to provide the DB URL.
  • With AbstractVault not being internal, it would be possible to replace it with a custom implementation, e.g. to load secrets from 1Password or similar. Though it probably will be easier to use the default vault and provide the secrets key using 1Password in such a scenario but in theory it would be possible.
  • The secret vaults can be used outside of Symfony altogether, even if the overhead of installing unnecessary stuff is necessary.
    • However, one could relatively easily automatically create a custom package automatically through git subtree, so that would be gone as well.

One thing that I am not sure about is, if the commands should also remove the@internal tag, though I don't think it is necessary as those are quite specifc to the framework bundle implementation and would make BC promise probably more tricky.

@nicolas-grekas
Copy link
Member

load secrets from 1Password or similar

It could be interesting to contribute those back here. If we have many of them, then maybe a new component would make sense, as a generic way to plug in specific vault implementations.

create a custom package automatically through git subtree

if that's the plan, then we can keep the@internal annotation!

@AndreasA
Copy link
ContributorAuthor

AndreasA commentedSep 15, 2022
edited
Loading

@nicolas-grekas regarding the custom package, that is currently not the plan - at least not by me 😄

Just mentioned it. And I think it would be easier without internal because then it is BC safe and easier to extract new versions, with internal it might break in the future 😄 but not that important.

The other things though are still valid. and they do help. As mentioned in the worst case, one can now use the vaults by requiring the framework-bundle, even if they only use the vaults.

Regarding additional vaults, not currenlty planned, but maybe in the future. In which case I might create a PR, once it works.

@fabpotfabpot mentioned this pull requestOct 24, 2022
@faizanakram99
Copy link
Contributor

@AndreasA

We are also loading secrets from aws secrets manager but it is encapsulated in a service which is loaded by anEnvVarLoader

See

/**
* EnvVarLoaderInterface objects return key/value pairs that are added to the list of available env vars.
*
* @author Nicolas Grekas <p@tchwork.com>
*/
interface EnvVarLoaderInterface
{
/**
* @return string[] Key/value pairs that can be accessed using the regular "%env()%" syntax
*/
publicfunctionloadEnvVars():array;
}

@AndreasA
Copy link
ContributorAuthor

@faizanakram99 I know that it is possible to use env var processors to do so, I am doing something similar in another project. I just think it would be nicer to use a corresponding vault as that would also allow thelist command to show those secrets etc. but yes strictly speaking it isn't necessary.

Thanks for the tip anyway.

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

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@artyuumartyuumartyuum requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

6 participants

@AndreasA@nicolas-grekas@chalasr@faizanakram99@artyuum@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp