fix(search): honor the gRPC client TLS mode in the index command by jmrplens · Pull Request #3620 · opencloud-eu/opencloud

jmrplens · 2026-09-30T08:15:01Z

Description

opencloud search index reads OC_GRPC_CLIENT_TLS_MODE with a meaning of its own: insecure dials without TLS, and every other value, including the default and off, dials with verified TLS and ignores OC_GRPC_CLIENT_TLS_CACERT. The setting itself (pkg/shared) and the clients the other services build through reva's pool mean something else: off is no TLS, insecure is TLS without verifying the certificate, and on verifies it against the configured CA.

This maps the mode with pool.StringToTLSMode and dials with pool.NewConn, the mapping and dialer the other services' reva clients use. NewConn dials the endpoint directly, so there is still no registry lookup. --insecure still forces a connection without TLS, and then the mode is not read at all, so scripts and the acceptance tests that pass it keep working. --endpoint now defaults to the configured gRPC address (SEARCH_GRPC_ADDR) instead of a hardcoded 127.0.0.1:9220.

Related Issue

No issue. It started in opencloud-eu/web-extensions#575 (comment), where I offered to add --insecure to the search README and @JammingBen welcomed it. The README already got it in #3505, and looking at why the flag is needed at all led here. @dschmidt had proposed the same fix in #3505 (comment), which I missed when opening this.

Motivation and Context

With the default setup, where the gRPC services run without TLS, opencloud search index --all-spaces fails with tls: first record does not look like a TLS handshake unless --insecure is passed. Setting OC_GRPC_CLIENT_TLS_MODE=off explicitly doesn't help. The upgrade guide already describes the intended behavior ("OC_GRPC_CLIENT_TLS_MODE defaults to off", "Drop it only if you run gRPC with TLS"), which only holds with this change.
With TLS enabled for the gRPC services and the generated certificate (OC_GRPC_TLS_ENABLED=true and OC_GRPC_CLIENT_TLS_MODE=insecure, the documented combination), the command can't connect with any mode or flag, and with a private CA it ignores OC_GRPC_CLIENT_TLS_CACERT.
It came in with 5854b60 (No registry lookup in cli #2755), when the command moved to a plain gRPC client. Before that it used the shared client, which maps the modes as described above.

Two behavior changes, both where the setting contradicts its documented meaning: with OC_GRPC_CLIENT_TLS_MODE=insecure against services without TLS, the command now fails like every other client does with that combination (before, it happened to work), and an unknown mode is now rejected with unknown TLS mode unless --insecure is passed, instead of dialing TLS.

#3505 added --insecure to the README, to services/search/MIGRATION.md and to the two re-index hints the service logs (pkg/mapping/reconcile.go). With this change the flag is no longer needed for the default setup, and with TLS enabled for gRPC it makes the command fail, since it turns TLS off, so it is dropped from the README and the log hints. In MIGRATION.md the section now covers every 8.x release, without the flag, and an IMPORTANT block says that 8.0.x and 8.1.x still need it.

How Has This Been Tested?

test environment: opencloudeu/opencloud-rolling:8.0.1 in a single container after opencloud init, once with the default gRPC setup and once with OC_GRPC_TLS_ENABLED=true and OC_GRPC_CLIENT_TLS_MODE=insecure. search index --all-spaces ran inside the container with the stock binary and with one built from this branch (Alpine, CGO).
test case 1, default setup: the stock binary fails with the mode unset or off and only works with --insecure or mode insecure. The patched binary works with the mode unset or off, and with --insecure; mode insecure or on now fails there, as the other clients do.
test case 2, gRPC with TLS: the stock binary fails in every combination of mode and flag. The patched binary works with the container's insecure mode.
The Ginkgo suite in services/search/pkg/command runs the command against an in-process gRPC server without TLS: unset, off, --insecure and an unknown mode with --insecure connect, insecure fails, an unknown mode is rejected with unknown TLS mode, and one covers --endpoint taking precedence, while the specs that omit it cover the default. All seven fail against main's index.go. What pool.NewConn does with each mode belongs to reva, so the TLS-server cases from the first version were dropped at review.
go vet and golangci-lint run --new-from-rev with the repo config report nothing for the changed files (the config excludes test files), and the pkg/mapping tests pass.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation added

The index command read OC_GRPC_CLIENT_TLS_MODE with a meaning of its own: "insecure" dialed without TLS, and every other value, including the default and "off", dialed with verified TLS and ignored OC_GRPC_CLIENT_TLS_CACERT. Against the default setup, where the gRPC services run without TLS, it failed unless --insecure was passed, and against services with TLS enabled it only connected when their certificate was trusted by the system roots, never with the generated certificate or a private CA. Map the mode with pool.StringToTLSMode and dial with pool.NewConn, the mapping and dialer the other services' reva clients use: "off" dials without TLS, "insecure" uses TLS without verifying the certificate and "on" verifies it against the configured CA. --insecure still forces a connection without TLS, and then the mode is not read at all.

codacy-production · 2026-09-30T08:16:12Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 9 complexity

Metric Results

Complexity 9

View in Codacy

🟢 Coverage 94.12% diff coverage · +0.09% coverage variation

Metric Results

Coverage variation ✅ +0.09% coverage variation (-1.00%)

Diff coverage ✅ 94.12% diff coverage

View coverage diff in Codacy

Coverage variation details

Coverable lines Covered lines Coverage

Common ancestor commit (c8dd8ce) 89061 21597 24.25%

Head commit (9cfb21a) 89066 (+5) 21678 (+81) 24.34% (+0.09%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

Coverable lines Covered lines Diff coverage

Pull request (#3620) 17 16 94.12%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer}
_{TIP This summary will be updated as you push new changes.}

dschmidt

Thanks, this is basically what I proposed in #3505 (comment):
the command should evaluate OC_GRPC_CLIENT_TLS_MODE with the same semantics as every other client.
Going through pool.StringToTLSMode/pool.NewConn instead of mirroring the switch by hand is the better variant, and it also covers OC_GRPC_CLIENT_TLS_CACERT, which I had left as a follow-up.
Rejecting unknown modes is also more consistent than silently falling back to verified TLS like my diff did.

Three requests from my side (although others might disagree):

Please do drop --insecure from the places #3505 added it (README, services/search/MIGRATION.md, and the two re-index hints in pkg/mapping/reconcile.go). With this fix the flag is unnecessary in the default setup and actively wrong with TLS-enabled gRPC, so keeping it documented as the go-to just re-establishes the workaround for the bug you fixed.
Optional, but since you are in there: default --endpoint to cfg.GRPC.Addr when the flag is not set (it is a hardcoded 127.0.0.1:9220 today), so a reconfigured OC_SEARCH_GRPC_ADDR works without passing --endpoint. That was the second hunk in my proposal.
We write new test files with Ginkgo/Gomega instead of plain testing.T tables, could you convert index_test.go? The coverage itself looks good.

cc @butonic, this is the config-semantics fix from the #3505 discussion.

dschmidt · 2026-10-01T11:05:31Z

Looked at it again and was thinking: isn't the test covering a lot of reva client territory?
The command's own logic is really just three things: --insecure forces TLS off, the mode string is mapped via pool.StringToTLSMode (including the error for unknown modes), and mode + CA cert are passed on to pool.NewConn.

The three TLS-server cases (mode insecure against TLS, mode on with the CA cert, mode off against TLS) verify what pool.NewConn does with those options, and they are the only reason for the whole selfSignedCert apparatus (~40 lines of x509/PEM boilerplate plus the TLS listener). That behavior belongs to reva's pool and would be better pinned by a test over there.

I'd drop the TLS server, selfSignedCert and those three cases. What remains covers the actual change completely, without any crypto imports:

mode unset / off / --insecure against the plain server connect
mode insecure against the plain server fails (the behavior change this PR documents, no cert needed)
bogus fails with unknown TLS mode, bogus + --insecure connects

--endpoint defaulted to a hardcoded 127.0.0.1:9220, so a search service moved with SEARCH_GRPC_ADDR could only be reached by passing the flag. Use the configured address when the flag is not set.

With the client TLS mode honored, the default setup no longer needs --insecure, and with TLS enabled for gRPC the flag makes the command fail, since it turns TLS off. Drop it from the README examples and from the two re-index hints the service logs. MIGRATION.md keeps it: its section covers upgrading to 8.0.x, which still needs the flag.

Run the command against a search service without TLS only: what the command decides is whether to turn TLS off, how the mode maps and which endpoint it dials. What pool.NewConn does with each mode belongs to reva. Also cover the endpoint default.

jmrplens · 2026-10-03T08:04:19Z

Thanks, and sorry I missed your proposal in #3505: I looked at its diff but not its discussion, so the description should have credited you. I've pushed three commits for your points:

--insecure is gone from the README and from the two re-index hints in pkg/mapping/reconcile.go. I left it in MIGRATION.md on purpose: that section is about upgrading to 8.0.x, and 8.0.0 and 8.0.1 still need the flag, while with this fix it's harmless in the default setup. If you'd rather drop it there too, or add a note about the release that fixes it, I'm happy to change it.
--endpoint now defaults to cfg.GRPC.Addr when it isn't passed, as in your second hunk. The flag's own default is empty and the help text says where the address comes from, so --help no longer shows a 127.0.0.1:9220 that may not be in use.
The test is Ginkgo now and only uses a server without TLS, as you suggested: unset, off, --insecure, and an unknown mode with --insecure connect; insecure fails against it; an unknown mode is rejected with unknown TLS mode; and two cases cover the endpoint default and --endpoint taking precedence. Against main's index.go six of the seven fail, and against the previous commit of this PR the four that rely on the endpoint default do.

dschmidt · 2026-10-03T08:23:13Z

Thanks, and sorry I missed your proposal in #3505: I looked at its diff but not its discussion, so the description should have credited you.

Oh no worries. I think it's great we came to the same conclusion independently ... and that you even found a more standard approach :)

I've pushed three commits for your points:

--insecure is gone from the README and from the two re-index hints in pkg/mapping/reconcile.go. I left it in MIGRATION.md on purpose: that section is about upgrading to 8.0.x, and 8.0.0 and 8.0.1 still need the flag, while with this fix it's harmless in the default setup. If you'd rather drop it there too, or add a note about the release that fixes it, I'm happy to change it.

That's a tough one indeed. I'm not sure, maybe we should make it # v7 to v8 or # v7.x.x to 8.x.x and then add an attention block that 8.0.1 needed the --insecure flag?

--endpoint now defaults to cfg.GRPC.Addr when it isn't passed, as in your second hunk. The flag's own default is empty and the help text says where the address comes from, so --help no longer shows a 127.0.0.1:9220 that may not be in use.

👍🏻

The test is Ginkgo now and only uses a server without TLS, as you suggested: unset, off, --insecure, and an unknown mode with --insecure connect; insecure fails against it; an unknown mode is rejected with unknown TLS mode; and two cases cover the endpoint default and --endpoint taking precedence. Against main's index.go six of the seven fail, and against the previous commit of this PR the four that rely on the endpoint default do.

So does the reindex command still work with --insecure or not?
If it doesn't it's an even stronger reason not to keep it in the default migration doc imho.

The section was titled v7.x.x to v8.0.0 and told everyone to pass --insecure, which only 8.0.0 and 8.0.1 need. Title it v7.x.x to v8.x.x, drop the flag from the commands and say in an IMPORTANT block which releases still need it.

jmrplens · 2026-10-03T10:38:50Z

So does the reindex command still work with --insecure or not?

Yes, in the default setup: --insecure forces a connection without TLS, which is what the search service offers unless OC_GRPC_TLS_ENABLED=true. It only breaks when gRPC runs with TLS, because it turns TLS off on the client side. I checked both against 8.0.1 with a binary built from this branch: with the default config --insecure connects, and with OC_GRPC_TLS_ENABLED=true it fails with error reading server preface.

Your MIGRATION.md suggestion works for me, so I've pushed it: the heading is now v7.x.x to v8.x.x, both commands drop --insecure, and an IMPORTANT block above them says that 8.0.0 and 8.0.1 still need the flag in the default setup.

dschmidt · 2026-10-03T13:25:06Z

Thanks, this looks good to me now. Two small things left, both in index_test.go:

Codacy flags grpc.NewServer() as an insecure gRPC server. A plaintext server is the whole point of the test, so it's a false positive, but it blocks the check. Spelling out the intent should satisfy the rule (it only fires on a server without grpc.Creds), with identical behavior:
```
srv := grpc.NewServer(grpc.Creds(insecure.NewCredentials()))
```
uses TLS when the mode is insecure only asserts HaveOccurred(), so any error satisfies it. It's also the one spec that is green against main's index.go. Matching the handshake error pins it to what the name says:
```
Expect(runIndex("insecure")).To(MatchError(ContainSubstring("first record does not look like a TLS handshake")))
```

Nits:

endpointFlag holds the configured address after the fallback, so endpoint would be the more honest name.
The reva import sits in the opencloud group, server.go next to it has reva in the third-party group.
The description mentions two endpoint cases, there is one (the default is covered implicitly by the specs that omit --endpoint).
With OC_GRPC_CLIENT_TLS_MODE=on the certificate is verified against the dial target, which is now the bind address. With SEARCH_GRPC_ADDR=0.0.0.0:9220 that fails with x509: certificate is valid for ..., not 0.0.0.0 unless --endpoint names a host from the certificate. Not a regression (main ignored the CA cert altogether), but a sentence in the README or the flag help would save people the search.

The insecure-mode spec only asserted an error, so against main it passed on a refused connection to the hardcoded endpoint. Match the TLS handshake error instead. Pass insecure credentials to the test server explicitly, which is what it already did, so the static analysis no longer reports a server without credentials.

After the fallback the variable holds the configured address rather than the flag, and the reva import belongs with the third-party ones, as in server.go. The flag help also says that with mode on the server certificate must be valid for that address.

With OC_GRPC_CLIENT_TLS_MODE=on the certificate is checked against the dialed address, which is now SEARCH_GRPC_ADDR by default. A bind address such as 0.0.0.0:9220 then fails verification unless --endpoint names a host the certificate is valid for.

jmrplens · 2026-10-03T14:00:18Z

Thanks! Done in three more commits:

The test server gets grpc.Creds(insecure.NewCredentials()), and the insecure spec matches the handshake error. With that, all seven specs fail against main's index.go.
endpointFlag is now endpoint, and the reva import sits with cobra, as in server.go.
The x509 case is in the README after the re-indexing commands, and briefly in the --endpoint help: with on the certificate is checked against the dialed address, so a bind address like 0.0.0.0:9220 needs --endpoint with a host the certificate is valid for.

I've also fixed the description: it's one endpoint spec, and the specs that omit --endpoint cover the default.

dschmidt · 2026-10-03T16:07:46Z

I'm sorry, I was a little quick with the endpointFlag - flag suffix is used with other non bools as well... can I ask you to revert that? 🤦‍♂️

I'm not sure about the versions referenced in the Migrations docs. 8.1.0 is about to be released and we probably dont get this in until then.
How about On 8.0.x and 8.1.x?

The Flag suffix is the convention for the command's flag variables, not only for booleans.

8.1.0 ships before this fix, so it still needs the flag too.

jmrplens · 2026-10-03T16:42:23Z

Ha, no worries: a rename that goes there and back is the cheapest review round there is 😄 endpointFlag is back with its siblings, and the migration note now says 8.0.x and 8.1.x, so it stays true until the release that actually ships this. I've updated the description to match.

dschmidt

LGTM, thanks for iterating on this :)

@butonic can we get your thoughts on this as well?
You somehow prefered the documentation approach if I understood you right

JammingBen requested review from aduffeck, dschmidt and fschade September 30, 2026 08:20

dschmidt reviewed Oct 1, 2026

View reviewed changes

dschmidt requested a review from butonic October 1, 2026 07:16

jmrplens added 3 commits October 3, 2026 09:44

fix(search): default the index endpoint to the configured gRPC address

57b2feb

--endpoint defaulted to a hardcoded 127.0.0.1:9220, so a search service moved with SEARCH_GRPC_ADDR could only be reached by passing the flag. Use the configured address when the flag is not set.

jmrplens added 3 commits October 3, 2026 15:58

jmrplens added 2 commits October 3, 2026 18:41

refactor(search): restore the endpointFlag name

a669437

The Flag suffix is the convention for the command's flag variables, not only for booleans.

docs(search): extend the --insecure note to 8.1.x [docs-only]

9cfb21a

8.1.0 ships before this fix, so it still needs the flag too.

dschmidt added the Type:Bug label Oct 3, 2026

dschmidt approved these changes Oct 3, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix(search): honor the gRPC client TLS mode in the index command - #3620

fix(search): honor the gRPC client TLS mode in the index command#3620
jmrplens wants to merge 10 commits into
opencloud-eu:mainfrom
jmrplens:fix-search-index-grpc-tls

jmrplens commented Sep 30, 2026 •

edited

Loading

Uh oh!

codacy-production Bot commented Sep 30, 2026 •

edited

Loading

Uh oh!

dschmidt left a comment

Uh oh!

dschmidt commented Oct 1, 2026

Uh oh!

jmrplens commented Oct 3, 2026

Uh oh!

dschmidt commented Oct 3, 2026

Uh oh!

jmrplens commented Oct 3, 2026

Uh oh!

dschmidt commented Oct 3, 2026

Uh oh!

jmrplens commented Oct 3, 2026

Uh oh!

dschmidt commented Oct 3, 2026

Uh oh!

jmrplens commented Oct 3, 2026

Uh oh!

dschmidt left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jmrplens commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

codacy-production Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Up to standards ✅

Uh oh!

dschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

dschmidt commented Oct 1, 2026

Uh oh!

jmrplens commented Oct 3, 2026

Uh oh!

dschmidt commented Oct 3, 2026

Uh oh!

jmrplens commented Oct 3, 2026

Uh oh!

dschmidt commented Oct 3, 2026

Uh oh!

jmrplens commented Oct 3, 2026

Uh oh!

dschmidt commented Oct 3, 2026

Uh oh!

jmrplens commented Oct 3, 2026

Uh oh!

dschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jmrplens commented Sep 30, 2026 •

edited

Loading

codacy-production Bot commented Sep 30, 2026 •

edited

Loading