Skip to content

fix: fill stub workflow in performing-ssl-tls-security-assessment#45

Open
xiaolai wants to merge 1 commit intomukul975:mainfrom
xiaolai:fix/nlpm-ssl-tls-stub
Open

fix: fill stub workflow in performing-ssl-tls-security-assessment#45
xiaolai wants to merge 1 commit intomukul975:mainfrom
xiaolai:fix/nlpm-ssl-tls-stub

Conversation

@xiaolai
Copy link
Copy Markdown

@xiaolai xiaolai commented Apr 21, 2026

Automated audit: This PR was generated by NLPM, a natural language programming linter, running via claude-code-action. Please evaluate the diff on its merits.

Bug

performing-ssl-tls-security-assessment/SKILL.md was a stub: 4 prose-only step descriptions with no Python code, and a single-line "Expected Output". The prerequisites listed sslyze, but no actual API usage was shown. An agent following this skill would have no concrete guidance to work from.

Fix

Replace the stub with a complete 5-step Python workflow using the sslyze library:

  1. Configure and run a ServerScanRequest — set up ServerNetworkLocation and queue all relevant ScanCommand values
  2. Evaluate protocol versions — check SSLv2/3 and TLS 1.0–1.3 acceptance, flagging insecure protocols
  3. Check certificate chain — extract expiry, key size, chain trust, and surface issues
  4. Scan for known vulnerabilities — Heartbleed, ROBOT, session renegotiation DoS, and HSTS header presence
  5. Generate a structured JSON report — consolidate all findings into a machine-readable output file

Also added:

  • Key Concepts table — cipher suites, Heartbleed, ROBOT, HSTS, OCSP Stapling, Forward Secrecy, certificate chain
  • Tools & Systems — sslyze, testssl.sh, OpenSSL, Qualys SSL Labs
  • Common Scenarios — PCI-DSS TLS compliance check, HSTS deployment verification (both with Pitfalls)
  • Output Format — structured text report block showing exact output shape

All additions match the style and structure of the 95-point skills in this repository.

The SKILL.md had 4 prose-only bullet steps and a one-line Expected
Output with no code, despite prerequisites listing sslyze. This made
the skill unusable for agents following it.

Add a full 5-step Python workflow using the sslyze API:
- Step 1: configure and run a ServerScanRequest with all scan commands
- Step 2: evaluate accepted protocol versions (SSLv2 through TLS 1.3)
- Step 3: check certificate chain validity and expiry
- Step 4: scan for Heartbleed, ROBOT, renegotiation, and HSTS
- Step 5: generate a structured JSON report

Also adds Key Concepts table, Tools section, two Common Scenarios with
pitfalls, and a structured Output Format block — matching the high-
quality template used in 95-point skills in this repo.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@mukul975
Copy link
Copy Markdown
Owner

Review — REQUEST_CHANGES

The sslyze v5 API usage is correct throughout (Scanner, ServerScanRequest, ScanCommand — no legacy SynchronousScanner), all 5 steps are complete and non-stubbed, and the output format section is genuinely useful. Two items need fixing before merge:

Blockers

1. Hardcoded scan target — Step 1 hardcodes ServerNetworkLocation("example.com", 443) as the target. The target must be parameterized — either sys.argv[1] or a function parameter. An agent using this skill cannot follow it safely with a hardcoded target baked in.

2. No MITRE ATT&CK mapping — The file has NIST CSF mappings but no ATT&CK techniques. For a defensive TLS assessment skill, please add at minimum: T1040 (Network Sniffing — detection context), T1557 (Adversary-in-the-Middle prevention), T1573 (Encrypted Channel). Can go in frontmatter as a mitre_attack: list or in a body section, consistent with how other network-security skills in the repo handle it.

Minor (non-blocking)

  • Consider wrapping scanner.get_results() in a try/except to handle TLS handshake failures explicitly (ServerRejectedTlsHandshake), beyond the current connectivity_error_trace check

Fix those two and this is a clean merge.

@mukul975
Copy link
Copy Markdown
Owner

Note on Co-Authored-By attribution: This PR's commits include Co-Authored-By: Claude Code <noreply@anthropic.com>. Please squash your commits and remove that line before the final merge — attribution should be yours only.

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