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

Fix #8168 - AWS secrets should not be exposed while running tests #8169

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mpryc
Copy link

@mpryc mpryc commented Aug 30, 2024

Thank you for contributing to Velero!

Please add a summary of your change

Changed the tests to use mocked function that will not read actual secrets from env variables nor AWS config file that may be on the system that is running tests.

As a second guard against exposed secrets comparison for the values does not shows the actual values for the AWS data. This is to prevent situation where programming error may still allow the test to read AWS config/env variables instead of using mocked function.

This change also fixes the test run on the system with AWS creds where the test was failing.

Does your change fix a particular issue?

Fixes #8168 8168

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@mpryc
Copy link
Author

mpryc commented Aug 30, 2024

/kind changelog-not-required

@github-actions github-actions bot added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Aug 30, 2024
Copy link
Contributor

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

Made an attempt at simplifying here WDYT?
No mocking, less complicated compares.

@mpryc
Copy link
Author

mpryc commented Aug 30, 2024

Made an attempt at simplifying here WDYT? No mocking, less complicated compares.

this won't solve problem where test is ran on the system where some AWS configs are defined, e.g. before sending PR:

$ cd pkg/repository/config/
$ export AWS_ACCESS_KEY_ID=exposed_creds
$ go test ./...


$ cat ~/.aws/credentials 
[default]
aws_access_key_id = exposed_key
aws_secret_access_key = exposed_access_key
$ go test ./...

@kaovilai
Copy link
Contributor

We should make unit tests not read from local credentials at all IMO.

@mpryc
Copy link
Author

mpryc commented Aug 30, 2024

As discussed offline on Slack with @kaovilai there may be an issue with running tests in parallel with this proposed change.

The TB.SetEnv function disables the test to run in parallel as written in the https://go-review.googlesource.com/c/go/+/260577.

I would like to know if that is an issue here?

@kaovilai
Copy link
Contributor

probably not an issue. When Setenv is used, isEnvSet is true, and any call to t.Parallel() after will panic.

	if t.isEnvSet {
		panic("testing: t.Parallel called after t.Setenv; cannot set environment variables in parallel tests")
	}

When we make tests more parallel we'll figure it out then. This is still an enhancement from before.. better an enhancement than no enhancement.

@mpryc
Copy link
Author

mpryc commented Aug 31, 2024

@kaovilai I could first check if those vars are set and only then use them, how about that? This will mitigate the problem a bit because in the container run scenario it should never execute TB.SetEnv

@kaovilai
Copy link
Contributor

These are internal to go testing package and the check is already done for you if you check the link

Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 59.09%. Comparing base (3408ffe) to head (6d0f726).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
pkg/repository/config/aws.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8169      +/-   ##
==========================================
+ Coverage   59.05%   59.09%   +0.03%     
==========================================
  Files         364      364              
  Lines       30324    30342      +18     
==========================================
+ Hits        17909    17931      +22     
+ Misses      10972    10968       -4     
  Partials     1443     1443              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blackpiglet blackpiglet added kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes and removed kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes labels Sep 2, 2024
@kaovilai
Copy link
Contributor

kaovilai commented Sep 3, 2024

please fix lint

Copy link
Contributor

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

lint test fails

…ng tests

Changed the tests to use mocked function that will not read actual
secrets from env variables nor AWS config file that may be
on the system that is running tests.

As a second guard against exposed secrets comparison for the values
does not shows the actual values for the AWS data. This is to prevent
situation where programming error may still allow the test to read
AWS config/env variables instead of using mocked function.

Signed-off-by: Michal Pryc <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-unit-tests kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS secrets may be exposed when running repository/config/aws tests in the CI workflows
4 participants