From 142a8f3f2cd3aac6330e4b1ac6c3f8fc2ae9664d Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 10 Nov 2026 21:24:23 +0100 Subject: [PATCH 1/5] feat(trino): Add TLS verification and impersonation options to the connection form Connections entered on /trino could neither skip TLS certificate verification nor turn off user impersonation. - "Skip TLS certificate verification" toggle, only shown and accepted when STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS=true (Helm: trino.tls.allowInsecureUserConnections) - "Impersonate the logged-in user" toggle for basic auth, defaulting to STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION - Both settings are persisted in local storage; the connection summary shows "TLS unverified" when verification is skipped --- .env.example | 4 +- deploy/helm/cockpit/README.md | 3 +- deploy/helm/cockpit/values.yaml | 6 +- e2e/trino/connection-form.spec.ts | 133 ++++++++++++++++++++-- messages/de.json | 6 + messages/en.json | 6 + src/lib/server/trino/client.test.ts | 2 + src/lib/server/trino/client.ts | 10 +- src/lib/server/trino/config.ts | 4 +- src/lib/server/trino/user-clients.test.ts | 68 ++++++++++- src/lib/server/trino/user-clients.ts | 39 ++++++- src/routes/(app)/trino/+page.server.ts | 27 ++++- src/routes/(app)/trino/+page.svelte | 61 +++++++++- src/routes/(app)/trino/validation.ts | 4 +- 14 files changed, 338 insertions(+), 35 deletions(-) diff --git a/.env.example b/.env.example index 09cc7f4e..2bc0df7f 100644 --- a/.env.example +++ b/.env.example @@ -54,8 +54,8 @@ DATABASE_PASSWORD=change-me # STACKABLE_COCKPIT_TRINO_AUTH_TYPE=basic # "none" (default) or "basic" # STACKABLE_COCKPIT_TRINO_AUTH_USERNAME=service-account # STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD=secret -# STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION=false # Disable impersonation: run all queries as the auth user (requires basic auth). Default: true -# STACKABLE_COCKPIT_TRINO_TLS_INSECURE=false # Set to "true" to skip TLS certificate verification +# STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION=false # Disable impersonation: run all queries as the auth user (requires basic auth). Also the default for connections entered in the UI with basic auth, where users can change it. Default: true +# STACKABLE_COCKPIT_TRINO_TLS_INSECURE=false # Set to "true" to skip TLS certificate verification. Without STACKABLE_COCKPIT_TRINO_URL, lets users skip it for connections entered in the UI # STACKABLE_COCKPIT_QUERY_TTL=1800 # Seconds before completed, unaccessed tab queries are evicted (default: 1800) # Feature flags diff --git a/deploy/helm/cockpit/README.md b/deploy/helm/cockpit/README.md index 96c53297..b9da1ded 100644 --- a/deploy/helm/cockpit/README.md +++ b/deploy/helm/cockpit/README.md @@ -168,6 +168,7 @@ At startup, the cockpit fetches the OIDC discovery document once and logs the ca ### Trino Connection Parameters Optional pre-configured Trino endpoint. When `trino.url` is set, the in-app connection form is hidden. +Otherwise users enter their own connection: the impersonation setting is its default for basic auth, and `trino.tls.insecure` lets them skip TLS certificate verification. | Parameter | Description | Default | | --- | --- | --- | @@ -179,7 +180,7 @@ Optional pre-configured Trino endpoint. When `trino.url` is set, the in-app conn | `trino.auth.username` | Username for basic auth. | `""` | | `trino.auth.password.secretKeyRef.name` | Name of the Secret holding the Trino password. Required when `type=basic`. | `""` | | `trino.auth.password.secretKeyRef.key` | Key inside the Secret. | `trino-auth-password` | -| `trino.tls.insecure` | Skip TLS certificate verification. Logs a warning in production; for a private CA, use `tls.caCerts` or `tls.secretClasses` instead. | `false` | +| `trino.tls.insecure` | Skip TLS certificate verification. Logs a warning in production; for a private CA, use `tls.caCerts` or `tls.secretClasses` instead. Without `trino.url`, users may skip verification for the connection they enter in the UI. | `false` | | `trino.queryTtl` | Query result TTL in seconds. | `1800` (application default) | ### Security Parameters diff --git a/deploy/helm/cockpit/values.yaml b/deploy/helm/cockpit/values.yaml index db5398e5..5591aaea 100644 --- a/deploy/helm/cockpit/values.yaml +++ b/deploy/helm/cockpit/values.yaml @@ -297,7 +297,8 @@ trino: publicUrl: "" userImpersonation: # Forward the logged-in user to Trino as X-Trino-User. When false, all queries - # run as trino.auth.username instead (requires basic auth). + # run as trino.auth.username instead (requires basic auth). Without trino.url, + # this is the default for connections entered in the UI with basic auth. enabled: true # OIDC claim used as the Trino user. Only consumed when enabled and OIDC is on. userClaim: preferred_username @@ -311,7 +312,8 @@ trino: name: "" key: trino-auth-password tls: - # Skip TLS certificate verification. A private CA goes into tls.caCerts or + # Skip TLS certificate verification. Without trino.url, users may skip it for + # the connection they enter in the UI. A private CA goes into tls.caCerts or # tls.secretClasses instead. insecure: false # Query result TTL in seconds. Default: 1800 (application default). diff --git a/e2e/trino/connection-form.spec.ts b/e2e/trino/connection-form.spec.ts index cb447aab..3fb96d2c 100644 --- a/e2e/trino/connection-form.spec.ts +++ b/e2e/trino/connection-form.spec.ts @@ -37,7 +37,9 @@ function interceptSaveSuccess(page: import('@playwright/test').Page) { connectionUrl: MOCK_TRINO_URL, authType: 'none', authUsername: '', - authPassword: '' + authPassword: '', + tlsInsecure: false, + impersonate: true }, message: { type: 'success' } }; @@ -50,24 +52,27 @@ function interceptSaveSuccess(page: import('@playwright/test').Page) { } /** - * Intercept SvelteKit responses to override the trinoConfigured boolean. + * Intercept SvelteKit responses to override boolean page data fields such as + * trinoConfigured. * * SvelteKit embeds page data in two formats: * - SSR HTML uses devalue.uneval with unquoted keys: `trinoConfigured:true` * - __data.json uses devalue.stringify with indexed arrays where booleans * are stored at positions referenced by the shape object at index 0 */ -function interceptTrinoConfigured(page: import('@playwright/test').Page, value: boolean) { +function interceptPageData( + page: import('@playwright/test').Page, + overrides: Record +) { page.route('**/trino', async (route, request) => { if (request.resourceType() !== 'document') return route.continue(); const response = await route.fetch(); let html = await response.text(); - // devalue.uneval uses literal booleans; minified builds may use !0 / !1 - html = html.replaceAll(`trinoConfigured:${!value}`, `trinoConfigured:${value}`); - html = html.replaceAll( - `trinoConfigured:${!value ? '!0' : '!1'}`, - `trinoConfigured:${value ? '!0' : '!1'}` - ); + for (const [key, value] of Object.entries(overrides)) { + // devalue.uneval uses literal booleans; minified builds may use !0 / !1 + html = html.replaceAll(`${key}:${!value}`, `${key}:${value}`); + html = html.replaceAll(`${key}:${!value ? '!0' : '!1'}`, `${key}:${value ? '!0' : '!1'}`); + } await route.fulfill({ response, body: html }); }); @@ -77,8 +82,9 @@ function interceptTrinoConfigured(page: import('@playwright/test').Page, value: for (const node of json.nodes ?? []) { if (node?.type === 'data' && Array.isArray(node.data)) { const shape = node.data[0]; - if (typeof shape === 'object' && shape !== null && 'trinoConfigured' in shape) { - node.data[shape.trinoConfigured as number] = value; + if (typeof shape !== 'object' || shape === null) continue; + for (const [key, value] of Object.entries(overrides)) { + if (key in shape) node.data[shape[key] as number] = value; } } } @@ -86,6 +92,51 @@ function interceptTrinoConfigured(page: import('@playwright/test').Page, value: }); } +/** Capture the fields posted by the next `?/save` request and abort it. */ +function captureSave(page: import('@playwright/test').Page): Promise { + return new Promise((resolve) => { + page.route( + (url) => url.pathname === '/trino' && url.search === '?/save', + async (route, request) => { + resolve(new URLSearchParams(request.postData() ?? '')); + await route.abort(); + } + ); + }); +} + +// --------------------------------------------------------------------------- +// 0. Per-user connections that may skip TLS verification +// --------------------------------------------------------------------------- +test.describe('Connection form (insecure TLS allowed)', () => { + test.use({ locale: 'en-US' }); + + test.beforeEach(async ({ page }) => { + await page.addInitScript(() => { + localStorage.removeItem('trino_url'); + localStorage.removeItem('trino_tls_insecure'); + }); + interceptPageData(page, { trinoConfigured: false, allowInsecureTls: true }); + }); + + test('skipping TLS verification is posted, persisted and shown in the summary', async ({ + page + }) => { + await page.goto('/trino'); + await waitForHydration(page); + + await page.getByLabel('URL').fill(MOCK_TRINO_URL); + await page.getByLabel('Skip TLS certificate verification').check(); + await expect(page.getByText('TLS unverified')).toBeVisible(); + + const posted = captureSave(page); + await page.getByRole('button', { name: 'Save' }).click(); + + expect((await posted).get('tlsInsecure')).toBe('on'); + expect(await page.evaluate(() => localStorage.getItem('trino_tls_insecure'))).toBe('true'); + }); +}); + // --------------------------------------------------------------------------- // 1. When Trino IS env-configured, the form must be hidden // --------------------------------------------------------------------------- @@ -116,7 +167,7 @@ test.describe('Connection form (manual mode)', () => { localStorage.removeItem('trino_password'); }); - interceptTrinoConfigured(page, false); + interceptPageData(page, { trinoConfigured: false }); }); test('connection form is visible', async ({ page }) => { @@ -171,6 +222,64 @@ test.describe('Connection form (manual mode)', () => { await expect(page.getByLabel('Password')).not.toBeVisible(); }); + test('impersonation toggle is only shown for basic auth', async ({ page }) => { + await page.goto('/trino'); + await waitForHydration(page); + + const toggle = page.getByLabel('Impersonate the logged-in user'); + await expect(toggle).not.toBeVisible(); + + await page.getByRole('radio', { name: 'Basic' }).click(); + await expect(toggle).toBeVisible(); + // Defaults to the deployment's impersonation setting (enabled in the test env). + await expect(toggle).toBeChecked(); + }); + + test('disabling impersonation is posted with the connection', async ({ page }) => { + await page.goto('/trino'); + await waitForHydration(page); + + await page.getByLabel('URL').fill(MOCK_TRINO_URL); + await page.getByRole('radio', { name: 'Basic' }).click(); + await page.getByLabel('Username').fill('alice'); + await page.getByLabel('Password').fill('secret'); + await page.getByLabel('Impersonate the logged-in user').uncheck(); + + const posted = captureSave(page); + await page.getByRole('button', { name: 'Save' }).click(); + + // An unchecked checkbox is not posted; superforms reads the missing field as false. + const body = await posted; + expect(body.get('authUsername')).toBe('alice'); + expect(body.has('impersonate')).toBe(false); + expect(await page.evaluate(() => localStorage.getItem('trino_impersonate'))).toBe('false'); + }); + + test('impersonation setting is kept while the toggle is hidden', async ({ page }) => { + await page.goto('/trino'); + await waitForHydration(page); + + await page.getByRole('radio', { name: 'Basic' }).click(); + await page.getByLabel('Impersonate the logged-in user').uncheck(); + await page.getByRole('radio', { name: 'No auth' }).click(); + await page.getByLabel('URL').fill(MOCK_TRINO_URL); + + const posted = captureSave(page); + await page.getByRole('button', { name: 'Save' }).click(); + expect((await posted).get('impersonate')).toBe('false'); + + await page.getByRole('radio', { name: 'Basic' }).click(); + await expect(page.getByLabel('Impersonate the logged-in user')).not.toBeChecked(); + }); + + test('TLS verification option is hidden unless allowed', async ({ page }) => { + await page.goto('/trino'); + await waitForHydration(page); + + await expect(page.getByLabel('URL')).toBeVisible(); + await expect(page.getByLabel('Skip TLS certificate verification')).not.toBeVisible(); + }); + test('submitting without a URL shows a validation error', async ({ page }) => { await page.goto('/trino'); await waitForHydration(page); diff --git a/messages/de.json b/messages/de.json index c8b51b1e..4c066ac3 100644 --- a/messages/de.json +++ b/messages/de.json @@ -99,6 +99,12 @@ "trino_connection_test_failed": "Verbindung zu Trino fehlgeschlagen – bitte URL und Zugangsdaten prüfen.", "trino_connection_save_failed": "Die Verbindung konnte nicht gespeichert werden. Bitte erneut versuchen.", "trino_connection_env_managed": "Die Verbindung wird über Umgebungsvariablen verwaltet.", + "trino_connection_impersonate": "Als angemeldeter Benutzer ausführen", + "trino_connection_impersonate_hint": "Abfragen laufen unter Ihrem Cockpit-Benutzer; die Zugangsdaten oben müssen sich als dieser Benutzer ausgeben dürfen. Ausgeschaltet laufen Abfragen unter dem Benutzer oben.", + "trino_connection_tls_insecure": "TLS-Zertifikatsprüfung überspringen", + "trino_connection_tls_insecure_hint": "Unsicher: Das Zertifikat des Trino-Servers wird nicht geprüft. Nur für Testumgebungen verwenden.", + "trino_connection_tls_insecure_not_allowed": "Das Überspringen der TLS-Zertifikatsprüfung ist auf diesem Server nicht erlaubt.", + "trino_connection_tls_unverified": "TLS ungeprüft", "trino_user_missing": "Ihrem Konto ist kein Trino-Benutzername zugeordnet. Melden Sie sich ab und erneut an; besteht das Problem weiterhin, wenden Sie sich an Ihre Administration.", "trino_cancel_query": "Abbrechen", "trino_state_submitting": "Wird eingereicht", diff --git a/messages/en.json b/messages/en.json index 8d29f704..db16edc5 100644 --- a/messages/en.json +++ b/messages/en.json @@ -99,6 +99,12 @@ "trino_connection_test_failed": "Could not connect to Trino — check the URL and credentials.", "trino_connection_save_failed": "Could not save the connection. Please try again.", "trino_connection_env_managed": "The connection is managed via environment variables.", + "trino_connection_impersonate": "Impersonate the logged-in user", + "trino_connection_impersonate_hint": "Queries run as your Cockpit user; the credentials above must be allowed to impersonate it. When off, queries run as the user above.", + "trino_connection_tls_insecure": "Skip TLS certificate verification", + "trino_connection_tls_insecure_hint": "Insecure: the Trino server certificate is not checked. Only use this for test setups.", + "trino_connection_tls_insecure_not_allowed": "Skipping TLS certificate verification is not allowed on this server.", + "trino_connection_tls_unverified": "TLS unverified", "trino_user_missing": "Your account has no Trino username. Sign out and sign in again; if the problem persists, contact your administrator.", "trino_cancel_query": "Cancel", "trino_state_submitting": "Submitting", diff --git a/src/lib/server/trino/client.test.ts b/src/lib/server/trino/client.test.ts index 050db98b..72dd8b06 100644 --- a/src/lib/server/trino/client.test.ts +++ b/src/lib/server/trino/client.test.ts @@ -36,12 +36,14 @@ describe('TrinoClient X-Trino-User header', () => { }); await client.submit('SELECT 1', { user: 'alice' }); expect(submittedHeaders()).not.toHaveProperty('X-Trino-User'); + expect(client.impersonates).toBe(false); }); it('still sends X-Trino-User when impersonation is disabled but no credentials are set', async () => { const client = new TrinoClient({ serverUrl: 'http://trino:8080', impersonate: false }); await client.submit('SELECT 1', { user: 'anonymous' }); expect(submittedHeaders()['X-Trino-User']).toBe('anonymous'); + expect(client.impersonates).toBe(true); }); }); diff --git a/src/lib/server/trino/client.ts b/src/lib/server/trino/client.ts index 10d5a247..f04e13c3 100644 --- a/src/lib/server/trino/client.ts +++ b/src/lib/server/trino/client.ts @@ -130,10 +130,16 @@ export class TrinoClient { return res.json() as Promise; } + /** Whether requests carry `X-Trino-User`. Without credentials it is the only + * user Trino gets, so impersonation can only be turned off when authenticated. */ + get impersonates(): boolean { + return this.impersonate || !this.authenticated; + } + /** commonHeaders plus X-Trino-User for the given user when impersonating. */ private headersForUser(user: string | undefined): Record { const headers = { ...this.commonHeaders }; - if (user && (this.impersonate || !this.authenticated)) { + if (user && this.impersonates) { headers['X-Trino-User'] = user; } return headers; @@ -231,7 +237,7 @@ const { tlsInsecure } = trinoConfig; -export const trinoUserImpersonation = trinoConfig.userImpersonation; +const trinoUserImpersonation = trinoConfig.userImpersonation; /** True when the Trino connection is pre-configured via environment variables. */ export const trinoConfigured = !!trinoUrl; diff --git a/src/lib/server/trino/config.ts b/src/lib/server/trino/config.ts index 8b8da79e..a791f9f5 100644 --- a/src/lib/server/trino/config.ts +++ b/src/lib/server/trino/config.ts @@ -45,5 +45,7 @@ export function parseTrinoConfig(env: Record) { /** Trino settings from the environment. The connection fields are only used * when `STACKABLE_COCKPIT_TRINO_URL` is set; otherwise users configure their - * own connection. Impersonation also applies to those per-user connections. */ + * own connection. For those per-user connections, impersonation is the default + * users can change with basic auth, and `tlsInsecure` decides whether users + * may skip certificate verification. */ export const trinoConfig = parseTrinoConfig(env); diff --git a/src/lib/server/trino/user-clients.test.ts b/src/lib/server/trino/user-clients.test.ts index a48b00e3..9c1818e1 100644 --- a/src/lib/server/trino/user-clients.test.ts +++ b/src/lib/server/trino/user-clients.test.ts @@ -1,28 +1,43 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -const { TrinoClientMock } = vi.hoisted(() => ({ +const { TrinoClientMock, AgentMock, trinoConfigMock, logMock } = vi.hoisted(() => ({ TrinoClientMock: vi.fn(function (this: Record, options: unknown) { this.options = options; - }) + }), + AgentMock: vi.fn(function (this: Record, options: unknown) { + this.options = options; + }), + trinoConfigMock: { tlsInsecure: false }, + logMock: { info: vi.fn(), warn: vi.fn(), debug: vi.fn() } })); vi.mock('$lib/server/logging', () => ({ - logger: { child: () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() }) } + logger: { child: () => logMock } })); vi.mock('./client.js', () => ({ TrinoClient: TrinoClientMock, - buildBasicAuthHeader: (user: string, pass: string) => `Basic ${user}:${pass}`, - trinoUserImpersonation: true + buildBasicAuthHeader: (user: string, pass: string) => `Basic ${user}:${pass}` })); +vi.mock('./config.js', () => ({ trinoConfig: trinoConfigMock })); +vi.mock('undici', () => ({ Agent: AgentMock })); type UserClients = typeof import('./user-clients.js'); let uc: UserClients; -const BASE = { url: 'http://trino:8080', authType: 'none', username: '', password: '' } as const; +const BASE = { + url: 'http://trino:8080', + authType: 'none', + username: '', + password: '', + tlsInsecure: false, + impersonate: true +} as const; +const BASIC = { ...BASE, authType: 'basic', username: 'alice', password: 'secret' } as const; beforeEach(async () => { vi.clearAllMocks(); vi.resetModules(); + trinoConfigMock.tlsInsecure = false; uc = await import('./user-clients.js'); }); @@ -33,6 +48,7 @@ describe('buildUserTrinoClient', () => { expect(TrinoClientMock).toHaveBeenCalledWith({ serverUrl: 'http://trino:8080', authorization: undefined, + dispatcher: undefined, impersonate: true }); }); @@ -56,6 +72,34 @@ describe('buildUserTrinoClient', () => { ); }); + // TrinoClient keeps sending X-Trino-User without credentials (see client.test.ts). + it('passes the impersonation setting to the client', () => { + uc.buildUserTrinoClient({ ...BASIC, impersonate: false }); + + expect(TrinoClientMock).toHaveBeenCalledWith( + expect.objectContaining({ authorization: 'Basic alice:secret', impersonate: false }) + ); + }); + + it('rejects skipping TLS verification unless allowed', () => { + expect(() => uc.buildUserTrinoClient({ ...BASE, tlsInsecure: true })).toThrow(/not allowed/); + expect(TrinoClientMock).not.toHaveBeenCalled(); + }); + + it('skips TLS verification when allowed and requested', () => { + trinoConfigMock.tlsInsecure = true; + uc.buildUserTrinoClient({ ...BASE, tlsInsecure: true }); + + uc.buildUserTrinoClient({ ...BASIC, tlsInsecure: true }); + + expect(AgentMock).toHaveBeenCalledTimes(1); + expect(AgentMock).toHaveBeenCalledWith({ connect: { rejectUnauthorized: false } }); + const dispatchers = TrinoClientMock.mock.calls.map( + ([options]) => (options as { dispatcher?: unknown }).dispatcher + ); + expect(dispatchers).toEqual([AgentMock.mock.instances[0], AgentMock.mock.instances[0]]); + }); + it('does not store the client', () => { uc.buildUserTrinoClient(BASE); @@ -84,4 +128,16 @@ describe('per-user connections', () => { expect(uc.getUserTrinoClient('u1')).toBe(TrinoClientMock.mock.instances[1]); expect(uc.getUserTrinoUrl('u1')).toBe('http://other:8443'); }); + + it('logs a warning when the connection skips TLS verification', () => { + trinoConfigMock.tlsInsecure = true; + uc.createUserTrinoClient('u1', BASE); + expect(logMock.warn).not.toHaveBeenCalled(); + + uc.createUserTrinoClient('u1', { ...BASE, tlsInsecure: true }); + expect(logMock.warn).toHaveBeenCalledWith( + expect.objectContaining({ user_id: 'u1', tls_insecure: true }), + expect.stringContaining('without TLS certificate verification') + ); + }); }); diff --git a/src/lib/server/trino/user-clients.ts b/src/lib/server/trino/user-clients.ts index 90a538f5..003ec8f1 100644 --- a/src/lib/server/trino/user-clients.ts +++ b/src/lib/server/trino/user-clients.ts @@ -1,5 +1,7 @@ +import { Agent } from 'undici'; import { logger } from '$lib/server/logging'; -import { TrinoClient, buildBasicAuthHeader, trinoUserImpersonation } from './client.js'; +import { TrinoClient, buildBasicAuthHeader } from './client.js'; +import { trinoConfig } from './config.js'; const log = logger.child({ module: 'trino-user-clients' }); @@ -8,6 +10,11 @@ export interface UserConnectionConfig { authType: 'none' | 'basic'; username: string; password: string; + /** Skip TLS certificate verification. Only allowed with `STACKABLE_COCKPIT_TRINO_TLS_INSECURE`. */ + tlsInsecure: boolean; + /** Forward the logged-in user as `X-Trino-User`. Only takes effect with basic + * auth: without credentials the client always sends the user header. */ + impersonate: boolean; } interface UserEntry { @@ -17,8 +24,23 @@ interface UserEntry { const userClients = new Map(); +// Shared by all per-user connections that skip verification, so they reuse one set of +// connection pools. Verifying connections use the default dispatcher, which trusts the +// private CAs from STACKABLE_COCKPIT_TLS_CA_CERTS via the process-wide trust store. +let insecureDispatcher: Agent | undefined; + +function dispatcherFor(tlsInsecure: boolean): Agent | undefined { + if (!tlsInsecure) return undefined; + insecureDispatcher ??= new Agent({ connect: { rejectUnauthorized: false } }); + return insecureDispatcher; +} + /** Build a per-user Trino client from a connection config without storing it. */ export function buildUserTrinoClient(config: UserConnectionConfig): TrinoClient { + if (config.tlsInsecure && !trinoConfig.tlsInsecure) { + throw new Error('Skipping TLS certificate verification is not allowed'); + } + const authorization = config.authType === 'basic' && config.username && config.password ? buildBasicAuthHeader(config.username, config.password) @@ -27,7 +49,8 @@ export function buildUserTrinoClient(config: UserConnectionConfig): TrinoClient return new TrinoClient({ serverUrl: config.url, authorization, - impersonate: trinoUserImpersonation + dispatcher: dispatcherFor(config.tlsInsecure), + impersonate: config.impersonate }); } @@ -35,7 +58,17 @@ export function buildUserTrinoClient(config: UserConnectionConfig): TrinoClient export function createUserTrinoClient(userId: string, config: UserConnectionConfig): void { const client = buildUserTrinoClient(config); userClients.set(userId, { client, url: config.url }); - log.info({ user_id: userId, trino_url: config.url }, 'user connection created'); + const context = { + user_id: userId, + trino_url: config.url, + tls_insecure: config.tlsInsecure, + user_impersonation: client.impersonates + }; + if (config.tlsInsecure) { + log.warn(context, 'user connection created without TLS certificate verification'); + } else { + log.info(context, 'user connection created'); + } } /** Returns the per-user TrinoClient, or null if none has been configured. */ diff --git a/src/routes/(app)/trino/+page.server.ts b/src/routes/(app)/trino/+page.server.ts index a9649095..330b2bd4 100644 --- a/src/routes/(app)/trino/+page.server.ts +++ b/src/routes/(app)/trino/+page.server.ts @@ -1,5 +1,5 @@ import { fail } from '@sveltejs/kit'; -import { superValidate, message, type Infer, type InferIn } from 'sveltekit-superforms'; +import { superValidate, message, setError, type Infer, type InferIn } from 'sveltekit-superforms'; import { zod4 as zod } from 'sveltekit-superforms/adapters'; import { findTrinoUser, getUserId } from '$lib/server/auth-utils.js'; import { getAllQuerySummaries, resetTabQueries } from '$lib/server/trino/queries.js'; @@ -9,6 +9,7 @@ import { CONN_TEST_TIMEOUT_MS } from '$lib/server/trino/client.js'; import { completionEnabled } from '$lib/server/feature-flags.js'; +import { trinoConfig } from '$lib/server/trino/config.js'; import { buildUserTrinoClient, createUserTrinoClient, @@ -26,10 +27,20 @@ export const load: PageServerLoad = async ({ locals }) => { const connectionForm = await superValidate( zod(ConnectionSchema) ); + // The deployment's impersonation setting is the default for per-user connections. + connectionForm.data.impersonate = trinoConfig.userImpersonation; const userId = getUserId(locals); const activeQueries = getAllQuerySummaries(userId); const userClientExists = getUserTrinoClient(userId) !== null; - return { connectionForm, activeQueries, trinoConfigured, userClientExists, completionEnabled }; + return { + connectionForm, + activeQueries, + trinoConfigured, + userClientExists, + completionEnabled, + // Without a pre-configured URL, STACKABLE_COCKPIT_TRINO_TLS_INSECURE lets users skip verification. + allowInsecureTls: !trinoConfigured && trinoConfig.tlsInsecure + }; }; export const actions: Actions = { @@ -55,6 +66,11 @@ export const actions: Actions = { return fail(400, { form }); } + if (form.data.tlsInsecure && !trinoConfig.tlsInsecure) { + log.info('connection save rejected, skipping TLS verification is not allowed'); + return setError(form, 'tlsInsecure', m.trino_connection_tls_insecure_not_allowed()); + } + const userId = getUserId(locals); const user = findTrinoUser(locals); if (!user) { @@ -64,12 +80,15 @@ export const actions: Actions = { { status: 500 } ); } - const { connectionUrl, authType, authUsername, authPassword } = form.data; + const { connectionUrl, authType, authUsername, authPassword, tlsInsecure, impersonate } = + form.data; const config = { url: connectionUrl, authType, username: authUsername, - password: authPassword + password: authPassword, + tlsInsecure, + impersonate }; // Verify connectivity before storing the client or cancelling running queries. diff --git a/src/routes/(app)/trino/+page.svelte b/src/routes/(app)/trino/+page.svelte index e5214181..e8e57bf5 100644 --- a/src/routes/(app)/trino/+page.svelte +++ b/src/routes/(app)/trino/+page.svelte @@ -142,6 +142,10 @@ $connectionForm.connectionUrl = getStoredValue('trino_url', ''); $connectionForm.authType = getStoredValue('trino_auth_type', 'none') as 'none' | 'basic'; $connectionForm.authUsername = getStoredValue('trino_username', ''); + $connectionForm.impersonate = + getStoredValue('trino_impersonate', String($connectionForm.impersonate)) === 'true'; + $connectionForm.tlsInsecure = + data.allowInsecureTls && getStoredValue('trino_tls_insecure', 'false') === 'true'; } defaultCatalog = getStoredValue('trino_default_catalog', ''); defaultSchema = getStoredValue('trino_default_schema', ''); @@ -183,6 +187,13 @@ localStorage.setItem('trino_url', $connectionForm.connectionUrl); localStorage.setItem('trino_auth_type', $connectionForm.authType); localStorage.setItem('trino_username', $connectionForm.authUsername); + // Only store a deviation from the deployment default, so a changed default still applies. + if ($connectionForm.impersonate === data.connectionForm.data.impersonate) { + localStorage.removeItem('trino_impersonate'); + } else { + localStorage.setItem('trino_impersonate', String($connectionForm.impersonate)); + } + localStorage.setItem('trino_tls_insecure', String($connectionForm.tlsInsecure)); }); // Persist settings. @@ -228,7 +239,9 @@ const url = $connectionForm.connectionUrl; const host = url ? url.replace(/^https?:\/\//, '').replace(/\/$/, '') : '-'; const auth = $connectionForm.authType === 'basic' ? m.trino_auth_basic() : m.trino_auth_none(); - return `${host} \u00b7 ${auth}`; + const parts = [host, auth]; + if ($connectionForm.tlsInsecure) parts.push(m.trino_connection_tls_unverified()); + return parts.join(' \u00b7 '); }); const stateLabel = $derived.by(() => { @@ -570,6 +583,52 @@ {/if} + +
+
+ +

+ {m.trino_connection_impersonate_hint()} +

+
+ +
+ {:else} + + + {/if} + + {#if data.allowInsecureTls} +
+
+ +

+ {m.trino_connection_tls_insecure_hint()} +

+ {#if $connectionErrors.tlsInsecure} +

{$connectionErrors.tlsInsecure}

+ {/if} +
+ +
{/if} diff --git a/src/routes/(app)/trino/validation.ts b/src/routes/(app)/trino/validation.ts index 973dcdff..6b89b8c8 100644 --- a/src/routes/(app)/trino/validation.ts +++ b/src/routes/(app)/trino/validation.ts @@ -8,7 +8,9 @@ export const ConnectionSchema = z connectionUrl: z.url({ error: () => m.trino_connection_url_invalid() }), authType: z.enum(['none', 'basic']).default('none'), authUsername: z.string().default(''), - authPassword: z.string().default('') + authPassword: z.string().default(''), + tlsInsecure: z.boolean().default(false), + impersonate: z.boolean().default(true) }) .superRefine((data, ctx) => { if (data.authType === 'basic') { From fa5b60e1a07d05a872b94d5991ae754ec0ff57c2 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Thu, 12 Nov 2026 11:57:23 +0100 Subject: [PATCH 2/5] refactor(trino): Drop the impersonation option from the connection form Per-user connections use the deployment's impersonation setting (STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION) again, as before. The TLS verification option stays. --- .env.example | 2 +- deploy/helm/cockpit/README.md | 2 +- deploy/helm/cockpit/values.yaml | 3 +- e2e/trino/connection-form.spec.ts | 53 +---------------------- messages/de.json | 2 - messages/en.json | 2 - src/lib/server/trino/client.test.ts | 2 - src/lib/server/trino/client.ts | 10 +---- src/lib/server/trino/config.ts | 5 +-- src/lib/server/trino/user-clients.test.ts | 15 ++----- src/lib/server/trino/user-clients.ts | 10 ++--- src/routes/(app)/trino/+page.server.ts | 8 +--- src/routes/(app)/trino/+page.svelte | 30 ------------- src/routes/(app)/trino/validation.ts | 3 +- 14 files changed, 17 insertions(+), 130 deletions(-) diff --git a/.env.example b/.env.example index 2bc0df7f..69c2e192 100644 --- a/.env.example +++ b/.env.example @@ -54,7 +54,7 @@ DATABASE_PASSWORD=change-me # STACKABLE_COCKPIT_TRINO_AUTH_TYPE=basic # "none" (default) or "basic" # STACKABLE_COCKPIT_TRINO_AUTH_USERNAME=service-account # STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD=secret -# STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION=false # Disable impersonation: run all queries as the auth user (requires basic auth). Also the default for connections entered in the UI with basic auth, where users can change it. Default: true +# STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION=false # Disable impersonation: run all queries as the auth user (requires basic auth). Default: true # STACKABLE_COCKPIT_TRINO_TLS_INSECURE=false # Set to "true" to skip TLS certificate verification. Without STACKABLE_COCKPIT_TRINO_URL, lets users skip it for connections entered in the UI # STACKABLE_COCKPIT_QUERY_TTL=1800 # Seconds before completed, unaccessed tab queries are evicted (default: 1800) diff --git a/deploy/helm/cockpit/README.md b/deploy/helm/cockpit/README.md index b9da1ded..168937f8 100644 --- a/deploy/helm/cockpit/README.md +++ b/deploy/helm/cockpit/README.md @@ -168,7 +168,7 @@ At startup, the cockpit fetches the OIDC discovery document once and logs the ca ### Trino Connection Parameters Optional pre-configured Trino endpoint. When `trino.url` is set, the in-app connection form is hidden. -Otherwise users enter their own connection: the impersonation setting is its default for basic auth, and `trino.tls.insecure` lets them skip TLS certificate verification. +Otherwise users enter their own connection, and `trino.tls.insecure` lets them skip TLS certificate verification. | Parameter | Description | Default | | --- | --- | --- | diff --git a/deploy/helm/cockpit/values.yaml b/deploy/helm/cockpit/values.yaml index 5591aaea..5d013cfe 100644 --- a/deploy/helm/cockpit/values.yaml +++ b/deploy/helm/cockpit/values.yaml @@ -297,8 +297,7 @@ trino: publicUrl: "" userImpersonation: # Forward the logged-in user to Trino as X-Trino-User. When false, all queries - # run as trino.auth.username instead (requires basic auth). Without trino.url, - # this is the default for connections entered in the UI with basic auth. + # run as trino.auth.username instead (requires basic auth). enabled: true # OIDC claim used as the Trino user. Only consumed when enabled and OIDC is on. userClaim: preferred_username diff --git a/e2e/trino/connection-form.spec.ts b/e2e/trino/connection-form.spec.ts index 3fb96d2c..d0598ff2 100644 --- a/e2e/trino/connection-form.spec.ts +++ b/e2e/trino/connection-form.spec.ts @@ -38,8 +38,7 @@ function interceptSaveSuccess(page: import('@playwright/test').Page) { authType: 'none', authUsername: '', authPassword: '', - tlsInsecure: false, - impersonate: true + tlsInsecure: false }, message: { type: 'success' } }; @@ -222,56 +221,6 @@ test.describe('Connection form (manual mode)', () => { await expect(page.getByLabel('Password')).not.toBeVisible(); }); - test('impersonation toggle is only shown for basic auth', async ({ page }) => { - await page.goto('/trino'); - await waitForHydration(page); - - const toggle = page.getByLabel('Impersonate the logged-in user'); - await expect(toggle).not.toBeVisible(); - - await page.getByRole('radio', { name: 'Basic' }).click(); - await expect(toggle).toBeVisible(); - // Defaults to the deployment's impersonation setting (enabled in the test env). - await expect(toggle).toBeChecked(); - }); - - test('disabling impersonation is posted with the connection', async ({ page }) => { - await page.goto('/trino'); - await waitForHydration(page); - - await page.getByLabel('URL').fill(MOCK_TRINO_URL); - await page.getByRole('radio', { name: 'Basic' }).click(); - await page.getByLabel('Username').fill('alice'); - await page.getByLabel('Password').fill('secret'); - await page.getByLabel('Impersonate the logged-in user').uncheck(); - - const posted = captureSave(page); - await page.getByRole('button', { name: 'Save' }).click(); - - // An unchecked checkbox is not posted; superforms reads the missing field as false. - const body = await posted; - expect(body.get('authUsername')).toBe('alice'); - expect(body.has('impersonate')).toBe(false); - expect(await page.evaluate(() => localStorage.getItem('trino_impersonate'))).toBe('false'); - }); - - test('impersonation setting is kept while the toggle is hidden', async ({ page }) => { - await page.goto('/trino'); - await waitForHydration(page); - - await page.getByRole('radio', { name: 'Basic' }).click(); - await page.getByLabel('Impersonate the logged-in user').uncheck(); - await page.getByRole('radio', { name: 'No auth' }).click(); - await page.getByLabel('URL').fill(MOCK_TRINO_URL); - - const posted = captureSave(page); - await page.getByRole('button', { name: 'Save' }).click(); - expect((await posted).get('impersonate')).toBe('false'); - - await page.getByRole('radio', { name: 'Basic' }).click(); - await expect(page.getByLabel('Impersonate the logged-in user')).not.toBeChecked(); - }); - test('TLS verification option is hidden unless allowed', async ({ page }) => { await page.goto('/trino'); await waitForHydration(page); diff --git a/messages/de.json b/messages/de.json index 4c066ac3..c745f6c7 100644 --- a/messages/de.json +++ b/messages/de.json @@ -99,8 +99,6 @@ "trino_connection_test_failed": "Verbindung zu Trino fehlgeschlagen – bitte URL und Zugangsdaten prüfen.", "trino_connection_save_failed": "Die Verbindung konnte nicht gespeichert werden. Bitte erneut versuchen.", "trino_connection_env_managed": "Die Verbindung wird über Umgebungsvariablen verwaltet.", - "trino_connection_impersonate": "Als angemeldeter Benutzer ausführen", - "trino_connection_impersonate_hint": "Abfragen laufen unter Ihrem Cockpit-Benutzer; die Zugangsdaten oben müssen sich als dieser Benutzer ausgeben dürfen. Ausgeschaltet laufen Abfragen unter dem Benutzer oben.", "trino_connection_tls_insecure": "TLS-Zertifikatsprüfung überspringen", "trino_connection_tls_insecure_hint": "Unsicher: Das Zertifikat des Trino-Servers wird nicht geprüft. Nur für Testumgebungen verwenden.", "trino_connection_tls_insecure_not_allowed": "Das Überspringen der TLS-Zertifikatsprüfung ist auf diesem Server nicht erlaubt.", diff --git a/messages/en.json b/messages/en.json index db16edc5..63b7f6b7 100644 --- a/messages/en.json +++ b/messages/en.json @@ -99,8 +99,6 @@ "trino_connection_test_failed": "Could not connect to Trino — check the URL and credentials.", "trino_connection_save_failed": "Could not save the connection. Please try again.", "trino_connection_env_managed": "The connection is managed via environment variables.", - "trino_connection_impersonate": "Impersonate the logged-in user", - "trino_connection_impersonate_hint": "Queries run as your Cockpit user; the credentials above must be allowed to impersonate it. When off, queries run as the user above.", "trino_connection_tls_insecure": "Skip TLS certificate verification", "trino_connection_tls_insecure_hint": "Insecure: the Trino server certificate is not checked. Only use this for test setups.", "trino_connection_tls_insecure_not_allowed": "Skipping TLS certificate verification is not allowed on this server.", diff --git a/src/lib/server/trino/client.test.ts b/src/lib/server/trino/client.test.ts index 72dd8b06..050db98b 100644 --- a/src/lib/server/trino/client.test.ts +++ b/src/lib/server/trino/client.test.ts @@ -36,14 +36,12 @@ describe('TrinoClient X-Trino-User header', () => { }); await client.submit('SELECT 1', { user: 'alice' }); expect(submittedHeaders()).not.toHaveProperty('X-Trino-User'); - expect(client.impersonates).toBe(false); }); it('still sends X-Trino-User when impersonation is disabled but no credentials are set', async () => { const client = new TrinoClient({ serverUrl: 'http://trino:8080', impersonate: false }); await client.submit('SELECT 1', { user: 'anonymous' }); expect(submittedHeaders()['X-Trino-User']).toBe('anonymous'); - expect(client.impersonates).toBe(true); }); }); diff --git a/src/lib/server/trino/client.ts b/src/lib/server/trino/client.ts index f04e13c3..10d5a247 100644 --- a/src/lib/server/trino/client.ts +++ b/src/lib/server/trino/client.ts @@ -130,16 +130,10 @@ export class TrinoClient { return res.json() as Promise; } - /** Whether requests carry `X-Trino-User`. Without credentials it is the only - * user Trino gets, so impersonation can only be turned off when authenticated. */ - get impersonates(): boolean { - return this.impersonate || !this.authenticated; - } - /** commonHeaders plus X-Trino-User for the given user when impersonating. */ private headersForUser(user: string | undefined): Record { const headers = { ...this.commonHeaders }; - if (user && this.impersonates) { + if (user && (this.impersonate || !this.authenticated)) { headers['X-Trino-User'] = user; } return headers; @@ -237,7 +231,7 @@ const { tlsInsecure } = trinoConfig; -const trinoUserImpersonation = trinoConfig.userImpersonation; +export const trinoUserImpersonation = trinoConfig.userImpersonation; /** True when the Trino connection is pre-configured via environment variables. */ export const trinoConfigured = !!trinoUrl; diff --git a/src/lib/server/trino/config.ts b/src/lib/server/trino/config.ts index a791f9f5..acfbb548 100644 --- a/src/lib/server/trino/config.ts +++ b/src/lib/server/trino/config.ts @@ -45,7 +45,6 @@ export function parseTrinoConfig(env: Record) { /** Trino settings from the environment. The connection fields are only used * when `STACKABLE_COCKPIT_TRINO_URL` is set; otherwise users configure their - * own connection. For those per-user connections, impersonation is the default - * users can change with basic auth, and `tlsInsecure` decides whether users - * may skip certificate verification. */ + * own connection. Impersonation also applies to those per-user connections, + * and `tlsInsecure` decides whether users may skip certificate verification. */ export const trinoConfig = parseTrinoConfig(env); diff --git a/src/lib/server/trino/user-clients.test.ts b/src/lib/server/trino/user-clients.test.ts index 9c1818e1..56863f24 100644 --- a/src/lib/server/trino/user-clients.test.ts +++ b/src/lib/server/trino/user-clients.test.ts @@ -16,7 +16,8 @@ vi.mock('$lib/server/logging', () => ({ })); vi.mock('./client.js', () => ({ TrinoClient: TrinoClientMock, - buildBasicAuthHeader: (user: string, pass: string) => `Basic ${user}:${pass}` + buildBasicAuthHeader: (user: string, pass: string) => `Basic ${user}:${pass}`, + trinoUserImpersonation: true })); vi.mock('./config.js', () => ({ trinoConfig: trinoConfigMock })); vi.mock('undici', () => ({ Agent: AgentMock })); @@ -29,8 +30,7 @@ const BASE = { authType: 'none', username: '', password: '', - tlsInsecure: false, - impersonate: true + tlsInsecure: false } as const; const BASIC = { ...BASE, authType: 'basic', username: 'alice', password: 'secret' } as const; @@ -72,15 +72,6 @@ describe('buildUserTrinoClient', () => { ); }); - // TrinoClient keeps sending X-Trino-User without credentials (see client.test.ts). - it('passes the impersonation setting to the client', () => { - uc.buildUserTrinoClient({ ...BASIC, impersonate: false }); - - expect(TrinoClientMock).toHaveBeenCalledWith( - expect.objectContaining({ authorization: 'Basic alice:secret', impersonate: false }) - ); - }); - it('rejects skipping TLS verification unless allowed', () => { expect(() => uc.buildUserTrinoClient({ ...BASE, tlsInsecure: true })).toThrow(/not allowed/); expect(TrinoClientMock).not.toHaveBeenCalled(); diff --git a/src/lib/server/trino/user-clients.ts b/src/lib/server/trino/user-clients.ts index 003ec8f1..727c99e7 100644 --- a/src/lib/server/trino/user-clients.ts +++ b/src/lib/server/trino/user-clients.ts @@ -1,6 +1,6 @@ import { Agent } from 'undici'; import { logger } from '$lib/server/logging'; -import { TrinoClient, buildBasicAuthHeader } from './client.js'; +import { TrinoClient, buildBasicAuthHeader, trinoUserImpersonation } from './client.js'; import { trinoConfig } from './config.js'; const log = logger.child({ module: 'trino-user-clients' }); @@ -12,9 +12,6 @@ export interface UserConnectionConfig { password: string; /** Skip TLS certificate verification. Only allowed with `STACKABLE_COCKPIT_TRINO_TLS_INSECURE`. */ tlsInsecure: boolean; - /** Forward the logged-in user as `X-Trino-User`. Only takes effect with basic - * auth: without credentials the client always sends the user header. */ - impersonate: boolean; } interface UserEntry { @@ -50,7 +47,7 @@ export function buildUserTrinoClient(config: UserConnectionConfig): TrinoClient serverUrl: config.url, authorization, dispatcher: dispatcherFor(config.tlsInsecure), - impersonate: config.impersonate + impersonate: trinoUserImpersonation }); } @@ -61,8 +58,7 @@ export function createUserTrinoClient(userId: string, config: UserConnectionConf const context = { user_id: userId, trino_url: config.url, - tls_insecure: config.tlsInsecure, - user_impersonation: client.impersonates + tls_insecure: config.tlsInsecure }; if (config.tlsInsecure) { log.warn(context, 'user connection created without TLS certificate verification'); diff --git a/src/routes/(app)/trino/+page.server.ts b/src/routes/(app)/trino/+page.server.ts index 330b2bd4..bc86075a 100644 --- a/src/routes/(app)/trino/+page.server.ts +++ b/src/routes/(app)/trino/+page.server.ts @@ -27,8 +27,6 @@ export const load: PageServerLoad = async ({ locals }) => { const connectionForm = await superValidate( zod(ConnectionSchema) ); - // The deployment's impersonation setting is the default for per-user connections. - connectionForm.data.impersonate = trinoConfig.userImpersonation; const userId = getUserId(locals); const activeQueries = getAllQuerySummaries(userId); const userClientExists = getUserTrinoClient(userId) !== null; @@ -80,15 +78,13 @@ export const actions: Actions = { { status: 500 } ); } - const { connectionUrl, authType, authUsername, authPassword, tlsInsecure, impersonate } = - form.data; + const { connectionUrl, authType, authUsername, authPassword, tlsInsecure } = form.data; const config = { url: connectionUrl, authType, username: authUsername, password: authPassword, - tlsInsecure, - impersonate + tlsInsecure }; // Verify connectivity before storing the client or cancelling running queries. diff --git a/src/routes/(app)/trino/+page.svelte b/src/routes/(app)/trino/+page.svelte index e8e57bf5..3c4f29ef 100644 --- a/src/routes/(app)/trino/+page.svelte +++ b/src/routes/(app)/trino/+page.svelte @@ -142,8 +142,6 @@ $connectionForm.connectionUrl = getStoredValue('trino_url', ''); $connectionForm.authType = getStoredValue('trino_auth_type', 'none') as 'none' | 'basic'; $connectionForm.authUsername = getStoredValue('trino_username', ''); - $connectionForm.impersonate = - getStoredValue('trino_impersonate', String($connectionForm.impersonate)) === 'true'; $connectionForm.tlsInsecure = data.allowInsecureTls && getStoredValue('trino_tls_insecure', 'false') === 'true'; } @@ -187,12 +185,6 @@ localStorage.setItem('trino_url', $connectionForm.connectionUrl); localStorage.setItem('trino_auth_type', $connectionForm.authType); localStorage.setItem('trino_username', $connectionForm.authUsername); - // Only store a deviation from the deployment default, so a changed default still applies. - if ($connectionForm.impersonate === data.connectionForm.data.impersonate) { - localStorage.removeItem('trino_impersonate'); - } else { - localStorage.setItem('trino_impersonate', String($connectionForm.impersonate)); - } localStorage.setItem('trino_tls_insecure', String($connectionForm.tlsInsecure)); }); @@ -583,28 +575,6 @@ {/if} - -
-
- -

- {m.trino_connection_impersonate_hint()} -

-
- -
- {:else} - - {/if} {#if data.allowInsecureTls} diff --git a/src/routes/(app)/trino/validation.ts b/src/routes/(app)/trino/validation.ts index 6b89b8c8..1123e33f 100644 --- a/src/routes/(app)/trino/validation.ts +++ b/src/routes/(app)/trino/validation.ts @@ -9,8 +9,7 @@ export const ConnectionSchema = z authType: z.enum(['none', 'basic']).default('none'), authUsername: z.string().default(''), authPassword: z.string().default(''), - tlsInsecure: z.boolean().default(false), - impersonate: z.boolean().default(true) + tlsInsecure: z.boolean().default(false) }) .superRefine((data, ctx) => { if (data.authType === 'basic') { From f971ddf6a9d6b7efc3815d87f8cc5f53e12aa566 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Thu, 12 Nov 2026 12:24:48 +0100 Subject: [PATCH 3/5] refactor(trino): Rename STACKABLE_COCKPIT_TRINO_TLS_INSECURE to STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS The variable skips TLS certificate verification for the pre-configured connection and, without STACKABLE_COCKPIT_TRINO_URL, lets users skip it for their own. The Helm value trino.tls.insecure becomes trino.tls.allowInsecure. --- .env.example | 2 +- deploy/helm/cockpit/README.md | 4 ++-- deploy/helm/cockpit/templates/deployment.yaml | 4 ++-- deploy/helm/cockpit/values.yaml | 2 +- dev/helm/values.yaml | 2 +- dev/modules/env.sh | 2 +- src/lib/server/trino/client.test.ts | 2 +- src/lib/server/trino/client.ts | 2 +- src/lib/server/trino/config.test.ts | 4 ++-- src/lib/server/trino/config.ts | 9 +++++---- src/lib/server/trino/user-clients.test.ts | 8 ++++---- src/lib/server/trino/user-clients.ts | 4 ++-- src/routes/(app)/trino/+page.server.ts | 6 +++--- 13 files changed, 26 insertions(+), 25 deletions(-) diff --git a/.env.example b/.env.example index 69c2e192..f712faf7 100644 --- a/.env.example +++ b/.env.example @@ -55,7 +55,7 @@ DATABASE_PASSWORD=change-me # STACKABLE_COCKPIT_TRINO_AUTH_USERNAME=service-account # STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD=secret # STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION=false # Disable impersonation: run all queries as the auth user (requires basic auth). Default: true -# STACKABLE_COCKPIT_TRINO_TLS_INSECURE=false # Set to "true" to skip TLS certificate verification. Without STACKABLE_COCKPIT_TRINO_URL, lets users skip it for connections entered in the UI +# STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS=false # Set to "true" to skip TLS certificate verification. Without STACKABLE_COCKPIT_TRINO_URL, lets users skip it for connections entered in the UI # STACKABLE_COCKPIT_QUERY_TTL=1800 # Seconds before completed, unaccessed tab queries are evicted (default: 1800) # Feature flags diff --git a/deploy/helm/cockpit/README.md b/deploy/helm/cockpit/README.md index 168937f8..9fa014a6 100644 --- a/deploy/helm/cockpit/README.md +++ b/deploy/helm/cockpit/README.md @@ -168,7 +168,7 @@ At startup, the cockpit fetches the OIDC discovery document once and logs the ca ### Trino Connection Parameters Optional pre-configured Trino endpoint. When `trino.url` is set, the in-app connection form is hidden. -Otherwise users enter their own connection, and `trino.tls.insecure` lets them skip TLS certificate verification. +Otherwise users enter their own connection, and `trino.tls.allowInsecure` lets them skip TLS certificate verification. | Parameter | Description | Default | | --- | --- | --- | @@ -180,7 +180,7 @@ Otherwise users enter their own connection, and `trino.tls.insecure` lets them s | `trino.auth.username` | Username for basic auth. | `""` | | `trino.auth.password.secretKeyRef.name` | Name of the Secret holding the Trino password. Required when `type=basic`. | `""` | | `trino.auth.password.secretKeyRef.key` | Key inside the Secret. | `trino-auth-password` | -| `trino.tls.insecure` | Skip TLS certificate verification. Logs a warning in production; for a private CA, use `tls.caCerts` or `tls.secretClasses` instead. Without `trino.url`, users may skip verification for the connection they enter in the UI. | `false` | +| `trino.tls.allowInsecure` | Skip TLS certificate verification. Logs a warning in production; for a private CA, use `tls.caCerts` or `tls.secretClasses` instead. Without `trino.url`, users may skip verification for the connection they enter in the UI. | `false` | | `trino.queryTtl` | Query result TTL in seconds. | `1800` (application default) | ### Security Parameters diff --git a/deploy/helm/cockpit/templates/deployment.yaml b/deploy/helm/cockpit/templates/deployment.yaml index 07628199..925d358c 100644 --- a/deploy/helm/cockpit/templates/deployment.yaml +++ b/deploy/helm/cockpit/templates/deployment.yaml @@ -275,8 +275,8 @@ spec: {{- end }} - name: STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION value: {{ .Values.trino.userImpersonation.enabled | quote }} - {{- if .Values.trino.tls.insecure }} - - name: STACKABLE_COCKPIT_TRINO_TLS_INSECURE + {{- if .Values.trino.tls.allowInsecure }} + - name: STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS value: "true" {{- end }} {{- with .Values.extraEnv }} diff --git a/deploy/helm/cockpit/values.yaml b/deploy/helm/cockpit/values.yaml index 5d013cfe..bad48d1a 100644 --- a/deploy/helm/cockpit/values.yaml +++ b/deploy/helm/cockpit/values.yaml @@ -314,6 +314,6 @@ trino: # Skip TLS certificate verification. Without trino.url, users may skip it for # the connection they enter in the UI. A private CA goes into tls.caCerts or # tls.secretClasses instead. - insecure: false + allowInsecure: false # Query result TTL in seconds. Default: 1800 (application default). # queryTtl: 1800 diff --git a/dev/helm/values.yaml b/dev/helm/values.yaml index 8f2ac47a..b60e99bc 100644 --- a/dev/helm/values.yaml +++ b/dev/helm/values.yaml @@ -19,4 +19,4 @@ trino: name: stackable-cockpit-credentials key: trino-auth-password tls: - insecure: true + allowInsecure: true diff --git a/dev/modules/env.sh b/dev/modules/env.sh index 92ff9015..88bcaa3a 100644 --- a/dev/modules/env.sh +++ b/dev/modules/env.sh @@ -22,7 +22,7 @@ STACKABLE_COCKPIT_TRINO_URL=${TRINO_BASE_URL} STACKABLE_COCKPIT_TRINO_AUTH_TYPE=basic STACKABLE_COCKPIT_TRINO_AUTH_USERNAME=stackable-cockpit STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD=stackable-cockpit-dev -STACKABLE_COCKPIT_TRINO_TLS_INSECURE=true +STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS=true STORAGE_ENCRYPTION_KEY=${encryption_key} STACKABLE_COCKPIT_STORAGE_BROWSER_ENABLED=true STACKABLE_COCKPIT_TEXT_PREVIEW_BYTES=262144 diff --git a/src/lib/server/trino/client.test.ts b/src/lib/server/trino/client.test.ts index 050db98b..47d13109 100644 --- a/src/lib/server/trino/client.test.ts +++ b/src/lib/server/trino/client.test.ts @@ -288,7 +288,7 @@ describe('insecure Trino TLS warning', () => { async function warnings(nodeEnv: string, insecure: string) { vi.stubEnv('NODE_ENV', nodeEnv); mockEnv.STACKABLE_COCKPIT_TRINO_URL = 'https://trino:8443'; - mockEnv.STACKABLE_COCKPIT_TRINO_TLS_INSECURE = insecure; + mockEnv.STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS = insecure; vi.resetModules(); const { logger } = await import('$lib/server/logging'); const log = logger.child({}); diff --git a/src/lib/server/trino/client.ts b/src/lib/server/trino/client.ts index 10d5a247..03d92ed1 100644 --- a/src/lib/server/trino/client.ts +++ b/src/lib/server/trino/client.ts @@ -228,7 +228,7 @@ const { authType, authUsername, authPassword, - tlsInsecure + allowInsecureTls: tlsInsecure } = trinoConfig; export const trinoUserImpersonation = trinoConfig.userImpersonation; diff --git a/src/lib/server/trino/config.test.ts b/src/lib/server/trino/config.test.ts index 08d45f7c..bd04853c 100644 --- a/src/lib/server/trino/config.test.ts +++ b/src/lib/server/trino/config.test.ts @@ -14,7 +14,7 @@ describe('parseTrinoConfig', () => { authType: 'none', authUsername: undefined, authPassword: undefined, - tlsInsecure: false, + allowInsecureTls: false, userImpersonation: true, queryTtlSeconds: 1800 }); @@ -23,7 +23,7 @@ describe('parseTrinoConfig', () => { it.each([ ['STACKABLE_COCKPIT_TRINO_URL', 'trino.example.com:8443'], ['STACKABLE_COCKPIT_TRINO_AUTH_TYPE', 'oauth2'], - ['STACKABLE_COCKPIT_TRINO_TLS_INSECURE', 'yes'], + ['STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS', 'yes'], ['STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION', 'off'], ['STACKABLE_COCKPIT_QUERY_TTL', '30m'] ])('rejects an invalid %s', (name, value) => { diff --git a/src/lib/server/trino/config.ts b/src/lib/server/trino/config.ts index acfbb548..4e9da49e 100644 --- a/src/lib/server/trino/config.ts +++ b/src/lib/server/trino/config.ts @@ -12,7 +12,7 @@ const schema = z STACKABLE_COCKPIT_TRINO_AUTH_TYPE: envString.pipe(z.enum(['none', 'basic']).default('none')), STACKABLE_COCKPIT_TRINO_AUTH_USERNAME: envString, STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD: envString, - STACKABLE_COCKPIT_TRINO_TLS_INSECURE: envBoolean(false), + STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS: envBoolean(false), STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION: envBoolean(true), STACKABLE_COCKPIT_QUERY_TTL: envInteger(1800) }) @@ -37,7 +37,7 @@ export function parseTrinoConfig(env: Record) { authType: config.STACKABLE_COCKPIT_TRINO_AUTH_TYPE, authUsername: config.STACKABLE_COCKPIT_TRINO_AUTH_USERNAME, authPassword: config.STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD, - tlsInsecure: config.STACKABLE_COCKPIT_TRINO_TLS_INSECURE, + allowInsecureTls: config.STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS, userImpersonation: config.STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION, queryTtlSeconds: config.STACKABLE_COCKPIT_QUERY_TTL }; @@ -45,6 +45,7 @@ export function parseTrinoConfig(env: Record) { /** Trino settings from the environment. The connection fields are only used * when `STACKABLE_COCKPIT_TRINO_URL` is set; otherwise users configure their - * own connection. Impersonation also applies to those per-user connections, - * and `tlsInsecure` decides whether users may skip certificate verification. */ + * own connection. Impersonation also applies to those per-user connections. + * `allowInsecureTls` skips certificate verification for the pre-configured + * connection, or lets users skip it for their own. */ export const trinoConfig = parseTrinoConfig(env); diff --git a/src/lib/server/trino/user-clients.test.ts b/src/lib/server/trino/user-clients.test.ts index 56863f24..1ad84ffe 100644 --- a/src/lib/server/trino/user-clients.test.ts +++ b/src/lib/server/trino/user-clients.test.ts @@ -7,7 +7,7 @@ const { TrinoClientMock, AgentMock, trinoConfigMock, logMock } = vi.hoisted(() = AgentMock: vi.fn(function (this: Record, options: unknown) { this.options = options; }), - trinoConfigMock: { tlsInsecure: false }, + trinoConfigMock: { allowInsecureTls: false }, logMock: { info: vi.fn(), warn: vi.fn(), debug: vi.fn() } })); @@ -37,7 +37,7 @@ const BASIC = { ...BASE, authType: 'basic', username: 'alice', password: 'secret beforeEach(async () => { vi.clearAllMocks(); vi.resetModules(); - trinoConfigMock.tlsInsecure = false; + trinoConfigMock.allowInsecureTls = false; uc = await import('./user-clients.js'); }); @@ -78,7 +78,7 @@ describe('buildUserTrinoClient', () => { }); it('skips TLS verification when allowed and requested', () => { - trinoConfigMock.tlsInsecure = true; + trinoConfigMock.allowInsecureTls = true; uc.buildUserTrinoClient({ ...BASE, tlsInsecure: true }); uc.buildUserTrinoClient({ ...BASIC, tlsInsecure: true }); @@ -121,7 +121,7 @@ describe('per-user connections', () => { }); it('logs a warning when the connection skips TLS verification', () => { - trinoConfigMock.tlsInsecure = true; + trinoConfigMock.allowInsecureTls = true; uc.createUserTrinoClient('u1', BASE); expect(logMock.warn).not.toHaveBeenCalled(); diff --git a/src/lib/server/trino/user-clients.ts b/src/lib/server/trino/user-clients.ts index 727c99e7..66725cbc 100644 --- a/src/lib/server/trino/user-clients.ts +++ b/src/lib/server/trino/user-clients.ts @@ -10,7 +10,7 @@ export interface UserConnectionConfig { authType: 'none' | 'basic'; username: string; password: string; - /** Skip TLS certificate verification. Only allowed with `STACKABLE_COCKPIT_TRINO_TLS_INSECURE`. */ + /** Skip TLS certificate verification. Only allowed with `STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS`. */ tlsInsecure: boolean; } @@ -34,7 +34,7 @@ function dispatcherFor(tlsInsecure: boolean): Agent | undefined { /** Build a per-user Trino client from a connection config without storing it. */ export function buildUserTrinoClient(config: UserConnectionConfig): TrinoClient { - if (config.tlsInsecure && !trinoConfig.tlsInsecure) { + if (config.tlsInsecure && !trinoConfig.allowInsecureTls) { throw new Error('Skipping TLS certificate verification is not allowed'); } diff --git a/src/routes/(app)/trino/+page.server.ts b/src/routes/(app)/trino/+page.server.ts index bc86075a..f7a92b99 100644 --- a/src/routes/(app)/trino/+page.server.ts +++ b/src/routes/(app)/trino/+page.server.ts @@ -36,8 +36,8 @@ export const load: PageServerLoad = async ({ locals }) => { trinoConfigured, userClientExists, completionEnabled, - // Without a pre-configured URL, STACKABLE_COCKPIT_TRINO_TLS_INSECURE lets users skip verification. - allowInsecureTls: !trinoConfigured && trinoConfig.tlsInsecure + // Without a pre-configured URL, STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS lets users skip verification. + allowInsecureTls: !trinoConfigured && trinoConfig.allowInsecureTls }; }; @@ -64,7 +64,7 @@ export const actions: Actions = { return fail(400, { form }); } - if (form.data.tlsInsecure && !trinoConfig.tlsInsecure) { + if (form.data.tlsInsecure && !trinoConfig.allowInsecureTls) { log.info('connection save rejected, skipping TLS verification is not allowed'); return setError(form, 'tlsInsecure', m.trino_connection_tls_insecure_not_allowed()); } From cbf7e089b0aa91e331e1a020acfeed2b05c97e04 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Thu, 12 Nov 2026 12:55:02 +0100 Subject: [PATCH 4/5] docs(trino): Document the renamed TLS setting and the user toggle --- docs/modules/cockpit/pages/modules/trino.adoc | 3 ++- .../cockpit/pages/reference/environment-variables.adoc | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/modules/cockpit/pages/modules/trino.adoc b/docs/modules/cockpit/pages/modules/trino.adoc index d1e728c5..abd27b35 100644 --- a/docs/modules/cockpit/pages/modules/trino.adoc +++ b/docs/modules/cockpit/pages/modules/trino.adoc @@ -27,10 +27,11 @@ From another namespace, append `..svc.cluster.local`. <3> `none` (default) or `basic`. `basic` requires `username` and `password`, otherwise Cockpit does not start. For a Trino cluster with a private CA, see xref:installation/helm.adoc#_ca_certificates[CA certificates]. -`trino.tls.insecure: true` skips certificate verification instead. +`trino.tls.allowInsecure: true` skips certificate verification instead. Without `trino.url`, each user enters a connection in the UI. These connections are kept in memory and are lost when Cockpit restarts. +With `trino.tls.allowInsecure: true`, users can skip certificate verification for their connection. == User identity diff --git a/docs/modules/cockpit/pages/reference/environment-variables.adoc b/docs/modules/cockpit/pages/reference/environment-variables.adoc index d6a9a365..1792ad32 100644 --- a/docs/modules/cockpit/pages/reference/environment-variables.adoc +++ b/docs/modules/cockpit/pages/reference/environment-variables.adoc @@ -138,9 +138,9 @@ Cockpit does not start if a value is invalid, and the error names every invalid | |Password for basic authentication. -|`STACKABLE_COCKPIT_TRINO_TLS_INSECURE` +|`STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS` |`false` -|Skip TLS certificate verification. +|Skip TLS certificate verification. Without `STACKABLE_COCKPIT_TRINO_URL`, lets users skip it for the connection they enter in the UI. |`STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION` |`true` From d2e2d1026e33c727a3156502f178db2da5c77946 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Fri, 13 Nov 2026 10:24:16 +0100 Subject: [PATCH 5/5] refactor(trino): Split the insecure TLS setting by connection kind STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS both skipped verification for the pre-configured connection and let users skip it for their own, depending on whether STACKABLE_COCKPIT_TRINO_URL was set. - STACKABLE_COCKPIT_TRINO_TLS_INSECURE (trino.tls.insecure) skips verification for the pre-configured connection, as on main. - STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE (trino.userConnection.tls.allowInsecure) lets users skip it for the connection they enter in the UI. Cockpit and the Helm chart reject either setting where it has no effect. --- .env.example | 3 +- deploy/helm/cockpit/README.md | 5 +-- deploy/helm/cockpit/templates/deployment.yaml | 14 ++++++-- deploy/helm/cockpit/values.yaml | 13 ++++--- dev/helm/values.yaml | 2 +- dev/modules/env.sh | 2 +- docs/modules/cockpit/pages/modules/trino.adoc | 4 +-- .../reference/environment-variables.adoc | 8 +++-- src/lib/server/trino/client.test.ts | 2 +- src/lib/server/trino/client.ts | 2 +- src/lib/server/trino/config.test.ts | 35 +++++++++++++++++-- src/lib/server/trino/config.ts | 31 +++++++++++++--- src/lib/server/trino/user-clients.test.ts | 8 ++--- src/lib/server/trino/user-clients.ts | 4 +-- src/routes/(app)/trino/+page.server.ts | 6 ++-- 15 files changed, 107 insertions(+), 32 deletions(-) diff --git a/.env.example b/.env.example index f712faf7..d0adc374 100644 --- a/.env.example +++ b/.env.example @@ -55,7 +55,8 @@ DATABASE_PASSWORD=change-me # STACKABLE_COCKPIT_TRINO_AUTH_USERNAME=service-account # STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD=secret # STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION=false # Disable impersonation: run all queries as the auth user (requires basic auth). Default: true -# STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS=false # Set to "true" to skip TLS certificate verification. Without STACKABLE_COCKPIT_TRINO_URL, lets users skip it for connections entered in the UI +# STACKABLE_COCKPIT_TRINO_TLS_INSECURE=false # Set to "true" to skip TLS certificate verification. Requires STACKABLE_COCKPIT_TRINO_URL +# STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE=false # Set to "true" to let users skip TLS certificate verification for connections entered in the UI. Requires STACKABLE_COCKPIT_TRINO_URL to be unset # STACKABLE_COCKPIT_QUERY_TTL=1800 # Seconds before completed, unaccessed tab queries are evicted (default: 1800) # Feature flags diff --git a/deploy/helm/cockpit/README.md b/deploy/helm/cockpit/README.md index 9fa014a6..2b8dd4e3 100644 --- a/deploy/helm/cockpit/README.md +++ b/deploy/helm/cockpit/README.md @@ -168,7 +168,7 @@ At startup, the cockpit fetches the OIDC discovery document once and logs the ca ### Trino Connection Parameters Optional pre-configured Trino endpoint. When `trino.url` is set, the in-app connection form is hidden. -Otherwise users enter their own connection, and `trino.tls.allowInsecure` lets them skip TLS certificate verification. +Otherwise users enter their own connection, and `trino.userConnection.tls.allowInsecure` lets them skip TLS certificate verification. | Parameter | Description | Default | | --- | --- | --- | @@ -180,7 +180,8 @@ Otherwise users enter their own connection, and `trino.tls.allowInsecure` lets t | `trino.auth.username` | Username for basic auth. | `""` | | `trino.auth.password.secretKeyRef.name` | Name of the Secret holding the Trino password. Required when `type=basic`. | `""` | | `trino.auth.password.secretKeyRef.key` | Key inside the Secret. | `trino-auth-password` | -| `trino.tls.allowInsecure` | Skip TLS certificate verification. Logs a warning in production; for a private CA, use `tls.caCerts` or `tls.secretClasses` instead. Without `trino.url`, users may skip verification for the connection they enter in the UI. | `false` | +| `trino.tls.insecure` | Skip TLS certificate verification for `trino.url`. Requires `trino.url`. Logs a warning in production; for a private CA, use `tls.caCerts` or `tls.secretClasses` instead. | `false` | +| `trino.userConnection.tls.allowInsecure` | Let users skip TLS certificate verification for the connection they enter in the UI. Requires an empty `trino.url`. | `false` | | `trino.queryTtl` | Query result TTL in seconds. | `1800` (application default) | ### Security Parameters diff --git a/deploy/helm/cockpit/templates/deployment.yaml b/deploy/helm/cockpit/templates/deployment.yaml index 925d358c..0f3ec828 100644 --- a/deploy/helm/cockpit/templates/deployment.yaml +++ b/deploy/helm/cockpit/templates/deployment.yaml @@ -275,8 +275,18 @@ spec: {{- end }} - name: STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION value: {{ .Values.trino.userImpersonation.enabled | quote }} - {{- if .Values.trino.tls.allowInsecure }} - - name: STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS + {{- if .Values.trino.tls.insecure }} + {{- if not .Values.trino.url }} + {{- fail "trino.tls.insecure only applies to the pre-configured connection and requires trino.url" }} + {{- end }} + - name: STACKABLE_COCKPIT_TRINO_TLS_INSECURE + value: "true" + {{- end }} + {{- if .Values.trino.userConnection.tls.allowInsecure }} + {{- if .Values.trino.url }} + {{- fail "trino.userConnection.tls.allowInsecure only applies to connections users enter and requires an empty trino.url" }} + {{- end }} + - name: STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE value: "true" {{- end }} {{- with .Values.extraEnv }} diff --git a/deploy/helm/cockpit/values.yaml b/deploy/helm/cockpit/values.yaml index bad48d1a..26186c3c 100644 --- a/deploy/helm/cockpit/values.yaml +++ b/deploy/helm/cockpit/values.yaml @@ -311,9 +311,14 @@ trino: name: "" key: trino-auth-password tls: - # Skip TLS certificate verification. Without trino.url, users may skip it for - # the connection they enter in the UI. A private CA goes into tls.caCerts or - # tls.secretClasses instead. - allowInsecure: false + # Skip TLS certificate verification for the connection to trino.url. Requires trino.url. + # A private CA goes into tls.caCerts or tls.secretClasses instead. + insecure: false + # Connections users enter in the UI. Only used when trino.url is empty. + userConnection: + tls: + # Let users skip TLS certificate verification for their connection. Requires an + # empty trino.url. + allowInsecure: false # Query result TTL in seconds. Default: 1800 (application default). # queryTtl: 1800 diff --git a/dev/helm/values.yaml b/dev/helm/values.yaml index b60e99bc..8f2ac47a 100644 --- a/dev/helm/values.yaml +++ b/dev/helm/values.yaml @@ -19,4 +19,4 @@ trino: name: stackable-cockpit-credentials key: trino-auth-password tls: - allowInsecure: true + insecure: true diff --git a/dev/modules/env.sh b/dev/modules/env.sh index 88bcaa3a..92ff9015 100644 --- a/dev/modules/env.sh +++ b/dev/modules/env.sh @@ -22,7 +22,7 @@ STACKABLE_COCKPIT_TRINO_URL=${TRINO_BASE_URL} STACKABLE_COCKPIT_TRINO_AUTH_TYPE=basic STACKABLE_COCKPIT_TRINO_AUTH_USERNAME=stackable-cockpit STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD=stackable-cockpit-dev -STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS=true +STACKABLE_COCKPIT_TRINO_TLS_INSECURE=true STORAGE_ENCRYPTION_KEY=${encryption_key} STACKABLE_COCKPIT_STORAGE_BROWSER_ENABLED=true STACKABLE_COCKPIT_TEXT_PREVIEW_BYTES=262144 diff --git a/docs/modules/cockpit/pages/modules/trino.adoc b/docs/modules/cockpit/pages/modules/trino.adoc index abd27b35..40b6b72c 100644 --- a/docs/modules/cockpit/pages/modules/trino.adoc +++ b/docs/modules/cockpit/pages/modules/trino.adoc @@ -27,11 +27,11 @@ From another namespace, append `..svc.cluster.local`. <3> `none` (default) or `basic`. `basic` requires `username` and `password`, otherwise Cockpit does not start. For a Trino cluster with a private CA, see xref:installation/helm.adoc#_ca_certificates[CA certificates]. -`trino.tls.allowInsecure: true` skips certificate verification instead. +`trino.tls.insecure: true` skips certificate verification instead. Without `trino.url`, each user enters a connection in the UI. These connections are kept in memory and are lost when Cockpit restarts. -With `trino.tls.allowInsecure: true`, users can skip certificate verification for their connection. +With `trino.userConnection.tls.allowInsecure: true`, users can skip certificate verification for their connection. == User identity diff --git a/docs/modules/cockpit/pages/reference/environment-variables.adoc b/docs/modules/cockpit/pages/reference/environment-variables.adoc index 1792ad32..d0d447cf 100644 --- a/docs/modules/cockpit/pages/reference/environment-variables.adoc +++ b/docs/modules/cockpit/pages/reference/environment-variables.adoc @@ -138,9 +138,13 @@ Cockpit does not start if a value is invalid, and the error names every invalid | |Password for basic authentication. -|`STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS` +|`STACKABLE_COCKPIT_TRINO_TLS_INSECURE` |`false` -|Skip TLS certificate verification. Without `STACKABLE_COCKPIT_TRINO_URL`, lets users skip it for the connection they enter in the UI. +|Skip TLS certificate verification for `STACKABLE_COCKPIT_TRINO_URL`. Requires `STACKABLE_COCKPIT_TRINO_URL`. + +|`STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE` +|`false` +|Let users skip TLS certificate verification for the connection they enter in the UI. Requires `STACKABLE_COCKPIT_TRINO_URL` to be unset. |`STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION` |`true` diff --git a/src/lib/server/trino/client.test.ts b/src/lib/server/trino/client.test.ts index 47d13109..050db98b 100644 --- a/src/lib/server/trino/client.test.ts +++ b/src/lib/server/trino/client.test.ts @@ -288,7 +288,7 @@ describe('insecure Trino TLS warning', () => { async function warnings(nodeEnv: string, insecure: string) { vi.stubEnv('NODE_ENV', nodeEnv); mockEnv.STACKABLE_COCKPIT_TRINO_URL = 'https://trino:8443'; - mockEnv.STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS = insecure; + mockEnv.STACKABLE_COCKPIT_TRINO_TLS_INSECURE = insecure; vi.resetModules(); const { logger } = await import('$lib/server/logging'); const log = logger.child({}); diff --git a/src/lib/server/trino/client.ts b/src/lib/server/trino/client.ts index 03d92ed1..10d5a247 100644 --- a/src/lib/server/trino/client.ts +++ b/src/lib/server/trino/client.ts @@ -228,7 +228,7 @@ const { authType, authUsername, authPassword, - allowInsecureTls: tlsInsecure + tlsInsecure } = trinoConfig; export const trinoUserImpersonation = trinoConfig.userImpersonation; diff --git a/src/lib/server/trino/config.test.ts b/src/lib/server/trino/config.test.ts index bd04853c..1f65c54d 100644 --- a/src/lib/server/trino/config.test.ts +++ b/src/lib/server/trino/config.test.ts @@ -14,7 +14,8 @@ describe('parseTrinoConfig', () => { authType: 'none', authUsername: undefined, authPassword: undefined, - allowInsecureTls: false, + tlsInsecure: false, + userConnectionAllowInsecureTls: false, userImpersonation: true, queryTtlSeconds: 1800 }); @@ -23,7 +24,7 @@ describe('parseTrinoConfig', () => { it.each([ ['STACKABLE_COCKPIT_TRINO_URL', 'trino.example.com:8443'], ['STACKABLE_COCKPIT_TRINO_AUTH_TYPE', 'oauth2'], - ['STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS', 'yes'], + ['STACKABLE_COCKPIT_TRINO_TLS_INSECURE', 'yes'], ['STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION', 'off'], ['STACKABLE_COCKPIT_QUERY_TTL', '30m'] ])('rejects an invalid %s', (name, value) => { @@ -42,4 +43,34 @@ describe('parseTrinoConfig', () => { ); expect(parseTrinoConfig(basic).authType).toBe('basic'); }); + + it('rejects an invalid STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE', () => { + expect(() => + parseTrinoConfig({ STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE: 'yes' }) + ).toThrow('STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE:'); + }); + + it('applies each TLS setting to its own kind of connection', () => { + expect( + parseTrinoConfig({ + STACKABLE_COCKPIT_TRINO_URL: TRINO_URL, + STACKABLE_COCKPIT_TRINO_TLS_INSECURE: 'true' + }) + ).toMatchObject({ tlsInsecure: true, userConnectionAllowInsecureTls: false }); + expect( + parseTrinoConfig({ STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE: 'true' }) + ).toMatchObject({ tlsInsecure: false, userConnectionAllowInsecureTls: true }); + }); + + it('rejects a TLS setting that has no effect', () => { + expect(() => parseTrinoConfig({ STACKABLE_COCKPIT_TRINO_TLS_INSECURE: 'true' })).toThrow( + /STACKABLE_COCKPIT_TRINO_TLS_INSECURE: only applies to the pre-configured connection/ + ); + expect(() => + parseTrinoConfig({ + STACKABLE_COCKPIT_TRINO_URL: TRINO_URL, + STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE: 'true' + }) + ).toThrow(/STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE: only applies to/); + }); }); diff --git a/src/lib/server/trino/config.ts b/src/lib/server/trino/config.ts index 4e9da49e..5c250031 100644 --- a/src/lib/server/trino/config.ts +++ b/src/lib/server/trino/config.ts @@ -12,7 +12,8 @@ const schema = z STACKABLE_COCKPIT_TRINO_AUTH_TYPE: envString.pipe(z.enum(['none', 'basic']).default('none')), STACKABLE_COCKPIT_TRINO_AUTH_USERNAME: envString, STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD: envString, - STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS: envBoolean(false), + STACKABLE_COCKPIT_TRINO_TLS_INSECURE: envBoolean(false), + STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE: envBoolean(false), STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION: envBoolean(true), STACKABLE_COCKPIT_QUERY_TTL: envInteger(1800) }) @@ -27,6 +28,26 @@ const schema = z message: 'is "basic" but STACKABLE_COCKPIT_TRINO_AUTH_USERNAME or STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD is missing' } + ) + // Each TLS setting applies to one kind of connection only; reject a setting that would + // silently have no effect. + .refine( + (config) => !config.STACKABLE_COCKPIT_TRINO_TLS_INSECURE || config.STACKABLE_COCKPIT_TRINO_URL, + { + path: ['STACKABLE_COCKPIT_TRINO_TLS_INSECURE'], + message: + 'only applies to the pre-configured connection, but STACKABLE_COCKPIT_TRINO_URL is not set' + } + ) + .refine( + (config) => + !config.STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE || + !config.STACKABLE_COCKPIT_TRINO_URL, + { + path: ['STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE'], + message: + 'only applies to connections users enter, but STACKABLE_COCKPIT_TRINO_URL is set and hides the connection form' + } ); export function parseTrinoConfig(env: Record) { @@ -37,7 +58,9 @@ export function parseTrinoConfig(env: Record) { authType: config.STACKABLE_COCKPIT_TRINO_AUTH_TYPE, authUsername: config.STACKABLE_COCKPIT_TRINO_AUTH_USERNAME, authPassword: config.STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD, - allowInsecureTls: config.STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS, + tlsInsecure: config.STACKABLE_COCKPIT_TRINO_TLS_INSECURE, + userConnectionAllowInsecureTls: + config.STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE, userImpersonation: config.STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION, queryTtlSeconds: config.STACKABLE_COCKPIT_QUERY_TTL }; @@ -46,6 +69,6 @@ export function parseTrinoConfig(env: Record) { /** Trino settings from the environment. The connection fields are only used * when `STACKABLE_COCKPIT_TRINO_URL` is set; otherwise users configure their * own connection. Impersonation also applies to those per-user connections. - * `allowInsecureTls` skips certificate verification for the pre-configured - * connection, or lets users skip it for their own. */ + * `tlsInsecure` skips certificate verification for the pre-configured connection; + * `userConnectionAllowInsecureTls` lets users skip it for their own. */ export const trinoConfig = parseTrinoConfig(env); diff --git a/src/lib/server/trino/user-clients.test.ts b/src/lib/server/trino/user-clients.test.ts index 1ad84ffe..06484320 100644 --- a/src/lib/server/trino/user-clients.test.ts +++ b/src/lib/server/trino/user-clients.test.ts @@ -7,7 +7,7 @@ const { TrinoClientMock, AgentMock, trinoConfigMock, logMock } = vi.hoisted(() = AgentMock: vi.fn(function (this: Record, options: unknown) { this.options = options; }), - trinoConfigMock: { allowInsecureTls: false }, + trinoConfigMock: { userConnectionAllowInsecureTls: false }, logMock: { info: vi.fn(), warn: vi.fn(), debug: vi.fn() } })); @@ -37,7 +37,7 @@ const BASIC = { ...BASE, authType: 'basic', username: 'alice', password: 'secret beforeEach(async () => { vi.clearAllMocks(); vi.resetModules(); - trinoConfigMock.allowInsecureTls = false; + trinoConfigMock.userConnectionAllowInsecureTls = false; uc = await import('./user-clients.js'); }); @@ -78,7 +78,7 @@ describe('buildUserTrinoClient', () => { }); it('skips TLS verification when allowed and requested', () => { - trinoConfigMock.allowInsecureTls = true; + trinoConfigMock.userConnectionAllowInsecureTls = true; uc.buildUserTrinoClient({ ...BASE, tlsInsecure: true }); uc.buildUserTrinoClient({ ...BASIC, tlsInsecure: true }); @@ -121,7 +121,7 @@ describe('per-user connections', () => { }); it('logs a warning when the connection skips TLS verification', () => { - trinoConfigMock.allowInsecureTls = true; + trinoConfigMock.userConnectionAllowInsecureTls = true; uc.createUserTrinoClient('u1', BASE); expect(logMock.warn).not.toHaveBeenCalled(); diff --git a/src/lib/server/trino/user-clients.ts b/src/lib/server/trino/user-clients.ts index 66725cbc..8dc51015 100644 --- a/src/lib/server/trino/user-clients.ts +++ b/src/lib/server/trino/user-clients.ts @@ -10,7 +10,7 @@ export interface UserConnectionConfig { authType: 'none' | 'basic'; username: string; password: string; - /** Skip TLS certificate verification. Only allowed with `STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS`. */ + /** Skip TLS certificate verification. Only allowed with `STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE`. */ tlsInsecure: boolean; } @@ -34,7 +34,7 @@ function dispatcherFor(tlsInsecure: boolean): Agent | undefined { /** Build a per-user Trino client from a connection config without storing it. */ export function buildUserTrinoClient(config: UserConnectionConfig): TrinoClient { - if (config.tlsInsecure && !trinoConfig.allowInsecureTls) { + if (config.tlsInsecure && !trinoConfig.userConnectionAllowInsecureTls) { throw new Error('Skipping TLS certificate verification is not allowed'); } diff --git a/src/routes/(app)/trino/+page.server.ts b/src/routes/(app)/trino/+page.server.ts index f7a92b99..ef78197f 100644 --- a/src/routes/(app)/trino/+page.server.ts +++ b/src/routes/(app)/trino/+page.server.ts @@ -36,8 +36,8 @@ export const load: PageServerLoad = async ({ locals }) => { trinoConfigured, userClientExists, completionEnabled, - // Without a pre-configured URL, STACKABLE_COCKPIT_TRINO_ALLOW_INSECURE_TLS lets users skip verification. - allowInsecureTls: !trinoConfigured && trinoConfig.allowInsecureTls + // STACKABLE_COCKPIT_TRINO_USER_CONNECTION_TLS_ALLOW_INSECURE lets users skip verification. + allowInsecureTls: !trinoConfigured && trinoConfig.userConnectionAllowInsecureTls }; }; @@ -64,7 +64,7 @@ export const actions: Actions = { return fail(400, { form }); } - if (form.data.tlsInsecure && !trinoConfig.allowInsecureTls) { + if (form.data.tlsInsecure && !trinoConfig.userConnectionAllowInsecureTls) { log.info('connection save rejected, skipping TLS verification is not allowed'); return setError(form, 'tlsInsecure', m.trino_connection_tls_insecure_not_allowed()); }