feat: make tinyauth an oidc provider #550
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds OpenID Connect (OIDC) provider functionality to tinyauth, allowing it to act as an OIDC identity provider for other applications. Features: - OIDC discovery endpoint at /.well-known/openid-configuration - Authorization endpoint for OAuth 2.0 authorization code flow - Token endpoint for exchanging authorization codes for tokens - ID token generation with JWT signing - JWKS endpoint for public key distribution - Support for PKCE (code challenge/verifier) - Nonce validation for ID tokens - Configurable OIDC clients with redirect URIs, scopes, and grant types Validation: - Docker Compose setup for local testing - OIDC test client (oidc-whoami) with session management - Nginx reverse proxy configuration - DNS server (dnsmasq) for custom domain resolution - Chrome launch script for easy testing Configuration: - OIDC configuration in config.yaml - Example configuration in config.example.yaml - Database migrations for OIDC client storage
…idation The validateAccessToken method was only decoding the JWT payload without verifying the signature, allowing attackers to forge tokens. This fix: - Adds ValidateAccessToken method to OIDCService that properly verifies JWT signature using RSA public key - Validates issuer, expiration, and required claims - Updates controller to use the secure validation method - Removes insecure manual JWT parsing code
PKCE was advertised in the discovery document but not actually implemented. This commit adds full PKCE support: - Store code_challenge and code_challenge_method in authorization code JWT - Accept code_verifier parameter in token endpoint - Validate code_verifier against stored code_challenge - Support both S256 (SHA256) and plain code challenge methods - PKCE validation is required when code_challenge is present This prevents authorization code interception attacks by requiring the client to prove possession of the code_verifier that was used to generate the code_challenge.
Per OAuth 2.0 RFC 6749 §4.1.2.1, errors should NOT redirect to unvalidated redirect_uri values. This fix: - Returns JSON errors for failures before redirect_uri validation (missing parameters, invalid client) - Only redirects to redirect_uri after it has been validated against registered client URIs - Prevents open redirect attacks where malicious redirect_uri values could be used to redirect users to attacker-controlled sites
The discovery document only advertises client_secret_basic and client_secret_post as supported authentication methods. Query parameters are insecure because they are: - Logged in access logs - Stored in browser history - Exposed in referrer headers This fix removes the query parameter fallback, ensuring client secrets are only accepted via: - Authorization header (client_secret_basic) - POST form body (client_secret_post) This aligns the implementation with the advertised capabilities and prevents client secret exposure through query strings.
User claims from ID tokens (username, name, email) were directly interpolated into HTML without escaping, allowing XSS attacks if malicious content was present in claims. This fix: - Imports html module for escaping - Escapes all user-controlled data before rendering in HTML - Escapes JSON output in pre tags as well - Prevents execution of malicious scripts in browser
The variable 'html' was being assigned to store HTML content, which caused Python to treat 'html' as a local variable throughout the function. This prevented access to the 'html' module (imported at the top) within f-strings that referenced html.escape(). Renamed the HTML content variable to 'html_content' to avoid the naming conflict with the html module.
Authorization codes were implemented as stateless JWTs with no tracking, allowing the same code to be exchanged for tokens multiple times. This violates OAuth 2.0 RFC 6749 Section 4.1.2 which mandates that authorization codes MUST be single-use. This change: - Adds oidc_authorization_codes table to track code usage - Stores authorization codes in database when generated - Validates code exists and hasn't been used before exchange - Marks code as used immediately after validation - Prevents replay attacks where intercepted codes could be reused Security impact: - Prevents attackers from reusing intercepted authorization codes - Ensures compliance with OAuth 2.0 security requirements - Adds database-backed single-use enforcement
Security improvements: 1. Client secret hashing: - Replace plaintext comparison with bcrypt.CompareHashAndPassword - Provides constant-time comparison to prevent timing attacks - Hash secrets with bcrypt before storing in database - Update SyncClientsFromConfig to hash incoming plaintext secrets 2. Deterministic RSA key loading: - Load most recently created key using ORDER BY created_at DESC - Add warning if multiple keys detected in database - Ensures consistent key selection on startup 3. Optional RSA key encryption: - Encrypt private keys with AES-256-GCM when OIDC_RSA_MASTER_KEY is set - Master key derived via SHA256 from environment variable - Backward compatible: stores plaintext if no master key set - Automatic detection of encrypted vs plaintext on load All changes maintain backward compatibility with existing deployments.
Security improvements:
1. HKDF key derivation:
- Replace raw sha256.Sum256() with proper HKDF (HMAC-based KDF)
- Uses domain-separated label 'oidc-aes-256-key-v1' for key derivation
- Applied to both encryptPrivateKey and decryptPrivateKey
- Provides better security properties than raw hash
2. Scope validation fix:
- Only add 'openid' scope if it's both requested AND in client's
allowedScopes
- Prevents bypassing client scope restrictions
- Respects configured allowedScopes
Both changes improve security posture while maintaining backward
compatibility.
The special case for adding 'openid' scope was redundant and could potentially bypass client scope restrictions. The main loop already correctly adds 'openid' to validScopes if it's in both requestedScopes and allowedScopes. Since 'openid' is already in the default scopes during client configuration (SyncClientsFromConfig), it will be available for clients that don't explicitly configure scopes. Clients can include or exclude 'openid' in their allowedScopes as needed. This ensures consistent enforcement of client scope restrictions with no special-case bypasses.
Access tokens include an 'aud' (audience) claim set to the client ID, but this was never validated during token validation. This allowed tokens issued for one client to be used by another client, violating the OAuth 2.0 security model. Changes: - Add ValidateAccessTokenForClient method that validates audience if expectedClientID is provided - Update ValidateAccessToken to call ValidateAccessTokenForClient (backward compatible, no audience check if not specified) - Update userinfo endpoint to accept optional client_id parameter and validate token audience matches it Security impact: - Prevents token reuse across different clients - Ensures tokens are scoped to specific clients as intended - Prevents attackers from using tokens issued for one client to access resources protected by another client
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@shreknel after spending some time reviewing this pull request, I will have to close it. This is because it involves a very sensitive aspect of a soon to become feature of Tinyauth. I found multiple issues that do not follow the project's quality standards. It seems that you have created this pull request largely using the assistance of an LLM and thus I cannot trust the output of an algorithm enough to merge into the main branch especially with something this complex. However, this doesn't mean that the feature will not make it into Tinyauth. This pull request gave me some ideas of how to implement OIDC which I will be doing very soon. I believe it's better to start from scratch rather than try to review hundreds of lines of code and fix countless issues. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.