security: avoid persisting GitHub SSH keys in PR pipelines#4477
security: avoid persisting GitHub SSH keys in PR pipelines#4477ti-chi-bot[bot] merged 2 commits intomainfrom
Conversation
![ti-chi-bot[bot]](https://avatars.githubusercontent.com/in/214286?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR removes the git.setSshKey(GIT_CREDENTIALS_ID) calls from untrusted pull request pipelines across multiple Groovy scripts to mitigate security risks associated with persisting SSH keys. Instead, the pipelines are updated to use checkout-scoped credentials via prow.checkoutRefs. The approach effectively limits SSH key usage to the checkout operation, preventing untrusted code from reusing the keys in subsequent steps. The changes are consistent, well-documented, and address a clear security concern without introducing additional complexity.
Critical Issues
None identified. The changes effectively mitigate the security risk of persisting SSH credentials in PR pipelines.
Code Improvements
-
Error Logging for
retry(2)Operations
Files: All modified pipeline scripts
Issue: Theretry(2)block currently lacks explicit logging or error handling for failed retries. If checkout fails twice, the pipeline may terminate without clear visibility into the cause.
Suggestion: Add error logging to provide better visibility into checkout failures.retry(2) { try { prow.checkoutRefs(REFS, credentialsId = GIT_CREDENTIALS_ID, timeout = 5, withSubmodule = true, gitBaseUrl = 'https://github.com') } catch (Exception e) { println "Checkout failed: ${e.message}" throw e } }
-
Parameterization of Timeout Values
Files: All modified pipeline scripts
Issue: Thetimeoutparameter inprow.checkoutRefsis hardcoded to5. This could cause problems if longer repository fetch times are needed, especially for large submodules.
Suggestion: Use a configurable timeout parameter to allow flexibility across environments.def checkoutTimeout = env.CHECKOUT_TIMEOUT ?: 5 prow.checkoutRefs(REFS, credentialsId = GIT_CREDENTIALS_ID, timeout = checkoutTimeout, withSubmodule = true, gitBaseUrl = 'https://github.com')
Best Practices
-
Testing Coverage for Updated Pipelines
Files: All modified pipeline scripts
Issue: The PR description mentions validation steps but does not specify whether automated tests were added to verify the updated pipeline behavior.
Suggestion: Ensure automated tests cover scenarios whereprow.checkoutRefsis used with checkout-scoped credentials, including edge cases with submodules and large repositories. -
Documentation for Credential Changes
Files: All modified pipeline scripts
Issue: While the PR description explains the rationale for removinggit.setSshKey, the pipeline scripts themselves lack inline comments explaining the security improvements.
Suggestion: Add comments to clarify whyprow.checkoutRefsis used instead ofgit.setSshKey.// Use prow.checkoutRefs with credentials scoped to the checkout operation to avoid persisting SSH keys. prow.checkoutRefs(REFS, credentialsId = GIT_CREDENTIALS_ID, timeout = 5, withSubmodule = true, gitBaseUrl = 'https://github.com')
-
Consistency in Credential Usage
Files:pipelines/pingcap/tiflash/latest/pull_integration_next_gen.groovy,pipelines/pingcap/tiflash/latest/pull_unit_test.groovy, etc.
Issue: Some TiFlash pipelines previously usedcredentialsId = ''for checkout, which could cause issues if private submodules require authentication. This PR fixes the issue by usingGIT_CREDENTIALS_ID, but it's worth confirming consistency across all pipelines to avoid regressions.
Suggestion: Audit any remaining pipelines wherecredentialsId = ''might still be in use outside this PR scope.
Conclusion
The PR addresses a critical security issue by removing persistent SSH key usage and implementing checkout-scoped credentials. The changes are solid, but adding error logging, parameterizing timeout values, improving testing coverage, and enhancing inline documentation would further strengthen the implementation.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors Git credential handling across numerous Jenkins pipeline files for TiDB and TiFlash. It removes redundant git.setSshKey(GIT_CREDENTIALS_ID) calls and updates prow.checkoutRefs to use GIT_CREDENTIALS_ID directly, ensuring consistent credential management across various release branches. I have no feedback to provide.
|
/retest |
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR removes the usage of git.setSshKey(GIT_CREDENTIALS_ID) from PR pipelines across multiple Groovy script files to prevent persistent SSH key storage in untrusted PR jobs. Instead, it scopes SSH credentials to the checkout step by using prow.checkoutRefs with credentialsId. The approach enhances security by limiting the exposure of sensitive credentials and ensures private repo access works without leaving reusable SSH keys on disk. The changes are straightforward and appear well-tested, with no obvious regressions introduced.
Critical Issues
No critical issues detected in the changes.
Code Improvements
- File:
pipelines/pingcap/tiflash/latest/pull_integration_test.groovy(and similar TiFlash pipeline files)- Issue: The
credentialsId = ''inprow.checkoutRefswas replaced withcredentialsId = GIT_CREDENTIALS_ID, which is a safer approach. However, the PR does not clarify whethercredentialsIdis properly scoped to the checkout step and prevents further misuse within the pipeline. - Suggestion: Ensure that
prow.checkoutRefswithcredentialsIdis using proper scoping mechanisms such assshagentinternally to avoid accidental credential leakage beyond the checkout step.
- Issue: The
Best Practices
-
Testing Coverage Gaps
- Files Affected: All modified pipeline files.
- Issue: While the PR mentions validation was done (
git diff --checkand manual verification), there is no indication of automated tests for these pipeline changes. Any subtle changes in Groovy pipeline behavior could lead to build/test failures. - Suggestion: Add automated tests (mock jobs) to verify that pipelines execute successfully with these changes and that SSH keys are not accessible beyond the checkout step.
-
Documentation Updates
- Files Affected: All modified pipeline files.
- Issue: There is no inline documentation explaining the rationale for removing
git.setSshKeyor howprow.checkoutRefsensures scoped authentication. - Suggestion: Add comments in the pipeline files to clarify:
// Removed git.setSshKey(GIT_CREDENTIALS_ID) to prevent persistent SSH key storage. // prow.checkoutRefs ensures SSH credentials are scoped to this step only.
-
Consistency in Credential Handling
- Files Affected:
pipelines/pingcap/tiflash/latest/pull_integration_test.groovyand others with TiFlash-specific changes. - Issue: TiFlash pipelines previously used
credentialsId = '', which may indicate an intentional lack of credentials. This change introducescredentialsId = GIT_CREDENTIALS_ID, which assumes that SSH credentials are required everywhere. The PR does not justify why credentials are now necessary in these specific scenarios. - Suggestion: Review TiFlash-specific pipelines to confirm that
credentialsId = GIT_CREDENTIALS_IDis truly required and does not introduce unnecessary credential dependencies.
- Files Affected:
Conclusion
The PR significantly improves security by removing persistent SSH key storage from untrusted pipelines, but it lacks clarity about the testing and credential scoping mechanisms in the checkout step. Addressing these gaps and adding inline documentation will ensure robustness and maintainability of these changes.
|
@wuhuizuo: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Jenkins Replay Status
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wuhuizuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
deleted the
ci-architect/audit-private-clone-cred-exposure
branch
1 participant
Summary
git.setSshKey(GIT_CREDENTIALS_ID)from PR / ghpr pipelines that run untrusted repository codeprow.checkoutRefs(..., credentialsId = GIT_CREDENTIALS_ID, ...)Risk Being Fixed
The audited PR pipelines cloned private repositories or private submodules with the
github-sre-bot-sshcredential and then persisted the SSH private key into~/.ssh/id_rsaviagit.setSshKey().Once that happened, build/test scripts coming from the target PR repository could reuse the same key to:
This PR removes that persistent-key path from PR jobs and narrows credential availability to the checkout helper's
sshagentscope.Audit Notes
Confirmed safe patterns that were left unchanged:
prow.checkoutRefs(..., credentialsId = GIT_CREDENTIALS_ID, ...)because it scopes SSH auth to the checkout helpercomponent.checkout(...)/checkoutPRWithPreMerge(...)because they use Jenkins checkout orsshagentduring checkout onlyjenkins/GitSCM checkouts that usecredentialsIdwithout copying keys into~/.ssh/id_rsaValidation
git diff --checkpipelines/still containsgit.setSshKey(...)git.setSshKey()now use checkout-scoped credentials instead ofcredentialsId = ''Scope
This intentionally updates PR / ghpr pipelines only. Remaining
git.setSshKey()usage is in merged / trusted jobs and can be reviewed separately if we want to remove that pattern repo-wide later.