Open
Conversation
📝 WalkthroughWalkthroughAdds Azure Arc Managed Identity support: a new MSI credential module implementing a two-step token flow, integration into the Geneva config client (new auth variant and token plumbing), a small dependency addition, and an end-to-end example demonstrating Arc MSI logging. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Client as GenevaConfigClient
participant ArcCred as AzureArcMSI_Credential
participant Endpoint as Arc MSI Endpoint
participant FS as FileSystem
App->>Client: request ingestion token(resource)
Client->>ArcCred: get_token_impl([resource/.default])
ArcCred->>Endpoint: GET /token?api-version=...&resource=<resource>
Endpoint-->>ArcCred: 401 + www-authenticate: Basic realm="<secret_path>"
ArcCred->>FS: read(<secret_path>)
FS-->>ArcCred: secret_content
ArcCred->>Endpoint: GET /token + Authorization: Basic <secret>
Endpoint-->>ArcCred: 200 {access_token, expires_on}
ArcCred-->>Client: AccessToken(token, expiry)
Client-->>App: token_string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@opentelemetry-exporter-geneva/geneva-uploader/src/config_service/azure_arc_msi.rs`:
- Around line 123-233: In get_token_impl, avoid allocating the whole secret
before size-check: call fs::metadata or otherwise stat the secret_path to get
file size and validate it against ARC_MAX_FILE_SIZE (and error with the same
ErrorKind::Credential message) before calling fs::read; alternatively open the
file and read with a size cap (e.g., read_exact into a Vec with a pre-checked
length or read up to ARC_MAX_FILE_SIZE+1 bytes to detect oversize) and then
proceed to use the secret for the second request (references: get_token_impl,
secret_path, fs::read, ARC_MAX_FILE_SIZE).
🧹 Nitpick comments (4)
opentelemetry-exporter-geneva/opentelemetry-exporter-geneva/examples/azure_arc_logging.rs (1)
63-69: Explicitly flush/shutdown the logger provider before exit.
Dropping the provider may not guarantee the last batch is exported. Consider using the SDK’s shutdown/force-flush API to avoid losing logs.♻️ Suggested change
- drop(logger_provider); + logger_provider.shutdown()?;opentelemetry-exporter-geneva/geneva-uploader/src/config_service/azure_arc_msi.rs (1)
271-317: PreferPathcomparison (or canonicalization) over string equality for directory checks.
String comparisons can mis-handle trailing separators or Windows path normalization. APathcomparison is more robust.♻️ Suggested change
- if parent.to_string_lossy() != expected_dir { + if parent != Path::new(expected_dir) {opentelemetry-exporter-geneva/geneva-uploader/src/config_service/client.rs (2)
306-318: Use a more accurate error variant for Arc MSI failures.
AuthMethodNotImplementedsuggests the feature is missing, but these are runtime init/token errors; this can mislead callers that match on error kinds.♻️ Proposed change
- AzureArcManagedIdentityCredential::new(azure_http_client) - .map_err(|e| GenevaConfigClientError::AuthMethodNotImplemented( - format!("Failed to initialize Azure Arc MSI credential: {}", e) - ))? + AzureArcManagedIdentityCredential::new(azure_http_client) + .map_err(|e| GenevaConfigClientError::InternalError( + format!("Failed to initialize Azure Arc MSI credential: {}", e) + ))?- let token = credential.get_token(&[&scope], None) - .await - .map_err(|e| GenevaConfigClientError::AuthMethodNotImplemented( - format!("Failed to get Azure Arc MSI token: {}", e) - ))?; + let token = credential.get_token(&[&scope], None) + .await + .map_err(|e| GenevaConfigClientError::InternalError( + format!("Failed to get Azure Arc MSI token: {}", e) + ))?;- None => Err(GenevaConfigClientError::AuthMethodNotImplemented( - "Azure Arc MSI credential not initialized".into() - )), + None => Err(GenevaConfigClientError::InternalError( + "Azure Arc MSI credential not initialized".into() + )),Also applies to: 453-475
492-499: Verify the AAD scope used for Geneva Config Service.
The hardcodedhttps://management.azure.comscope may not be correct for all environments; if it differs, requests will fail. Consider making it configurable or at least a named constant with docs.
opentelemetry-exporter-geneva/geneva-uploader/src/config_service/azure_arc_msi.rs
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| fn print_configuration(config: &ExampleConfig) { | ||
| println!(" 📍 Endpoint: {}", config.geneva_endpoint); | ||
| println!(" 🌍 Environment: {}", config.geneva_environment); | ||
| println!(" 👤 Account: {}", config.geneva_account); |
Check failure
Code scanning / CodeQL
Cleartext logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, to fix cleartext logging issues, avoid logging raw sensitive values. If you need context in logs, log non-sensitive metadata (e.g., that a value was present, or a hashed/redacted form) instead of the full value.
For this specific case, the best minimal fix is to stop printing the exact geneva_account string in print_configuration while preserving the rest of the behavior. We can either remove that line or replace it with a redacted message that indicates whether an account is configured without exposing the value. No other parts of the snippet log geneva_account, and there is no need for new imports or helper functions.
Concretely:
- In
opentelemetry-exporter-geneva/opentelemetry-exporter-geneva/examples/azure_arc_logging.rs, infn print_configuration(config: &ExampleConfig), change line 112 from:println!(" 👤 Account: {}", config.geneva_account);
to either remove it or, better, print a redacted indicator such as:println!(" 👤 Account: [redacted]");
or a length-based message if you prefer. This prevents the actual account identifier from being logged but keeps the configuration printout semantics for debugging.
No additional methods, imports, or definitions are required.
Suggested changeset
1
opentelemetry-exporter-geneva/opentelemetry-exporter-geneva/examples/azure_arc_logging.rs
| @@ -109,7 +109,7 @@ | ||
| fn print_configuration(config: &ExampleConfig) { | ||
| println!(" 📍 Endpoint: {}", config.geneva_endpoint); | ||
| println!(" 🌍 Environment: {}", config.geneva_environment); | ||
| println!(" 👤 Account: {}", config.geneva_account); | ||
| println!(" 👤 Account: [redacted]"); | ||
| println!(" 📦 Namespace: {}", config.geneva_namespace); | ||
| println!(" 🗺️ Region: {}", config.geneva_region); | ||
| println!(" 📊 Config Version: {}", config.geneva_config_version); |
Copilot is powered by AI and may make mistakes. Always verify output.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@opentelemetry-exporter-geneva/geneva-uploader/src/config_service/azure_arc_msi.rs`:
- Around line 404-504: Tests mutate the ARC_TOKENS_DIR_OVERRIDE env var in
test_arc_challenge_flow and test_validate_arc_secret_path which can race when
tests run in parallel; serialize these mutations by creating a static Mutex
(e.g., via once_cell::sync::Lazy or lazy_static) and acquire the lock at the
start of each test before calling std::env::set_var / remove_var and release
after cleanup so only one test changes the env at a time, preserving current
behavior of using ARC_TOKENS_DIR_OVERRIDE.
In `@opentelemetry-exporter-geneva/geneva-uploader/src/config_service/client.rs`:
- Around line 143-144: The doc comment for `auth_method` is too narrow (mentions
only Certificate/AzureArcManagedIdentity); update it to a general description
that reflects all supported variants of the authentication enum (referencing
`AuthMethod` or the `auth_method` field) — e.g., describe that it selects the
authentication mechanism used by the client and list or mention the enum
variants supported rather than only Certificate and AzureArcManagedIdentity so
the comment stays accurate as variants change.
🧹 Nitpick comments (2)
opentelemetry-exporter-geneva/geneva-uploader/src/config_service/azure_arc_msi.rs (1)
281-327: Harden secret‑path validation against symlink/relative escapes.
Line 309 currently relies on parent string checks; a symlink inside the tokens dir or a relative path can bypass intent. Consider absolute‑path + symlink checks and canonicalize‑based containment.🛡️ Proposed hardening
fn validate_arc_secret_path(path_str: &str, expected_dir: &str) -> Result<()> { let path = Path::new(path_str); + if !path.is_absolute() { + return Err(Error::message( + ErrorKind::Credential, + "Azure Arc secret file path must be absolute", + )); + } + if path.extension().and_then(|e| e.to_str()) != Some(&ARC_FILE_EXTENSION[1..]) { return Err(Error::message( ErrorKind::Credential, format!( "Azure Arc secret file must have {} extension", ARC_FILE_EXTENSION ), )); } if !path.exists() { return Err(Error::message( ErrorKind::Credential, "Azure Arc secret file does not exist", )); } + + let meta = fs::symlink_metadata(path).with_context(ErrorKind::Credential, || { + format!("Azure Arc failed to stat secret file '{}'", path.display()) + })?; + if meta.file_type().is_symlink() { + return Err(Error::message( + ErrorKind::Credential, + "Azure Arc secret file must not be a symlink", + )); + } // Directory validation (best-effort) - if let Some(parent) = path.parent() { - if parent.to_string_lossy() != expected_dir { - // Soft validation: don't fail outright if override var provided - if std::env::var(ARC_TOKENS_DIR_OVERRIDE).is_err() { - return Err(Error::message( - ErrorKind::Credential, - format!( - "Azure Arc secret file directory mismatch (expected {}, got {})", - expected_dir, - parent.to_string_lossy() - ), - )); - } - } - } + if std::env::var(ARC_TOKENS_DIR_OVERRIDE).is_err() { + let canonical = fs::canonicalize(path).with_context(ErrorKind::Credential, || { + format!("Azure Arc failed to canonicalize secret path '{}'", path.display()) + })?; + let expected = fs::canonicalize(expected_dir).with_context(ErrorKind::Credential, || { + format!("Azure Arc failed to canonicalize tokens dir '{}'", expected_dir) + })?; + if !canonical.starts_with(&expected) { + return Err(Error::message( + ErrorKind::Credential, + format!( + "Azure Arc secret file directory mismatch (expected {}, got {})", + expected.display(), + canonical.display() + ), + )); + } + } Ok(()) }opentelemetry-exporter-geneva/geneva-uploader/src/config_service/client.rs (1)
775-797: Make Arc MSI resource configurable (avoid hard‑coded management endpoint).
Line 835 hard‑codes the resource; if Geneva expects a different AAD resource, auth will fail. Consider usingconfig.msi_resource(or a dedicated Arc resource field) with a safe default, and verify the correct resource with service docs.♻️ Suggested change
AuthMethod::AzureArcManagedIdentity => { - let token = self - .get_azure_arc_token("https://management.azure.com") - .await?; + let resource = self + .config + .msi_resource + .as_deref() + .unwrap_or("https://management.azure.com"); + let token = self.get_azure_arc_token(resource).await?; request = request.header(AUTHORIZATION, format!("Bearer {}", token)); }Also applies to: 834-839
Comment on lines
+404
to
+504
| #[tokio::test] | ||
| async fn test_arc_challenge_flow() { | ||
| // Create temp directory and secret file | ||
| let base_dir = { | ||
| let p = std::env::temp_dir().join(format!( | ||
| "arc_test_{}", | ||
| SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_nanos() | ||
| )); | ||
| std::fs::create_dir(&p).unwrap(); | ||
| p | ||
| }; | ||
| let secret_path = base_dir.join("secret.key"); | ||
| let mut f = std::fs::File::create(&secret_path).unwrap(); | ||
| write!(f, "arc-secret").unwrap(); | ||
|
|
||
| // Set environment variable for token directory override | ||
| unsafe { std::env::set_var(ARC_TOKENS_DIR_OVERRIDE, base_dir.to_string_lossy().as_ref()) }; | ||
|
|
||
| let expires_on = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_secs() | ||
| + 3600; | ||
|
|
||
| // First response: 401 with challenge | ||
| let mut h1 = Headers::default(); | ||
| let header_value = format!("Basic realm={}", secret_path.to_string_lossy()); | ||
| let header_value_static: &'static str = Box::leak(header_value.into_boxed_str()); | ||
| h1.insert( | ||
| HeaderName::from_static(HEADER_WWW_AUTHENTICATE), | ||
| header_value_static, | ||
| ); | ||
| let resp1 = RawResponse::from_bytes(StatusCode::Unauthorized, h1, Bytes::from_static(b"")); | ||
|
|
||
| // Second response: 200 with token | ||
| let body2 = format!( | ||
| r#"{{"access_token":"test_token","expires_on":"{}","token_type":"Bearer","resource":"https://management.azure.com"}}"#, | ||
| expires_on | ||
| ); | ||
| let resp2 = RawResponse::from_bytes(StatusCode::Ok, Headers::default(), Bytes::from(body2)); | ||
|
|
||
| let mock = MockHttpClient::new(vec![resp1, resp2]); | ||
|
|
||
| // Test the credential | ||
| let credential = AzureArcManagedIdentityCredential::new(mock.clone()).unwrap(); | ||
| let token = credential | ||
| .get_token(&["https://management.azure.com/.default"], None) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(token.token.secret(), "test_token"); | ||
| assert_eq!(mock.call_count(), 2); | ||
|
|
||
| // Cleanup | ||
| unsafe { std::env::remove_var(ARC_TOKENS_DIR_OVERRIDE) }; | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_basic_realm_path() { | ||
| let header = r#"Basic realm="/var/opt/azcmagent/tokens/secret.key""#; | ||
| let path = parse_basic_realm_path(header).unwrap(); | ||
| assert_eq!(path, "/var/opt/azcmagent/tokens/secret.key"); | ||
|
|
||
| let header2 = "Basic realm=/path/without/quotes.key"; | ||
| let path2 = parse_basic_realm_path(header2).unwrap(); | ||
| assert_eq!(path2, "/path/without/quotes.key"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_arc_secret_path() { | ||
| // Create temp file for testing | ||
| let base_dir = std::env::temp_dir().join("arc_test_validation"); | ||
| std::fs::create_dir_all(&base_dir).unwrap(); | ||
| let secret_path = base_dir.join("test.key"); | ||
| std::fs::File::create(&secret_path).unwrap(); | ||
|
|
||
| // Set override to avoid directory mismatch | ||
| unsafe { std::env::set_var(ARC_TOKENS_DIR_OVERRIDE, base_dir.to_string_lossy().as_ref()) }; | ||
|
|
||
| // Valid path should succeed | ||
| let result = | ||
| validate_arc_secret_path(&secret_path.to_string_lossy(), &base_dir.to_string_lossy()); | ||
| assert!(result.is_ok()); | ||
|
|
||
| // Invalid extension should fail | ||
| let bad_path = base_dir.join("test.txt"); | ||
| std::fs::File::create(&bad_path).unwrap(); | ||
| let result = | ||
| validate_arc_secret_path(&bad_path.to_string_lossy(), &base_dir.to_string_lossy()); | ||
| assert!(result.is_err()); | ||
| assert!(result | ||
| .unwrap_err() | ||
| .to_string() | ||
| .contains("must have .key extension")); | ||
|
|
||
| // Cleanup | ||
| unsafe { std::env::remove_var(ARC_TOKENS_DIR_OVERRIDE) }; | ||
| } |
There was a problem hiding this comment.
Serialize env‑var mutations to avoid flaky tests.
Line 423 and Line 484 mutate global env vars; tests run in parallel by default. Guard with a static mutex to prevent interference.
🔒 Proposed test isolation
use std::{
io::Write,
sync::{
atomic::{AtomicUsize, Ordering},
- Mutex,
+ Mutex,
+ OnceLock,
},
time::{SystemTime, UNIX_EPOCH},
};
+
+ static ENV_LOCK: OnceLock<Mutex<()>> = OnceLock::new();
+ fn env_lock() -> std::sync::MutexGuard<'static, ()> {
+ ENV_LOCK.get_or_init(|| Mutex::new(())).lock().unwrap()
+ }
#[tokio::test]
async fn test_arc_challenge_flow() {
// Create temp directory and secret file
let base_dir = {
@@
- // Set environment variable for token directory override
+ // Set environment variable for token directory override
+ let _env_guard = env_lock();
unsafe { std::env::set_var(ARC_TOKENS_DIR_OVERRIDE, base_dir.to_string_lossy().as_ref()) };
@@
- // Cleanup
+ // Cleanup
unsafe { std::env::remove_var(ARC_TOKENS_DIR_OVERRIDE) };
}
@@
fn test_validate_arc_secret_path() {
// Create temp file for testing
let base_dir = std::env::temp_dir().join("arc_test_validation");
std::fs::create_dir_all(&base_dir).unwrap();
let secret_path = base_dir.join("test.key");
std::fs::File::create(&secret_path).unwrap();
// Set override to avoid directory mismatch
+ let _env_guard = env_lock();
unsafe { std::env::set_var(ARC_TOKENS_DIR_OVERRIDE, base_dir.to_string_lossy().as_ref()) };
@@
// Cleanup
unsafe { std::env::remove_var(ARC_TOKENS_DIR_OVERRIDE) };
}🤖 Prompt for AI Agents
In
`@opentelemetry-exporter-geneva/geneva-uploader/src/config_service/azure_arc_msi.rs`
around lines 404 - 504, Tests mutate the ARC_TOKENS_DIR_OVERRIDE env var in
test_arc_challenge_flow and test_validate_arc_secret_path which can race when
tests run in parallel; serialize these mutations by creating a static Mutex
(e.g., via once_cell::sync::Lazy or lazy_static) and acquire the lock at the
start of each test before calling std::env::set_var / remove_var and release
after cleanup so only one test changes the env at a time, preserving current
behavior of using ARC_TOKENS_DIR_OVERRIDE.
Comment on lines
+143
to
144
| /// * `auth_method` - Authentication method to use (Certificate or AzureArcManagedIdentity) | ||
| /// |
There was a problem hiding this comment.
Auth‑method doc comment is now too narrow.
Line 143 suggests only Certificate/AzureArcManagedIdentity, but the enum supports more variants. Consider a more general description.
✏️ Suggested doc tweak
-/// * `auth_method` - Authentication method to use (Certificate or AzureArcManagedIdentity)
+/// * `auth_method` - Authentication method to use (see `AuthMethod` variants)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// * `auth_method` - Authentication method to use (Certificate or AzureArcManagedIdentity) | |
| /// | |
| /// * `auth_method` - Authentication method to use (see `AuthMethod` variants) | |
| /// |
🤖 Prompt for AI Agents
In `@opentelemetry-exporter-geneva/geneva-uploader/src/config_service/client.rs`
around lines 143 - 144, The doc comment for `auth_method` is too narrow
(mentions only Certificate/AzureArcManagedIdentity); update it to a general
description that reflects all supported variants of the authentication enum
(referencing `AuthMethod` or the `auth_method` field) — e.g., describe that it
selects the authentication mechanism used by the client and list or mention the
enum variants supported rather than only Certificate and AzureArcManagedIdentity
so the comment stays accurate as variants change.
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
1 participant
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 #
Design discussion issue (if applicable) #
Changes
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changesSummary by CodeRabbit
New Features
Documentation
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.