From 3a85fd6d6957cb8858ff3620c446765ba306e111 Mon Sep 17 00:00:00 2001 From: taohungyang Date: Tue, 3 May 2022 18:55:17 -0400 Subject: [PATCH] Enhance config validation for bad regex in CWAgent (#459) * Check if regex is valid in GO when applying rules for log filter and timestamp. * Add test case for log filtering validation. * fmt changes. * Remove redundant test and duplicate logging * make fmt && make fmt-sh * Add test case for invalid timestamp format. --- .../clean_dedicated_host.go | 5 +- integration/pkg/tools/create_pkg.sh | 6 +- integration/pkg/tools/postinstall.sh | 12 ++-- integration/pkg/tools/preinstall.sh | 18 ++--- integration/test/agent_util.go | 7 +- .../test/cloudwatchlogs/publish_logs_test.go | 3 +- .../metrics_number_dimension_test.go | 9 +-- .../sanity/resources/verifyLinuxCtlScript.sh | 65 ++++++++++--------- packaging/debian/preinst | 12 ++-- translator/totomlconfig/toTomlConfig.go | 4 +- .../files/collect_list/collect_list.go | 7 +- .../files/collect_list/collect_list_test.go | 18 +++++ .../files/collect_list/ruleLogFilters.go | 5 ++ .../files/collect_list/ruleLogFilters_test.go | 15 +++++ .../files/collect_list/ruleTimestampFormat.go | 8 +++ .../collect_list/collectlist.go | 1 - .../collect_list/collectlist_test.go | 3 +- .../translate/logs/util/validate_retention.go | 3 +- 18 files changed, 127 insertions(+), 74 deletions(-) diff --git a/integration/clean/clean_dedicated_host/clean_dedicated_host.go b/integration/clean/clean_dedicated_host/clean_dedicated_host.go index d08d51c855..ae65d9d246 100644 --- a/integration/clean/clean_dedicated_host/clean_dedicated_host.go +++ b/integration/clean/clean_dedicated_host/clean_dedicated_host.go @@ -8,12 +8,13 @@ package main import ( "context" + "log" + "time" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/config" "github.com/aws/aws-sdk-go-v2/service/ec2" "github.com/aws/aws-sdk-go-v2/service/ec2/types" - "log" - "time" ) // Can't release a host if it was being used within the last 24 hr add 2 hr as a buffer diff --git a/integration/pkg/tools/create_pkg.sh b/integration/pkg/tools/create_pkg.sh index 4dee1a28f6..fb45f9f8ed 100644 --- a/integration/pkg/tools/create_pkg.sh +++ b/integration/pkg/tools/create_pkg.sh @@ -11,8 +11,8 @@ COMMON_CONFIG_PATH=/tmp/AmazonCWAgentPackage/opt/aws/amazon-cloudwatch-agent/etc SAMPLE_SUFFIX=SAMPLE_DO_NOT_MODIFY mv ${COMMON_CONFIG_PATH} ${COMMON_CONFIG_PATH}.${SAMPLE_SUFFIX} if [ $? -ne 0 ]; then - echo "Failed to mv common-config.toml" - exit 1 + echo "Failed to mv common-config.toml" + exit 1 fi mkdir /tmp/AmazonAgentScripts @@ -32,4 +32,4 @@ aws s3 cp ./artifact/amazon-cloudwatch-agent.pkg "s3://$1/integration-test/packa #tar -cvzf package.tar.gz manifest.yaml artifact.gz # ##upload the .pkg file created -#/usr/local/bin/aws s3 cp /tmp/package.tar.gz "s3://macos-cwagent-binaries/$AGENT_VERSION/pre-signed/package.tar.gz" --acl public-read \ No newline at end of file +#/usr/local/bin/aws s3 cp /tmp/package.tar.gz "s3://macos-cwagent-binaries/$AGENT_VERSION/pre-signed/package.tar.gz" --acl public-read diff --git a/integration/pkg/tools/postinstall.sh b/integration/pkg/tools/postinstall.sh index c89b841c64..52dbc2fe2d 100644 --- a/integration/pkg/tools/postinstall.sh +++ b/integration/pkg/tools/postinstall.sh @@ -4,15 +4,15 @@ echo "Creating a default user cwagent" COMMON_CONFIG_PATH=/opt/aws/amazon-cloudwatch-agent/etc/common-config.toml SAMPLE_SUFFIX=SAMPLE_DO_NOT_MODIFY if [ ! -f ${COMMON_CONFIG_PATH} ]; then - cp ${COMMON_CONFIG_PATH}.${SAMPLE_SUFFIX} ${COMMON_CONFIG_PATH} + cp ${COMMON_CONFIG_PATH}.${SAMPLE_SUFFIX} ${COMMON_CONFIG_PATH} fi -if [[ cwagent == `sudo dscl . -list /Users UniqueID | awk '{print $1}' | grep -w cwagent` ]]; then - echo "User already exists!" - exit 0 +if [[ cwagent == $(sudo dscl . -list /Users UniqueID | awk '{print $1}' | grep -w cwagent) ]]; then + echo "User already exists!" + exit 0 fi -LastID=`sudo dscl . -list /Users UniqueID | awk '{print $2}' | sort -n | tail -1` +LastID=$(sudo dscl . -list /Users UniqueID | awk '{print $2}' | sort -n | tail -1) NextID=$((LastID + 1)) . /etc/rc.common @@ -27,4 +27,4 @@ sudo dscl . create /Users/cwagent NFSHomeDirectory /Users/cwagent sudo createhomedir -u cwagent -c echo " " -echo "New user `sudo dscl . -list /Users UniqueID | awk '{print $1}' | grep -w cwagent` has been created with unique ID `sudo dscl . -list /Users UniqueID | grep -w cwagent | awk '{print $2}'`" \ No newline at end of file +echo "New user $(sudo dscl . -list /Users UniqueID | awk '{print $1}' | grep -w cwagent) has been created with unique ID $(sudo dscl . -list /Users UniqueID | grep -w cwagent | awk '{print $2}')" diff --git a/integration/pkg/tools/preinstall.sh b/integration/pkg/tools/preinstall.sh index 7b45777611..40b62c3121 100644 --- a/integration/pkg/tools/preinstall.sh +++ b/integration/pkg/tools/preinstall.sh @@ -3,16 +3,16 @@ COMMON_CONFIG_PATH=/opt/aws/amazon-cloudwatch-agent/etc/common-config.toml SAMPLE_SUFFIX=SAMPLE_DO_NOT_MODIFY if [ -e ${COMMON_CONFIG_PATH}.${SAMPLE_SUFFIX} -a -e ${COMMON_CONFIG_PATH} ]; then - diff ${COMMON_CONFIG_PATH}.${SAMPLE_SUFFIX} ${COMMON_CONFIG_PATH} >/dev/null 2>&1 - if [ $? -eq 0 ]; then - rm -r ${COMMON_CONFIG_PATH} - fi + diff ${COMMON_CONFIG_PATH}.${SAMPLE_SUFFIX} ${COMMON_CONFIG_PATH} >/dev/null 2>&1 + if [ $? -eq 0 ]; then + rm -r ${COMMON_CONFIG_PATH} + fi fi launchctl list com.amazon.cloudwatch.agent >/dev/null 2>&1 if [ $? -eq 0 ]; then - echo "Agent is running in the instance" - echo "Stopping the agent" - launchctl unload /Library/LaunchDaemons/com.amazon.cloudwatch.agent.plist - echo "Agent stopped" -fi \ No newline at end of file + echo "Agent is running in the instance" + echo "Stopping the agent" + launchctl unload /Library/LaunchDaemons/com.amazon.cloudwatch.agent.plist + echo "Agent stopped" +fi diff --git a/integration/test/agent_util.go b/integration/test/agent_util.go index dd10fdded9..8bc68c1349 100644 --- a/integration/test/agent_util.go +++ b/integration/test/agent_util.go @@ -9,13 +9,14 @@ package test import ( "context" "fmt" - "github.com/aws/aws-sdk-go-v2/config" - "github.com/aws/aws-sdk-go-v2/feature/ec2/imds" - "github.com/aws/aws-sdk-go-v2/service/cloudwatch" "log" "os/exec" "path/filepath" "time" + + "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/feature/ec2/imds" + "github.com/aws/aws-sdk-go-v2/service/cloudwatch" ) func CopyFile(pathIn string, pathOut string) { diff --git a/integration/test/cloudwatchlogs/publish_logs_test.go b/integration/test/cloudwatchlogs/publish_logs_test.go index cac789a2c6..37c66bcc45 100644 --- a/integration/test/cloudwatchlogs/publish_logs_test.go +++ b/integration/test/cloudwatchlogs/publish_logs_test.go @@ -8,11 +8,12 @@ package cloudwatchlogs import ( "fmt" - "github.com/aws/amazon-cloudwatch-agent/integration/test" "log" "os" "strings" + "github.com/aws/amazon-cloudwatch-agent/integration/test" + "testing" "time" ) diff --git a/integration/test/metrics_number_dimension/metrics_number_dimension_test.go b/integration/test/metrics_number_dimension/metrics_number_dimension_test.go index 162851cbbf..9ac0df0b8c 100644 --- a/integration/test/metrics_number_dimension/metrics_number_dimension_test.go +++ b/integration/test/metrics_number_dimension/metrics_number_dimension_test.go @@ -9,14 +9,15 @@ package metrics_number_dimension import ( "context" "fmt" - "github.com/aws/amazon-cloudwatch-agent/integration/test" - "github.com/aws/aws-sdk-go-v2/aws" - "github.com/aws/aws-sdk-go-v2/service/cloudwatch" - "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" "log" "strings" "testing" "time" + + "github.com/aws/amazon-cloudwatch-agent/integration/test" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/cloudwatch" + "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" ) const configOutputPath = "/opt/aws/amazon-cloudwatch-agent/bin/config.json" diff --git a/integration/test/sanity/resources/verifyLinuxCtlScript.sh b/integration/test/sanity/resources/verifyLinuxCtlScript.sh index 4f2088044e..2c324e3645 100755 --- a/integration/test/sanity/resources/verifyLinuxCtlScript.sh +++ b/integration/test/sanity/resources/verifyLinuxCtlScript.sh @@ -4,43 +4,44 @@ # SPDX-License-Identifier: MIT assertStatus() { - keyToCheck="${1:-}" - expectedVal="${2:-}" - - grepKey='unknown' - case "${keyToCheck}" in - cwa_running_status) - grepKey="\"status\"" - ;; - cwa_config_status) - grepKey="\"configstatus\"" - ;; - cwoc_running_status) - grepKey="\"cwoc_status\"" - ;; - cwoc_config_status) - grepKey="\"cwoc_configstatus\"" - ;; - *) echo "Invalid Key To Check: ${keyToCheck}" >&2 - exit 1 - ;; - esac - - result=$(/usr/bin/amazon-cloudwatch-agent-ctl -a status | grep "${grepKey}" | awk -F: '{print $2}' | sed 's/ "//; s/",//') - - if [ "${result}" = "${expectedVal}" ]; then - echo "In step ${step}, ${keyToCheck} is expected" - else - echo "In step ${step}, ${keyToCheck} is NOT expected. (actual="${result}"; expected="${expectedVal}")" - exit 1 - fi + keyToCheck="${1:-}" + expectedVal="${2:-}" + + grepKey='unknown' + case "${keyToCheck}" in + cwa_running_status) + grepKey="\"status\"" + ;; + cwa_config_status) + grepKey="\"configstatus\"" + ;; + cwoc_running_status) + grepKey="\"cwoc_status\"" + ;; + cwoc_config_status) + grepKey="\"cwoc_configstatus\"" + ;; + *) + echo "Invalid Key To Check: ${keyToCheck}" >&2 + exit 1 + ;; + esac + + result=$(/usr/bin/amazon-cloudwatch-agent-ctl -a status | grep "${grepKey}" | awk -F: '{print $2}' | sed 's/ "//; s/",//') + + if [ "${result}" = "${expectedVal}" ]; then + echo "In step ${step}, ${keyToCheck} is expected" + else + echo "In step ${step}, ${keyToCheck} is NOT expected. (actual="${result}"; expected="${expectedVal}")" + exit 1 + fi } # init step=0 aoc_user=$(cat /etc/passwd | grep aoc) if [ "${aoc_user}" = "" ]; then - echo 'aoc:x:995:991:AOC Agent:/home/aoc:/sbin/nologin' | sudo tee -a /etc/passwd + echo 'aoc:x:995:991:AOC Agent:/home/aoc:/sbin/nologin' | sudo tee -a /etc/passwd fi /usr/bin/amazon-cloudwatch-agent-ctl -a remove-config -c all -o all /usr/bin/amazon-cloudwatch-agent-ctl -a stop @@ -129,4 +130,4 @@ step=12 assertStatus "cwa_running_status" "stopped" assertStatus "cwoc_running_status" "stopped" assertStatus "cwa_config_status" "not configured" -assertStatus "cwoc_config_status" "configured" \ No newline at end of file +assertStatus "cwoc_config_status" "configured" diff --git a/packaging/debian/preinst b/packaging/debian/preinst index bdbcecae06..54abcaaefc 100755 --- a/packaging/debian/preinst +++ b/packaging/debian/preinst @@ -3,8 +3,8 @@ # SPDX-License-Identifier: MIT if [ "$1" = "upgrade" ]; then - /opt/aws/amazon-cloudwatch-agent/bin/amazon-cloudwatch-agent-ctl -a prep-restart - /opt/aws/amazon-cloudwatch-agent/bin/amazon-cloudwatch-agent-ctl -a stop + /opt/aws/amazon-cloudwatch-agent/bin/amazon-cloudwatch-agent-ctl -a prep-restart + /opt/aws/amazon-cloudwatch-agent/bin/amazon-cloudwatch-agent-ctl -a stop fi if ! grep "^cwagent:" /etc/group >/dev/null 2>&1; then @@ -18,11 +18,11 @@ if ! id cwagent >/dev/null 2>&1; then fi if ! grep "^aoc:" /etc/group >/dev/null 2>&1; then - groupadd -r aoc >/dev/null 2>&1 - echo "create group aoc, result: $?" + groupadd -r aoc >/dev/null 2>&1 + echo "create group aoc, result: $?" fi if ! id aoc >/dev/null 2>&1; then - useradd -r -M aoc -d /home/aoc -g aoc >/dev/null 2>&1 - echo "create user aoc, result: $?" + useradd -r -M aoc -d /home/aoc -g aoc >/dev/null 2>&1 + echo "create user aoc, result: $?" fi diff --git a/translator/totomlconfig/toTomlConfig.go b/translator/totomlconfig/toTomlConfig.go index 4b1dda232b..e7506cd8c7 100755 --- a/translator/totomlconfig/toTomlConfig.go +++ b/translator/totomlconfig/toTomlConfig.go @@ -4,8 +4,8 @@ package totomlconfig import ( - "log" "bytes" + "log" "github.com/aws/amazon-cloudwatch-agent/translator/translate" _ "github.com/aws/amazon-cloudwatch-agent/translator/translate/agent" @@ -61,7 +61,7 @@ func ToTomlConfig(c interface{}) string { enc := toml.NewEncoder(&buf) err := enc.Encode(val) if err != nil { - log.Panicf("Encode to a valid TOML config fails because of %v",err) + log.Panicf("Encode to a valid TOML config fails because of %v", err) } return buf.String() } diff --git a/translator/translate/logs/logs_collected/files/collect_list/collect_list.go b/translator/translate/logs/logs_collected/files/collect_list/collect_list.go index dc7a7e1235..ee0b4fe97b 100644 --- a/translator/translate/logs/logs_collected/files/collect_list/collect_list.go +++ b/translator/translate/logs/logs_collected/files/collect_list/collect_list.go @@ -5,6 +5,10 @@ package collect_list import ( "encoding/json" + "io/ioutil" + "path/filepath" + "sort" + "github.com/aws/amazon-cloudwatch-agent/translator" "github.com/aws/amazon-cloudwatch-agent/translator/context" "github.com/aws/amazon-cloudwatch-agent/translator/jsonconfig/mergeJsonRule" @@ -12,9 +16,6 @@ import ( "github.com/aws/amazon-cloudwatch-agent/translator/translate/agent" parent "github.com/aws/amazon-cloudwatch-agent/translator/translate/logs/logs_collected/files" logUtil "github.com/aws/amazon-cloudwatch-agent/translator/translate/logs/util" - "io/ioutil" - "path/filepath" - "sort" ) type Rule translator.Rule diff --git a/translator/translate/logs/logs_collected/files/collect_list/collect_list_test.go b/translator/translate/logs/logs_collected/files/collect_list/collect_list_test.go index b2308593bc..26337f3011 100644 --- a/translator/translate/logs/logs_collected/files/collect_list/collect_list_test.go +++ b/translator/translate/logs/logs_collected/files/collect_list/collect_list_test.go @@ -310,6 +310,22 @@ func TestTimestampFormat_Template(t *testing.T) { assert.Equal(t, time.Date(0, 8, 9, 20, 45, 51, 0, time.Local), parsedTime) } +func TestTimestampFormat_InvalidRegex(t *testing.T) { + translator.ResetMessages() + r := new(TimestampRegax) + var input interface{} + e := json.Unmarshal([]byte(`{ + "timestamp_format":"%Y-%m-%dT%H:%M%S+00:00" + }`), &input) + assert.Nil(t, e) + + retKey, retVal := r.ApplyRule(input) + assert.Equal(t, "timestamp_regex", retKey) + assert.Nil(t, retVal) + assert.Len(t, translator.ErrorMessages, 1) + +} + func TestMultiLineStartPattern(t *testing.T) { f := new(FileConfig) var input interface{} @@ -371,6 +387,7 @@ func TestEncoding(t *testing.T) { } func TestEncoding_Invalid(t *testing.T) { + translator.ResetMessages() f := new(FileConfig) var input interface{} e := json.Unmarshal([]byte(`{ @@ -687,6 +704,7 @@ func TestDuplicateRetention(t *testing.T) { } func TestConflictingRetention(t *testing.T) { + translator.ResetMessages() f := new(FileConfig) var input interface{} e := json.Unmarshal([]byte(`{ diff --git a/translator/translate/logs/logs_collected/files/collect_list/ruleLogFilters.go b/translator/translate/logs/logs_collected/files/collect_list/ruleLogFilters.go index 18712d2dd9..391e4ecaa7 100644 --- a/translator/translate/logs/logs_collected/files/collect_list/ruleLogFilters.go +++ b/translator/translate/logs/logs_collected/files/collect_list/ruleLogFilters.go @@ -5,6 +5,7 @@ package collect_list import ( "fmt" + "regexp" "github.com/aws/amazon-cloudwatch-agent/translator" ) @@ -37,6 +38,10 @@ func (lf *LogFilter) ApplyRule(input interface{}) (returnKey string, returnVal i translator.AddErrorMessages(GetCurPath()+FiltersSectionKey, fmt.Sprintf("Filter %s is invalid", filter)) continue } + if _, err := regexp.Compile(filterVal.(string)); err != nil { + translator.AddErrorMessages(GetCurPath()+FiltersSectionKey, fmt.Sprintf("Filter expression %s is invalid", filter)) + continue + } filterMap[FiltersExpressionSectionKey] = filterVal res = append(res, filterMap) } diff --git a/translator/translate/logs/logs_collected/files/collect_list/ruleLogFilters_test.go b/translator/translate/logs/logs_collected/files/collect_list/ruleLogFilters_test.go index 2dd3ca4dc6..1eb3f69699 100644 --- a/translator/translate/logs/logs_collected/files/collect_list/ruleLogFilters_test.go +++ b/translator/translate/logs/logs_collected/files/collect_list/ruleLogFilters_test.go @@ -62,3 +62,18 @@ func TestApplyLogFiltersRuleMissingConfigInsideFilters(t *testing.T) { assert.Nil(t, retVal) assert.Len(t, translator.ErrorMessages, 2) } + +func TestApplyLogFiltersRuleInvalidRegex(t *testing.T) { + translator.ResetMessages() + r := new(LogFilter) + var input interface{} + e := json.Unmarshal([]byte(`{ + "filters": [ + {"type": "exclude", "expression": "(?!re)"} + ] + }`), &input) + assert.Nil(t, e) + _, retVal := r.ApplyRule(input) + assert.Nil(t, retVal) + assert.Len(t, translator.ErrorMessages, 1) +} diff --git a/translator/translate/logs/logs_collected/files/collect_list/ruleTimestampFormat.go b/translator/translate/logs/logs_collected/files/collect_list/ruleTimestampFormat.go index adfac2633b..a78307c52b 100644 --- a/translator/translate/logs/logs_collected/files/collect_list/ruleTimestampFormat.go +++ b/translator/translate/logs/logs_collected/files/collect_list/ruleTimestampFormat.go @@ -4,7 +4,11 @@ package collect_list import ( + "fmt" + "regexp" "strings" + + "github.com/aws/amazon-cloudwatch-agent/translator" ) /* @@ -155,6 +159,10 @@ func (t *TimestampRegax) ApplyRule(input interface{}) (returnKey string, returnV res = strings.TrimPrefix(res, "\\s{0,1}") res = "(" + res + ")" returnKey = "timestamp_regex" + if _, err := regexp.Compile(res); err != nil { + translator.AddErrorMessages(GetCurPath()+"timestamp_format", fmt.Sprintf("Timestamp format %s is invalid", val)) + return + } returnVal = res } return diff --git a/translator/translate/logs/logs_collected/windows_events/collect_list/collectlist.go b/translator/translate/logs/logs_collected/windows_events/collect_list/collectlist.go index 425cb1c46d..e9f7888dab 100644 --- a/translator/translate/logs/logs_collected/windows_events/collect_list/collectlist.go +++ b/translator/translate/logs/logs_collected/windows_events/collect_list/collectlist.go @@ -12,7 +12,6 @@ import ( parent "github.com/aws/amazon-cloudwatch-agent/translator/translate/logs/logs_collected/windows_events" logUtil "github.com/aws/amazon-cloudwatch-agent/translator/translate/logs/util" "github.com/aws/amazon-cloudwatch-agent/translator/util" - ) type Rule translator.Rule diff --git a/translator/translate/logs/logs_collected/windows_events/collect_list/collectlist_test.go b/translator/translate/logs/logs_collected/windows_events/collect_list/collectlist_test.go index efbf8f4874..6b8eb96df7 100644 --- a/translator/translate/logs/logs_collected/windows_events/collect_list/collectlist_test.go +++ b/translator/translate/logs/logs_collected/windows_events/collect_list/collectlist_test.go @@ -5,9 +5,10 @@ package collectlist import ( "encoding/json" - "github.com/aws/amazon-cloudwatch-agent/translator" "testing" + "github.com/aws/amazon-cloudwatch-agent/translator" + "github.com/stretchr/testify/assert" ) diff --git a/translator/translate/logs/util/validate_retention.go b/translator/translate/logs/util/validate_retention.go index 649b09ab3e..e408917e31 100644 --- a/translator/translate/logs/util/validate_retention.go +++ b/translator/translate/logs/util/validate_retention.go @@ -2,8 +2,9 @@ package util import ( "fmt" - "github.com/aws/amazon-cloudwatch-agent/translator" "strings" + + "github.com/aws/amazon-cloudwatch-agent/translator" ) const (