Skip to content

Conversation

@oleonardolima
Copy link
Contributor

@oleonardolima oleonardolima commented Dec 16, 2024

fixes #1775

Description

I used zizmor on all current CI workflows, it's a tool that helps detecting possible vulnerabilities in our CI jobs, see https://woodruffw.github.io/zizmor/.

It can run against most of it's audit rules, however the ones that require the GitHub API Token would require some with access to it in order to test against it. So this PR does not cover for impostor-commit, ref-confusion known-vulnerable-actions audit rules.

Notes to the reviewers

Changelog notice

  • Do not persist credentials on GitHub Actions.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@oleonardolima oleonardolima self-assigned this Dec 16, 2024
@oleonardolima oleonardolima force-pushed the ci/apply-zizmor-security-audit branch 2 times, most recently from a0f984f to 94daa75 Compare December 18, 2024 01:44
@oleonardolima oleonardolima requested review from ValuedMammal and notmandatory and removed request for ValuedMammal January 14, 2025 23:34
@oleonardolima oleonardolima marked this pull request as ready for review January 14, 2025 23:35
@notmandatory notmandatory added this to the 1.1.0 milestone Jan 28, 2025
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 94daa75

This just needs to be rebased and then it's ready to merge.

notmandatory added a commit to notmandatory/bdk that referenced this pull request Jan 28, 2025
94daa75 fix(ci): do not persist credentials (Leonardo Lima)

Pull request description:

  fixes bitcoindevkit#1775

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### Description

  I used `zizmor` on all current CI workflows, it's a tool that helps detecting possible vulnerabilities in our CI jobs, see https://woodruffw.github.io/zizmor/.

  It can run against most of it's audit rules, however the ones that require the GitHub API Token would require some with access to it in order to test against it. So this PR does not cover for impostor-commit, ref-confusion known-vulnerable-actions audit rules.

  <!-- Describe the purpose of this PR, what's being adding and/or fixed -->

  ### Notes to the reviewers

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  ### Changelog notice

  - Do not persist credentials on GitHub Actions.

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

ACKs for top commit:
  notmandatory:
    ACK 94daa75

Tree-SHA512: 7809b019e31d3495d3b3b6c2bb2c71043451558cf64585aa37b2ab73331d2a5cf33cce11adb7dafc9e87894121dc930146b88220c7c50f840e5b47acec8aca41
fix(cron-update-rust): don't persist credentials

fix(audit): don't persist credentials

fix(code-coverage): don't persist credentials

fix(nightly-docs): don't persist credentials

fix(cont-integration): don't persist credentials
@oleonardolima oleonardolima force-pushed the ci/apply-zizmor-security-audit branch from 94daa75 to b0c6849 Compare January 28, 2025 14:09
@oleonardolima
Copy link
Contributor Author

oleonardolima commented Jan 28, 2025

I did rebase it, and also pushed a new commit 30dce98 to fix the template-injection audit rule, as we're now manipulating the matrix.rust.version when running the job.

@oleonardolima oleonardolima force-pushed the ci/apply-zizmor-security-audit branch 2 times, most recently from d51db6b to 135255c Compare January 28, 2025 14:33
- fixes the `template_injection` audit failure due to
  `matrix.rust.version` usage, use an environement var instead
  see: https://woodruffw.github.io/zizmor/audits/#template-injection
@oleonardolima oleonardolima force-pushed the ci/apply-zizmor-security-audit branch from 135255c to 30dce98 Compare January 28, 2025 14:44
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 30dce98

@notmandatory notmandatory merged commit 88330f6 into bitcoindevkit:master Jan 28, 2025
23 checks passed
@oleonardolima oleonardolima deleted the ci/apply-zizmor-security-audit branch January 29, 2025 05:14
@ValuedMammal ValuedMammal mentioned this pull request Feb 4, 2025
41 tasks
notmandatory added a commit that referenced this pull request Mar 17, 2025
8b0b3a4 grant write permission to publish_docs (Musab1258)

Pull request description:

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### Description

  <!-- Describe the purpose of this PR, what's being adding and/or fixed -->

  The publish_docs workflow pushes changes to this repository: "bitcoindevkit/bitcoindevkit.org" which requires valid credentials. By setting the persist-credentials to false in #1778, the credentials required are not made available.

  ### Notes to the reviewers

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  To fix the issue I added a write permission to the publish_jobs, which will allow it to push changes without the credentials.

  ### Changelog notice

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  * [ ] I've added tests for the new feature
  * [ ] I've added docs for the new feature

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [ ] I've added tests to reproduce the issue which are now passing
  * [ ] I'm linking the issue being fixed by this PR

ACKs for top commit:
  notmandatory:
    ACK 8b0b3a4

Tree-SHA512: d72aaf79f99010b75f95404eadc61f0ce35f38cb9c24b0dcfc90dbe043affef06c477b6faa02dd488820c472bfc415993affdf67ebd55138dde0865e625ebf3d
notmandatory added a commit that referenced this pull request May 20, 2025
a50fa4c ci: add zizmor github actions security analysis workflow (Steve Myers)

Pull request description:

  ### Description

  Added workflow to run zizmor github actions security analysis.

  See: https://woodruffw.github.io/zizmor/usage/#use-in-github-actions

  ### Notes to the reviewers

  I built this PR on top of #1778.

  I pinned zizmor to version 1.6.0.

  ### Changelog notice

  ci: add zizmor github actions security analysis workflow and fix possible vulnerabilities

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

Top commit has no ACKs.

Tree-SHA512: 5eaa6b6ce59fb3f724a368098174000e096ddc0c798c5132e089a02c611f5132ea9e123418c54cb98628361de80faaf562742eae45ccb68c5040e7475f948b72
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Use zizmor to audit github actions

2 participants