From de936fd8f43d2a633e50555c0392805a76e15dc3 Mon Sep 17 00:00:00 2001 From: Daniel Mikusa Date: Tue, 12 Oct 2021 16:28:02 -0400 Subject: [PATCH] Relax escaping of values to enable limited access to the shell for AppDynamnics config [Previous PRs escaped configuration values for AppD](https://github.com/cloudfoundry/java-buildpack/pull/870). This was done to support things like names with spaces and other characters that, if used, would result in a broken start command. The PR broke a use case which was documented for the node or tier name. In some cases, you may want to set the node or tier name to a dynamic value that is loaded at runtime so that you can incorporate things like the application instance index. For example `DCX:$(echo $VCAP_APPLICATION | jq -r '.application_name'):$(echo $VCAP_APPLICATION | jq -r '.instance_index')`. This new PR will use `Shellwords.escape(..)` on the value for all properties unless that property value contains what looks like a subshell `$(..)` or environment variable `${..}` reference. If it looks like a subshell or env variable is being referenced, we will not escape but just wrap the value in escaped quotes. We wrap it in escaped quotes in case the shell variable or subshell returns something which includes spaces. This is not perfect though, and you need to be careful if using subshell/env variables, you should ensure the output is properly escaped. For example: - `DCX:$(echo $VCAP_APPLICATION | jq -r '.application_name'):$(echo $VCAP_APPLICATION | jq -r '.instance_index')` - `$(echo 'Hello world!') and stuff` becomes `\"$(echo 'Hello world!') and stuff\"` - `--> ${SOME_VAR} <--` becomes `\"--> ${SOME_VAR} <--\"` Signed-off-by: Daniel Mikusa --- docs/framework-app_dynamics_agent.md | 2 +- .../framework/app_dynamics_agent.rb | 26 +++++++----- .../framework/app_dynamics_agent_spec.rb | 40 +++++++++++++++++++ 3 files changed, 58 insertions(+), 10 deletions(-) diff --git a/docs/framework-app_dynamics_agent.md b/docs/framework-app_dynamics_agent.md index c0f76c135c..e1ec4a53e0 100644 --- a/docs/framework-app_dynamics_agent.md +++ b/docs/framework-app_dynamics_agent.md @@ -26,7 +26,7 @@ When binding AppDynamics using a user-provided service, it must have name or tag | `node-name` | (Optional) the application's node name | `tier-name` | (Optional) the application's tier name -To provide more complex values such as the `tier-name`, using the interactive mode when creating a user-provided service will manage the character escaping automatically. For example, the default `tier-name` could be set with a value of `Tier-$(expr "$VCAP_APPLICATION" : '.*instance_index[": ]*\([[:digit:]]*\).*')` to calculate a value from the Cloud Foundry instance index. +To provide more complex values such as the `tier-name`, using the interactive mode when creating a user-provided service will manage the character escaping automatically. For example, the default `tier-name` could be set with a value of `Tier-$(expr "${VCAP_APPLICATION}" : '.*instance_index[": ]*\([[:digit:]]*\).*')` to calculate a value from the Cloud Foundry instance index. **Note:** Some credentials were previously marked as "(Optional)" as requirements have changed across versions of the AppDynamics agent. Please see the [AppDynamics Java Agent Configuration Properties][] for the version of the agent used by your application for more details. diff --git a/lib/java_buildpack/framework/app_dynamics_agent.rb b/lib/java_buildpack/framework/app_dynamics_agent.rb index dada5ce87b..f9d41c0198 100644 --- a/lib/java_buildpack/framework/app_dynamics_agent.rb +++ b/lib/java_buildpack/framework/app_dynamics_agent.rb @@ -86,35 +86,35 @@ def supports? private_constant :FILTER def application_name(java_opts, credentials) - name = Shellwords.escape(@application.details['application_name']) + name = escape(@application.details['application_name']) name = @configuration['default_application_name'] if @configuration['default_application_name'] - name = Shellwords.escape(credentials['application-name']) if credentials['application-name'] + name = escape(credentials['application-name']) if credentials['application-name'] java_opts.add_system_property('appdynamics.agent.applicationName', name.to_s) end def account_access_key(java_opts, credentials) account_access_key = credentials['account-access-key'] || credentials.dig('account-access-secret', 'secret') - account_access_key = Shellwords.escape(account_access_key) + account_access_key = escape(account_access_key) java_opts.add_system_property 'appdynamics.agent.accountAccessKey', account_access_key if account_access_key end def account_name(java_opts, credentials) account_name = credentials['account-name'] - java_opts.add_system_property 'appdynamics.agent.accountName', Shellwords.escape(account_name) if account_name + java_opts.add_system_property 'appdynamics.agent.accountName', escape(account_name) if account_name end def host_name(java_opts, credentials) host_name = credentials['host-name'] raise "'host-name' credential must be set" unless host_name - java_opts.add_system_property 'appdynamics.controller.hostName', Shellwords.escape(host_name) + java_opts.add_system_property 'appdynamics.controller.hostName', escape(host_name) end def node_name(java_opts, credentials) name = @configuration['default_node_name'] - name = Shellwords.escape(credentials['node-name']) if credentials['node-name'] + name = escape(credentials['node-name']) if credentials['node-name'] java_opts.add_system_property('appdynamics.agent.nodeName', name.to_s) end @@ -130,15 +130,15 @@ def ssl_enabled(java_opts, credentials) end def tier_name(java_opts, credentials) - name = Shellwords.escape(@application.details['application_name']) + name = escape(@application.details['application_name']) name = @configuration['default_tier_name'] if @configuration['default_tier_name'] - name = Shellwords.escape(credentials['tier-name']) if credentials['tier-name'] + name = escape(credentials['tier-name']) if credentials['tier-name'] java_opts.add_system_property('appdynamics.agent.tierName', name.to_s) end def unique_host_name(java_opts) - name = Shellwords.escape(@application.details['application_name']) + name = escape(@application.details['application_name']) name = @configuration['default_unique_host_name'] if @configuration['default_unique_host_name'] java_opts.add_system_property('appdynamics.agent.uniqueHostId', name.to_s) @@ -180,6 +180,14 @@ def save_cfg_file(file, conf_file) FileUtils.cp_r file, target_directory + '/conf/' + conf_file end end + + def escape(value) + if /\$[\(\{][^\)\}]+[\)\}]/ =~ value + "\\\"#{value}\\\"" + else + Shellwords.escape(value) + end + end end end end diff --git a/spec/java_buildpack/framework/app_dynamics_agent_spec.rb b/spec/java_buildpack/framework/app_dynamics_agent_spec.rb index f8f988d567..cecf5d2b1e 100644 --- a/spec/java_buildpack/framework/app_dynamics_agent_spec.rb +++ b/spec/java_buildpack/framework/app_dynamics_agent_spec.rb @@ -92,6 +92,26 @@ end end + context do + let(:credentials) { super().merge 'tier-name' => '--> ${SOME_VAR} <--' } + + it 'adds tier_name from credentials with shell variable in it to JAVA_OPTS if specified' do + component.release + + expect(java_opts).to include('-Dappdynamics.agent.tierName=\"--> ${SOME_VAR} <--\"') + end + end + + context do + let(:credentials) { super().merge 'tier-name' => '$(echo \'Hello World!\') and stuff' } + + it 'adds tier_name from credentials with subshell in it to JAVA_OPTS if specified' do + component.release + + expect(java_opts).to include('-Dappdynamics.agent.tierName=\"$(echo \'Hello World!\') and stuff\"') + end + end + context do let(:credentials) { super().merge 'application-name' => 'another-test application-name' } @@ -102,6 +122,26 @@ end end + context do + let(:credentials) { super().merge 'application-name' => '$(echo \'Hello World!\') and stuff' } + + it 'adds application_name from credentials with subshell in value to JAVA_OPTS if specified' do + component.release + + expect(java_opts).to include('-Dappdynamics.agent.applicationName=\"$(echo \'Hello World!\') and stuff\"') + end + end + + context do + let(:credentials) { super().merge 'application-name' => 'Name ${MY_APP_NAME}' } + + it 'adds application_name from credentials with env variable in value to JAVA_OPTS if specified' do + component.release + + expect(java_opts).to include('-Dappdynamics.agent.applicationName=\"Name ${MY_APP_NAME}\"') + end + end + context do let(:configuration) do { 'default_tier_name' => nil,