Add security guards and canary env to ASIM CI#14531
Open
v-sabiraj wants to merge 2 commits into
Open
Conversation
Harden the ASIM workflow by adding explicit security boundaries and reviewer gating. Updates include:
- Add a SECURITY BOUNDARIES header describing OIDC federated identity scoping (canary workspace/rg) and fork PR SafeToRun labeling requirements.
- Enhance the PR guidance comment to include critical checks for KQL and sample-data CSVs (review ParserQuery for cross-workspace/cluster/externaldata use, validate CSV Type column/table names, check for injection/sensitive data, ensure queries are read-only and target the canary workspace).
- Preserve audit trail by always creating a new comment and replace commentBody with enhancedCommentBody when appropriate.
- Require SafeToRun ('labeled') gating for forked PRs on secret-bearing jobs and bind OIDC/secret access to a protected GitHub environment (environment: asim-canary) so platform-enforced approvals are required.
- Add inline security notes around azure/login usage to emphasize that AZURE_ASIM_CLIENT_ID must be restricted to the canary workspace/resource group and cannot access other workspaces/subscriptions.
These changes reduce the risk of credential misuse and KQL/CSV-based abuse by enforcing reviewer approval and limiting federated identity scope.
Add a security check that validates the GitHub user who applies the 'SafeToRun' label on forked PRs has sufficient repository permissions. The workflow now runs an actions/github-script step to call repos.getCollaboratorPermissionLevel and only allows admin/maintain/write; it fails the security-gate if the approver is triage/read/none. Also remove the environment: asim-canary bindings from several jobs (previously required reviewers via a protected Environment) so the dynamic permission check governs approval for fork PR test runs instead.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Hardens the ASIM CI workflow by documenting security boundaries, tightening fork-PR approval gating, and improving reviewer guidance for potentially risky KQL/CSV changes.
Changes:
- Added explicit security-boundary documentation for OIDC scope and fork PR SafeToRun requirements.
- Added a permission check for SafeToRun labelers and expanded the guidance comment with KQL/CSV abuse-review steps.
- Added inline security notes around
azure/loginusage and adjusted fork-PR job gating logic.
Comment on lines
2
to
+7
| # The script runs ASIM Schema and Data testers on the "eco-connector-test" workspace. | ||
| # | ||
| # SECURITY BOUNDARIES: | ||
| # - OIDC federated identity (AZURE_ASIM_CLIENT_ID) is scoped to canary workspace only: | ||
| # Workspace: ASIM-SchemaDataTester-GithubShared-Canary | ||
| # Resource Group: asim-schemadatatester-githubshared-canary |
Comment on lines
+542
to
+546
| # SECURITY: same fork-PR gate as the other secret-bearing jobs - fork PRs may only | ||
| # run on the 'labeled' (SafeToRun) event; internal PRs run unconditionally. | ||
| if: | | ||
| (github.event.pull_request.head.repo.fork == false || | ||
| github.event.action == 'labeled') |
| @@ -377,6 +439,11 @@ jobs: | |||
| pip install azure-monitor-ingestion | |||
| pip install azure-core | |||
| - name: Login to Azure Public Cloud | |||
Comment on lines
447
to
449
| uses: azure/login@a457da9ea143d694b1b9c7c869ebb04ebe844ef5 | ||
| with: | ||
| client-id: ${{ secrets.AZURE_ASIM_CLIENT_ID }} |
Comment on lines
+311
to
+324
| // Update security guidance to emphasize KQL and CSV review | ||
| let enhancedCommentBody = commentBody; | ||
| if (enhancedCommentBody.includes('**Verify file types**')) { | ||
| enhancedCommentBody = enhancedCommentBody.replace( | ||
| '2. ✅ **Verify file types** - This PR should only contain `.yml`, `.yaml`, or `.json` files. Check for any executable scripts (.ps1, .py, .sh, .exe, etc.) which are not allowed in this context.', | ||
| '2. ✅ **Verify file types** - This PR should only contain `.yml`, `.yaml`, `.json`, or `.csv` files. Reject any executable scripts (.ps1, .py, .sh, .exe, etc.).\n ⚠️ **CRITICAL: Review KQL and CSV content for abuse:**\n - Read the **ParserQuery** KQL for unauthorized data access/manipulation and reject cross-workspace/cluster references (e.g. `workspace(...)`, `cluster(...)`, `externaldata`)\n - Inspect the **sample-data** CSV `Type` column for path-traversal-shaped or non-allowlisted table names (must match the builtin allowlist or a `_CL` suffix)\n - Examine **sample-data** CSVs for injection attempts or sensitive data disclosure\n - Check that queries are read-only and target only the fixed test/canary workspace' | ||
| ); | ||
| } | ||
| if (enhancedCommentBody.includes('**Verify file types** - Ensure')) { | ||
| enhancedCommentBody = enhancedCommentBody.replace( | ||
| '2. ✅ **Verify file types** - Ensure commits only contain `.yml`, `.yaml`, or `.json` files. Reject if any executable scripts (.ps1, .py, .sh, .exe, etc.) are included.', | ||
| '2. ✅ **Verify file types** - Ensure commits only contain `.yml`, `.yaml`, `.json`, or `.csv` files. Reject if any executable scripts (.ps1, .py, .sh, .exe, etc.) are included.\n ⚠️ **CRITICAL: Review KQL and CSV content for abuse:**\n - Read the **ParserQuery** KQL for unauthorized data access/manipulation and reject cross-workspace/cluster references (e.g. `workspace(...)`, `cluster(...)`, `externaldata`)\n - Inspect the **sample-data** CSV `Type` column for path-traversal-shaped or non-allowlisted table names (must match the builtin allowlist or a `_CL` suffix)\n - Examine **sample-data** CSVs for injection attempts or sensitive data disclosure\n - Check that queries are read-only and target only the fixed test/canary workspace' | ||
| ); | ||
| } |
| # Identity permissions: Canary RG/workspace only; deny cross-workspace and out-of-RG access | ||
| # - Fork PRs require explicit "SafeToRun" label approval from maintainer | ||
| # - Label is auto-removed on new commits (synchronize event) to prevent TOCTOU | ||
| # - All validation scripts are re-downloaded from master branch before execution |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Harden the ASIM workflow by adding explicit security boundaries and reviewer gating. Updates include:
These changes reduce the risk of credential misuse and KQL/CSV-based abuse by enforcing reviewer approval and limiting federated identity scope.
Required items, please complete
Change(s):
Reason for Change(s):
Version Updated:
Testing Completed:
Checked that the validations are passing and have addressed any issues that are present:
Guidance <- remove section before submitting
Before submitting this PR please ensure that you have read the following sections and filled out the changes, reason for change and testing complete sections:
Thank you for your contribution to the Microsoft Sentinel Github repo.
Change(s):
Reason for Change(s):
Version updated:
Testing Completed:
Note: If updating a detection, you must update the version field.
Checked that the validations are passing and have addressed any issues that are present:
Note: Let us know if you have tried fixing the validation error and need help.