-
Notifications
You must be signed in to change notification settings - Fork 7
OCPEDGE-2576: Add security-review plugin with credential and safety hooks #74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
jeff-roche
wants to merge
5
commits into
openshift-eng:main
Choose a base branch
from
jeff-roche:feat/security-review-plugin
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
5c91e0a
feat(plugins): add security-review plugin with credential and safety …
jeff-roche d85701b
fix(plugins): address PR review feedback for security-review plugin
jeff-roche c5a0849
chore(plugins): register security-review in marketplace catalog
jeff-roche 051be4b
fix(plugins): address review feedback for security-review hooks
jeff-roche 56af75a
fix(plugins): improve destructive command detection and credential regex
jeff-roche File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| { | ||
| "name": "security-review", | ||
| "description": "Security guards for credential protection and destructive command prevention", | ||
| "version": "1.0.0", | ||
| "author": { "name": "jeff-roche" }, | ||
| "homepage": "https://github.com/openshift-eng/edge-tooling", | ||
| "license": "Apache-2.0" | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| # security-review | ||
|
|
||
| Security guards for credential protection and destructive command prevention. Hooks only -- no skills or commands. | ||
|
|
||
| ## What It Does | ||
|
|
||
| Automatic PreToolUse hooks fire on every relevant tool invocation: | ||
|
|
||
| ### Bash hooks | ||
|
|
||
| - **check-secrets.sh** -- Scans `git diff --cached` for credentials before `git commit` or `git add` runs. Detects AWS keys, private keys, hardcoded passwords/tokens/secrets/API keys, and `.env` files. | ||
| - **block-destructive.sh** -- Blocks dangerous commands: `rm -rf /`, `git push --force`, `git reset --hard`, `git clean -fd`, `git checkout -- .`, `git restore .`, `chmod -R 777`. Suggests safer alternatives. | ||
|
|
||
| ### Write hooks | ||
|
|
||
| - **check-file-secrets.sh** -- Scans file content for credential patterns before writing. Detects the same credential types as check-secrets.sh plus database connection strings with embedded passwords. | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| ## Detected Patterns | ||
|
|
||
| | Category | Examples | | ||
| |----------|----------| | ||
| | AWS keys | `AKIA` followed by 16 alphanumeric characters | | ||
| | Private keys | PEM-encoded RSA, generic, or OpenSSH private keys | | ||
| | Hardcoded credentials | `password`, `token`, `secret`, `api_key` assignments | | ||
| | Connection strings | Database URIs with embedded passwords | | ||
| | Sensitive files | `.env` files in staged git content | | ||
| | Destructive commands | Force push, hard reset, root deletion, insecure chmod | | ||
|
|
||
| ## False Positives | ||
|
|
||
| If a hook blocks a legitimate action, Claude Code will present the block reason and let you approve the action through the permission prompt. No configuration changes needed. | ||
|
|
||
| ## Installation | ||
|
|
||
| ```bash | ||
| /plugin marketplace add openshift-eng/edge-tooling security-review | ||
| ``` | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| { | ||
| "hooks": { | ||
| "PreToolUse": [ | ||
| { | ||
| "matcher": "Bash", | ||
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "${CLAUDE_PLUGIN_ROOT}/scripts/check-secrets.sh", | ||
| "timeout": 5, | ||
| "statusMessage": "Scanning for secrets..." | ||
| }, | ||
| { | ||
| "type": "command", | ||
| "command": "${CLAUDE_PLUGIN_ROOT}/scripts/block-destructive.sh", | ||
| "timeout": 2, | ||
| "statusMessage": "Safety check..." | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "matcher": "Write", | ||
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "${CLAUDE_PLUGIN_ROOT}/scripts/check-file-secrets.sh", | ||
| "timeout": 3, | ||
| "statusMessage": "Scanning file content..." | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| #!/usr/bin/bash | ||
| # PreToolUse(Bash) hook — block dangerous commands | ||
| set -euo pipefail | ||
|
|
||
| parse_command() { | ||
| if command -v jq &>/dev/null; then | ||
| echo "$1" | jq -r '.toolInput.command // empty' | ||
| elif command -v python3 &>/dev/null; then | ||
| echo "$1" | python3 -c 'import sys,json;print(json.load(sys.stdin).get("toolInput",{}).get("command",""))' | ||
| else | ||
| echo "security-review: neither jq nor python3 available — cannot parse hook input" >&2 | ||
| exit 1 | ||
| fi | ||
| } | ||
|
|
||
| INPUT=$(cat) | ||
| COMMAND=$(parse_command "$INPUT") | ||
|
|
||
| if [[ -z "$COMMAND" ]]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # rm with recursive+force targeting root or home | ||
| # Detect rm, then check for both -r and -f (in any order, combined or separate), | ||
| # then check for dangerous target paths | ||
| if echo "$COMMAND" | grep -qE '(^|[[:space:]])(sudo[[:space:]]+)?rm[[:space:]]'; then | ||
| HAS_R=false | ||
| HAS_F=false | ||
| echo "$COMMAND" | grep -qE '[[:space:]]+-[a-zA-Z]*r' && HAS_R=true | ||
| echo "$COMMAND" | grep -qE '[[:space:]]+-[a-zA-Z]*f' && HAS_F=true | ||
| echo "$COMMAND" | grep -qE '[[:space:]]+--recursive' && HAS_R=true | ||
| echo "$COMMAND" | grep -qE '[[:space:]]+--force' && HAS_F=true | ||
|
|
||
| if [[ "$HAS_R" = true && "$HAS_F" = true ]]; then | ||
| # Check for dangerous targets: standalone /, ~, ~/, $HOME, $HOME/ | ||
| if echo "$COMMAND" | grep -qE '([[:space:]]|/)(/|~|\$HOME)([[:space:]]|$)' || \ | ||
| echo "$COMMAND" | grep -qE '[[:space:]](~/|\$HOME/)([[:space:]]|$)'; then | ||
| echo "Blocked: \`rm -rf /\` (or ~ / \$HOME) would delete critical filesystem content." >&2 | ||
| exit 1 | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| # git push --force / -f (but not --force-with-lease) | ||
| if echo "$COMMAND" | grep -qE 'git[[:space:]]+push[[:space:]]+.*--force($|[[:space:]])' && ! echo "$COMMAND" | grep -q '\-\-force-with-lease'; then | ||
| echo "Blocked: \`git push --force\` can overwrite remote history. Use \`git push --force-with-lease\` instead." >&2 | ||
| exit 1 | ||
| fi | ||
| if echo "$COMMAND" | grep -qE 'git[[:space:]]+push[[:space:]]+.*[[:space:]]-[a-zA-Z]*f' && ! echo "$COMMAND" | grep -q '\-\-force-with-lease'; then | ||
| echo "Blocked: \`git push -f\` can overwrite remote history. Use \`git push --force-with-lease\` instead." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| # git reset --hard | ||
| if echo "$COMMAND" | grep -qE 'git[[:space:]]+reset[[:space:]]+--hard'; then | ||
| echo "Blocked: \`git reset --hard\` discards all uncommitted changes. Use \`git stash\` to preserve work." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| # git clean with both -f and -d flags (any order, combined or separate) | ||
| if echo "$COMMAND" | grep -qE 'git[[:space:]]+clean[[:space:]]'; then | ||
| CLEAN_HAS_F=false | ||
| CLEAN_HAS_D=false | ||
| echo "$COMMAND" | grep -qE '[[:space:]]+-[a-zA-Z]*f' && CLEAN_HAS_F=true | ||
| echo "$COMMAND" | grep -qE '[[:space:]]+-[a-zA-Z]*d' && CLEAN_HAS_D=true | ||
| if [[ "$CLEAN_HAS_F" = true && "$CLEAN_HAS_D" = true ]]; then | ||
| echo "Blocked: \`git clean -fd\` permanently deletes untracked files. Use \`git stash --include-untracked\` instead." >&2 | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| # git checkout -- . (discard all changes) | ||
| if echo "$COMMAND" | grep -qE 'git[[:space:]]+checkout[[:space:]]+--[[:space:]]+\.'; then | ||
| echo "Blocked: \`git checkout -- .\` discards all uncommitted changes. Use \`git stash\` to preserve work." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| # git restore . (discard all changes) | ||
| if echo "$COMMAND" | grep -qE 'git[[:space:]]+restore[[:space:]]+\.'; then | ||
| echo "Blocked: \`git restore .\` discards all uncommitted changes. Use \`git stash\` to preserve work." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| # chmod -R 777 | ||
| if echo "$COMMAND" | grep -qE 'chmod[[:space:]]+(-R[[:space:]]+777|777[[:space:]]+-R)'; then | ||
| echo "Blocked: \`chmod -R 777\` sets insecure permissions. Use more restrictive permissions (e.g., 755 for directories, 644 for files)." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| exit 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| #!/usr/bin/bash | ||
| # PreToolUse(Write) hook — scan file content for credentials before writing | ||
| set -euo pipefail | ||
|
|
||
| parse_json_field() { | ||
| local input="$1" | ||
| local field="$2" | ||
| if command -v jq &>/dev/null; then | ||
| echo "$input" | jq -r "($field) // empty" | ||
| elif command -v python3 &>/dev/null; then | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| echo "$input" | python3 -c " | ||
| import sys, json, functools | ||
| data = json.load(sys.stdin) | ||
| keys = '$field'.strip('.').split('.') | ||
| val = functools.reduce(lambda d, k: d.get(k, '') if isinstance(d, dict) else '', keys, data) | ||
| print(val if val else '') | ||
| " | ||
| else | ||
| echo "security-review: neither jq nor python3 available — cannot parse hook input" >&2 | ||
| exit 1 | ||
| fi | ||
| } | ||
|
|
||
| INPUT=$(cat) | ||
| FILE_PATH=$(parse_json_field "$INPUT" '.toolInput.file_path') | ||
| CONTENT=$(parse_json_field "$INPUT" '.toolInput.content') | ||
|
|
||
| if [[ -z "$CONTENT" ]]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| FOUND="" | ||
|
|
||
| # Credential value character class: reject $, {, quotes, and whitespace as first char | ||
| # This skips env var refs ($VAR, ${VAR}), templates ({{ }}), and quoted vars ("$VAR") | ||
| CRED_REJECT='[^${"'"'"'{}[:space:]]' | ||
|
|
||
| # AWS access key | ||
| if echo "$CONTENT" | grep -qE 'AKIA[0-9A-Z]{16}'; then | ||
| FOUND="${FOUND}\n- AWS access key (AKIA...) detected" | ||
| fi | ||
|
|
||
| # Private keys | ||
| if echo "$CONTENT" | grep -qE 'BEGIN (RSA )?PRIVATE KEY|BEGIN OPENSSH PRIVATE KEY'; then | ||
| FOUND="${FOUND}\n- Private key detected" | ||
| fi | ||
|
|
||
| # Password assignments | ||
| if echo "$CONTENT" | grep -qiE "(export[[:space:]]+)?password[[:space:]]*[:=][[:space:]]*${CRED_REJECT}"; then | ||
| FOUND="${FOUND}\n- Hardcoded password detected" | ||
| fi | ||
|
|
||
| # Token assignments | ||
| if echo "$CONTENT" | grep -qiE "(export[[:space:]]+)?token[[:space:]]*[:=][[:space:]]*${CRED_REJECT}"; then | ||
| FOUND="${FOUND}\n- Hardcoded token detected" | ||
| fi | ||
|
|
||
| # Secret assignments | ||
| if echo "$CONTENT" | grep -qiE "(export[[:space:]]+)?secret[[:space:]]*[:=][[:space:]]*${CRED_REJECT}"; then | ||
| FOUND="${FOUND}\n- Hardcoded secret detected" | ||
| fi | ||
|
|
||
| # API key assignments | ||
| if echo "$CONTENT" | grep -qiE "(export[[:space:]]+)?api_key[[:space:]]*[:=][[:space:]]*${CRED_REJECT}"; then | ||
| FOUND="${FOUND}\n- Hardcoded API key detected" | ||
| fi | ||
|
|
||
| # Database connection strings with embedded passwords (skip variable refs in password position) | ||
| if echo "$CONTENT" | grep -qiE '(mysql|postgres|postgresql|mongodb|redis)://[^:]+:[^${"'"'"'{}[:space:]@][^@]*@'; then | ||
| FOUND="${FOUND}\n- Database connection string with embedded password detected" | ||
| fi | ||
|
|
||
| # Sensitive file path check (warn only if content also has secrets) | ||
| if [[ -n "$FOUND" ]]; then | ||
| if echo "$FILE_PATH" | grep -qiE '\.(pem|key|p12|pfx)$'; then | ||
| FOUND="${FOUND}\n- Writing to sensitive file type: ${FILE_PATH}" | ||
| fi | ||
| if echo "$FILE_PATH" | grep -qiE '(credential|secret)'; then | ||
| FOUND="${FOUND}\n- Writing to sensitive path: ${FILE_PATH}" | ||
| fi | ||
| if echo "$FILE_PATH" | grep -qE '\.env$'; then | ||
| FOUND="${FOUND}\n- Writing to .env file: ${FILE_PATH}" | ||
| fi | ||
| fi | ||
|
|
||
| if [[ -n "$FOUND" ]]; then | ||
| echo -e "BLOCKED: Potential credentials detected in file content (${FILE_PATH}):${FOUND}\n\nUse environment variables or external secret management instead of hardcoded credentials." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| exit 0 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| #!/usr/bin/bash | ||
| # PreToolUse(Bash) hook — detect credentials in git staged content | ||
| set -euo pipefail | ||
|
|
||
| parse_command() { | ||
| if command -v jq &>/dev/null; then | ||
| echo "$1" | jq -r '.toolInput.command // empty' | ||
| elif command -v python3 &>/dev/null; then | ||
| echo "$1" | python3 -c 'import sys,json;print(json.load(sys.stdin).get("toolInput",{}).get("command",""))' | ||
| else | ||
| echo "security-review: neither jq nor python3 available — cannot parse hook input" >&2 | ||
| exit 1 | ||
| fi | ||
| } | ||
|
|
||
| INPUT=$(cat) | ||
| COMMAND=$(parse_command "$INPUT") | ||
|
|
||
| if [[ -z "$COMMAND" ]]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Only inspect git commit — git add is covered by the Write hook (check-file-secrets.sh) | ||
| if ! echo "$COMMAND" | grep -qE 'git\s+commit'; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| STAGED=$(git diff --cached --diff-filter=ACM 2>/dev/null || true) | ||
| if [[ -z "$STAGED" ]]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| FOUND="" | ||
|
|
||
| if echo "$STAGED" | grep -qE 'AKIA[0-9A-Z]{16}'; then | ||
| FOUND="${FOUND}\n- AWS access key (AKIA...) detected in staged content" | ||
| fi | ||
|
|
||
| if echo "$STAGED" | grep -qE 'BEGIN (RSA )?PRIVATE KEY|BEGIN OPENSSH PRIVATE KEY'; then | ||
| FOUND="${FOUND}\n- Private key detected in staged content" | ||
| fi | ||
|
|
||
| if echo "$STAGED" | grep -qiE '(export\s+)?password\s*[:=]\s*\S'; then | ||
| FOUND="${FOUND}\n- Hardcoded password detected in staged content" | ||
| fi | ||
|
|
||
| if echo "$STAGED" | grep -qiE '(export\s+)?token\s*[:=]\s*\S'; then | ||
| FOUND="${FOUND}\n- Hardcoded token detected in staged content" | ||
| fi | ||
|
|
||
| if echo "$STAGED" | grep -qiE '(export\s+)?secret\s*[:=]\s*\S'; then | ||
| FOUND="${FOUND}\n- Hardcoded secret detected in staged content" | ||
| fi | ||
|
|
||
| if echo "$STAGED" | grep -qiE '(export\s+)?api_key\s*[:=]\s*\S'; then | ||
| FOUND="${FOUND}\n- Hardcoded API key detected in staged content" | ||
| fi | ||
|
|
||
| if echo "$STAGED" | grep -qE '^\+\+\+ b/.*\.env$'; then | ||
| FOUND="${FOUND}\n- .env file detected in staged content" | ||
| fi | ||
|
|
||
| if [[ -n "$FOUND" ]]; then | ||
| echo -e "BLOCKED: Potential credentials found in staged changes:${FOUND}\n\nUse environment variables instead of hardcoded credentials. Remove sensitive data and use .gitignore to exclude secret files." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| exit 0 |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: does this needs to be a Claude plugin? sounds more like a git pre-commit, it could be more efficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that's fair (also echo's what @brandisher mentioned)
I think there are arguments that it could be both so you can apply it across the repos you work in but we enforce it within this repo via githooks/claude hooks