Add authentication-review skill

.claude-plugin/marketplace.json

@@ -8,7 +8,7 @@
     {
       "name": "skilldeck",
       "source": "./claude-plugin",
-      "description": "Security and code-review skills for Claude Code: ci-workflow-review, code-smells, dependency-review, iac-review, logging, migration-review, resilience-review, security-review, test-review"
+      "description": "Security and code-review skills for Claude Code: authentication-review, ci-workflow-review, code-smells, dependency-review, iac-review, logging, migration-review, resilience-review, security-review, test-review"
     }
   ]
 }

CHANGELOG.md

@@ -13,6 +13,21 @@ All notable changes to this project are documented here. The format is based on
   a comment), and a Dependabot config keeps the pins and dev dependencies
   current (#34).
 
+- `authentication-review` skill (0.1.0) — reviews authentication changes in
+  depth: password storage and policy, recovery/reset flows, MFA bypass and OTP
+  handling, session fixation and cookie hardening, JWT/API-token verification
+  (alg confusion, key selection), OAuth 2.0 (PKCE, `state`, `redirect_uri`
+  exact match, deprecated grants), OIDC (`nonce`, `iss`+`sub` identity, JWKS
+  trust), SAML (assertion vs response signatures, XSW, replay/conditions), and
+  LDAP sign-in (empty-password bind, unchecked bind result). Classified
+  against OWASP ASVS 5.0 (V6/V7/V9/V10, with V11/V3 for KDF and cookie
+  findings) with patterns from RFC 9700, NIST SP 800-63B, and the OWASP
+  Authentication, Password Storage, Session Management, MFA, Forgot Password,
+  and SAML Security cheat sheets. Pairs with `security-review` (bumped to
+  0.3.2 for the reciprocal cross-reference), which keeps breadth coverage.
+  Ships with OAuth (hand-rolled code flow missing `state`/PKCE) and SAML
+  (response-envelope-only signature check + raw-document `NameID` read) eval
+  fixtures.
 - `ci-workflow-review` skill (0.2.0) — reviews CI/CD pipeline changes for
   injection and poisoned pipeline execution (untrusted `github.event` /
   GitLab predefined-variable interpolation, `pull_request_target` + head

claude-plugin/.claude-plugin/plugin.json

@@ -1,7 +1,7 @@
 {
   "name": "skilldeck",
   "version": "0.3.0",
-  "description": "Security and code-review skills for Claude Code: ci-workflow-review, code-smells, dependency-review, iac-review, logging, migration-review, resilience-review, security-review, test-review",
+  "description": "Security and code-review skills for Claude Code: authentication-review, ci-workflow-review, code-smells, dependency-review, iac-review, logging, migration-review, resilience-review, security-review, test-review",
   "author": {
     "name": "Richard Hope",
     "url": "https://github.com/IcebergAI/skilldeck"

claude-plugin/skills/security-review/SKILL.md

@@ -43,7 +43,8 @@ Unless told otherwise, review to **L2**.
 - **V5 File Handling** — upload validation, type/size limits, safe storage paths,
   SSRF via file/URL fetches, deserialization of untrusted data.
 - **V6 Authentication** — credential handling, password storage, MFA, secure
-  recovery and lockout; no auth bypass.
+  recovery and lockout; no auth bypass. (See the `authentication-review` skill
+  for depth on V6, V7, V9, and V10, plus SAML and LDAP sign-in.)
 - **V7 Session Management** — secure session creation, rotation on privilege
   change, timeout, secure/HttpOnly cookies, invalidation on logout.
 - **V8 Authorization** — missing or weakened access checks, privilege escalation,

docs/finding-output.md

@@ -1,6 +1,7 @@
 # Finding output format
 
-Every review skill (`security-review`, `code-smells`, `dependency-review`,
+Every review skill (`security-review`, `authentication-review`,
+`ci-workflow-review`, `iac-review`, `code-smells`, `dependency-review`,
 `test-review`, and the review half of `logging`) reports its findings in one
 shared shape. A consistent shape means findings from different skills can be
 read, sorted, deduplicated, and posted as inline PR comments without per-skill
@@ -36,6 +37,9 @@ Report each finding as a single top-level list item:
 | Skill | `classifier` is… | Example |
 | --- | --- | --- |
 | `security-review` | the ASVS 5.0 category | `V8 Authorization` |
+| `authentication-review` | the ASVS 5.0 category (V6/V7/V9/V10; V11 for password-storage KDFs, V3 for cookie attributes) | `V10 OAuth & OIDC` |
+| `ci-workflow-review` | the CICD-SEC category | `CICD-SEC-4 Poisoned Pipeline Execution` |
+| `iac-review` | the misconfiguration kind | `Open security group`, `Wildcard IAM` |
 | `code-smells` | the smell and its group | `Long Method (Bloaters)` |
 | `dependency-review` | the advisory ID or supply-chain concern | `CVE-2024-12345`, `Typosquatting` |
 | `test-review` | the weakness kind | `Coverage gap`, `Assertion-free test` |

evals/fixtures/authentication-review-saml/base/app/app.py

@@ -0,0 +1,21 @@
+import os
+
+from flask import Flask, abort, jsonify, session
+
+import saml_acs
+
+app = Flask(__name__)
+app.secret_key = os.environ["SECRET_KEY"]
+app.config.update(
+    SESSION_COOKIE_SECURE=True,
+    SESSION_COOKIE_HTTPONLY=True,
+    SESSION_COOKIE_SAMESITE="Lax",
+)
+app.register_blueprint(saml_acs.bp)
+
+
+@app.get("/me")
+def me():
+    if "user" not in session:
+        abort(401)
+    return jsonify(user=session["user"])

evals/fixtures/authentication-review-saml/base/app/idp_config.py

@@ -0,0 +1,6 @@
+import os
+
+# The IdP signing certificate is provisioned from the IdP's metadata at
+# deploy time, not taken from the SAML response.
+IDP_CERT = os.environ["SAML_IDP_CERT"]
+SP_ENTITY_ID = "https://app.example.com/saml/metadata"

evals/fixtures/authentication-review-saml/base/app/saml_acs.py

@@ -0,0 +1,30 @@
+"""SAML assertion consumer service for IdP sign-in."""
+
+import base64
+
+from flask import Blueprint, abort, redirect, request, session
+from signxml import XMLVerifier
+
+import idp_config
+
+bp = Blueprint("saml", __name__)
+
+NS = {"saml": "urn:oasis:names:tc:SAML:2.0:assertion"}
+
+
+@bp.post("/saml/acs")
+def acs():
+    document = base64.b64decode(request.form["SAMLResponse"])
+    # Verify the signature and work only with the signed subtree from here on.
+    verified = (
+        XMLVerifier().verify(document, x509_cert=idp_config.IDP_CERT).signed_xml
+    )
+    audience = verified.findtext(".//saml:Audience", namespaces=NS)
+    if audience != idp_config.SP_ENTITY_ID:
+        abort(403)
+    name_id = verified.findtext(".//saml:Subject/saml:NameID", namespaces=NS)
+    if not name_id:
+        abort(403)
+    session.clear()
+    session["user"] = name_id
+    return redirect("/me")

evals/fixtures/authentication-review-saml/base/requirements.txt

@@ -0,0 +1,3 @@
+flask
+lxml
+signxml

evals/fixtures/authentication-review-saml/change/app/saml_acs.py

@@ -0,0 +1,32 @@
+"""SAML assertion consumer service for IdP sign-in."""
+
+import base64
+
+from flask import Blueprint, abort, redirect, request, session
+from lxml import etree
+from signxml import XMLVerifier
+
+import idp_config
+
+bp = Blueprint("saml", __name__)
+
+NS = {"saml": "urn:oasis:names:tc:SAML:2.0:assertion"}
+
+_PARSER = etree.XMLParser(resolve_entities=False, no_network=True)
+
+
+@bp.post("/saml/acs")
+def acs():
+    document = base64.b64decode(request.form["SAMLResponse"])
+    # Check the response signature, then pull out the fields we need.
+    XMLVerifier().verify(document, x509_cert=idp_config.IDP_CERT)
+    tree = etree.fromstring(document, parser=_PARSER)
+    assertion = tree.find(".//saml:Assertion", NS)
+    if assertion is None:
+        abort(403)
+    name_id = assertion.findtext(".//saml:NameID", namespaces=NS)
+    if not name_id:
+        abort(403)
+    session.clear()
+    session["user"] = name_id
+    return redirect("/me")

evals/fixtures/authentication-review-saml/expected.yaml

@@ -0,0 +1,5 @@
+skill: authentication-review
+plants:
+  - file: app/saml_acs.py
+    keywords: [assertion, signature, XSW, wrapping]
+max-findings: 5

evals/fixtures/authentication-review/base/app/app.py

@@ -0,0 +1,33 @@
+import os
+
+from flask import Flask, abort, jsonify, request, session
+from werkzeug.security import check_password_hash
+
+import users
+
+app = Flask(__name__)
+app.secret_key = os.environ["SECRET_KEY"]
+app.config.update(
+    SESSION_COOKIE_SECURE=True,
+    SESSION_COOKIE_HTTPONLY=True,
+    SESSION_COOKIE_SAMESITE="Lax",
+)
+
+
+@app.post("/login")
+def login():
+    username = request.form.get("username", "")
+    password = request.form.get("password", "")
+    stored_hash = users.password_hash(username)
+    if not check_password_hash(stored_hash, password):
+        abort(401, "Invalid username or password.")
+    session.clear()
+    session["user"] = username
+    return jsonify(ok=True)
+
+
+@app.get("/me")
+def me():
+    if "user" not in session:
+        abort(401)
+    return jsonify(user=session["user"])

evals/fixtures/authentication-review/base/app/users.py

@@ -0,0 +1,12 @@
+from werkzeug.security import generate_password_hash
+
+# Placeholder store; real deployments load users from the database.
+_USERS: dict[str, str] = {}
+
+# Verifying against a dummy hash keeps response time uniform for
+# unknown usernames.
+_DUMMY_HASH = generate_password_hash("not-a-real-password")
+
+
+def password_hash(username: str) -> str:
+    return _USERS.get(username, _DUMMY_HASH)

evals/fixtures/authentication-review/base/requirements.txt

@@ -0,0 +1,2 @@
+flask
+requests

evals/fixtures/authentication-review/change/app/app.py

@@ -0,0 +1,35 @@
+import os
+
+from flask import Flask, abort, jsonify, request, session
+from werkzeug.security import check_password_hash
+
+import sso
+import users
+
+app = Flask(__name__)
+app.secret_key = os.environ["SECRET_KEY"]
+app.config.update(
+    SESSION_COOKIE_SECURE=True,
+    SESSION_COOKIE_HTTPONLY=True,
+    SESSION_COOKIE_SAMESITE="Lax",
+)
+app.register_blueprint(sso.bp)
+
+
+@app.post("/login")
+def login():
+    username = request.form.get("username", "")
+    password = request.form.get("password", "")
+    stored_hash = users.password_hash(username)
+    if not check_password_hash(stored_hash, password):
+        abort(401, "Invalid username or password.")
+    session.clear()
+    session["user"] = username
+    return jsonify(ok=True)
+
+
+@app.get("/me")
+def me():
+    if "user" not in session:
+        abort(401)
+    return jsonify(user=session["user"])

evals/fixtures/authentication-review/change/app/sso.py

@@ -0,0 +1,47 @@
+"""Sign in with the corporate identity provider (OAuth 2.0 code flow)."""
+
+import os
+
+import requests
+from flask import Blueprint, redirect, request, session
+
+bp = Blueprint("sso", __name__)
+
+AUTHORIZE_URL = "https://idp.example.com/oauth/authorize"
+TOKEN_URL = "https://idp.example.com/oauth/token"
+USERINFO_URL = "https://idp.example.com/oauth/userinfo"
+CLIENT_ID = os.environ["SSO_CLIENT_ID"]
+REDIRECT_URI = "https://app.example.com/sso/callback"
+
+
+@bp.get("/sso/login")
+def sso_login():
+    return redirect(
+        f"{AUTHORIZE_URL}?response_type=code"
+        f"&client_id={CLIENT_ID}"
+        f"&redirect_uri={REDIRECT_URI}"
+        "&scope=openid+profile+email"
+    )
+
+
+@bp.get("/sso/callback")
+def sso_callback():
+    code = request.args["code"]
+    token = requests.post(
+        TOKEN_URL,
+        data={
+            "grant_type": "authorization_code",
+            "code": code,
+            "client_id": CLIENT_ID,
+            "redirect_uri": REDIRECT_URI,
+        },
+        timeout=10,
+    ).json()
+    profile = requests.get(
+        USERINFO_URL,
+        headers={"Authorization": f"Bearer {token['access_token']}"},
+        timeout=10,
+    ).json()
+    session.clear()
+    session["user"] = profile["email"]
+    return redirect("/me")

evals/fixtures/authentication-review/expected.yaml

@@ -0,0 +1,5 @@
+skill: authentication-review
+plants:
+  - file: app/sso.py
+    keywords: [state, PKCE, CSRF]
+max-findings: 6

src/skilldeck/skills/authentication-review/meta.yaml

@@ -0,0 +1,10 @@
+name: authentication-review
+description: Review authentication changes — passwords, MFA, sessions, tokens, OAuth/OIDC, SAML, and LDAP sign-in — for login-bypass and account-takeover defects, aligned to OWASP ASVS 5.0.
+category: security
+version: 0.1.0
+supported-agents:
+  - claude
+  - codex
+  - copilot
+  - cursor
+  - kiro