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

Add support for masking secret values in a GHA context #299

Merged
merged 5 commits into from
Dec 19, 2024

Conversation

fheinecke
Copy link
Contributor

This adds support for masking secret values when the tool is ran in a GHA pipeline.

This can be broken down into two parts:

  • Rather than treating values as strings, they're now treated as a new values.Value type. This type contains not only the raw value, but also an indication as to whether or not it should be treated as a secret. All values from SOPS YAML files are now marked as ShouldMask.
  • A new GHA-specific writer was added that outputs secret values with the ::add-mask:: prefix. Per the docs, this prefix causes the subsequent string (until line end) to be masked from GHA logs.

Caution

It is security-critical that all corner cases are covered. Please check the tests thoroughly, specifically for the SOPS loader and the GHA mask writer.

This is a large PR by line count, but the vast majority of changes are either:

  • tests (this is critical to prevent leaking values so the tests are pretty extensive)
  • changing the type used for actual values from a string to a new struct (this touches a lot of places but is logically simple)

@fheinecke fheinecke requested a review from a team as a code owner December 17, 2024 01:00
Copy link
Contributor

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Code-wise looks fine but my concern is this is yet another GHA-specific thing we now make use of. Is there a simpler way to accomplish this without having to rely on GHA-specific logic to mask things? For example, can we just not log sensitive data at all?

@fheinecke
Copy link
Contributor Author

Is there a simpler way to accomplish this without having to rely on GHA-specific logic to mask things?

No, there is no universal way to ensure that secret values to not end up in logs. GHA handles this for us when using GHA environment secrets, but GHA needs to be told about secret values loaded at runtime so that it can omit them from logs.

This is not a GHA specific problem. All pipeline systems (Jenkins, Drone, GHA etc.) are affected by this, and require system-specific logic to mask them in logs.

For example, can we just not log sensitive data at all?

This would be great except:

  • Any values loaded into workflow run environment variables will be logged by GitHub. Here's an example from one of yesterday's builds:
    image
  • Our software, or dependencies, or dependencies of dependencies etc. will inevitably contain bugs that end up logging these values (see staging/dev env yesterday as an example).

The only way to avoid logging sensitive values is to build everything right the first time and have no bugs in our release software going forward, which isn't feasible.

@fheinecke fheinecke requested a review from r0mant December 17, 2024 19:10
tools/env-loader/pkg/values/valuetype.go Outdated Show resolved Hide resolved
func (ew *GHAMaskWriter) FormatEnvironmentValues(values map[string]values.Value) (string, error) {
renderedValues := make([]string, 0, len(values))
for key, value := range values {
if key == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems strange that the key emptiness check is inside this specific writer. Is there a way to validate the key validity when loading the value rather than in each specific writer?

Copy link
Contributor Author

@fheinecke fheinecke Dec 17, 2024

Choose a reason for hiding this comment

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

While this conceptually makes, I think it would add some unnecessary complexity. Here's what I've done instead:

  • Moved all the logic from the if statement to the String() function, which now also hashes the value to help users with debugging
  • Added tests for all writers to verify that they contain logic that covers this case

All writers still need to handle the case of key == "", but they can do it in their own way, without additional complexity.

@fheinecke fheinecke requested a review from r0mant December 17, 2024 21:31
@fheinecke fheinecke merged commit 05e3ef9 into main Dec 19, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants