Skip to content

Conversation

@jiridanek
Copy link
Member

@jiridanek jiridanek commented Dec 4, 2025

https://issues.redhat.com/browse/RHAIENG-2348

Somebody might have leaked our git-crypt key in the shai-hulud breach, we need to get new pull-secret.json and encrypt it with new key. The pull-secret is used for github actions.

Description

❯ base64 -i .git-crypt-key | gh secret set GIT_CRYPT_KEY --repo red-hat-data-services/notebooks
✓ Set Actions secret GIT_CRYPT_KEY for red-hat-data-services/notebooks

How Has This Been Tested?

Self checklist (all need to be checked):

  • Ensure that you have run make test (gmake on macOS) before asking for review
  • Changes to everything except Dockerfile.konflux files should be done in odh/notebooks and automatically synced to rhds/notebooks. For Konflux-specific changes, modify Dockerfile.konflux files directly in rhds/notebooks as these require special attention in the downstream repository and flow to the upcoming RHOAI release.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

* notebooks/ci/secrets/pull-secret.json
```
$ git-crypt lock
$ rm .git-crypt-key
$ git-crypt init
$ git-crypt export-key .git-crypt-key
$ git-crypt unlock .git-crypt-key
```
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 4, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 4, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign atheo89 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions github-actions bot added the review-requested GitHub Bot creates notification on #pr-review-ai-ide-team slack channel label Dec 4, 2025
@openshift-ci openshift-ci bot added the size/xs label Dec 4, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 4, 2025

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

💤 Files selected but had no reviewable changes (7)
  • ci/secrets/pull-secret.json
  • ci/secrets/rhsm/rhsm.conf
  • ci/secrets/rhsm/syspurpose/valid_fields.json
  • ci/secrets/run/secrets/redhat.repo
  • ci/secrets/run/secrets/rhsm/rhsm.conf
  • ci/secrets/run/secrets/rhsm/syspurpose/syspurpose.json
  • ci/secrets/run/secrets/rhsm/syspurpose/valid_fields.json
⛔ Files ignored due to path filters (8)
  • ci/secrets/pki/consumer/cert.pem is excluded by !**/*.pem
  • ci/secrets/pki/consumer/key.pem is excluded by !**/*.pem
  • ci/secrets/rhsm/ca/redhat-entitlement-authority.pem is excluded by !**/*.pem
  • ci/secrets/rhsm/ca/redhat-uep.pem is excluded by !**/*.pem
  • ci/secrets/run/secrets/etc-pki-entitlement/1358292728734056503-key.pem is excluded by !**/*.pem
  • ci/secrets/run/secrets/etc-pki-entitlement/1358292728734056503.pem is excluded by !**/*.pem
  • ci/secrets/run/secrets/rhsm/ca/redhat-entitlement-authority.pem is excluded by !**/*.pem
  • ci/secrets/run/secrets/rhsm/ca/redhat-uep.pem is excluded by !**/*.pem

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jiridanek jiridanek added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed size/xs review-requested GitHub Bot creates notification on #pr-review-ai-ide-team slack channel labels Dec 4, 2025
@jiridanek jiridanek marked this pull request as ready for review December 4, 2025 15:40
@openshift-ci openshift-ci bot requested review from atheo89 and daniellutz December 4, 2025 15:40
@openshift-ci openshift-ci bot added the size/xs label Dec 4, 2025
@jiridanek
Copy link
Member Author

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 4, 2025

✅ Actions performed

Full review triggered.

@jiridanek
Copy link
Member Author

@coderabbitai review commit messages, the PR summary, and the general approach

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 4, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Get commit messages from the PR
echo "=== Commit Messages ==="
gh pr view 2756 --json commits --jq '.commits[] | "Commit: \(.oid[0:7])\nAuthor: \(.authors[0].login)\nMessage: \(.messageHeadline)\n---"'

echo ""
echo "=== Changed Files ==="
gh pr view 2756 --json files --jq '.files[] | .path'

echo ""
echo "=== PR Details ==="
gh pr view 2756 --json title,body,additions,deletions --jq '{title, additions, deletions}'

Length of output: 1205


🏁 Script executed:

#!/bin/bash

# Check .gitattributes for git-crypt configuration
echo "=== .gitattributes (git-crypt config) ==="
if [ -f .gitattributes ]; then
    cat .gitattributes
else
    echo "No .gitattributes file found"
fi

echo ""
echo "=== Check for git-crypt status file ==="
ls -la .git-crypt/ 2>/dev/null || echo "No .git-crypt directory found"

echo ""
echo "=== Full first commit message ==="
gh pr view 2756 --json commits --jq '.commits[0].messageBody'

echo ""
echo "=== Check for any documentation about key rotation ==="
fd -t f -e md . ci/secrets/ 2>/dev/null | head -5
fd -t f README ci/ 2>/dev/null | head -5

echo ""
echo "=== PR description ==="
gh pr view 2756 --json body --jq '.body'

Length of output: 1936


@jiridanek, I've reviewed the commit messages, PR summary, and general approach. Here's my analysis:

🔴 Commit Messages - Issues Found

  1. Truncation: The first commit message is truncated ("...remove outdated se…"), making it unclear what was actually removed.

  2. Needs Squashing: Per your merge criteria, these three commits should be squashed into one cohesive commit. The iterative approach (cleanup → delete → reinit) was probably wise during development, but should now be consolidated.

  3. Security Context: For a security-sensitive operation like key rotation, the commit message should include:

    • Why the rotation was necessary (breach reference)
    • What was rotated (all secrets, not just pull-secret)
    • How to recover if issues arise (link to docs)

Recommended squashed commit message:

NO-JIRA: chore(shai-hulud): rotate git-crypt key and re-encrypt all secrets

Rotate git-crypt key due to potential compromise in shai-hulud security
incident. Re-encrypted all 15 secret files in ci/secrets/ with new key.

The new GIT_CRYPT_KEY has been updated in GitHub Actions secrets.

Files re-encrypted:
- ci/secrets/pull-secret.json
- ci/secrets/pki/* (consumer certs)
- ci/secrets/rhsm/* (Red Hat subscription configs)
- ci/secrets/run/secrets/* (runtime secrets)

🟡 PR Summary - Scope Mismatch

  1. Title vs Reality: The PR title mentions only pull-secret.json, but 15 files were re-encrypted. The title should reflect that all secrets were rotated.

    Suggested title: NO-JIRA: chore(shai-hulud): rotate git-crypt key and re-encrypt all CI secrets

  2. Missing Context: "shai-hulud breach" is mentioned without explanation. Future maintainers won't understand what this refers to. Consider:

    • Brief explanation of the incident (if not sensitive)
    • Or reference to internal incident documentation
    • Or at minimum: "potential key compromise" if details are confidential
  3. Incomplete Checklists: Testing section is empty, and all checkboxes are unchecked. For a security change, you should document:

    • How you verified the secrets decrypt correctly
    • Whether GitHub Actions can still access them
    • Test run confirmation

🟢 General Approach - Sound with Recommendations

What's Good:

  • ✅ Comprehensive: Re-encrypted all secrets in one go (correct approach)
  • ✅ Proper scope: All files under ci/secrets/** per .gitattributes config
  • ✅ GitHub Actions updated: New GIT_CRYPT_KEY secret properly set
  • ✅ Clean execution: 0 additions/0 deletions confirms proper git-crypt binary re-encryption

Recommendations:

  1. Verification: Add testing details showing GitHub Actions can still access secrets after the rotation
  2. Documentation: Consider adding ci/secrets/README.md with key rotation procedure for next time
  3. Old Key: Document or confirm the old key has been revoked/removed from any systems
  4. Notification: If this is used by other team members, ensure they know to pull the new encrypted files

Summary

The technical approach is sound ✅, but the PR needs:

  • Squash commits into one cohesive message
  • Update PR title to reflect full scope (all secrets, not just pull-secret.json)
  • Add testing verification details
  • Complete the checklists
  • Consider adding rotation procedure documentation

Would you like me to help draft the complete PR description with testing instructions?

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 4, 2025

@jiridanek: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/images a8a7e5f link true /test images

Full PR test history. Your PR dashboard.

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

@jiridanek jiridanek changed the title NO-JIRA: chore(shai-hulud): reinit the git-crypt with new pull-secret.json RHAIENG-2348: chore(shai-hulud): reinit the git-crypt with new pull-secret.json Dec 5, 2025
@openshift-ci openshift-ci bot added size/xs and removed size/xs labels Dec 5, 2025
@jiridanek jiridanek changed the title RHAIENG-2348: chore(shai-hulud): reinit the git-crypt with new pull-secret.json RHAIENG-2348: chore(shai-hulud): reinit the git-crypt with new pull-secret.json and delete the secrets we no longer need Dec 8, 2025
@openshift-ci openshift-ci bot added size/xs and removed size/xs labels Dec 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/xs tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant