fix(auth): keep token endpoint on target scheme for insecure targets#103
Merged
Merged
Conversation
leoparente
requested review from
MicahParks,
davidlanouette,
jajeffries,
ltucker,
marc-barry and
mfiedorowicz
as code owners
July 21, 2026 16:41
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||
leoparente
marked this pull request as draft
July 21, 2026 16:45
Contributor
Author
|
@codex review |
davidlanouette
approved these changes
Jul 21, 2026
davidlanouette
left a comment
davidlanouette
left a comment
Contributor
There was a problem hiding this comment.
2 questions, but generally looks good.
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The auth token URL scheme was derived from tls_verify plus DIODE_SKIP_TLS_VERIFY, which forced HTTPS for an insecure grpc:// (or http://) target whenever DIODE_SKIP_TLS_VERIFY was set, producing an SSL WRONG_VERSION_NUMBER against a plain-HTTP server. Track the target's secure/insecure scheme separately and build the auth URL from it. DIODE_SKIP_TLS_VERIFY now only controls certificate verification, never http vs https. Fixes #101 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
leoparente
force-pushed
the
fix/auth-endpoint-scheme-insecure-target
branch
from
July 21, 2026 16:56
cb6a89b to
d549c0f
Compare
leoparente
marked this pull request as ready for review
July 21, 2026 18:03
|
🎉 This PR is included in version 1.13.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #101
Problem
With an insecure
grpc://(orhttp://) target, settingDIODE_SKIP_TLS_VERIFYforced the OAuth token request onto
https://, causingSSL: WRONG_VERSION_NUMBERagainst a plain-HTTP server.
Cause
_get_full_auth_url()chose the scheme fromtls_verify or skip_tls_env. That heuristicwas added (#76) to keep a
grpcs://target on HTTPS when verification is disabled, becauseparse_target()had already reduced the scheme to a booleantls_verify(#62 foldedDIODE_SKIP_TLS_VERIFYinto it). Butskip_tls_envis only a proxy for "scheme was https",so an insecure
grpc://target plus the env var was wrongly bumped to HTTPS.Fix
Track the target's secure/insecure scheme separately (
_secure) and build the auth URL fromit.
DIODE_SKIP_TLS_VERIFYnow only disables certificate verification, never picks the scheme.This preserves the
grpcs://+ skip-verify case the old heuristic was protecting.DIODE_SKIP_TLS_VERIFYgrpc://grpc://grpcs://grpcs://Behavior change
grpc:///http://targets withDIODE_SKIP_TLS_VERIFYset now emit anhttp://auth URLinstead of
https://.DIODE_SKIP_TLS_VERIFYis now strictly a certificate-verificationcontrol, not a scheme selector.
Tests
Strengthened
test_auth_url_scheme_follows_targetto parametrizesecureandtls_verifyindependently (mutation-verified: a
scheme = self._tls_verifyregression fails the new rows,including
grpcs://+ skip). Added a client-level test assertingDiodeClient._securefollowsthe target scheme even with the env var set. Full suite: 127 passed.
Note:
skip_tls_verify=Truewas never aDiodeClientargument, so thatTypeErrorfrom thereport is expected.
Out of scope (separate follow-up)
Pre-existing and untouched here: for
grpcs://+DIODE_SKIP_TLS_VERIFY, the gRPC ingest/OTLPdata channels key off the same
tls_verifyboolean and build a plaintextinsecure_channelagainst a TLS server, so skip-verify downgrades the transport instead of just relaxing cert
verification. Worth a separate issue.