Skip to content

Conversation

@retoo
Copy link

@retoo retoo commented Dec 3, 2025

Subsystem
ktor-network-tls, ktor-client-cio

Motivation
Currently, the Ktor CIO engine silently ignores client certificates that use the ECDSA

This limitation is not immediately obvious.

See also KTOR-5391.

Solution
Added a Note to the KDoc to the affected methods

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 3, 2025

Walkthrough

Added documentation comments to TLSConfigBuilder class and related methods clarifying that ECDSA certificates are not supported for client authentication, supporting only RSA and DSA. No code or behavioral changes.

Changes

Cohort / File(s) Change Summary
TLS Configuration Documentation
ktor-network/ktor-network-tls/jvm/src/io/ktor/network/tls/TLSConfigBuilder.kt
Added KDoc comments to class and methods (takeFrom, addCertificateChain, addKeyStore) noting that ECDSA certificates are unsupported for client authentication, with reference to KT0R-5391

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: documenting an ECDSA certificate limitation for client authentication, with reference to the ticket number.
Description check ✅ Passed The description includes all required template sections (Subsystem, Motivation, Solution) with sufficient detail about the problem and the documentation solution.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
ktor-network/ktor-network-tls/jvm/src/io/ktor/network/tls/TLSConfigBuilder.kt (1)

24-26: Consider verifying issue reference format consistency.

The three new documentation notes reference [KTOR-5391] using plain bracket syntax. However, other cross-references in this file use full markdown link syntax, e.g., [Report a problem](https://ktor.io/feedback/?fqname=...) (lines 16, 27, 34, etc.).

Verify whether your documentation tooling auto-converts [KTOR-5391] to a clickable link, or if this should use an explicit URL format for consistency:

// Example: if explicit link is needed
[KTOR-5391](https://youtrack.jetbrains.com/issue/KTOR-5391)

If the reference format is intentional or handled by a documentation plugin, no change is needed.

Also applies to: 103-105, 116-118

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93c5c96 and ad2c533.

📒 Files selected for processing (1)
  • ktor-network/ktor-network-tls/jvm/src/io/ktor/network/tls/TLSConfigBuilder.kt (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.kt: Follow Kotlin official style guide (https://kotlinlang.org/docs/coding-conventions.html)
Use star imports for io.ktor.* packages
Document all public APIs including parameters, return types, and exceptions
Mark internal APIs with @InternalAPI annotation
Run ./gradlew lintKotlin and fix all linting issues before giving control back to the user
Use ./gradlew formatKotlin to automatically fix formatting issues
Run ./gradlew updateLegacyAbi after making ABI changes to update ABI signature files
Binary compatibility is enforced - all public API changes must be tracked in the /api/ directories
Validate ABI with ./gradlew checkLegacyAbi and update with ./gradlew updateLegacyAbi
API changes must be intentional and well-documented
Error handling follows Kotlin conventions with specific Ktor exceptions

Files:

  • ktor-network/ktor-network-tls/jvm/src/io/ktor/network/tls/TLSConfigBuilder.kt
🧠 Learnings (2)
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : API changes must be intentional and well-documented

Applied to files:

  • ktor-network/ktor-network-tls/jvm/src/io/ktor/network/tls/TLSConfigBuilder.kt
📚 Learning: 2025-11-25T09:38:19.393Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:38:19.393Z
Learning: Applies to **/*.kt : Document all public APIs including parameters, return types, and exceptions

Applied to files:

  • ktor-network/ktor-network-tls/jvm/src/io/ktor/network/tls/TLSConfigBuilder.kt
🔇 Additional comments (2)
ktor-network/ktor-network-tls/jvm/src/io/ktor/network/tls/TLSConfigBuilder.kt (2)

19-29: Documentation of ECDSA limitation is clear and well-placed.

The note accurately documents the limitation and informs users at the property level where certificates are declared. This directly addresses the PR objective to reduce silent failures.


98-110: Excellent—documentation covers all certificate-addition entry points.

Adding the same ECDSA limitation note to both addCertificateChain and addKeyStore ensures users cannot miss this limitation regardless of which API they call. Together with the property-level note, this provides comprehensive visibility.

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.

1 participant