fda: add --tls-cert flag to trust custom CA for HTTPS by mvanhorn · Pull Request #6074 · feldera/feldera · GitHub
Skip to content

fda: add --tls-cert flag to trust custom CA for HTTPS#6074

Merged
gz merged 1 commit intofeldera:mainfrom
mvanhorn:osc/5475-fda-tls-cert
Apr 21, 2026
Merged

fda: add --tls-cert flag to trust custom CA for HTTPS#6074
gz merged 1 commit intofeldera:mainfrom
mvanhorn:osc/5475-fda-tls-cert

Conversation

@mvanhorn
Copy link
Copy Markdown
Contributor

@mvanhorn mvanhorn commented Apr 19, 2026

Closes #5475

Adds a new global option `--tls-cert` (env: `FELDERA_HTTPS_TLS_CERT`) to the `fda` CLI. When set, the path is read and parsed as one or more PEM-encoded certificates via `reqwest::Certificate::from_pem_bundle`, and each certificate is added to the client's set of trusted roots via `add_root_certificate`. This lets operators point `fda` at a Feldera deployment that serves HTTPS with a self-signed cert or a cert issued by a private CA not in the system trust store, without having to resort to the blanket `--insecure`/`-k`.

Usage matches the issue:

```bash
FELDERA_HTTPS_TLS_CERT=/path/to/my/cacert fda --host https://localhost pipelines

or

fda --tls-cert /path/to/my/cacert --host https://localhost pipelines
```

Behavior notes

  • Files that cannot be read, that parse as empty PEM bundles, or whose contents fail PEM parsing produce a clear error message naming the path.
  • Multiple certs in one PEM bundle are all added.
  • The option is orthogonal to `--insecure`: `--insecure` disables verification entirely; `--tls-cert` extends the trust store while keeping verification on. Using both continues to be valid and remains controlled by existing `--insecure` semantics.
  • Backwards compatible: when `--tls-cert` is not set the client is built exactly as before.

Describe Manual Test Plan

  • `cargo test -p fda --locked` passes 4 tests, including the three new `make_client_tls_cert_*` cases covering missing file, invalid PEM, and a valid PEM bundle.
  • `cargo clippy -p fda --locked --tests -- -D warnings` clean.
  • Manual smoke check: `fda --help` now lists `--tls-cert` in Global Options with the env var documented.

Checklist

  • Unit tests added/updated
  • Integration tests added/updated
  • Documentation updated
  • Changelog updated

Breaking Changes?

Mark if you think the answer is yes for any of these components:

  • OpenAPI / REST HTTP API / feldera-types / manager
  • Feldera SQL (Syntax, Semantics)
  • feldera-sqllib
  • Python SDK
  • fda (CLI arguments)

Additive change only: new optional `--tls-cert` flag with no change to existing flag behavior.

mythical-fred
mythical-fred suggested changes Apr 19, 2026
View reviewed changes
Copy link
Copy Markdown

@mythical-fred mythical-fred left a comment

Choose a reason for hiding this comment

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

Please add the missing docs update, see inline.

Comment thread crates/fda/src/cli.rs
@mvanhorn
Copy link
Copy Markdown
Contributor Author

mvanhorn commented Apr 19, 2026

mythical-fred
mythical-fred approved these changes Apr 19, 2026
View reviewed changes
Copy link
Copy Markdown

@mythical-fred mythical-fred left a comment

Choose a reason for hiding this comment

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

LGTM

mihaibudiu
mihaibudiu approved these changes Apr 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

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

I think @gz will have to also review this from a security pov, but this looks fine.

gz
gz requested changes Apr 20, 2026
View reviewed changes
Comment thread docs.feldera.com/docs/interface/cli.md Outdated
Comment thread docs.feldera.com/docs/interface/cli.md Outdated
Comment thread crates/fda/src/main.rs
Comment thread crates/fda/src/cli.rs
@gz
Copy link
Copy Markdown
Contributor

gz commented Apr 20, 2026 •
edited
Loading

Thanks a lot for the contribution!

Only semantic I would change is the "Using both continues to be valid and remains controlled by existing --insecure semantics." -> make it a hard failure if both are specified.

mvanhorn added a commit to mvanhorn/feldera that referenced this pull request Apr 20, 2026
@mvanhorn
fda: make --tls-cert and --insecure mutually exclusive
890cbdc 
Per @gz review on feldera#6074: combining TLS trust-store extension with
certificate verification disabled is confusing and not a combination
that ever makes sense. clap now rejects the pair at parse time with
a hard error (exit 2), before any HTTPS request runs.

- crates/fda/src/cli.rs: add conflicts_with = "insecure" on --tls-cert
- docs.feldera.com/docs/interface/cli.md: mention the mutual exclusion
  and use /etc/ssl/ca-bundle.pem in the example (a private CA bundle
  is not a "private-ca.pem" singular file in practice)
- crates/fda/test.bash: add two fail_on_success cases covering both
  --insecure and -k shorthand combined with --tls-cert
@mvanhorn
Copy link
Copy Markdown
Contributor Author

mvanhorn commented Apr 20, 2026

Done in 890cbdc. --tls-cert now carries conflicts_with = "insecure" on clap, so supplying both (-k/--insecure alongside --tls-cert) fails at parse time with a hard error before any request runs:

error: the argument '--insecure' cannot be used with '--tls-cert <TLS_CERT>'

Docs updated to mention the mutual exclusion. test.bash exercises both --insecure/--tls-cert and -k/--tls-cert via fail_on_success. Verified: cargo test -p fda (4 passed), manual exit=2.

gz
gz approved these changes Apr 20, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@gz gz left a comment

Choose a reason for hiding this comment

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

Awesome thank you <3

@mihaibudiu
Copy link
Copy Markdown
Contributor

mihaibudiu commented Apr 20, 2026

@mvanhorn I think these commits can be squashed to just 1, and then I will merge the PR.
Thank you!

@mvanhorn
fda: add --tls-cert flag to trust custom CA for HTTPS
a1ac231 
Adds a new global option `--tls-cert` (env: `FELDERA_HTTPS_TLS_CERT`) to
the `fda` CLI. When set, the path is read and parsed as one or more
PEM-encoded certificates via `reqwest::Certificate::from_pem_bundle`,
and each certificate is added to the client's set of trusted roots via
`add_root_certificate`. This lets operators point `fda` at a Feldera
deployment that serves HTTPS with a self-signed cert or a cert issued by
a private CA not in the system trust store, without having to resort to
`--insecure`/`-k`.

`--tls-cert` and `--insecure`/`-k` are mutually exclusive at the clap
parse level: combining a trust-store extension with verification
disabled doesn't make sense and is rejected with a hard error (exit 2)
before any HTTPS request runs. `crates/fda/test.bash` covers both
forms.

Docs at `docs.feldera.com/docs/interface/cli.md` document the flag, the
env var, and the mutual exclusion under a new
'Connecting to HTTPS with a custom CA' subsection.

Closes feldera#5475

Signed-off-by: Matt Van Horn <mvanhorn@gmail.com>
@mvanhorn mvanhorn force-pushed the osc/5475-fda-tls-cert branch from 890cbdc to a1ac231 Compare April 21, 2026 15:42
@mvanhorn
Copy link
Copy Markdown
Contributor Author

mvanhorn commented Apr 21, 2026

@gz gz enabled auto-merge April 21, 2026 16:29
@gz gz added this pull request to the merge queue Apr 21, 2026
Merged via the queue into feldera:main with commit 7aa75c9 Apr 21, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gz gz gz approved these changes

@mihaibudiu mihaibudiu mihaibudiu approved these changes

+1 more reviewer

@mythical-fred mythical-fred mythical-fred approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[fda] provide self signed for connecting over https

4 participants

@mvanhorn @gz @mihaibudiu @mythical-fred