Skip to content

Conversation

@pboling
Copy link
Member

@pboling pboling commented Nov 6, 2025

In lib/omniauth-ldap/adaptor.rb, we set TLS options via tls_options:

If disable_verify_certificates is true, we explicitly set { verify_mode: OpenSSL::SSL::VERIFY_NONE }

Otherwise (default), we use OpenSSL::SSL::SSLContext::DEFAULT_PARAMS, which verifies server certs (VERIFY_PEER) and uses sane defaults

This means verification is enabled by default for both “ssl” (LDAPS/simple_tls) and “tls” (STARTTLS). Only an explicit disable_verify_certificates: true turns verification off.

Users can merge their own tls_options on top of secure defaults; we sanitize out nil/blank values so they don’t accidentally wipe defaults.

@pboling pboling self-assigned this Nov 6, 2025
Copilot AI review requested due to automatic review settings November 6, 2025 11:26
@pboling pboling merged commit 01586f5 into main Nov 6, 2025
35 of 36 checks passed
@pboling pboling deleted the feat/docs-tls-verification branch November 6, 2025 11:26
@pboling pboling mentioned this pull request Nov 6, 2025
6 tasks
@github-actions
Copy link

github-actions bot commented Nov 6, 2025

Code Coverage

Package Line Rate Branch Rate Health
omniauth-ldap 98% 80%
Summary 98% (290 / 297) 80% (102 / 128)

Minimum allowed line rate is 97%

@codecov
Copy link

codecov bot commented Nov 6, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.82%. Comparing base (97d99fc) to head (0105435).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #110   +/-   ##
=======================================
  Coverage   96.82%   96.82%           
=======================================
  Files           4        4           
  Lines         315      315           
  Branches      117      117           
=======================================
  Hits          305      305           
  Misses         10       10           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds documentation for TLS certificate verification features that are already implemented in the codebase. The changes include updating the README and generated YARD documentation with a new section explaining how TLS certificate verification works by default, how to customize it, and how to disable it.

  • Added new "TLS certificate verification" section to README.md explaining secure-by-default behavior
  • Updated CHANGELOG.md to document the new documentation
  • Regenerated YARD documentation files with updated timestamps and new content

Reviewed Changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
README.md Added TLS certificate verification documentation section with examples
CHANGELOG.md Added documentation entry to unreleased changes
docs/index.html Regenerated with new TLS verification section
docs/file.README.html Regenerated with new TLS verification section
docs/file.CHANGELOG.html Regenerated with changelog updates
docs/OmniAuth/LDAP/Adaptor.html Regenerated with code changes showing ConnectionError raise
docs/*.html (various) Updated generation timestamps

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tls_options: {
ca_file: "/etc/ssl/private/my_org_ca.pem",
ssl_version: "TLSv1_2",
ciphers: ["TLS_AES_256_GCM_SHA384", "TLS_CHACHA20_POLY1305_SHA256"],
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

The ciphers option in OpenSSL's tls_options expects a colon-delimited string (e.g., 'TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256'), not an array. The current example would likely cause an error when passed to OpenSSL. Consider updating the example to use the correct format: ciphers: 'TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256'

Suggested change
ciphers: ["TLS_AES_256_GCM_SHA384", "TLS_CHACHA20_POLY1305_SHA256"],
ciphers: "TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256",

Copilot uses AI. Check for mistakes.
tls_options: {
ca_file: "/etc/ssl/private/my_org_ca.pem",
ssl_version: "TLSv1_2",
ciphers: ["TLS_AES_256_GCM_SHA384", "TLS_CHACHA20_POLY1305_SHA256"],
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

The ciphers option in OpenSSL's tls_options expects a colon-delimited string (e.g., 'TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256'), not an array. The current example would likely cause an error when passed to OpenSSL. Consider updating the example to use the correct format: ciphers: 'TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256'

Copilot uses AI. Check for mistakes.
tls_options: {
ca_file: "/etc/ssl/private/my_org_ca.pem",
ssl_version: "TLSv1_2",
ciphers: ["TLS_AES_256_GCM_SHA384", "TLS_CHACHA20_POLY1305_SHA256"],
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

The ciphers option in OpenSSL's tls_options expects a colon-delimited string (e.g., 'TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256'), not an array. The current example would likely cause an error when passed to OpenSSL. Consider updating the example to use the correct format: ciphers: 'TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256'

Suggested change
ciphers: ["TLS_AES_256_GCM_SHA384", "TLS_CHACHA20_POLY1305_SHA256"],
ciphers: "TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256",

Copilot uses AI. Check for mistakes.
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