Set both DEVELOCITY_ACCESS_KEY and GRADLE_ENTERPRISE_ACCESS_KEY env vars #225
Conversation
Job Summary for GradleDemo adding Build Scan® comment to PR :: successful-build-with-always-comment
|
Job Summary for GradleDemo adding Build Scan® comment to PR :: failing-build-with-comment-on-failure
|
| plugin-version: [3.16.2, 3.17.3] | ||
| include: | ||
| - plugin-version: 3.16.2 | ||
| accessKeyEnv: GRADLE_ENTERPRISE_ACCESS_KEY |
There was a problem hiding this comment.
This test doesn't require the accessKeyEnv, since the access key isn't being set as an environment variable.
| DEVELOCITY_CCUD_PLUGIN_VERSION: '2.0' | ||
| # Access key also set as an env var, we want to check it does not leak | ||
| DEVELOCITY_ACCESS_KEY: ${{ secrets.DEVELOCITY_ACCESS_KEY }} | ||
| ${{matrix.accessKeyEnv}}: ${{ secrets.DEVELOCITY_ACCESS_KEY }} |
There was a problem hiding this comment.
For this test, we probably want to set both DEVELOCITY_ACCESS_KEY and GRADLE_ENTERPRISE_ACCESS_KEY, so that we can confirm that both are cleared by the implementation.
So again, the accessKeyEnv isn't required.
There was a problem hiding this comment.
right, that's a better test, will change to setting both env vars
| } else { | ||
| // In case of not being able to generate a token we set the env variable to empty to avoid leaks | ||
| core.exportVariable(develocityAccesskeyEnvVar, '') | ||
| exportAccessKeyEnvVars('') |
There was a problem hiding this comment.
How will this work if a user has an older DV installation that doesn't support short-lived access tokens? Won't this break build-scan publishing for these users?
There was a problem hiding this comment.
Indeed, build scan publication (or other DV features) will fail. We chose security other backward compatibility, we implemented the same in other CI implementations and advertised the DV 2024.1 requirement in the docs and release notes.
There was a problem hiding this comment.
I'm not comfortable with this, since there's no way to opt-out of this feature.
Given the way that most users are automatically using the latest release of the action, they will start having build-scan-publish failures without any clear indication of what's changed.
It's not trivial for folks to upgrade to the latest Develocity, and we shouldn't prevent them from publishing build scans in the meantime.
Options as I see it:
- Never clear the
DEVELOCITY_ACCESS_KEYorGRADLE_ENTERPRISE_ACCESS_KEYenv vars - Never clear the
GRADLE_ENTERPRISE_ACCESS_KEYvariable, and assume that folks that have switched toDEVELOCITY_ACCESS_KEYare using a newer version of Develocity with short-lived token support. - Determine if the Develocity server is able to provide a token, and only clear the access key if the server could have provided a token but failed in this instance.
- Fail the entire build process if we cannot obtain an access token. This will at least prompt users to take action.
There was a problem hiding this comment.
Actually, there is a workaround: the user would have to set the DEVELOCITY_ACCESS_KEY variable directly on the run step that invokes Gradle.
There was a problem hiding this comment.
Combined with "never clear the access key" we could emit a deprecation warning. We would then add clearing the access key in the next major release of gradle/actions.
There was a problem hiding this comment.
Yes, that will do it.
As for the other options, it depends where we want to put the cursor on the security vs backward compatibility/functionality scale:
Never clear the DEVELOCITY_ACCESS_KEY or GRADLE_ENTERPRISE_ACCESS_KEY env vars
I would not go with this one, the less secure option
Never clear the GRADLE_ENTERPRISE_ACCESS_KEY variable, and assume that folks that have switched to
DEVELOCITY_ACCESS_KEY are using a newer version of Develocity with short-lived token support.
Could be a good compromise/transition solution for GH action and GitLab
Determine if the Develocity server is able to provide a token, and only clear the access key if the server could have provided a token but failed in this instance.
We chose not to go that road as there was no publicly available endpoint to determine this capability or reliable return code on auth/token (ie distinguishing between a server failing because the endpoint is not there or because the endpoint is there but non functional)
Fail the entire build process if we cannot obtain an access token. This will at least prompt users to take action.
A bit too radical, til now we've aimed to avoid breaking builds when DV related things are broken
Actually, there is a workaround: the user would have to set the DEVELOCITY_ACCESS_KEY variable directly on the run step that invokes Gradle.
Right, but maybe we don't want to advertise this since when they upgrade, they'll still rely on the old access key
Job Summary for GradleDemo adding Build Scan® comment to PR :: failing-build-with-comment-on-failure
|
Job Summary for GradleDemo adding Build Scan® comment to PR :: successful-build-with-always-comment
|
Follow up of #224, set both old and new access key env variables to either a short lived token or blank.