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

Filter build-info and modules properties #716

Merged
merged 7 commits into from
Mar 6, 2023
Merged

Conversation

Or-Geva
Copy link
Contributor

@Or-Geva Or-Geva commented Mar 2, 2023

  • All tests passed. If this feature is not already covered by the tests, I added new tests.

Prior to this PR, build-info properties were filtered only for environment variables and system properties.
The PR moves all filter logic into one place and performs exclude/include logic for all properties, including:

  1. Environment variables (including properties with buildInfo.env prefix)
  2. System properties
  3. Properties from file
  4. Build-info modules properties
  5. Exclude not just the key but also the value of properties

@Or-Geva Or-Geva requested a review from yahavi March 2, 2023 17:01
@Or-Geva Or-Geva added the safe to test Approve running integration tests on a pull request label Mar 2, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Mar 2, 2023
@yahavi yahavi added the safe to test Approve running integration tests on a pull request label Mar 5, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Mar 5, 2023
Copy link
Member

@yahavi yahavi left a comment

Choose a reason for hiding this comment

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

Please don't merge this code before my final approval. I want to go through it again.

private static final String apiKeySecretPrefix = "AKCp8";
private static final int apiKeySecretMinimalLength = 73;
private static final String referenceTokenSecretPrefix = "cmVmdGtuOjAxOj";
private static final int referenceTokenSecretMinimalLength = 64;
Copy link
Member

Choose a reason for hiding this comment

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

I almost sure that reference tokens are 64 characters fixed

@@ -107,16 +114,10 @@ public static Properties stripPrefixFromProperties(Properties source, String pre
}

public static Properties getEnvProperties(Properties startProps, Log log) {
IncludeExcludePatterns patterns = new IncludeExcludePatterns(
Copy link
Member

Choose a reason for hiding this comment

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

Removing the filtering code from this method sounds very risky to me. Let's do the "get" and the "filter" logic in one place to enforce filtering in all flows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand the concern, but this is not the place to do filtering. According to the name of this function, it returns all environment variables. By moving the filter logic to a new location, we can filter not only the environment variables, but also modules. Putting the filter logic here would prevent us from filtering things unrelated to environment variables, so we would have to split the logic across the code, and that will cause a mess in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to security risks, we will return the filter logic to here despite the above comment .

@Or-Geva Or-Geva requested a review from yahavi March 5, 2023 19:59
@Or-Geva Or-Geva added the safe to test Approve running integration tests on a pull request label Mar 5, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Mar 5, 2023
@Or-Geva Or-Geva added the safe to test Approve running integration tests on a pull request label Mar 6, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Mar 6, 2023
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