-
Notifications
You must be signed in to change notification settings - Fork 0
P2: Enterprise policy.json (updates/auth constraints, rollout control) #45
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| # Enterprise policy.json | ||
|
|
||
| Enterprise deployments can enforce guardrails using a machine-scope `policy.json`. | ||
|
|
||
| Default location: | ||
| - `%ProgramData%\CloudSQLCTL\policy.json` | ||
|
|
||
| Override location (for testing): | ||
| - `CLOUDSQLCTL_POLICY_PATH=<path>` | ||
|
|
||
| ## Example | ||
|
|
||
| ```json | ||
| { | ||
| "updates": { | ||
| "enabled": false, | ||
| "channel": "stable", | ||
| "pinnedVersion": "0.4.15" | ||
| }, | ||
| "auth": { | ||
| "allowUserLogin": false, | ||
| "allowAdcLogin": true, | ||
| "allowServiceAccountKey": true, | ||
| "allowedScopes": ["Machine"] | ||
| } | ||
| } | ||
| ``` | ||
|
Comment on lines
+13
to
+27
|
||
|
|
||
| ## Behavior | ||
|
|
||
| - If `updates.enabled` is `false`, `cloudsqlctl upgrade` will fail with a policy error. | ||
| - If `updates.channel` is set, `cloudsqlctl upgrade --channel` cannot override it. | ||
| - If `updates.pinnedVersion` is set, `--version`, `--pin`, and `--unpin` are restricted. | ||
| - `auth.login`, `auth.adc`, and `auth set-service-account` can be allowed/blocked via `auth.*`. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,96 @@ | ||||||||||||||||||||||||||
| import fs from 'fs-extra'; | ||||||||||||||||||||||||||
| import path from 'path'; | ||||||||||||||||||||||||||
| import { SYSTEM_PATHS } from '../system/paths.js'; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export type PolicyUpdateChannel = 'stable' | 'beta'; | ||||||||||||||||||||||||||
| export type PolicyScope = 'User' | 'Machine'; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export interface EnterprisePolicy { | ||||||||||||||||||||||||||
| updates?: { | ||||||||||||||||||||||||||
| enabled?: boolean; | ||||||||||||||||||||||||||
| channel?: PolicyUpdateChannel; | ||||||||||||||||||||||||||
| pinnedVersion?: string; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| auth?: { | ||||||||||||||||||||||||||
| allowUserLogin?: boolean; | ||||||||||||||||||||||||||
| allowAdcLogin?: boolean; | ||||||||||||||||||||||||||
| allowServiceAccountKey?: boolean; | ||||||||||||||||||||||||||
| allowedScopes?: PolicyScope[]; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export interface ResolvedUpgradePolicy { | ||||||||||||||||||||||||||
| channel?: PolicyUpdateChannel; | ||||||||||||||||||||||||||
| targetVersion?: string; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export function getPolicyPath(): string { | ||||||||||||||||||||||||||
| const fromEnv = process.env.CLOUDSQLCTL_POLICY_PATH; | ||||||||||||||||||||||||||
| if (fromEnv) return path.resolve(fromEnv); | ||||||||||||||||||||||||||
| return SYSTEM_PATHS.POLICY_FILE; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| function normalizeVersion(version: string): string { | ||||||||||||||||||||||||||
| return version.startsWith('v') ? version.slice(1) : version; | ||||||||||||||||||||||||||
|
Comment on lines
+33
to
+34
|
||||||||||||||||||||||||||
| function normalizeVersion(version: string): string { | |
| return version.startsWith('v') ? version.slice(1) : version; | |
| const SEMVER_REGEX = /^\d+\.\d+\.\d+(?:-[0-9A-Za-z-.]+)?(?:\+[0-9A-Za-z-.]+)?$/; | |
| function normalizeVersion(version: string): string { | |
| const normalized = version.startsWith('v') ? version.slice(1) : version; | |
| if (!SEMVER_REGEX.test(normalized)) { | |
| throw new Error(`Invalid version format '${version}'. Expected semantic version like '1.2.3'.`); | |
| } | |
| return normalized; |
Copilot
AI
Dec 22, 2025
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.
The readPolicy function performs no validation on the structure or values of the parsed JSON. While TypeScript types provide compile-time safety, at runtime the JSON could contain invalid values (e.g., updates.channel = "invalid", allowedScopes = ["InvalidScope"]). Consider adding runtime validation to ensure the policy adheres to expected schemas, especially for enum-like fields such as channel (stable/beta) and allowedScopes (User/Machine).
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.
suggestion: Explicitly type the return value of resolveUpgradePolicy to keep the ResolvedUpgradePolicy contract stable.
The current implementation relies on inference and only the early return is checked with satisfies ResolvedUpgradePolicy. Other return paths aren’t validated against that shape. Declaring the return type as : ResolvedUpgradePolicy ensures all branches conform and prevents future changes from adding or omitting fields without a type error.
| export function resolveUpgradePolicy(policy: EnterprisePolicy | null, options: { channel?: string; version?: string; pin?: string; unpin?: boolean; }) { | |
| export function resolveUpgradePolicy(policy: EnterprisePolicy | null, options: { channel?: string; version?: string; pin?: string; unpin?: boolean; }): ResolvedUpgradePolicy { |
Copilot
AI
Dec 22, 2025
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.
When policy.auth.allowedScopes is defined but empty (e.g., []), the includes check on line 92 will always return false, blocking all service account operations regardless of scope. While this might be intentional, consider handling this edge case explicitly or documenting this behavior, as an empty array might be a configuration mistake rather than an intent to block all scopes.
| if (action === 'set-service-account' && scope && policy.auth?.allowedScopes && !policy.auth.allowedScopes.includes(scope)) { | |
| throw new Error(`Scope '${scope}' is not allowed by enterprise policy.`); | |
| if (action === 'set-service-account' && scope && policy.auth?.allowedScopes) { | |
| if (policy.auth.allowedScopes.length === 0) { | |
| throw new Error("Service account scope is not allowed: enterprise policy 'allowedScopes' is configured but empty, so no scopes are permitted."); | |
| } | |
| if (!policy.auth.allowedScopes.includes(scope)) { | |
| throw new Error(`Scope '${scope}' is not allowed by enterprise policy.`); | |
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,55 @@ | ||||||||||||||||||||||||
| import fs from 'fs-extra'; | ||||||||||||||||||||||||
| import os from 'os'; | ||||||||||||||||||||||||
| import path from 'path'; | ||||||||||||||||||||||||
| import { readPolicy, resolveUpgradePolicy, assertPolicyAllowsAuth } from '../src/core/policy.js'; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| function tmpFile(name: string) { | ||||||||||||||||||||||||
| return path.join(os.tmpdir(), `cloudsqlctl-${name}-${Date.now()}-${Math.random().toString(16).slice(2)}.json`); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| describe('Policy Module', () => { | ||||||||||||||||||||||||
| const originalEnv = process.env.CLOUDSQLCTL_POLICY_PATH; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| afterEach(async () => { | ||||||||||||||||||||||||
| if (originalEnv === undefined) { | ||||||||||||||||||||||||
| delete process.env.CLOUDSQLCTL_POLICY_PATH; | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| process.env.CLOUDSQLCTL_POLICY_PATH = originalEnv; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| it('returns null if policy does not exist', async () => { | ||||||||||||||||||||||||
| process.env.CLOUDSQLCTL_POLICY_PATH = tmpFile('missing'); | ||||||||||||||||||||||||
| const policy = await readPolicy(); | ||||||||||||||||||||||||
| expect(policy).toBeNull(); | ||||||||||||||||||||||||
|
Comment on lines
+21
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (testing): Add a positive-path test for successfully reading a valid policy file, including the default path behavior Currently we only cover failure modes (missing file, invalid JSON). Please add a test that writes a valid Suggested implementation: it('returns null if policy does not exist', async () => {
process.env.CLOUDSQLCTL_POLICY_PATH = tmpFile('missing');
const policy = await readPolicy();
expect(policy).toBeNull();
});
it('reads a valid policy file from CLOUDSQLCTL_POLICY_PATH', async () => {
const file = tmpFile('policy');
const policyContent = {
// Adjust this shape to the minimal valid EnterprisePolicy for your implementation
// (e.g. allowedProjects, rules, etc.)
exampleKey: 'example-value',
};
await fs.promises.writeFile(file, JSON.stringify(policyContent), 'utf8');
process.env.CLOUDSQLCTL_POLICY_PATH = file;
const policy = await readPolicy();
expect(policy).not.toBeNull();
expect(policy).toMatchObject(policyContent);
});
|
||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| it('throws if policy exists but is invalid json', async () => { | ||||||||||||||||||||||||
| const p = tmpFile('invalid'); | ||||||||||||||||||||||||
| await fs.writeFile(p, '{not-json', 'utf8'); | ||||||||||||||||||||||||
| process.env.CLOUDSQLCTL_POLICY_PATH = p; | ||||||||||||||||||||||||
| await expect(readPolicy()).rejects.toThrow(/Invalid policy\.json/); | ||||||||||||||||||||||||
| await fs.remove(p); | ||||||||||||||||||||||||
|
Comment on lines
+29
to
+32
|
||||||||||||||||||||||||
| await fs.writeFile(p, '{not-json', 'utf8'); | |
| process.env.CLOUDSQLCTL_POLICY_PATH = p; | |
| await expect(readPolicy()).rejects.toThrow(/Invalid policy\.json/); | |
| await fs.remove(p); | |
| try { | |
| await fs.writeFile(p, '{not-json', 'utf8'); | |
| process.env.CLOUDSQLCTL_POLICY_PATH = p; | |
| await expect(readPolicy()).rejects.toThrow(/Invalid policy\.json/); | |
| } finally { | |
| await fs.remove(p); | |
| } |
Copilot
AI
Dec 22, 2025
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.
There is no test case that verifies reading a valid, well-formed policy.json file. The tests only cover missing files (line 21-25) and invalid JSON (line 27-33), but don't verify that a correctly formatted policy file is parsed successfully with all fields intact. Consider adding a test that writes a complete policy JSON, reads it, and verifies all fields are correctly parsed.
Copilot
AI
Dec 22, 2025
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.
Missing test coverage for the case where a policy sets only channel without pinnedVersion. The function returns different results in line 76 (returns only channel) versus line 73 (returns both channel and targetVersion). Add a test to verify that when only policy.updates.channel is set, the function correctly returns just the enforced channel without a targetVersion.
Copilot
AI
Dec 22, 2025
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.
When a policy enforces a channel restriction but the user doesn't provide any channel option, the code correctly uses the enforced channel. However, there's no test case covering the scenario where policy.updates.channel is set WITHOUT pinnedVersion and the user provides a version option. In this case, the function would return just the enforced channel (line 76), which seems correct, but this behavior should be explicitly tested to ensure channel enforcement works independently of version constraints.
Copilot
AI
Dec 22, 2025
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.
The test for auth guardrails only covers allowUserLogin and allowedScopes, but doesn't test the allowAdcLogin and allowServiceAccountKey policy fields that are checked on lines 85-90 of src/core/policy.ts. Consider adding test cases to verify that these policy settings are enforced correctly for the 'adc' and 'set-service-account' actions.
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.
Documenting
CLOUDSQLCTL_POLICY_PATHas an override for the machine-scope policy means any local user can point the CLI at a non-existent or user-controlled JSON file, causingreadPolicyto returnnulland effectively disabling enterprise policy enforcement forauthandupgradecommands. Because environment variables are entirely under the caller's control (including per-process overrides that bypass system-wide settings), this undermines the guarantee that policy guardrails are centrally enforced and non-bypassable. Consider restricting or disabling this override in production/enterprise scenarios (e.g., only honoring it in explicit dev/test modes or when running with admin context) so that the machine-scope policy cannot be trivially bypassed.