Evaluate ERB syntax in FileDataSource #189
Conversation
|
We'd like to find out more about why this is necessary to you. Although ERB and similar "embed some executable code in a data file" mechanisms have been widely used in Ruby, we have concerns about adding support for it in the SDK— both from a general security point of view, and because it is pretty far outside the intended purpose of the file data source feature which was meant as a way to fully describe an environment in a file. Why isn't it possible to simply generate the file yourself at startup time? |
|
I fully see how this might not be the intended use of the SDK, but not how it's a security risk. It's game over if someone malicious gets access to change an EVAR. My main goal is to replace our current feature flipping library and begin using Launch Darkly SDK ahead of time as cross-team training and standing up the dashboard may take weeks to put together. When we're finally ready to adopt it across the organization, we can use the configuration file as a guide for what flags are in place, configure them, drop in the SDK key and be off to the races. This is basically generating the file myself at startup time by using environment variable. The fallback flags file is packaged in deployment so I can't update it without a costly redeploy. If there's another way to feed dynamic properties into the configuration without being connected I'm all for that, but essentially this enables a true offline mode where I can configure per environment. Again, open to alternatives if there are any. In general, I'd like to use this SDK with projects where we never intend to stand up a remote configuration as it's more powerful than standard Feature Flipping gems, but if the only options are phone home or redeploy it's not an option. |
I don't quite understand your point there. In the simple ERB example you showed, your code is only reading an environment variable. But that's not the only kind of code that could be executed via ERB. Here's the security issue as I understand it; please correct me if I'm mistaken. Previously, loading a YAML file into the SDK could not ever cause anything to happen other than 1. it loads some flag configurations or 2. it fails because it's invalid. So the file itself was not an attack vector that could affect anything other than those two behaviors: if you were copying that file in from some other location, an attacker with write access to the file in that location could not do anything other than change the flag values or make the application fail to load flags. But if ERB is enabled, this file is now a vector for code injection. You can set sandboxing parameters in ERB to make it safer, but the SDK has no way to know if it's been configured that way. In a typical use case of ERB where the template file is part of the source distribution, that may not be a big deal, because then anyone who could modify that file could modify any of the application code anyway. But with the SDK's file data source feature it's quite common for data files to be located somewhere outside the application. |
|
So, it's one thing to say that the specific way you are using the feature there is safe enough, but if we're going to enable this behavior in general then we are enabling it for everyone and they may or may not be using files in that way. A customer who uses ERB for other purposes within their Rails app does not necessarily want this behavior enabled in an SDK file. That's why at the very least I would make this an opt-in feature, not something that is automatically on just because the gem is present. |
|
There may be another option based on a feature that's under development now. Some of the other SDKs have a fixture that injects flag data programmatically from application code, rather than from a file. It's not available in Ruby yet, but it's in the works. To get an idea of what the usage would be, see the API docs for the Java version. Assuming the Ruby SDK API ends up looking more or less the same, this would mean that for the equivalent of your file example above, you would have a piece of configuration code like this when setting up the SDK (instead of the code you're currently using to enable the file data source): test_data = LaunchDarkly::Integrations::TestData.new_data_source
test_data.update(test_data.flag("my-feature-flag").
variation(ENV['FEATURE_MY_FEATURE_FLAG'] == 'true')) # repeat for each flag
sdk_config.data_source = test_dataThis would also be somewhat more convenient than the file data source if you ever want to configure a flag with not just a single value, but for instance a list of specific user keys who should get a different value; you can sort of do that with a file but it would require writing out the entire flag data structure in YAML. |
|
It's a widely accepted practice to do this type of thing in the Rails. Data/Config is often augmented by code and code is often driven by data. You can use ERB code in Knowingly committing executable code into your application is not an attack vector for injecting code. Particularly ironic is you have a feature that literally has a fixture inject flag data programmatically for the Java SDK, although from the name it might be for tests. The fear of someone changing the ENV in a way that breaks the system is not increased by locking the file down statically. One could just as easily configure things incorrectly in the LaunchDarkly dashboard and break the system. I'm actually more concerned about relying completely the API's availability than I will just do the Rails'y thing, monkey patch it and move on. If there are changes I could make to this PR so that it would be merged, it i'd be happy to but I'll close it for now. |
|
@jhirn Again, I am not only talking about your use case. I understand that in the setup you want to do, the data files are being committed as part of your source. I thought I already made it clear that I understand that kind of practice and why it is OK, but that is not the only way people are currently using the file data source feature. We have previously been able to guarantee that importing a YAML file that was not deployed in your source code was safe, and that is a thing people do. Please reread the last paragraph of this comment. |
|
Also, I don't understand what your point is in this sentence at all - why is this "ironic"? I feel like we are talking past each other on multiple levels.
|
detect http/https proxy env vars when creating HTTP clients
Requirements
Describe the solution you've provided
Hello. In transitioning to LaunchDarkly, it's essential we continue to support existing flags/environments. There are also situations where we need to spin up temporary environments and for that we'd like to avoid ephemeral LD environments.
The file fall back works perfectly fine for these situations except when we want to toggle something via EVAR. Gems such as Feature allow ERB syntax in yaml to allow this. Many Rails tools I actually just assume i can use ERB syntax or at least rename a file
config.yml.erbThis PR adds ERB evaluation when ERB is available (i.e. most rails projects) allowing one to do something such as:
I'm using this patch locally without issues and have added tests. It didn't feel right to add a configuration flag as not all languages have ERB and it's almost assumed in most Rails based configuration tools to have ERB be evaluated. I can contribute a documentation change if required for merging.
Also if there's another way to accomplish this that I missed, please point me there. Not married to this approach but definitely need something for a true disconnected option.