Skip to content

Limit env variables propagated #13922

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jul 30, 2020
Merged

Limit env variables propagated #13922

merged 7 commits into from
Jul 30, 2020

Conversation

blindpirate
Copy link
Collaborator

No description provided.

@blindpirate
Copy link
Collaborator Author

@bot-gradle test QF

@bot-gradle
Copy link
Collaborator

OK, I've already triggered QuickFeedback build for you.

@blindpirate blindpirate force-pushed the blindpirate/limit-env-variables branch from 5abd10c to bf2bd3e Compare July 24, 2020 12:35
@blindpirate
Copy link
Collaborator Author

@bot-gradle test QF

@bot-gradle
Copy link
Collaborator

Sorry some internal error occurs, please contact the administrator @blindpirate

@bot-gradle
Copy link
Collaborator

OK, I've already cancelled the old build and triggered a new QuickFeedback build for you.

wolfs
wolfs reviewed Jul 24, 2020
View reviewed changes
@blindpirate blindpirate force-pushed the blindpirate/limit-env-variables branch from bf2bd3e to 8f44394 Compare July 27, 2020 01:59
@blindpirate
Copy link
Collaborator Author

@bot-gradle test QF

@bot-gradle
Copy link
Collaborator

OK, I've already triggered QuickFeedback build for you.

@blindpirate blindpirate force-pushed the blindpirate/limit-env-variables branch from 8f44394 to 9995e10 Compare July 27, 2020 02:11
@blindpirate
Copy link
Collaborator Author

@bot-gradle test QF

@bot-gradle
Copy link
Collaborator

OK, I've already cancelled the old build and triggered a new QuickFeedback build for you.

@blindpirate blindpirate force-pushed the blindpirate/limit-env-variables branch 6 times, most recently from 7ed8b95 to 7cbcc1a Compare July 27, 2020 07:30
wolfs
wolfs requested changes Jul 27, 2020
View reviewed changes
@blindpirate blindpirate force-pushed the blindpirate/limit-env-variables branch 2 times, most recently from 32cf9d1 to 2e65f1f Compare July 27, 2020 09:55
@blindpirate
Copy link
Collaborator Author

@bot-gradle test RFN

@blindpirate blindpirate marked this pull request as ready for review July 27, 2020 09:58
@bot-gradle
Copy link
Collaborator

OK, I've already triggered ReadyForNightly build for you.

big-guy
big-guy requested changes Jul 27, 2020
View reviewed changes
buildSrc/subprojects/jvm/src/main/kotlin/gradlebuild/propagated-env-variables.kt Outdated

fun Test.configurePropagatedEnvVariables() {
if (BuildEnvironment.isCiServer) {
environment = System.getenv().entries.mapNotNull(::sanitize).toMap()
Copy link
Member

Choose a reason for hiding this comment

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

Can we also have something that looks at the propagated environment variables and does a sanity check for "bad words"?

e.g., things like BUILD_CACHE_PASSWORD and TEAMCITY_TOKEN would make it through our filter, so I think we should make it so the build fails at this point if any env var contains (case insensitive) "password" or "token" so we're forced to pick a different name.

The failures at the test level below are OK, but it's too late at that point.

Copy link
Member

Choose a reason for hiding this comment

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

A few candidates to blacklist:

  • apikey
  • secret
  • auth
  • credential

@big-guy big-guy requested a review from donat July 27, 2020 12:54
donat
donat reviewed Jul 27, 2020
View reviewed changes
buildSrc/subprojects/jvm/src/main/kotlin/gradlebuild/propagated-env-variables.kt Outdated


fun Test.configurePropagatedEnvVariables() {
if (BuildEnvironment.isCiServer) {
Copy link
Member

Choose a reason for hiding this comment

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

❌ Local builds might also leak credentials. I think we should filter env vars for all builds.

@blindpirate
Copy link
Collaborator Author

@bot-gradle test RFN