feat: add spire support to test apps - #1939
Merged
Merged
Conversation
Signed-off-by: Michele Papalini <micpapal@cisco.com>
📝 WalkthroughWalkthroughReceiver and sender test apps now support SPIRE/SPIFFE authentication as an alternative to shared secret authentication, selectable via a CLI argument. Control-plane/client TLS validation is now chosen based on the endpoint scheme (https vs http). Sender app's participant invite operation is wrapped with a 35-second timeout. ChangesSPIRE Authentication Support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant App as Receiver/Sender App
participant Args as CLI Args
participant SpireConfig
participant AuthProvider
participant ControlPlane
App->>Args: parse auth_method
alt auth_method == shared_secret
Args->>AuthProvider: shared_secret_from_str
else auth_method == spire
Args->>SpireConfig: build socket_path, audience, target
SpireConfig->>AuthProvider: AuthProvider::spire / AuthVerifier::spire
else unsupported
Args->>App: return error
end
App->>ControlPlane: connect with TLS (default if https, insecure if http)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
micpapal
marked this pull request as ready for review
August 4, 2026 14:36
Tehsmash
approved these changes
Aug 4, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/testing/src/bin/receiver_app.rs`:
- Around line 121-127: Use case-insensitive HTTPS scheme detection when
selecting TLS configuration in the client configuration setup at
crates/testing/src/bin/receiver_app.rs lines 121-127 and
crates/testing/src/bin/sender_app.rs lines 180-186. Normalize the URI scheme
before checking for HTTPS so mixed-case https prefixes select
TlsClientConfig::default(), while non-HTTPS endpoints continue using
TlsClientConfig::insecure().
In `@crates/testing/src/bin/sender_app.rs`:
- Around line 304-313: Update the invite flow around
controller.invite_participant so the asynchronous controller-channel send and
its completion are both executed inside the 35-second tokio::time::timeout async
block. Preserve the existing “invite failed,” timeout, and completion-failed
context messages while ensuring timeout measurement starts before either await.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7507f3d0-473a-4f27-8a9a-bcd796dbdffa
📒 Files selected for processing (2)
crates/testing/src/bin/receiver_app.rscrates/testing/src/bin/sender_app.rs
Comment on lines
+121
to
+127
| let client_config = ClientConfig::with_endpoint(&args.slim).with_tls_setting( | ||
| if args.slim.starts_with("https://") { | ||
| TlsClientConfig::default() | ||
| } else { | ||
| TlsClientConfig::insecure() | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Trace endpoint parsing and TLS configuration into the connection implementation.
ast-grep outline crates/config/src/client.rs --items all
ast-grep outline crates --items all --type function --match 'connect'
rg -n -C 8 --type rust \
'async\s+fn\s+connect|fn\s+connect|Endpoint::from_shared|Uri::from_str|Url::parse|scheme_str|TlsClientConfig' \
cratesRepository: agntcy/slim
Length of output: 50368
Security Misconfiguration (CWE-295): Improper Certificate Validation
Reachability: External
Use case-insensitive scheme matching when selecting TLS configuration.
Both sites use a case-sensitive starts_with("https://") check. A mixed-case HTTPS URI falls through to TlsClientConfig::insecure(), disabling certificate validation.
Apply normalized scheme logic in:
crates/testing/src/bin/receiver_app.rs#L121-L127crates/testing/src/bin/sender_app.rs#L180-L186
📍 Affects 2 files
crates/testing/src/bin/receiver_app.rs#L121-L127(this comment)crates/testing/src/bin/sender_app.rs#L180-L186
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/testing/src/bin/receiver_app.rs` around lines 121 - 127, Use
case-insensitive HTTPS scheme detection when selecting TLS configuration in the
client configuration setup at crates/testing/src/bin/receiver_app.rs lines
121-127 and crates/testing/src/bin/sender_app.rs lines 180-186. Normalize the
URI scheme before checking for HTTPS so mixed-case https prefixes select
TlsClientConfig::default(), while non-HTTPS endpoints continue using
TlsClientConfig::insecure().
Comment on lines
+304
to
+313
| tokio::time::timeout( | ||
| Duration::from_secs(35), | ||
| controller | ||
| .invite_participant(participant) | ||
| .await | ||
| .context("invite failed")?, | ||
| ) | ||
| .await | ||
| .context("invite timed out after 35s")? | ||
| .context("invite completion failed")?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect whether the initial invite future can wait on I/O or a channel.
ast-grep outline crates --items all --type function --match 'invite_participant'
rg -n -C 12 --type rust \
'async\s+fn\s+invite_participant|fn\s+invite_participant|\binvite_participant\s*\(' \
cratesRepository: agntcy/slim
Length of output: 50367
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- invite implementation ---'
sed -n '780,830p' crates/session/src/session_controller.rs
printf '%s\n' '--- completion handle definition and publish path ---'
rg -n -C 10 'type CompletionHandle|struct CompletionHandle|pub type CompletionHandle|async fn publish_message|fn publish_message' crates/session crates
printf '%s\n' '--- sender imports and target block ---'
sed -n '1,45p' crates/testing/src/bin/sender_app.rs
sed -n '296,318p' crates/testing/src/bin/sender_app.rs
printf '%s\n' '--- analogous timeout usage ---'
sed -n '1378,1398p' crates/slimctl/src/commands/bench.rsRepository: agntcy/slim
Length of output: 12890
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- app-to-session publish path ---'
rg -n -C 18 'async fn on_message_from_app|fn on_message_from_app|on_message_from_app\s*\(' crates/session/src crates
printf '%s\n' '--- completion future implementation ---'
sed -n '1,180p' crates/session/src/completion_handle.rs
printf '%s\n' '--- relevant Cargo/toolchain metadata ---'
rg -n 'rust-version|edition' Cargo.toml crates/session/Cargo.toml crates/testing/Cargo.tomlRepository: agntcy/slim
Length of output: 33584
Wrap both invite awaits in the 35-second timeout. invite_participant awaits an asynchronous controller-channel send, so it can block before the current timeout starts. Place both awaits inside the timed async block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/testing/src/bin/sender_app.rs` around lines 304 - 313, Update the
invite flow around controller.invite_participant so the asynchronous
controller-channel send and its completion are both executed inside the
35-second tokio::time::timeout async block. Preserve the existing “invite
failed,” timeout, and completion-failed context messages while ensuring timeout
measurement starts before either await.
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
3 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.
Description
add spire authentication to test applications
Type of Change
Checklist
Summary by CodeRabbit