Skip to content

Conversation

@ktpedre
Copy link
Collaborator

@ktpedre ktpedre commented Nov 20, 2025

No description provided.

import requests
import base64
import json
import time

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'time' is not used.
@garland3 garland3 requested a review from Copilot November 20, 2025 23:22
Copilot finished reviewing on behalf of garland3 November 20, 2025 23:25
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for AWS Application Load Balancer (ALB) as an authentication reverse proxy by implementing JWT validation for the x-amzn-oidc-data header. This expands the authentication options beyond simple header-based auth to support OIDC-compliant JWT tokens issued by AWS ALB, enabling secure authentication in AWS-hosted deployments.

Key changes:

  • Added new authentication flow for AWS ALB JWT tokens with signature validation against AWS public keys
  • Introduced configurable auth header types (email-string vs aws-alb-jwt) via AUTH_USER_HEADER_TYPE
  • Extended configuration to support AWS-specific settings (expected ALB ARN and region)

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 17 comments.

Show a summary per file
File Description
docs/02_admin_guide.md Documents AWS ALB authentication configuration with required environment variables
backend/modules/config/config_manager.py Adds configuration fields for auth header type, AWS ALB ARN, and AWS region
backend/main.py Passes new AWS configuration parameters to AuthMiddleware initialization
backend/core/middleware.py Implements conditional authentication logic based on header type, routing to AWS ALB JWT validation when configured
backend/core/auth.py Implements get_user_from_aws_alb_jwt() function to validate AWS ALB JWTs by fetching public keys and verifying signatures
Comments suppressed due to low confidence (1)

backend/core/auth.py:13

  • Import of 'time' is not used.
import time

import requests
import base64
import json
import time
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Unused import. The time module is imported but never used in the function. Remove the import to clean up the code.

Suggested change
import time

Copilot uses AI. Check for mistakes.

# Step 3: Get the public key from the regional endpoint
url = f'https://public-keys.auth.elb.{aws_region}.amazonaws.com/{kid}'
req = requests.get(url)
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Missing security: No timeout specified for external HTTP request. The requests.get() call should include a timeout to prevent the authentication from hanging indefinitely if AWS's public key endpoint is slow or unresponsive. Add a timeout parameter, e.g., requests.get(url, timeout=5.0).

Suggested change
req = requests.get(url)
req = requests.get(url, timeout=5.0)

Copilot uses AI. Check for mistakes.

# Step 2: Validate the signer matches the expected ALB ARN
if received_alb_arn != expected_alb_arn:
print(f"Error: Invalid signer ARN. Expected {expected_alb_arn}, got {received_alb_arn}")
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Use logger.error() instead of print() for error messages. The codebase uses structured logging throughout (see lines 48, 51 in the existing code), and error messages should be logged using the logger for consistency and proper log aggregation.

Copilot uses AI. Check for mistakes.
print(f"Error: Invalid token - {e}")
return None
except requests.exceptions.RequestException as e:
print(f"Error fetching public key: {e}")
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Use logger.error() instead of print() for error messages. This maintains consistency with the error logging patterns used throughout the codebase.

Copilot uses AI. Check for mistakes.
print(f"Error fetching public key: {e}")
return None
except Exception as e:
print(f"An unexpected error occurred: {e}")
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Use logger.error() instead of print() for error messages. All error messages should be logged consistently using the logger.

Copilot uses AI. Check for mistakes.

# Step 3: Get the public key from the regional endpoint
url = f'https://public-keys.auth.elb.{aws_region}.amazonaws.com/{kid}'
req = requests.get(url)
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Missing error handling for HTTP request failures. The requests.get() call at line 100 should check the response status code before using the text. If the public key fetch fails (e.g., 404, 500), pub_key will contain an error message instead of a valid key, causing JWT validation to fail silently. Add: req.raise_for_status() after line 100 to ensure proper error handling.

Suggested change
req = requests.get(url)
req = requests.get(url)
req.raise_for_status()

Copilot uses AI. Check for mistakes.
Comment on lines +95 to 101
user_email = get_user_from_header(x_auth_header)
else:
# Get test user from config
config_manager = app_factory.get_config_manager()
user_email = config_manager.app_settings.test_user
# logger.info(f"Debug mode: using user {user_email}")
else:
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Inconsistent authentication logic in debug mode. In debug mode (lines 93-95), the code uses get_user_from_header() regardless of the auth_header_type setting. This means AWS ALB JWT authentication cannot be tested in debug mode. Consider applying the same authentication type logic in debug mode to allow proper testing, or document that AWS ALB JWT authentication only works in production mode.

Suggested change
user_email = get_user_from_header(x_auth_header)
else:
# Get test user from config
config_manager = app_factory.get_config_manager()
user_email = config_manager.app_settings.test_user
# logger.info(f"Debug mode: using user {user_email}")
else:
if self.auth_header_type == "aws-alb-jwt":
user_email = get_user_from_aws_alb_jwt(x_auth_header, self.auth_aws_expected_alb_arn, self.auth_aws_region)
else:
user_email = get_user_from_header(x_auth_header)
else:
# Get test user from config
config_manager = app_factory.get_config_manager()
user_email = config_manager.app_settings.test_user
# logger.info(f"Debug mode: using user {user_email}")

Copilot uses AI. Check for mistakes.
if email_address:
return email_address
else:
print("Error: 'email' claim not found in JWT payload.")
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Use logger.error() instead of print() for error messages. This error logging should be consistent with the existing patterns in this module.

Copilot uses AI. Check for mistakes.
Comment on lines +225 to +229
default="arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/app/your-alb-name/...",
description="The expected AWS ALB ARN",
validation_alias="AUTH_AWS_EXPECTED_ALB_ARN"
)

Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The default ARN value appears to be a placeholder and may cause confusion in production. Consider using an empty string as the default or adding validation that warns/errors if the default placeholder value is still in use when auth_header_type is set to aws-alb-jwt. This prevents accidental misconfigurations in production.

Suggested change
default="arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/app/your-alb-name/...",
description="The expected AWS ALB ARN",
validation_alias="AUTH_AWS_EXPECTED_ALB_ARN"
)
default="",
description="The expected AWS ALB ARN",
validation_alias="AUTH_AWS_EXPECTED_ALB_ARN"
)
def model_post_init(self, __context):
# Validate that if auth_user_header_type is aws-alb-jwt, ARN must be set and not placeholder
if self.auth_user_header_type == "aws-alb-jwt":
placeholder = "arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/app/your-alb-name/..."
if not self.auth_aws_expected_alb_arn or self.auth_aws_expected_alb_arn == placeholder:
raise ValueError(
"auth_aws_expected_alb_arn must be set to a valid AWS ALB ARN when auth_user_header_type is 'aws-alb-jwt'. "
"Current value is empty or a placeholder."
)

Copilot uses AI. Check for mistakes.
debug_mode: bool = False,
auth_header_name: str = "X-User-Email",
auth_header_type: str = "email-string",
auth_aws_expected_alb_arn: str = "arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/app/your-alb-name/...",
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The default ARN value in middleware matches the config default, which appears to be a placeholder. Consider using an empty string or None as default in the middleware signature to make it clearer that this value should come from configuration, not hardcoded defaults.

Copilot uses AI. Check for mistakes.
@garland3 garland3 merged commit 0e9e7e3 into sandialabs:main Nov 21, 2025
13 of 15 checks passed
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