Secure ROCm apt repo setup in build_cmake workflow#4531
Secure ROCm apt repo setup in build_cmake workflow#4531sanjay20m wants to merge 6 commits intofacebookresearch:mainfrom
Conversation
|
Hi @sanjay20m! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Hi, can I know what further changes I will have to make, like any tweaks required ?? |
Our friends at AMD are best equipped to review this one @ItsPitt (when you have time) |
|
After a long time, just to ask if any changes are required, please tell me :) |
I think this looks good from our side! |
|
@sanjay20m looks there is a build failure, could you help take a look? Thanks! After everything is green, I will import the change |
|
Done ! @junjieqi |
|
@sanjay20m looks like there is another failure on arm64 SVE |
|
Hi @junjieqi Thanks for reviewing! I wanted to clarify that the change in this PR is strictly scoped to hardening the ROCm apt repository setup (HTTPS, signed-by keyring, checksum verification, removal of insecure flags). The current build failure is happening in the Conda environment step, where the solver is revising libblas/libcblas/liblapack and exiting with code 1: The following packages will be REVISED: This looks like a Conda dependency resolution issue unrelated to the apt repo setup. If needed, I can help pin the BLAS/LAPACK versions in the workflow or enable strict channel priority to resolve it, but I think it makes sense to review/merge the security fix separately since the two are orthogonal. |
Hi @sanjay20m , thanks for the reply. I'm wondering if we need to pin the BLAS/LAPACK first before this one. Once we pin them, and we could rebase current pr with master to resolve the build failure |
|
Hi! @junjieqi, now I think it may satisfy you ... |
Thank you for your prompt response! Looks the linuxx86_64 still failed and it is related with the latest change. Thank you for your patience again! |
|
Hi @junjieqi can you give me 48 hours to fix the issue... |
@sanjay20m Sure, take your time. Feel free to ping me on the thread when you are ready |
|
Hello @junjieqi , |
…ion) - Replaced deprecated apt-key with signed-by keyring - Enforced HTTPS for ROCm repo - Removed insecure apt-get flags (--allow-insecure-repositories, --allow-unauthenticated) - Added SHA256 verification for ROCm GPG key - Scoped trust to ROCm repo only This prevents potential MITM/supply chain attacks in CI builds.
Fix ROCm setup: remove duplicate cleanup and target.lst entries - Removed redundant cleanup commands that were outside the ROCm patch. - Ensure MI200-class accelerator fake entry (`gfx90a`) is only added once. - Streamlined BLAS/LAPACK pinning and ROCm package installation steps.
@sanjay20m looks like it is still error out |
|
Hi @junjieqi |
@sanjay20m from the logging, it showed the error message below I'm not sure if you able to see this one https://github.com/facebookresearch/faiss/actions/runs/18180222500/job/51754524877?pr=4531 |
|
Reassigned to @sanjay20m to take a look when time permits |
|
@mnorris11 has imported this pull request. If you are a Meta employee, you can view this in D82143282. |
|
Hi @sanjay20m , the ROCm runner was disabled for some time. It is now up again. We used some information from this PR to re-enable it. So I will close this one because changes should already be integrated, but wanted to give you kudos for it. Thanks for the contribution! |
Summary
This PR hardens the ROCm apt repository setup in the
build_cmakeGitHub Actions workflow to prevent potential supply chain attacks.Changes Made
--allow-insecure-repositories--allow-unauthenticatedapt-keyusage withsigned-bykeyring methodSecurity Impact
These changes:
Rationale
GitHub-hosted workflows that install packages from third-party repos must ensure:
This change addresses a potential supply chain vulnerability in the GitHub Actions workflow by enforcing secure package verification.