feat: Add AWS Cognito as OIDC Identity Provider (#325) - #331
Conversation
--------- Signed-off-by: Zvi Grinberg <zgrinber@redhat.com> Co-authored-by: Tamar Weisskopf <tweissko@redhat.com> Co-authored-by: Zvi Grinberg <zgrinber@redhat.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
zvigrinberg
left a comment
There was a problem hiding this comment.
@TamarW0 LGTM Approved.
Don't forget tests in a follow up PR.
Code Review — 8 findingsCritical / High (must fix before merge)1. Missing wildcard case in The match handles Fix: Add explicit case "disabled" | None:
return None
case _:
logger.error(
"Unrecognized auth_type '%s'. Valid values: basic, bearer, keycloak, cognito, disabled",
http_config.auth_type,
)
return None2. When Cognito token fetch fails, Fix: Log a warning at call sites when auth was configured but returned None: auth_header = get_auth_header(http_output_config)
if auth_header is None and http_output_config.auth_type not in ("disabled", None):
logger.warning(
"Auth type is '%s' but get_auth_header returned None — "
"credential backend calls will fall back to SA token",
http_output_config.auth_type,
)3. The field description says Fix: Either prepend domain = http_config.cognito_domain.rstrip("/")
if not domain.startswith("https://"):
domain = f"https://{domain}"
token_url = f"{domain}/oauth2/token"Or update the field description to require the scheme. 4. The type signature accepts Fix: def get_auth_header(http_config: CVEHttpOutputConfig | None) -> str | None:
if http_config is None:
return None
match http_config.auth_type:
...High (should fix)5. No warning when configured auth fails — When Fix: Add an else branch logging a warning when 6. Broad Catches Medium (recommended)7. Token fetched 4× per pipeline run — Each pipeline node independently calls 8. Zero test coverage for new code paths
|
No description provided.