Skip to content

fix: align Admin TLS trust behavior with S3 for self-signed CAs#35

Merged
overtrue merged 2 commits intomainfrom
overtrue/issue-9-admin-tls-trust
Mar 8, 2026
Merged

fix: align Admin TLS trust behavior with S3 for self-signed CAs#35
overtrue merged 2 commits intomainfrom
overtrue/issue-9-admin-tls-trust

Conversation

@overtrue
Copy link
Copy Markdown
Contributor

@overtrue overtrue commented Mar 8, 2026

Summary

This PR fixes issue #9 by aligning Admin API TLS trust behavior with S3 behavior.

What changed

  • Switch reqwest features to include both:
    • rustls-tls-native-roots
    • rustls-tls-webpki-roots
  • Update AdminClient::new to:
    • keep honoring --insecure
    • explicitly enable native/webpki built-in certs
    • load and add custom ca_bundle certs when configured
  • Add tests for Admin CA bundle error paths:
    • invalid CA bundle file path
    • invalid CA bundle PEM

Why

On latest main, issue #9 was still reproducible:

  • trust + no insecure:
    • rc ls succeeded
    • rc admin user list failed

After this change:

  • trust + no insecure:
    • rc ls succeeded
    • rc admin user list succeeded

Validation

  • cargo fmt --all --check
  • cargo clippy --workspace -- -D warnings
  • cargo test --workspace
  • Docker TLS scenario re-check against rustfs/rustfs:1.0.0-alpha.81:
    • no trust + no insecure => both fail (expected)
    • trust + no insecure => both succeed (fixed)

Closes #9.

Copilot AI review requested due to automatic review settings March 8, 2026 16:00
Copy link
Copy Markdown
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

Aligns the Admin API client’s TLS trust behavior with the S3 client so aliases that rely on system trust stores and/or self-signed CA bundles behave consistently (fixing issue #9).

Changes:

  • Switch reqwest workspace features to enable both native-root and webpki-root trust stores.
  • Update AdminClient::new to explicitly enable built-in roots and to load an optional Alias.ca_bundle PEM as an additional root.
  • Add unit tests covering Admin CA bundle error paths (missing file, invalid PEM).

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File Description
crates/s3/src/admin.rs Configure reqwest TLS roots + optional CA bundle in AdminClient::new; add tests for CA bundle failures.
Cargo.toml Adjust workspace reqwest features to include native + webpki rustls root support.
Cargo.lock Lockfile update reflecting new rustls native-certs dependency.

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

@overtrue overtrue merged commit ff1a88f into main Mar 8, 2026
13 checks passed
@overtrue overtrue deleted the overtrue/issue-9-admin-tls-trust branch March 8, 2026 18:39
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.

Can partially perform commands on servers with self-signed CA

2 participants