Skip to content

Conversation

sanjay20m
Copy link

Summary

This PR hardens the ROCm apt repository setup in the build_cmake GitHub Actions workflow to prevent potential supply chain attacks.

Changes Made

  • Enforced HTTPS for ROCm repository URLs
  • Removed insecure flags:
    • --allow-insecure-repositories
    • --allow-unauthenticated
  • Replaced deprecated apt-key usage with signed-by keyring method
  • Added SHA256 checksum verification for ROCm GPG key before adding
  • Scoped GPG key trust to ROCm repository only, reducing risk of key misuse

Security Impact

These changes:

  • Prevent possible MITM attacks by requiring TLS for repo communication
  • Ensure ROCm packages are verified via a trusted, scoped GPG key
  • Avoid bypassing signature and authentication checks in CI builds

Rationale

GitHub-hosted workflows that install packages from third-party repos must ensure:

  • The repo is served over HTTPS
  • Keys are verified and scoped to the intended repository
  • No flags weaken apt’s security model

This change addresses a potential supply chain vulnerability in the GitHub Actions workflow by enforcing secure package verification.

…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.
Copy link

meta-cla bot commented Aug 14, 2025

Hi @sanjay20m!

Thank you for your pull request and welcome to our community.

Action Required

In 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.

Process

In 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 CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@mnorris11
Copy link

@ItsPitt

@meta-cla meta-cla bot added the CLA Signed label Aug 14, 2025
Copy link

meta-cla bot commented Aug 14, 2025

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@sanjay20m
Copy link
Author

Hi, can I know what further changes I will have to make, like any tweaks required ??

@mnorris11
Copy link

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)

@junjieqi junjieqi self-requested a review September 9, 2025 16:45
@sanjay20m
Copy link
Author

After a long time, just to ask if any changes are required, please tell me :)

@ItsPitt
Copy link
Contributor

ItsPitt commented Sep 9, 2025

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)

I think this looks good from our side!

@junjieqi
Copy link
Contributor

junjieqi commented Sep 9, 2025

@sanjay20m looks there is a build failure, could you help take a look? Thanks! After everything is green, I will import the change

@sanjay20m
Copy link
Author

sanjay20m commented Sep 10, 2025

Done ! @junjieqi

@facebook-github-bot
Copy link
Contributor

@junjieqi has imported this pull request. If you are a Meta employee, you can view this in D82143282.

@junjieqi
Copy link
Contributor

@sanjay20m looks like there is another failure on arm64 SVE

@sanjay20m
Copy link
Author

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:
libblas 3.9.0-35 → 3.9.0-31
libcblas 3.9.0-35 → 3.9.0-31
liblapack 3.9.0-35 → 3.9.0-31

This looks like a Conda dependency resolution issue unrelated to the apt repo setup.
The security hardening applied here is valid and independent of that failure.

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.

@junjieqi
Copy link
Contributor

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: libblas 3.9.0-35 → 3.9.0-31 libcblas 3.9.0-35 → 3.9.0-31 liblapack 3.9.0-35 → 3.9.0-31

This looks like a Conda dependency resolution issue unrelated to the apt repo setup. The security hardening applied here is valid and independent of that failure.

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

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
Copy link
Author

Hi! @junjieqi, now I think it may satisfy you ...

@junjieqi
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants