Skip to content

Add OIDC credential providers - #903

Open
vgvoleg wants to merge 7 commits into
mainfrom
feature/oidc-credentials
Open

vgvoleg wants to merge 7 commits into
mainfrom
feature/oidc-credentials

Conversation

@vgvoleg

@vgvoleg vgvoleg commented Sep 21, 2026

Copy link
Copy Markdown
Member

Related to #876.

Adds credential providers for three OAuth 2.0 authentication modes used with an external OIDC provider:

  • an access token supplied by the application and sent using the Bearer scheme;
  • Client Credentials Grant;
  • Device Authorization Grant with polling and refresh-token support.

Client Credentials and Device Authorization obtain their endpoints from the issuer OIDC configuration. Both synchronous and asynchronous implementations are included.

The change also adds documentation, a usage example, and a self-contained Docker Compose environment with YDB and Keycloak for exercising all three modes.

@codecov

codecov Bot commented Sep 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.18%. Comparing base (33b601a) to head (13d0bcd).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #903      +/-   ##
==========================================
+ Coverage   82.66%   83.18%   +0.52%     
==========================================
  Files          99      103       +4     
  Lines       12951    13355     +404     
  Branches     1268     1341      +73     
==========================================
+ Hits        10706    11110     +404     
+ Misses       1794     1793       -1     
- Partials      451      452       +1     
Flag Coverage Δ
integration 81.04% <100.00%> (+0.59%) ⬆️
unit 48.10% <17.43%> (-0.96%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
ydb/__init__.py 94.59% <100.00%> (ø)
ydb/aio/credentials.py 89.47% <100.00%> (-0.19%) ⬇️
ydb/aio/oidc.py 100.00% <100.00%> (ø)
ydb/credentials.py 83.94% <100.00%> (+0.11%) ⬆️
ydb/oidc/__init__.py 100.00% <100.00%> (ø)
ydb/oidc/_common.py 100.00% <100.00%> (ø)
ydb/oidc/credentials.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

OAuth scope interoperability, device-flow deadline handling, and the local YDB certificate trust configuration need correction.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 5 Medium severity

Open (6)
What changed in this PR

Adds synchronous and asynchronous OIDC credential providers, including token acquisition, refresh, documentation, tests, and a local Keycloak environment.

Changes:

  • Adds static-token, Client Credentials, and Device Authorization providers.
  • Adds unit and end-to-end smoke-test coverage.
  • Adds user documentation, an example, and release notes.
File Description
ydb/​oidc/​credentials.py Implements synchronous providers.
ydb/​oidc/​_common.py Provides shared OAuth processing and validation.
ydb/​oidc/​__init__.py Exports the public OIDC API.
ydb/​aio/​oidc.py Implements asynchronous providers.
ydb/​__init__.py Adds lazy OIDC module loading.
tests/​oidc/​ydb.yaml Configures YDB external IdP authentication.
tests/​oidc/​static_token_smoke.py Exercises static-token authentication.
tests/​oidc/​README.md Documents the local test environment.
tests/​oidc/​prepare.sh Generates certificates, secrets, and realm configuration.
tests/​oidc/​device_authorization_smoke.py Exercises device authorization and refresh.
tests/​oidc/​compose.yaml Defines the YDB and Keycloak environment.
tests/​oidc/​client_credentials_smoke.py Exercises client credentials and refresh.
tests/​oidc/​.gitignore Excludes generated test state.
tests/​oidc/​_smoke_common.py Provides shared smoke-test utilities.
tests/​auth/​test_credentials.py Adds synchronous unit coverage.
tests/​aio/​test_credentials.py Adds asynchronous unit coverage.
examples/​oidc-credentials/​main.py Demonstrates the credential modes.
docs/​driver.rst Documents the public API.
CHANGELOG.md Records the new feature.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/oidc/compose.yaml
Comment thread ydb/aio/oidc.py Outdated
Comment thread ydb/oidc/_common.py
Comment thread ydb/oidc/_common.py Outdated
Comment thread ydb/oidc/_common.py
Comment thread ydb/oidc/credentials.py

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Token lifetime accounting, refresh scopes, and async redirect handling contain correctness and security issues.

Review effort: Balanced
Findings: None

Resolved since last review (6)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Handle case-insensitive HTTPS when applying CA context

ydb/​aio/​oidc.py:35

HTTPS schemes are case-insensitive, and _is_https_url accepts values such as HTTPS://issuer.example, but this case-sensitive check then omits the configured CA context. Such valid issuer or discovered endpoint URLs fail against private/self-signed CAs in the async provider while the sync provider works.

This issue also appears in the following locations of the same file:

  • line 38
  • line 181
Medium severity Avoid fixed safety margin invalidating short-lived tokens

ydb/​oidc/​_common.py:146

Any positive expires_in is accepted here, but the inherited cache subtracts a fixed 30-second safety window (ydb/credentials.py:102-105). Tokens with a standards-valid lifetime of 30 seconds or less are therefore never considered valid, causing every RPC to request another token and eventually hit IdP throttling. Clamp the safety margin relative to the reported lifetime (with corresponding sync/async tests).

This issue also appears on line 174 of the same file.

Medium severity Cache token timestamp after synchronous token request

ydb/​oidc/​credentials.py:200

AbstractExpiringTokenCredentials._refresh_token records current_time before calling this method and later uses that old value to cache the returned token (ydb/credentials.py:108-114). A device flow can spend minutes waiting for the user, so a freshly issued token may be considered expired immediately (and, if no refresh token was returned, the next RPC starts another user interaction). Timestamp the cache after the token request completes; apply the equivalent correction to the async base as well.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Redirect handling can expose OAuth secrets, and device refresh can initiate duplicate concurrent authorization flows.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity

Open (2)

Comment thread ydb/aio/oidc.py
Comment thread ydb/oidc/credentials.py Outdated
@vgvoleg
vgvoleg requested a balanced review from Copilot September 21, 2026 14:58

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Unresolved findings affect authentication behavior, smoke-test setup, validation, and connection reuse.

Review effort: Lite
Findings: None

@vgvoleg
vgvoleg marked this pull request as ready for review September 21, 2026 15:35
@vgvoleg
vgvoleg requested a lite review from Copilot September 21, 2026 15:35

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Three moderate issues remain in async session reuse, refresh-audience preservation, and issuer validation.

Review effort: Lite
Findings: None

This branch has not been deployed

No deployments
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