feat: add CIS GitHub organization controls (CIS GH PR 2 of 2)#1763
Draft
thetechgy wants to merge 33 commits into
Draft
feat: add CIS GitHub organization controls (CIS GH PR 2 of 2)#1763thetechgy wants to merge 33 commits into
thetechgy wants to merge 33 commits into
Conversation
Introduces the foundation layer for Maester CIS GitHub Enterprise Cloud
benchmark tests. CIS controls consuming this infrastructure arrive in a
follow-up PR; this PR intentionally ships connection-only to allow
maintainer review of the integration pattern before controls are added.
New public function:
- Connect-MtGitHub: validates a PAT via GET /user and GET /orgs/{org},
stores session state in $__MtSession, supports MAESTER_GITHUB_TOKEN
and GH_TOKEN env vars, and is configurable for GHE.com EMU deployments
via -ApiBaseUri.
New internal functions:
- Invoke-MtGitHubRequest: PS 5.1-compatible (Invoke-WebRequest) cached,
paginating GET wrapper with rate-limit detection.
- Get-MtGitHubResponseHeaderValue: case-insensitive header extraction
compatible with both PS 5.1 WebHeaderCollection and PS 7
HttpResponseHeaders.
- Get-MtGitHubErrorStatusCode: safely extracts HTTP status codes
from exception responses cross-platform.
- Get-MtGitHubErrorMessage: extracts GitHub API error body for skip
and error detail output.
Changes to existing files:
- Maester.psm1: adds GitHubCache = @{} to $__MtSession initializer.
- Clear-ModuleVariable.ps1: resets GitHubConnection, GitHubAuthHeader,
and GitHubCache on each Invoke-Maester run.
- Test-MtConnection.ps1: adds 'GitHub' service region using cached
$__MtSession.GitHubConnection state (no auto-detect; requires
explicit Connect-MtGitHub call).
- Add-MtTestResultDetail.ps1: adds 'NotConnectedGitHub' to ValidateSet.
- Get-MtSkippedReason.ps1: adds 'NotConnectedGitHub' skip message.
- Maester.psd1: exports Connect-MtGitHub.
- tests/maester-config.json: adds GitHubOrganization, GitHubApiBaseUri,
and GitHubApiVersion to GlobalSettings.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers all branching paths in the cross-version header extraction helper: null input, IDictionary path (PS 5.1 WebHeaderCollection — exact case, different case, array value, missing header), GetValues path (PS 7 HttpResponseHeaders — success and throw), and TryGetValues path (PS 7 — true and false return). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Clear-ModuleVariable nulled GitHubConnection and GitHubAuthHeader on every Invoke-Maester call, discarding any Connect-MtGitHub session established beforehand. Only GitHubCache needs to be reset per run, consistent with how $__MtSession.Connections (MS Graph) is handled. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Connect-MtGitHub relied on Get-MtMaesterConfigGlobalSetting for the Organization, ApiBaseUri, and ApiVersion fallbacks, but MaesterConfig is only populated inside Invoke-Maester — after Clear-ModuleVariable has already run. Calling Connect-MtGitHub before Invoke-Maester (the documented flow) silently dropped all three config fallbacks. Add a lazy-load block that calls Get-MtMaesterConfig once when MaesterConfig is null and any config-backed parameter is omitted, covering all three values rather than Organization alone. Also fix a latent ValidatePattern bug: assigning $null from a config lookup directly to $ApiBaseUri or $ApiVersion (both declared with [ValidatePattern]) throws ValidationMetadataException inside the function body. Use local intermediate variables for the lookups and only update the parameter variable when the result is non-empty. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Clear-ModuleVariable: regression guard verifying GitHubConnection and GitHubAuthHeader survive Invoke-Maester's run-start reset while GitHubCache is cleared (Count -eq 0) and Test-MtConnection GitHub still returns true. Connect-MtGitHub: covers NotConfigured, NoToken, TokenInvalid, and OrgAccessFailed (403 and 404) failure modes; successful connection with probe URI and X-GitHub-Api-Version header assertions; config resolution from pre-loaded MaesterConfig without calling Get-MtMaesterConfig; and lazy-load paths for all three config-backed parameters (Organization, ApiBaseUri, ApiVersion). Invoke-MtGitHubRequest: covers connection guard (null and false), cache hit (web request called once for two identical calls), cache store (key verified as "$version|$absUri"), -DisableCache bypass without overwriting an existing entry, single-page and paginated (Link rel=next) responses, verbose rate-limit warning at remaining=0, primary rate-limit throw (403/429), and secondary rate-limit throw. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add GitHub to Test-MtConnection comment-based help: valid service list, behavior note (explicit Connect-MtGitHub required, treated as required in -Service All), updated example descriptions, new -Service GitHub example - Improve Get-MtGitHubErrorMessage to extract the JSON .message field from GitHub REST error bodies so failures show clean strings instead of raw JSON - Add Test-MtConnection.Tests.ps1 with 7 GitHub-focused unit tests covering null/sentinel/disconnected/connected states and -Details/-Service All paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
GitHub was unconditionally entered when -Service All was used, stamping a NotCalled sentinel and setting ConnectionState = $false whenever Connect-MtGitHub had not been called. This silently broke existing users relying on -Service All for Microsoft services only. GitHub is now skipped under All unless a real connection attempt has been made (GitHubConnection is non-null and FailureReason is not 'NotCalled'). Explicit -Service GitHub remains strict. Update docstrings and examples to match the new opt-in semantics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous -Service All -Details test hit real cmdlets (Get-AzContext, Get-MgContext, Get-MtExo, Get-CsTenant, Get-ADOPSConnection), taking 60+ seconds and emitting Azure token-cache warnings. Replace with four mocked tests using Mock -ModuleName Maester: - -Service GitHub -Details: details-object shape when GitHub connected - Regression test: all MS services mocked connected + GitHub null → AllConnected must be $true (proves the fix, not just property absence) - NotCalled sentinel: GitHub skipped even when sentinel exists from a prior -Service GitHub probe - GitHub connected: property populated when Connect-MtGitHub was called Also reset AzureDevOpsConnection in BeforeEach/AfterEach to prevent state leak between tests. Suite now completes in ~7s. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…st perf
- Get-MtSession: redact GitHubAuthHeader Authorization on output (case-insensitive,
fail-closed for non-dictionary shapes); live session unchanged so internal callers
still work. Prevents PAT disclosure in troubleshooting dumps.
- Add Disconnect-MtGitHub to clear GitHub session state; export from manifest.
- Disconnect-Maester (and Disconnect-MtMaester alias) now also clears GitHub state;
Disconnect-MtGraph alias keeps its narrow Graph-only semantic.
- Connect-MtGitHub: add /orgs/{org}/memberships/{user} probe to verify org role.
New fields Role, RoleState, RoleVerified, RoleVerificationFailureReason
distinguish "known non-admin" from "could not check". Warns on member/pending/
probe-failure/malformed-body without blocking connection.
- Test-MtConnection.Tests.ps1: stub MS service cmdlets in BeforeAll when absent
(cleaned up in AfterAll), bypassing Pester's slow command-resolution path.
Cuts the -Service All regression test from ~63s to ~180ms.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Satisfies the repo's PSUseBOMForUnicodeEncodedFile analyzer rule (enforced by powershell/tests/pester.ps1) on every .ps1 this branch touches that contains non-ASCII content. Without the BOM, build-validation.yaml fails. BOM was prepended via a byte-preserving rewrite (File.ReadAllBytes + Buffer.BlockCopy + File.WriteAllBytes) so file content after the BOM is byte-identical to the prior version. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…nect
Connect-MtGitHub: promote /orgs/{org}/memberships/{user} from advisory to
blocking. /orgs/{org} returns public metadata even for tokens with no
relationship to the org, so it is not a real access proof. 4xx, malformed
body, or missing state/role on the membership probe now sets
FailureReason = 'OrgMembershipFailed' and aborts before storing auth
headers; valid non-admin / pending states still connect with a warning.
Connect-MtGitHub: revalidate the resolved ApiBaseUri after the
param→config→default fallback. [ValidatePattern('^https://')] only fires
on the bound parameter, so a config-sourced http:// URI could otherwise
reach Invoke-WebRequest with a Bearer header. Resolution now uses a local
variable, then enforces an absolute https:// URI via [uri]::TryCreate +
case-sensitive scheme check, failing with FailureReason = 'InvalidApiBaseUri'.
Disconnect-Maester: normalize $MyInvocation.InvocationName by splitting on
the literal '\\' qualifier (PowerShell uses '\\' on all OSes; Split-Path
-Leaf would no-op on Linux). 'Maester\\Disconnect-Maester' now routes to
the GitHub-clearing branch alongside the bare and Disconnect-MtMaester
forms; Disconnect-MtGraph keeps its narrow Graph-only semantic.
Tests: move the five membership-failure cases into a new OrgMembershipFailed
context with Connected=$false assertions; add an InvalidApiBaseUri context
covering http:// config, non-URI config, and trailing-slash trim on a valid
parameter; add a Maester\\Disconnect-Maester clearing test. Failure-path
tests also assert GitHubAuthHeader stays \$null to lock in the security
property that no Bearer header is left behind on a failed connection.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…-MtGitHub
Adds a fourth non-blocking probe to GET /orgs/{org}/actions/permissions
to verify the token can reach an org-administration endpoint. Failure
records AdministrationPermissionVerified = $false and warns about both
classic (admin:org) and fine-grained (Administration: read) permission
models, but does not flip Connected — the session remains usable for
controls that do not require org administration access. Captures the
x-accepted-github-permissions response header on failures.
Validates the resolved ApiVersion locally so a malformed config value
no longer reaches /user as X-GitHub-Api-Version and gets misreported
as TokenInvalid. Removes the parameter [ValidatePattern] so invalid
-ApiVersion input also flows through the resolved-value check, which
ensures session-clearing logic runs first. /user catch now maps 410
and 400 with API/version wording (including GitHub's documented "Not
a supported version" message) to InvalidApiVersion instead of
TokenInvalid.
Updates synopsis to describe the session as organization-scoped (not
enterprise-admin) and lists required token permissions for both PAT
types.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Remove [ValidatePattern] on -ApiBaseUri so invalid parameter values flow through the existing in-body URI check after session state is cleared, instead of throwing before the cleanup runs. - Split /user error classification: 401 stays TokenInvalid, $null status code (DNS/TLS/connect failure) becomes ApiBaseUriFailed, and 5xx becomes a new ApiUnavailable reason. Host messages no longer describe transport or service failures as token validation failures. - Refresh stale comments that referenced the removed [ValidatePattern]. - Add regression tests for invalid -ApiBaseUri parameter values, /user transport failures (no Response), and /user 5xx responses. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds a GitHub ListItem to the Maester.Connections list view so Test-MtConnection -Service GitHub -Details visibly renders connection details. The formatter only displays safe metadata (Connected, Organization, TokenLogin, ApiBaseUri, Role, RoleState, AdministrationPermissionVerified) and never touches the auth header, Authorization, or token values. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Compute the invocation-name check up front, then wrap the existing Graph/Azure/EXO/Teams disconnect calls in try/finally so Disconnect-MtGitHub runs even when an earlier service-disconnect throws. Original exceptions still propagate. Disconnect-MtGraph alias keeps its Graph-only semantic and does not clear GitHub. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
A malicious or buggy upstream that injects a foreign URL into the Link header would otherwise receive the Authorization header on a cross-origin request. Validate each next URL against the configured ApiBaseUri (scheme, host, port, and base path prefix so GHE-style https://host/api/v3 bases work) and throw before issuing the request when the origin does not match. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Connect-MtGitHub bootstrap probes used raw Invoke-WebRequest catches that
did not detect GitHub primary/secondary rate-limit responses. A valid
token receiving HTTP 403/429 with rate-limit headers could be reported
as TokenInvalid, OrgAccessFailed, OrgMembershipFailed, or as a misleading
admin-permission warning.
Extract rate-limit detection into Get-MtGitHubRateLimitMessage so both
Invoke-MtGitHubRequest and Connect-MtGitHub share one implementation.
The helper uses [int]::TryParse / [long]::TryParse so a malformed
x-ratelimit-remaining or x-ratelimit-reset header cannot mask the
underlying error with a parse exception.
Blocking probes (/user, /orgs/{org}, /orgs/{org}/memberships/{user})
short-circuit on rate limit with FailureReason = 'RateLimited' and do
not store the auth header. The non-blocking admin probe keeps the
session connected but records a rate-limit-specific failure reason and
warning instead of implying missing permissions.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ConvertFrom-Json '[]' yields $null because it enumerates the empty array, so AddRange(@($body)) was contributing one $null item per empty page. A list endpoint with no results would surface as a single-$null collection. Short-circuit the empty-array case before parsing and filter $null while merging pages so empty intermediate pages also don't pollute the result. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Pending membership (invitation not yet accepted) was previously treated as a successful connection with a warning. Org-scoped controls would silently fail or report misleading data because the user can't actually act on the org until membership is active. Treat any non-active membership state as a connection failure with FailureReason = 'OrgMembershipPending'. Auth header is not retained because the session-clear at the top of Connect-MtGitHub already nulled it before the failure return. Active non-admin roles still connect with a limited-coverage warning, unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
GitHub returns 403/429 for secondary rate limits, but does not always include a retry-after header (older abuse-detection responses, or response chains where a proxy strips the header). Fall back to matching "secondary rate limit" / "abuse detection" wording in the response body so callers see a rate-limit message instead of a generic permission failure. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tion Three related Connect-MtGitHub changes from the latest review pass: - Restrict ApiBaseUri to api.github.com and api.<subdomain>.ghe.com, with no path, query, fragment, or non-default port. Rejected URIs short-circuit before any web request runs, so the PAT cannot be sent to an attacker-supplied or misconfigured host. - Apply the existing /user transport/5xx classification to the /orgs and /memberships probes too: a transport failure now reports ApiBaseUriFailed and a 5xx reports ApiUnavailable instead of being conflated with org-access or membership permission failures. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Surrounding whitespace from a config file or shell-quoted parameter would otherwise be URL-encoded into probe paths and silently 404, or fail the ApiVersion format check despite a valid date. Add UTF-8 BOM to the test file so ScriptAnalyzer no longer flags PSUseBOMForUnicodeEncodedFile. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
A malformed x-ratelimit-remaining (proxy-rewritten value) or an out-of-range x-ratelimit-reset epoch would raise a parse exception that masked an otherwise successful GitHub response. Use TryParse for both headers and wrap FromUnixTimeSeconds in try/catch, falling back to 'unknown' when the reset epoch is outside the [-62135596800, 253402300799] range accepted by .NET. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Whitespace-padded ApiBaseUri values from -ApiBaseUri or config-supplied GitHubApiBaseUri were only trailing-slash trimmed, so they passed URI validation but URL-encoded into probe paths and silently 404'd. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add cached GitHub organization evidence helpers, five CIS GitHub v1.2.0 organization-setting tests, companion docs, Pester coverage, manifest exports, and TestSettings metadata.
Keep deletion controls strict by default while adding explicit manual-review config branches for CIS audit paths. Make internal repository creation informational for CIS.GH.1.2.2, normalize GitHub CIS docs/help, and centralize GitHub cache key construction. Update Pester coverage for manual review handling, Test-MtConnection GitHub behavior, cache helper usage, and manifest export surface.
- mark repository and issue deletion controls as Investigate when evidence is present - remove unreleased deletion opt-in config keys - align repository creation evidence with members_allowed_repository_creation_type - add GitHub command docs and refresh generated CIS docs/tests
Up to standards ✅🟢 Issues
|
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
1 participant
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.
📑 Description
Adds the second GitHub CIS contribution on top of #1756, focused on the first defensible set of organization-level CIS GitHub Benchmark v1.2.0 controls that can be backed by documented GitHub REST API evidence.
Depends on #1756.
This PR adds:
$falsesettings are treated as available evidence instead of missing dataCIS.GH.1.2.2- Ensure repository creation is limited to specific membersCIS.GH.1.2.3- Ensure repository deletion is limited to specific usersCIS.GH.1.2.4- Ensure issue deletion is limited to specific usersCIS.GH.1.3.2- Ensure team creation is limited to specific membersCIS.GH.1.3.8- Ensure strict base permissions are set for repositoriesCIS marks these recommendations as Manual. The tests added here automate the portion of each audit procedure that maps directly to documented GitHub REST API fields, and the docs call out that this is automated evidence collection rather than a claim that every possible manual review path has been fully evaluated.
The original action-token, fork-secret, and Actions approval checks are intentionally excluded from this CIS PR because they do not clearly map to CIS GitHub Benchmark v1.2.0 control IDs. They can be considered later as non-CIS GitHub hardening tests under a separate namespace.
✅ Checks
/powershell/tests/pester.ps1locally.ℹ️ Additional Information
Validation:
/powershell/tests/pester.ps1- all 5863 tests executed without failuresStacking note: this branch is based on
thetechgy:feat/github-cis-foundation, which is open as #1756. Until #1756 is merged, GitHub may show the foundation commits in this PR's comparison againstmain.Future follow-up candidates, kept out of this PR to keep the scope focused:
CIS.GH.1.3.3CIS.GH.1.3.5CIS.GH.3.1.1How to Contribute
🏗️ Read our full contributing guide for the Maester project.
🧪 We also have additional instructions and a checklist for creating tests.
Join us at the Maester repository discussions or Entra Discord for more help and conversations!
While you wait for a review, why not spread some Maester love on social media? Thank you! 💖