Skip to content

feat(teradata): log on with a JWT, pasted or minted with client credentials - #821

Open
tabossert wants to merge 2 commits into
mainfrom
trevor/product-1071-teradata-sql-jwt-auth
Open

tabossert wants to merge 2 commits into
mainfrom
trevor/product-1071-teradata-sql-jwt-auth

Conversation

@tabossert

@tabossert tabossert commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

What & why

Problem: The Teradata connector (source and destination) authenticates with a database username and password only. A site whose Teradata database trusts JWTs from its identity provider cannot use that identity for the connector. Adding a field to the connector's config here is also what lets the Unstructured platform accept one: its connector-config validation reads these models, and it drops any key they do not declare. Today it drops token and demands password.

Change: The connector takes exactly one of three credential sets, selected by which fields are present:

Method Fields What teradatasql.connect gets
Username and password (unchanged) user, access_config.password host, user, password, dbs_port, database, the same keys in the same order
JWT access_config.token host, logmech="JWT", logdata="token=<JWT>", dbs_port, database, with no user or password
JWT via client credentials token_url, client_id, access_config.client_secret, optional scope as JWT, with a token the connector mints (RFC 6749 4.4, client secret sent with HTTP Basic, https only)
  • Validation: a config with none, two, or a partial set is refused with a message that names fields, never values.
  • Where the fields live: secrets live in access_config next to the password. The non-secret OAuth fields sit on the connection config beside host, so a platform that encrypts every string in access_config does not hide them.
  • Checked before sending: the connector opens a connection per query, so it reads the token at every logon. A malformed token, or one past its exp, is refused before the database is contacted; with 20.0.0.66 the driver does not check logdata for JWT before connecting. A client-credentials token is renewed two minutes before it expires, or halfway through a shorter life. If the identity provider is unreachable, the held token keeps working until it expires. If the provider refuses the client, the run stops.
  • Errors: on the JWT path, driver logon errors are replaced with fixed text and dropped from the chain:
    • JWT Token expired becomes TokenExpiredError (401).
    • Any other error naming JWT, or database error 8017, becomes UserAuthError (401).
    • Anything else keeps the existing connection-error summary.
    • Identity-provider failures are terminal (401) for a refused client, an untrusted certificate, a redirect or a non-JWT access token. They are a retryable 503 ProviderError for a connection failure, any other TLS failure, 408, 429 or 5xx. Only a registered RFC 6749 error code is ever echoed.
  • Audit: each connection config logs one line per outcome, never the credential:
    Teradata authentication: auth.method=<password|jwt|jwt_client_credentials> outcome=<authenticated|ErrorType> job_id=<JOB_ID> dag_node_id=<DAG_NODE_ID>.

Driver facts this relies on (teradatasql README, logmech table, JWT row): "logdata must contain token= followed by the JSON Web Token. The database user must have the "logon with null password" permission." The database takes the user from the token's sub claim by default. user is not sent on the JWT path; teradataml's create_context builds a JWT logon the same way.

Impact

  • Customers: password configs behave exactly as before. A Teradata site that trusts an identity provider can now use a pasted JWT, or client credentials for long and scheduled runs.
  • Wire contract / clients: TeradataAccessConfig.password and TeradataConnectionConfig.user become optional, and the model validator requires them together. New optional fields: access_config.token, access_config.client_secret, token_url, client_id, scope. requests joins the teradata extra.
  • Behaviour change on the password path: both prechecks now raise their connection error from None, so the driver's text no longer rides along as the exception's __context__. The message and type are unchanged.
  • Deployment target considerations: a pasted token makes no call from the connector to an identity provider. Client credentials need a route to token_url.

Risk / rollback

  • The new fields are additive and every one is optional, so an existing password config validates and connects byte-for-byte as before. Pinned by test_password_logon_is_unchanged, which also asserts parameter order.
  • JWT logon errors are classified by text (JWT Token expired, the word JWT, database 8017). The exact wrapper Teradata puts around a server-side JWT error has not been observed live; an unrecognised one falls through to the existing connection-error summary.
  • Revert the PR to back it out; nothing persists.

How it was verified

  • make test-unit (the full target, -n auto): 1991 passed. The Teradata modules are 169 existing, 82 new in test_teradata_auth.py and 45 new in test_teradata_jwt.py.
  • make check (ruff check .): all checks passed. ruff format is clean on the new files. teradata.py was already format-dirty at HEAD: the formatter asks for exactly the same changes on main and on this branch, so I did not reformat it.
  • make check-version was not run locally (it needs GNU sed). CHANGELOG's top heading and __version__.py both read 1.11.21.
  • Red first: with only the new test files and module added, the connector tests against main's teradata.py give 43 failed, 2 passed. The two that pass are the password-path guards, which must pass on main.
  • Platform validation, before and after. I ran the Unstructured platform's legacy connector-config validation (flatten_dict / extract_config / validate_dataclass, copied verbatim) against this model:
    • main: a JWT config loses token and fails with access_config.password: Field is required and user: Field is required.
    • This branch: token / client_secret are kept inside access_config, and token_url / client_id / scope at top level, with no errors.
    • Password configs are unchanged, including a stored row whose password is already an encrypted secret reference.
  • The same change, live in a plugin: the identical logic ran inside a platform deployment's plugin processes, over HTTP, against the real teradatasql 20.0.0.66 driver, a local HTTPS OAuth token endpoint, and a local TCP listener on the Teradata port.
    • JWT logon reached the driver as logmech=JWT, logdata=token=..., with no user or password.
    • Expired and malformed tokens were refused with zero connections.
    • A client-credentials token was renewed mid-run.
    • A leak scan over every response and log found no token or secret.

Proof

Proof waived (environment): no Teradata that trusts a JWT issuer is reachable from here. What is unverified:

  • the real server's error text for an expired, untrusted, or unmapped JWT;
  • a successful JWT logon end to end.

Unblocked by a Vantage configured to trust an identity provider (TDGSS JWT mechanism), with a database user that has "logon with null password" and is mapped from the token.

Not in this PR

  • Encrypted tokens (JWE) are refused. The shape check accepts a compact JWS only.
  • The driver's own OIDC flows (logmech=SECRET / CRED / BEARER) are not exposed. They depend on the database advertising its identity provider, which could not be tested here.

🤖 Generated with Claude Code

Review in cubic

…ntials

The Teradata connector (source and destination) now logs on with exactly one
of three credential sets, selected by which fields are present: username and
password (unchanged), a pasted JWT, or a JWT the connector mints with an
OAuth 2.0 client credentials grant.

JWT logon is the driver's logmech=JWT with logdata=token=<JWT> and no user or
password. The connector opens a connection per query, so the token is read at
every logon: a malformed or expired pasted token is refused before it is sent,
and a client-credentials token is renewed before it expires. JWT logon errors
become fixed-text errors, with a distinct one for an expired token, and each
run logs the method it authenticated with, never the credential.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 8 files

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread unstructured_ingest/processes/connectors/sql/teradata_auth.py
Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md Outdated
Comment thread unstructured_ingest/processes/connectors/sql/teradata.py
- The client-credentials token source refuses an http token URL itself, not
  only through the connection config, so direct construction cannot send the
  client secret in cleartext.
- An auth verdict raised while the destination precheck probes CREATE TABLE
  (a JWT that expired or was refused since the first logon) now fails the
  precheck instead of reading as an inconclusive probe.
- CHANGELOG: `from None` suppresses the driver text in tracebacks rather than
  removing it, and the audit line is once per outcome per connection config.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 5 files (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread test/unit/connectors/sql/test_teradata_jwt.py
@tabossert
tabossert requested a review from a team September 27, 2026 17:52
@paulkarayan

Copy link
Copy Markdown
Contributor

Automated review, forwarded by @paulkarayan. Findings are code-verified where they name a file and line; the strong-review gpt-pro leg and cubic did not run.

NO-GO -- 1 blocker(s), 0 concern(s), 3 note(s), 1 nit(s). (verdict: BLOCK)

JWT and client-credential Teradata configs cannot be pickled, so the default 2-process ingest pipeline crashes; the fix is ~4 lines plus a pickle test.

Blockers (1)

  1. teradata_auth.py:180 RefreshingToken holds a threading.Lock in the config's PrivateAttr, so pickling a JWT or client-credentials TeradataConnectionConfig raises TypeError; PipelineStep.process_multiprocess (default num_processes=2) crashes before any document runs (reproduced on DownloadStep; codex and fable independently confirm)

Notes (3)

  1. PR body says from None removes the driver error from context; it only suppresses display (CHANGELOG states it correctly)
  2. CHANGELOG's 'auth verdict no longer taken for an inconclusive probe' covers only the CREATE TABLE probe; a token expiring before the write probe is logged as skipped, and the job still fails correctly at upload
  3. Live JWT logon and the real server's JWT error texts are unverified (proof waived in the PR body); unrecognised texts fall back to the connection-error summary

Nits (1)

  1. On the JWT path a non-JWT driver failure surfaces as base ConnectionError instead of Source/DestinationConnectionError (same 400 and same text)
Full review
# unstructured-ingest#821 -- feat(teradata): log on with a JWT, pasted or minted with client credentials

Requested by trevor, DM: https://unstructuredai.slack.com/archives/D0B1BKPQ7CK/p1790531389494899
PR: https://github.com/Unstructured-IO/unstructured-ingest/pull/821 (author tabossert, head `caf999028ae4`, base `main`, +1726/-18 over 8 files)

**Verdict:** BLOCK
**Blast radius:** 4/5

The PR adds two JWT logon methods to the Teradata source and destination connectors: a pasted token, or a token minted from OAuth client credentials. The password path is unchanged. The auth logic is careful and well tested. One defect blocks it: a JWT-configured connection config cannot be pickled, so the OSS ingest pipeline crashes at its default `num_processes=2` as soon as a step fans out. Reproduced here, and codex and fable each found it independently. The fix is about four lines (drop the lock on pickle, recreate it on unpickle), plus a pickle round-trip test. Once that lands, trusting the change still depends on the live JWT logon against a real Vantage, which the PR body waives.

## Notes

- The PR body says the prechecks now raise `from None`, "so the driver's text no longer rides along as the exception's `__context__`". That is inaccurate: `from None` sets `__suppress_context__`, and `__context__` still holds the driver exception. The CHANGELOG states it correctly ("it stays reachable as `__context__`"). Default traceback formatting and `logger.error(exc_info=...)` honour the suppression, so nothing leaks through those paths. Code that walks `__context__` itself would still reach the driver text.
- The CHANGELOG says "An auth verdict raised while the destination precheck probes CREATE TABLE is no longer taken for an inconclusive probe." That holds for the CREATE TABLE probe only. A pasted token that expires between that probe and the write probe is swallowed by the base `SQLUploader.check_write_permissions` (`sql.py`, generic `except Exception`, "write-permission check skipped"). The job then fails at upload with the correct `TokenExpiredError`, so no answer is wrong, but the claim covers less than it reads. (From fable; the catch sites were checked in the source.)
- The live-environment proof is waived in the PR body: no JWT-trusting Teradata was reachable. The real server's error text for an expired, untrusted or unmapped JWT is unobserved, and the text-based classification in `_jwt_logon_error` (`teradata.py`, `JWT token expired`, `\bJWT\b`, error 8017) rests on that gap. An unrecognised text falls through to the connection-error summary, so a miss degrades the message rather than producing a wrong verdict.

## Findings

**1. BLOCKER: a JWT or client-credentials `TeradataConnectionConfig` cannot be pickled, so the multiprocess pipeline step crashes.**
`unstructured_ingest/processes/connectors/sql/teradata_auth.py:180` (`self._lock = threading.Lock()` in `RefreshingToken.__init__`). The token is attached as `PrivateAttr` `_token` in `TeradataConnectionConfig._one_credential_set` (`teradata.py`, around lines 452 and 458).
https://github.com/Unstructured-IO/unstructured-ingest/blob/caf999028ae4b3dd59540996f388952146201ae6/unstructured_ingest/processes/connectors/sql/teradata_auth.py#L180

- Failing path: `PipelineStep.process_multiprocess` (`unstructured_ingest/pipeline/interfaces.py:85-104`) runs `pool.map(self._wrap_mp, iterable)`. That pickles the bound method, so it pickles the step, the connector process, and `connection_config`. Pydantic includes `__pydantic_private__` in pickled state. `ProcessorConfig.num_processes` defaults to 2, so `mp_supported` is True by default, and any step with two or more items takes this path.
- Observed here, at the PR head in an isolated `uv sync --frozen --extra teradata` env:
  - `pickle.dumps(cfg)` succeeds for a password config and raises `TypeError: cannot pickle '_thread.lock' object` for a pasted-JWT config and for a client-credentials config.
  - `DownloadStep(process=TeradataDownloader(<jwt cfg>), context=ProcessorConfig(num_processes=2)).process_multiprocess([2 items])` crashes in the parent with the same `TypeError`, before any item runs. The identical call with a password config returns normally.
  - Fable reports the same failure on `UploadStep` and from `copy.deepcopy(cfg)`.
- Who walks it: anyone running `unstructured-ingest` (CLI or `Pipeline`) with Teradata and either new credential type, at default settings, with more than one document. The feature fails for those users on first use. The platform plugin path may run in-process and not hit this; that was not verified (see Not covered). The 298 Teradata unit tests pass because none of them crosses a process or copy boundary.
- Fix, checked here in memory: give `RefreshingToken` a `__getstate__` that drops `_lock` and a `__setstate__` that recreates it. After that, `pickle.loads(pickle.dumps(cfg))._token.value` returns the original token. Pin it with a test that parametrizes the pasted-token and client-credentials configs, round-trips each through `pickle` and `copy.deepcopy`, and asserts the `logdata` it logs on with. Each child process then holds its own token copy, so client credentials mint once per worker. That is expected, not a defect.

## Dismissed

- cheshire, Slop, `_is_certificate_failure` walks a general exception graph. SKIP: requests and urllib3 nest the ssl error differently across versions (urllib3 1.x vs 2.x wrap `reason` differently). The bounded walk with a seen-set is the version-robust choice, and nothing fails.
- cheshire, Docs, no connector docs or example config updated. SKIP: this repo has no Teradata docs page or example config (`git ls-files | grep -i teradata` shows only source, tests and the schema SQL). The new fields carry pydantic `description`s, which are the docs surface here.
- cheshire, Summary, XL diff, "consider whether this is one logical change". SKIP: 1118 of the 1726 added lines are the two new test files. The source change is one feature.
- fable, NOTE 3: on the JWT path a non-JWT driver failure surfaces as base `ConnectionError` instead of `SourceConnectionError`/`DestinationConnectionError`. Dropped as a finding: same 400 status and same message text, and no consumer in this repo keys on the subclass. Kept only as a nit.
- ruff format reports `teradata.py` would be reformatted. SKIP: `origin/main`'s `teradata.py` fails `ruff format --check` identically (checked), CI lint is green, and the PR body discloses it.

## What each leg said

- **blast-radius:** 4/5. Wire-contract change to a config the platform validator parses field by field, new security-sensitive credential handling, live verification waived, clean revert. `needs_live_test: true`.
- **cheshire:** 3 flags (Summary XL, one Slop, one Docs), all LLM judgment and all dismissed above. The first attempt returned `INCOMPLETE: claude review slot unavailable after 600s`. The one retry completed, running without AGENTS.md and with the local model down, so it fell back to `claude -p`. Briefing: /Users/pk/.cheshire/briefings/Unstructured-IO-unstructured-ingest-821.md
- **fable (strong-review, report-only, no PR marker):** `NOT SAFE TO MERGE`. Finding 1 is the pickle/deepcopy defect, reproduced on `UploadStep` with the same suggested fix. The two NOTEs are the write-probe swallow (see Notes) and the `ConnectionError` subclass drift (dismissed as a nit). It ran the 298 Teradata unit tests: all passed. Report: /Users/pk/.strong-review/ui-821/ui-821-fable.md
- **codex (`codex exec review --base origin/main`):** one P1, the same pickle defect via the default two-process download path. Its test run could not resolve locked dependencies inside its network-restricted sandbox. Output: /Users/pk/.strong-review/ui-821/codex.md
- **ruff:** `ruff check` passes on the four changed Python files and repo-wide. `ruff format --check`: the three new files are clean; `teradata.py` is format-dirty, and it is equally dirty on `main`.
- **Local tests (this review):** `test_teradata.py`, `test_teradata_auth.py` and `test_teradata_jwt.py` give 298 passed.
- **CI (`gh pr checks`, read 2026-09-28 ~02:00 +03):** all run checks pass, including unit tests on 3.11 to 3.13, lint, check-version, changelog and CodeQL. The integration and e2e jobs report `skipping`.
- **gpt-pro / oracle leg: NOT RUN.** It needs a browser on a shared CDP port, and this run is headless and unattended. This is a stated limit of the run.
- **cubic: NOT RUN.** Disabled for this run. The CI-side cubic check shows pass.

## Not covered

- The oracle (gpt-pro) second opinion and cubic did not run.
- Whether the platform's Teradata plugin pickles or deep-copies the connection config (process pool, config cloning) was not checked. If it does, finding 1 also breaks the platform path.
- There was no live Teradata. JWT logon end to end, the server's real JWT error texts, and the 8017 mapping are unverified; the author waives these too.
- No live identity provider. The client-credentials mint, refresh timing and 5xx/429 retry classification are covered only by the PR's unit tests.
- Test-critic (test quality) and the marmalade reuse pass were not run. They are outside the legs this unattended run was scoped to.
- The cheshire check-quality log, the finding-verdict sink and the review-run manifest were not written: this run may write only this packet. The legs themselves wrote their own outputs under /Users/pk/.strong-review/ui-821/ and /Users/pk/.cheshire/briefings/, and the review used a temporary detached worktree and venv under /tmp, removed afterwards.

This branch was successfully deployed

1 active deployment
ci — caf99902 Deployed Sep 26, 2026 by tabossert via test_ingest_help (3.12) #4253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants