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

support env var from report yaml #3188

Merged
merged 17 commits into from
Jan 31, 2023

Conversation

xm1k3
Copy link
Contributor

@xm1k3 xm1k3 commented Jan 12, 2023

Proposed changes

This feature allows for the updating of configuration values in a struct based on a YAML file.
The struct is populated with the values from the YAML file, and then a recursive function is used to check all fields of the struct and replace them with corresponding environment variable values.

For example, given the following YAML file:

github:
  username: $GITHUB_USER
  owner: $GITHUB_OWNER
  token: $GITHUB_TOKEN
  project-name: $GITHUB_PROJECT
  issue-label: $ISSUE_LABEL
  severity-as-label: false

The struct would be populated with the values from the file, and then the recursive function would check each field and replace it with the corresponding environment variable value, if it exists.

This allows for easy management of configuration values, as the values can be easily updated in the environment without needing to change the code.

Technical Implementation

The struct is populated using the yaml package and the fields are updated using the reflect package. The function AssignEnvVarsToFields is used to iterate through all the fields of the struct, and Elem() and Set() functions are used to access and update the values respectively.

It's important to check if the field is a pointer and if it's nil before accessing its value. The function IsNil() can be used for this check.

Checklist

  • Pull request is created against the dev branch
  • All checks passed (lint, unit/integration/regression tests etc.) with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@xm1k3 xm1k3 self-assigned this Jan 12, 2023
@xm1k3 xm1k3 added the Type: Enhancement Most issues will probably ask for additions or changes. label Jan 12, 2023
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

reporting what discussed via DM:

  • After analysis yaml unmarshaling doesn't seem a good fit, so we reverted to original proposal from @xm1k3 about using reflect

Todo:

  • Nil pointer improvements
  • Only string leaf fields having a value similar to $name and yaml named tag must be considered and replaced with env var (eg yaml:"-" must be ignored and not walked recursively)
  • Remove debug code
  • Add tests covering various cases like:
    • fields with existing/non-existing variables name or values starting with $ but being static values
    • Nested structs
    • Nil pointers

Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

  • One level struct is not working:
type TestStruct1 struct {
	A      string       `yaml:"a"`
	Struct *TestStruct2 `yaml:"b"`
}

type TestStruct2 struct {
	B string `yaml:"b"`
}

func main() {
	test := &TestStruct1{
		A: "$AAAA",
		Struct: &TestStruct2{
			B: "$test2",
		},
	}

	os.Setenv("AAAA", "testaaaa")
	os.Setenv("test2", "testtest")

	val := reflect.ValueOf(*test)
	assignEnvVarToReportingOpt(val)
	log.Println(test.A)
	log.Println(test.Struct.B)
}
// $ go run .
// 2023/01/18 21:41:43 $AAAA => testaaaa expected
// 2023/01/18 21:41:43 testtest
  • The function is aimed to be generic, so the tests can also use generic structs as in the previous example.
  • The test related to nested structs is missing

v2/internal/runner/runner.go Outdated Show resolved Hide resolved
v2/pkg/reporting/reporting.go Outdated Show resolved Hide resolved
@tarunKoyalwar tarunKoyalwar removed their request for review January 20, 2023 10:50
@xm1k3 xm1k3 requested a review from Mzack9999 January 24, 2023 10:01
@@ -785,3 +788,32 @@ func (r *Runner) SaveResumeConfig(path string) error {

return os.WriteFile(path, data, os.ModePerm)
}

// replace $VAR_EXAMPLE with the correct variable in os ENV
Copy link
Member

Choose a reason for hiding this comment

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

Please update the comment with a generic description of what the function does (eg. expands string leafs ... etc)

v2/internal/runner/runner.go Outdated Show resolved Hide resolved
v2/internal/runner/runner_test.go Outdated Show resolved Hide resolved
@xm1k3 xm1k3 requested a review from Mzack9999 January 30, 2023 17:17
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

  • Add missing function description/comments
  • Remove YAML unmarshaling from Walk&Set tests.
    For example, the following:
data := `
github:
  username: $GITHUB_USER
  owner: $GITHUB_OWNER
  token: $GITHUB_TOKEN
  project-name: $GITHUB_PROJECT
  issue-label: $ISSUE_LABEL
  severity-as-label: false`

	header := http.Header{}
	header.Add("test", "test")

	reportingOptions := &reporting.Options{
		HttpClient: &retryablehttp.Client{
			HTTPClient: &http.Client{
				Transport: &http.Transport{
					ProxyConnectHeader: header,
				},
			},
		},
	}
	err := yamlwrapper.DecodeAndValidate(strings.NewReader(data), reportingOptions)
	require.Nil(t, err)

and can be replaced with the equivalent:

	type B struct {
		A string `yaml:"a"`
	}
	type A struct {
		A string `yaml:"a"`
		B *B     `yaml:"b"`
		C string
	}
	test := &A{
		A: "$GITHUB_USER",
		B: &B{
			A: "$GITHUB_USER",
		},
		C: "$GITHUB_USER",
	}

	os.Setenv("GITHUB_USER", "testuser")

	Walk(test, expandEndVars)
	assert.Equal(t, "testuser", test.A)
	assert.Equal(t, "testuser", test.B.A)
	assert.Equal(t, "$GITHUB_USER", test.C)

We just need to simulate exported fields of type string, nested at any level, hierarchically tagged with yaml

@xm1k3 xm1k3 requested a review from Mzack9999 January 30, 2023 22:26
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

Investigate why validation was removed (e.g. interferes with $ENV variable expansion?)

v2/internal/runner/runner.go Outdated Show resolved Hide resolved
@xm1k3 xm1k3 requested a review from Mzack9999 January 31, 2023 09:38
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

  • Implementation: lgtm
  • Some tests are failing. It's unclear if it's related to the chance of the first uppercase letter to lowercase in integration_tests/test-issue-tracker-config2.yaml or some pending issues (e.g. fix nuclei panic with ratelimit v0.0.5 #3257), please ensure it's unrelated to the former (I don't think so as the name in the YAML file seems to match the one in the struct tag, anyway it's worth a second check)

@xm1k3
Copy link
Contributor Author

xm1k3 commented Jan 31, 2023

After doing the necessary tests, I see that there are no problems in keeping uppercase/lowercase letters for the yaml file integration_tests/test-issue-tracker-config2.yaml

@xm1k3 xm1k3 requested a review from Mzack9999 January 31, 2023 15:58
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

lgtm - Later on the whole Walk core should be moved to https://github.com/projectdiscovery/utils/tree/main/reflect

@xm1k3 please rebase onto dev (panic should be fixed in #3257) and check if test errors persist

@xm1k3 xm1k3 force-pushed the issue-3015-support-env-var-from-report-yaml branch from d18d837 to f6a5085 Compare January 31, 2023 17:23
@ehsandeep ehsandeep merged commit a81c754 into dev Jan 31, 2023
@ehsandeep ehsandeep deleted the issue-3015-support-env-var-from-report-yaml branch January 31, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for reading keys from environment variables in report config file
3 participants