-
Notifications
You must be signed in to change notification settings - Fork 17
feat: Add AWS Cognito as OIDC Identity Provider (#325) #331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e31ad2c
7402e3a
cfeedf2
b2cd1bb
7a5bf08
c36975c
047f230
bbcc1c8
208f75d
0142a62
3cffe0a
30108bb
0da736a
eaf2255
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ | |
| AES_256_KEY_SIZE_BYTES = 32 | ||
|
|
||
| _credential_id_ctx: ContextVar[str | None] = ContextVar("credential_id", default=None) | ||
| _http_auth_header_ctx: ContextVar[str | None] = ContextVar("http_auth_header", default=None) | ||
|
|
||
|
|
||
| @contextmanager | ||
|
|
@@ -52,6 +53,18 @@ def credential_context(credential_id: str | None) -> Generator[None]: | |
| _credential_id_ctx.reset(token) | ||
|
|
||
|
|
||
| @contextmanager | ||
| def http_auth_header_context(auth_header: str | None) -> Generator[None]: | ||
| """Make a pre-resolved Authorization header available to | ||
| fetch_and_decrypt_credential via ContextVar. When set, the header is | ||
| used instead of the SA token / JWT fallback.""" | ||
| token = _http_auth_header_ctx.set(auth_header) | ||
| try: | ||
| yield | ||
| finally: | ||
| _http_auth_header_ctx.reset(token) | ||
|
|
||
|
|
||
| def _resolve_jwt_token(jwt_token: str | None) -> str: | ||
| """ | ||
| Resolve JWT token for authenticating with the credential backend. | ||
|
|
@@ -181,9 +194,13 @@ def fetch_and_decrypt_credential( | |
| RuntimeError | ||
| Unexpected HTTP status or network error. | ||
| """ | ||
| resolved_token = _resolve_jwt_token(jwt_token) | ||
| auth_header = _http_auth_header_ctx.get() | ||
| if auth_header is None: | ||
| resolved_token = _resolve_jwt_token(jwt_token) | ||
| auth_header = f"Bearer {resolved_token}" | ||
|
|
||
| url = f"{backend_url.rstrip('/')}/api/v1/credentials/{credential_id}" | ||
| headers = {"Authorization": f"Bearer {resolved_token}"} | ||
| headers = {"Authorization": auth_header} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This override means the credential backend is now authenticated with whatever
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding the comment about reusing the This pattern was introduced in commit |
||
|
|
||
| logger.info("Fetching credential: credential_id=%s", credential_id) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,8 @@ | |
| import os | ||
| import re | ||
|
|
||
| HTTP_OUTPUT_AGENT_CONFIG = "cve_http_output" | ||
|
|
||
| if TYPE_CHECKING: | ||
| from vuln_analysis.data_models.output import ExploitIqOutput, FailureReport | ||
|
|
||
|
|
@@ -89,7 +91,7 @@ class CVEHttpOutputConfig(FunctionBaseConfig, name="cve_http_output"): | |
| """ | ||
| url: str = Field(description="URL to send CVE workflow output") | ||
| endpoint: str = Field(description="Endpoint to send CVE workflow output") | ||
| auth_type: str = Field(default="disabled", description="Type of auth - bearer, basic, keycloak or disabled") | ||
| auth_type: str = Field(default="disabled", description="Type of auth - bearer, basic, keycloak, cognito or disabled") | ||
| token: str | None = Field(default=None, description="Token to authenticate when sending CVE workflow output") | ||
| token_path: str | None = Field(default=None, description="Path to token file containing auth token") | ||
| verify_path: str | None = Field(default=None, description="Path to certificate to validate the token key found in ") | ||
|
|
@@ -98,6 +100,10 @@ class CVEHttpOutputConfig(FunctionBaseConfig, name="cve_http_output"): | |
| keycloak_server: str | None = Field(default=None, description="Keycloak server URL (e.g. https://keycloak.example.com)") | ||
| keycloak_realm: str | None = Field(default=None, description="Keycloak realm name") | ||
| verify_path_keycloak: str | None = Field(default=None, description="Path to ca to validate the certificate of keycloak instance ") | ||
| cognito_domain: str | None = Field(default=None, description="Cognito domain (e.g. https://myapp.auth.us-east-1.amazoncognito.com or myapp.auth.us-east-1.amazoncognito.com)") | ||
| cognito_scope: str | None = Field(default=None, description="Cognito custom scope (e.g. api/read)") | ||
| cognito_client_id: str | None = Field(default=None, description="OAuth2 client ID for Cognito M2M authentication") | ||
| cognito_client_secret: str | None = Field(default=None, description="OAuth2 client secret for Cognito M2M authentication") | ||
| client_id: str | None = Field(default=None, description="OAuth2 client ID for keycloak authentication") | ||
| client_secret: str | None = Field(default=None, description="OAuth2 client secret for keycloak authentication") | ||
| failure_endpoint: str = Field(default="/api/v1/reports/failed", | ||
|
|
@@ -220,6 +226,7 @@ async def _arun(message: ExploitIqOutput) -> ExploitIqOutput: | |
| logger.error('Unable to send job to MLOps API at %s. Error: %s', mlops_url, mlops_e) | ||
| except Exception as e: | ||
| logger.error('Unable to send output response to %s. Error: %s', payload.url, e) | ||
| raise | ||
| else: | ||
| logger.info('Successfully sent output to %s', payload.url) | ||
|
|
||
|
|
@@ -277,23 +284,87 @@ def _fetch_keycloak_token(http_config: CVEHttpOutputConfig) -> str | None: | |
| return None | ||
|
|
||
|
|
||
| def _fetch_cognito_token(http_config: CVEHttpOutputConfig) -> str | None: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Important — redundant token fetch per pipeline node + mid-operation expiry risk. With Also: the header is resolved once at the start of a minutes-long Consider: caching the token respecting |
||
| domain = http_config.cognito_domain.rstrip("/") | ||
| if not domain.startswith("https://"): | ||
| domain = f"https://{domain}" | ||
| token_url = f"{domain}/oauth2/token" | ||
|
|
||
| # Cognito requires Basic auth header for client_credentials | ||
| credentials = base64.b64encode( | ||
| f"{http_config.cognito_client_id}:{http_config.cognito_client_secret}".encode() | ||
| ).decode() | ||
| headers = { | ||
| "Content-Type": "application/x-www-form-urlencoded", | ||
| "Authorization": f"Basic {credentials}", | ||
| } | ||
| data = {"grant_type": "client_credentials"} | ||
| if http_config.cognito_scope: | ||
| data["scope"] = http_config.cognito_scope | ||
| try: | ||
| resp = requests.post(token_url, headers=headers, data=data, timeout=30) | ||
| resp.raise_for_status() | ||
| return resp.json()["access_token"] | ||
| except requests.RequestException as e: | ||
| logger.error("Unable to obtain Cognito access token from %s: %s", token_url, e) | ||
| return None | ||
| except (KeyError, ValueError) as e: | ||
| logger.error("Invalid Cognito token response from %s: %s", token_url, e) | ||
| return None | ||
|
|
||
|
|
||
| def resolve_http_auth_header(builder) -> str | None: | ||
| """ | ||
| Resolve HTTP authentication header from cve_http_output config. | ||
|
|
||
| Returns None if config is missing (falls back to SA token) or if auth is disabled. | ||
| Logs warnings when configured auth fails to produce a token. | ||
| """ | ||
| try: | ||
| http_output_config = builder.get_function_config(HTTP_OUTPUT_AGENT_CONFIG) | ||
| except KeyError as e: | ||
| logger.info("HTTP output config not registered, using SA token authentication: %s", e) | ||
| return None | ||
|
|
||
| try: | ||
| return get_auth_header(http_output_config) | ||
| except AuthHeaderError as e: | ||
| logger.warning( | ||
| "Auth type '%s' failed, falling back to SA token: %s", | ||
| http_output_config.auth_type, | ||
| e | ||
| ) | ||
| return None | ||
|
|
||
|
|
||
| class AuthHeaderError(RuntimeError): | ||
| """Raised when a configured auth type fails to produce a valid header.""" | ||
|
|
||
|
|
||
| def get_auth_header(http_config: CVEHttpOutputConfig | None) -> str | None: | ||
| """Return an Authorization header value, or None when auth is disabled. | ||
|
|
||
| Raises AuthHeaderError when auth is configured but cannot produce a | ||
| valid header (missing credentials, token fetch failure, etc.). | ||
| """ | ||
| if http_config is None: | ||
| return None | ||
|
|
||
| match http_config.auth_type: | ||
| case "basic": | ||
| if http_config.username and http_config.password: | ||
| credentials = f"{http_config.username}:{http_config.password}" | ||
| encoded_creds = base64.b64encode(credentials.encode('utf-8')).decode('utf-8') | ||
| return f"Basic {encoded_creds}" | ||
| return None | ||
| raise AuthHeaderError("Basic auth requires username and password") | ||
| case "keycloak": | ||
| if not all([http_config.keycloak_server, http_config.keycloak_realm, | ||
| http_config.client_id, http_config.client_secret]): | ||
| logger.error("Keycloak auth requires keycloak_server, keycloak_realm, client_id, client_secret") | ||
| return None | ||
| raise AuthHeaderError("Keycloak auth requires keycloak_server, keycloak_realm, client_id, client_secret") | ||
| token = _fetch_keycloak_token(http_config) | ||
| if token: | ||
| return f"Bearer {token}" | ||
| return None | ||
| raise AuthHeaderError("Failed to fetch Keycloak access token") | ||
| case "bearer": | ||
| if http_config.token: | ||
| return f"Bearer {http_config.token}" | ||
|
|
@@ -302,10 +373,22 @@ def get_auth_header(http_config: CVEHttpOutputConfig | None) -> str | None: | |
| with open(http_config.token_path, 'r') as file: | ||
| return f"Bearer {file.read().strip()}" | ||
| except Exception as e: | ||
| logger.warn(f"Unable to read OAuth token: {e}") | ||
| return None | ||
| case None: | ||
| raise AuthHeaderError(f"Unable to read OAuth token from {http_config.token_path}: {e}") from e | ||
| raise AuthHeaderError("Bearer auth requires token or token_path") | ||
| case "cognito": | ||
| if not all([http_config.cognito_domain, http_config.cognito_client_id, http_config.cognito_client_secret]): | ||
| raise AuthHeaderError("Cognito auth requires cognito_domain, cognito_client_id, and cognito_client_secret") | ||
| token = _fetch_cognito_token(http_config) | ||
| if token: | ||
| return f"Bearer {token}" | ||
| raise AuthHeaderError("Failed to fetch Cognito access token") | ||
| case "disabled" | None: | ||
| return None | ||
| case _: | ||
| raise AuthHeaderError( | ||
| f"Unrecognized auth_type '{http_config.auth_type}'. " | ||
| "Valid values: basic, bearer, keycloak, cognito, disabled" | ||
| ) | ||
|
|
||
|
|
||
| def _http_params_override(http_config: CVEHttpOutputConfig, mlops_config: MLOpsConfig,http_headers) -> dict[str, Any]: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Critical — the entire integration half of this feature is untested.
This new context-var branch (and the
http_auth_header_contextmanager above it) has no test.test_credential_client.pywasn't touched, and there are no tests for the try/except wiring added tocve_clone_and_deps.py/cve_generate_vdbs.py/cve_segmentation.py. The unit tests cover_fetch_cognito_token/get_auth_headerwell, but not the load-bearing wiring.Please add (extend the untouched
test_credential_client.py):_resolve_jwt_tokenis not called;Bearer <jwt>when the context value isNone;withexits (concurrency-critical — this codebase runs parallel async tasks);auth_header=Nonefallback.Also verify the real exception type
builder.get_function_configraises when the function isn't registered is actually within(KeyError, ValueError, AttributeError)— if not, the step crashes instead of falling back.