Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .nx/version-plans/version-plan-1781270291055.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@pagopa/dx-cli": minor
---

For users in PagoPA organization, logs are sent to Application Insights
5 changes: 5 additions & 0 deletions .nx/version-plans/version-plan-1781270390103.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
docs: minor
---

Add GitHub login as a requirement to use the cli
14 changes: 14 additions & 0 deletions apps/cli/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,20 @@ npm run build
node ./apps/cli/bin/index.js --help
```

## 🔐 Authentication

The CLI requires GitHub authentication for every command **except** `--version`
and `--help`. Authenticate using one of the following (checked in this order):

1. The `GH_TOKEN` environment variable
2. The `GITHUB_TOKEN` environment variable
3. The [GitHub CLI](https://cli.github.com/) — run `gh auth login`

If no credential is found, the CLI exits with an error asking you to log in.

> Telemetry is emitted only for members of the `pagopa` GitHub organization;
> all other users can use the CLI normally with telemetry disabled.

## 🛠️ Usage

### Available Commands
Expand Down
16 changes: 13 additions & 3 deletions apps/cli/bin/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
#!/usr/bin/env node

import { runCli } from "../dist/index.js";
import packageJson from "../package.json" with { type: "json" };
// The instrumentation module must be loaded first so that the OpenTelemetry
// module hook (import-in-the-middle) is registered before any of the CLI's
// modules are imported. Static imports are always hoisted before code runs, so
// sequential dynamic imports are the only way to guarantee this load order.
await import("../dist/adapters/azure-monitor/instrumentation.js");

runCli(packageJson.version).catch((error) => console.error(error.message));
const { runCli } = await import("../dist/index.js");
const { default: packageJson } = await import("../package.json", {
with: { type: "json" },
});

await runCli(packageJson.version).catch((error) =>
console.error(error.message),
);
8 changes: 8 additions & 0 deletions apps/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@
"@azure/arm-authorization": "^9.0.0",
"@azure/arm-keyvault": "^4.0.0",
"@azure/arm-msi": "^2.2.0",
"@azure/monitor-opentelemetry": "catalog:opentelemetry",
"@opentelemetry/api": "catalog:opentelemetry",
"@opentelemetry/api-logs": "catalog:opentelemetry",
"@opentelemetry/instrumentation": "catalog:opentelemetry",
"@opentelemetry/instrumentation-undici": "catalog:opentelemetry",
"@opentelemetry/sdk-logs": "catalog:opentelemetry",
"import-in-the-middle": "^3.0.0",
"@azure/arm-resourcegraph": "^4.2.1",
"@azure/arm-resources": "^7.0.0",
"@azure/arm-resources-subscriptions": "^2.1.0",
Expand All @@ -32,6 +39,7 @@
"@azure/keyvault-secrets": "^4.11.1",
"@azure/storage-blob": "catalog:azure",
"@logtape/logtape": "catalog:",
"@logtape/otel": "catalog:opentelemetry",
"@microsoft/microsoft-graph-client": "^3.0.7",
"@pagopa/dx-savemoney": "workspace:^",
"chalk": "^5.6.2",
Expand Down
93 changes: 93 additions & 0 deletions apps/cli/src/adapters/azure-monitor/instrumentation.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/**
* OTel preload module for the dx CLI.
*
* This module is a deliberate side-effect entrypoint. It MUST be the first
* module loaded by the process (via a sequential dynamic import in bin/index.js)
* so that import-in-the-middle can register its hook before any HTTP client
* libraries (undici/fetch, octokit, Azure SDKs) are loaded.
*
* Initialising the Azure Monitor exporter is deferred to `enableAzureMonitor()`
* so the CLI can decide at runtime whether to emit telemetry (only PagoPA org
* members do). The import-in-the-middle hook is still registered eagerly at
* preload because it must run before any instrumented module is imported.
*
* Reads APPLICATIONINSIGHTS_CONNECTION_STRING from process.env because this IS
* the configuration boundary — no higher caller can inject config at this point
* in the process lifecycle.
*/

import { useAzureMonitor } from "@azure/monitor-opentelemetry";
import { metrics, trace } from "@opentelemetry/api";
import { registerInstrumentations } from "@opentelemetry/instrumentation";
import { UndiciInstrumentation } from "@opentelemetry/instrumentation-undici";
// import-in-the-middle and module must be the first imports so that
// createAddHookMessageChannel / register run as early as possible.
import { createAddHookMessageChannel } from "import-in-the-middle";
import { register } from "module";
import os from "node:os";

const { registerOptions, waitForAllMessagesAcknowledged } =
createAddHookMessageChannel();

// Register the ESM module hook so that subsequent dynamic imports are
// intercepted by OpenTelemetry's module instrumentation.
register("import-in-the-middle/hook.mjs", import.meta.url, registerOptions);

// Set service name before useAzureMonitor() initialises the Resource so
// that Azure Monitor picks it up as the cloud_RoleName.
process.env["OTEL_SERVICE_NAME"] = "dx-cli";
process.env["ATTR_SERVICE_NAMESPACE"] = "dx";

Comment on lines +38 to +40
// Append OS resource attributes to any pre-existing OTEL_RESOURCE_ATTRIBUTES
// value (e.g. set by the environment). OTel picks these up automatically when
// it builds the Resource inside useAzureMonitor().
// os.platform() returns "win32" on Windows; the OTel os.type enum uses "windows".
const osPlatform = os.platform();
const osType = osPlatform === "win32" ? "windows" : osPlatform;
process.env["OTEL_RESOURCE_ATTRIBUTES"] = [
process.env["OTEL_RESOURCE_ATTRIBUTES"],
`os.type=${osType}`,
`os.version=${os.release()}`,
`os.machine=${os.machine()}`,
]
.filter(Boolean)
.join(",");

let azureMonitorEnabled = false;

/**
* Initialises the Azure Monitor exporter and registers HTTP instrumentation.
*
* Deferred (not run at preload) so the CLI only emits telemetry once it has
* confirmed the user is a PagoPA org member. Idempotent: safe to call more than
* once. Must be called before logging is configured so the logtape OTel sink
* binds to the real (rather than no-op) LoggerProvider.
*/
export const enableAzureMonitor = (): void => {
if (azureMonitorEnabled) {
return;
}
azureMonitorEnabled = true;

useAzureMonitor({
azureMonitorExporterOptions: {
connectionString:
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING ||
"InstrumentationKey=e0ff8094-78fa-45e5-a21d-e62b453dc5d1;IngestionEndpoint=https://italynorth-0.in.applicationinsights.azure.com/;LiveEndpoint=https://italynorth.livediagnostics.monitor.azure.com/;ApplicationId=ce469d55-2ff7-4dfd-a249-cc787291e672",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question(non-blocking)
The Application Insights connection string is currently hardcoded into the public npm package. Although the CLI only utilizes it for PagoPA members, anyone could extract it and send spoofed telemetry data directly to that endpoint. Is this an accepted risk, or would we prefer to inject it at build or runtime?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted risk, we can rotate or block connections at any time. Moreover we have a daily cap

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: we could use managed identity. I did some experiments with that a few weeks ago, and the steps to use Managed Identity should be:

  • force our Application Insights to use Managed Identity by adding local_authentication_disabled = true
  • add Monitoring Metrics Publisher role to the instances that need to push telemetry data
  • use the @pagopa/azure-tracing to initialize Azure Monitor using the Managed Identity (the package already supports Managed Identity authentication)

Please note that with this current approach, the Connection String will still be publicly available, but it won't be enough to send data.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is not possible to use the @pagopa/azure-tracing package, then we should add here the logic to use the Managed Identity instead (through the @azure/identity package)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue in using IAM is that dx cli is a client app, so it doesn't have any role but the user identity. This requires giving access to all the users.
An alternative is using service principals, but it requires saving a secret locally

},
enableLiveMetrics: true,
});
Comment thread
krusty93 marked this conversation as resolved.

// Registered after useAzureMonitor() so the instrumentation binds to the
// real tracer/meter providers it installs (rather than the no-op defaults).
registerInstrumentations({
instrumentations: [new UndiciInstrumentation()],
meterProvider: metrics.getMeterProvider(),
tracerProvider: trace.getTracerProvider(),
});
};

// Block module evaluation until the hook is fully acknowledged.
// This ensures that when bin/index.js proceeds to import the CLI entry
// module, all transitive imports go through the registered hook.
await waitForAllMessagesAcknowledged();
28 changes: 28 additions & 0 deletions apps/cli/src/adapters/azure-monitor/telemetry.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* Telemetry helpers for the dx CLI.
*
* All functions are no-ops when the OTel SDK has not been initialised (i.e.,
* when telemetry is disabled or the connection string is absent).
*/

/**
* Flush pending telemetry before the process exits.
*
* Uses shutdownAzureMonitor() which force-flushes AND shuts down every
* provider (traces, logs, metrics). Flushing only the logger provider is not
* enough: the root span ends right before the process exits, so without a
* trace flush it would never be exported and would not appear in the
* Application Insights timeline.
*
* Safe to call unconditionally — swallows all errors and is a no-op when the
* SDK was never initialised (telemetry disabled or no connection string).
*/
export const flushTelemetry = async (): Promise<void> => {
try {
const { shutdownAzureMonitor } =
await import("@azure/monitor-opentelemetry");
await shutdownAzureMonitor();
} catch {
// Telemetry must never break the CLI.
}
};
2 changes: 2 additions & 0 deletions apps/cli/src/adapters/commander/commands/add.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ export const authorizeCloudAccounts =

type AddResult = {
authorizationPrs: AuthorizationResult[];
payload: EnvironmentPayload;
};

const displaySummary = (result: AddResult) => {
Expand Down Expand Up @@ -147,6 +148,7 @@ const addEnvironmentAction = (
authorizeCloudAccounts(authorizationService)(payload).map(
(authorizationPrs) => ({
authorizationPrs,
payload,
}),
),
);
Expand Down
3 changes: 3 additions & 0 deletions apps/cli/src/adapters/commander/commands/doctor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
*/
import chalk from "chalk";
import { Command } from "commander";
import process from "node:process";

import type { CommandPresenter } from "../../../domain/command-presenter.js";
import type { Dependencies } from "../../../domain/dependencies.js";
Expand Down Expand Up @@ -52,5 +53,7 @@ export const makeDoctorCommand = (

reportDoctorResult(presenter, output)(result);

// Use process.exitCode instead of process.exit() so that the finally
// block in runCli() can flush telemetry before the process terminates.
process.exitCode = result.hasErrors ? 1 : 0;
});
92 changes: 91 additions & 1 deletion apps/cli/src/adapters/octokit/__tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
RepositoryNotFoundError,
} from "../../../domain/github.js";
import { fetchLatestRelease, fetchLatestTag } from "../index.js";
import { OctokitGitHubService } from "../index.js";
import { isPagopaOrgMember, OctokitGitHubService } from "../index.js";

const makeEnv = () => {
const mockOctokit = mockDeep<Octokit>();
Expand Down Expand Up @@ -512,3 +512,93 @@ describe("octokit adapter", () => {
});
});
});

describe("isPagopaOrgMember", () => {
const token = "test-token";
const client = mockDeep<Octokit>();

beforeEach(() => {
mockReset(client);
});

it("returns true when the membership state is active", async () => {
client.rest.orgs.getMembershipForAuthenticatedUser.mockResolvedValue({
data: { state: "active" },
headers: {},
status: 200,
url: "",
} as never);

const result = await isPagopaOrgMember(token, client);

expect(result).toBe(true);
expect(
client.rest.orgs.getMembershipForAuthenticatedUser,
).toHaveBeenCalledWith({ org: "pagopa" });
});

it("returns false when the membership state is pending", async () => {
client.rest.orgs.getMembershipForAuthenticatedUser.mockResolvedValue({
data: { state: "pending" },
headers: {},
status: 200,
url: "",
} as never);

const result = await isPagopaOrgMember(token, client);

expect(result).toBe(false);
});

it("returns false when the user is not a member (404)", async () => {
const error = new RequestError("Not Found", 404, {
request: {
headers: {},
method: "GET",
url: "https://api.github.com/user/memberships/orgs/pagopa",
},
response: {
data: { message: "Not Found" },
headers: {},
status: 404,
url: "https://api.github.com/user/memberships/orgs/pagopa",
},
});
client.rest.orgs.getMembershipForAuthenticatedUser.mockRejectedValue(error);

const result = await isPagopaOrgMember(token, client);

expect(result).toBe(false);
});

it("returns false when access is forbidden (403)", async () => {
const error = new RequestError("Forbidden", 403, {
request: {
headers: {},
method: "GET",
url: "https://api.github.com/user/memberships/orgs/pagopa",
},
response: {
data: { message: "Forbidden" },
headers: {},
status: 403,
url: "https://api.github.com/user/memberships/orgs/pagopa",
},
});
client.rest.orgs.getMembershipForAuthenticatedUser.mockRejectedValue(error);

const result = await isPagopaOrgMember(token, client);

expect(result).toBe(false);
});

it("returns false on a network error", async () => {
client.rest.orgs.getMembershipForAuthenticatedUser.mockRejectedValue(
new Error("network down"),
);

const result = await isPagopaOrgMember(token, client);

expect(result).toBe(false);
});
});
28 changes: 28 additions & 0 deletions apps/cli/src/adapters/octokit/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,34 @@ export const getGitHubPAT = async (): Promise<string | undefined> => {
return undefined;
};

/** GitHub organization whose members are allowed to emit CLI telemetry. */
const PAGOPA_ORG = "pagopa";

/**
* Returns true when the authenticated user is an active member of the PagoPA
* GitHub organization.
*
* Fails closed: any error (network failure, missing `read:org` scope, 403/404)
* resolves to `false` so telemetry stays disabled when membership cannot be
* confirmed.
*
* @param octokit injectable client, primarily for testing; defaults to a client
* authenticated with the supplied token.
*/
export const isPagopaOrgMember = async (
token: string,
octokit: Octokit = new Octokit({ auth: token }),
): Promise<boolean> => {
try {
const { data } = await octokit.rest.orgs.getMembershipForAuthenticatedUser({
org: PAGOPA_ORG,
});
return data.state === "active";
} catch {
return false;
}
};

export const fetchLatestTag = ({ client, owner, repo }: GitHubReleaseParam) =>
ResultAsync.fromPromise(
// Get repository tags
Expand Down
4 changes: 4 additions & 0 deletions apps/cli/src/adapters/plop/actions/setup-pnpm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ export default function (plop: NodePlopAPI) {
const env = Object.fromEntries(
Object.entries(process.env)
.filter(([key]) => !key.startsWith("npm_config_"))
// Strip Node.js debugger env vars so child processes don't try to
// attach to the VS Code debugger and hang/fail.
.filter(([key]) => key !== "NODE_OPTIONS")
.filter(([key]) => !key.startsWith("VSCODE_INSPECTOR"))
// Disable corepack download prompt
.concat([["COREPACK_ENABLE_DOWNLOAD_PROMPT", "0"]]),
);
Expand Down
Loading