[WIP] Add OpenTelemetry spans for secret key generation and retrieval operations #2869
Conversation
|
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
There was a problem hiding this comment.
Pull request overview
This work-in-progress pull request adds OpenTelemetry instrumentation to secret key generation and retrieval operations in the Common library's cryptographic components. The PR introduces new telemetry attributes and adds span creation around key cryptographic operations to improve observability.
Changes:
- Added 5 new telemetry attributes to track secret key serialization, key pair sources, device cryptographic capabilities, and failure history
- Instrumented
generateNewSecretKey()andreadSecretKeyFromStorage()with dedicated spans and status tracking - Refactored
wrapSecretKey()andunwrapSecretKey()to remove dedicated spans while retaining attribute reporting - Modified error history tracking from a list to a map structure to correlate failures with specific key generation specs
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
| AttributeName.java | Renamed key_pair_gen_encryptionPaddings to key_pair_gen_encryption_paddings for consistency; added 4 new attributes for tracking serializer IDs, key pair sources, and device capabilities |
| WrappedSecretKey.kt | Added telemetry attribute setting in serialize/deserialize methods to track serializer version IDs |
| KeyStoreBackedSecretKeyProvider.kt | Added span creation and lifecycle management for secret key generation/retrieval; refactored wrap/unwrap to remove dedicated spans; enhanced error tracking with spec-to-exception mapping |
Comments suppressed due to low confidence (1)
common4j/src/main/com/microsoft/identity/common/java/opentelemetry/AttributeName.java:557
- Each attribute added to AttributeName.java in the Common repo MUST also be defined in the AttributeName.java file in the broker repo (AzureAD/ad-accounts-for-android) for cross-repo consistency. The new attributes
wrapped_secret_key_serializer_id,key_pair_from_keystore,key_pair_supported_paddings, andavailable_cipher_specsneed to be added to the broker repo's AttributeName enum as well.
/**
* Indicates if the key pair was retrieved from keystore or generated anew.
*/
key_pair_from_keystore,
/**
* Indicates the supported paddings for key pair in the device.
*/
key_pair_supported_paddings,
/**
* Indicates the available cipher specs in the device.
*/
available_cipher_specs,
}
| private fun unwrapSecretKey( | ||
| wrappedSecretKey: WrappedSecretKey, | ||
| keyPair: KeyPair | ||
| ): SecretKey { | ||
| val methodTag = "$TAG:unwrapSecretKey" | ||
| val span = OTelUtility.createSpanFromParent( | ||
| SpanName.SecretKeyWrapping.name, | ||
| SpanExtension.current().spanContext | ||
| val cipherParamsSpec = getKeyPairCompatibleCipherSpecs(keyPair).firstOrNull { spec -> | ||
| spec.transformation.contains(wrappedSecretKey.cipherTransformation, ignoreCase = true) | ||
| } ?: throw ClientException( | ||
| ClientException.UNKNOWN_CRYPTO_ERROR, | ||
| "No compatible cipher specs found for key pair: $keyPair" | ||
| ) | ||
| SpanExtension.current().setAttribute( | ||
| AttributeName.secret_key_wrapping_transformation.name, | ||
| cipherParamsSpec.transformation | ||
| ) | ||
| Logger.info(methodTag, "Unwrapping secret key with cipher spec: $cipherParamsSpec") | ||
| return AndroidKeyStoreUtil.unwrap( | ||
| wrappedSecretKey.wrappedKeyData, | ||
| wrappedSecretKey.algorithm, | ||
| keyPair, | ||
| cipherParamsSpec.transformation, | ||
| cipherParamsSpec.algorithmParameterSpec | ||
| ) | ||
|
|
||
| return try { | ||
| SpanExtension.makeCurrentSpan(span).use { _ -> | ||
| span.setAttribute(AttributeName.secret_key_wrapping_operation.name, "UNWRAP") | ||
| val cipherParamsSpec = getKeyPairCompatibleCipherSpecs(keyPair).firstOrNull { spec -> | ||
| spec.transformation.contains(wrappedSecretKey.cipherTransformation, ignoreCase = true) | ||
| } ?: throw ClientException( | ||
| ClientException.UNKNOWN_CRYPTO_ERROR, | ||
| "No compatible cipher specs found for key pair: $keyPair" | ||
| ) | ||
|
|
||
| span.setAttribute( | ||
| AttributeName.secret_key_wrapping_transformation.name, | ||
| cipherParamsSpec.transformation | ||
| ) | ||
| Logger.info(methodTag, "Unwrapping secret key with cipher spec: $cipherParamsSpec") | ||
| val key = AndroidKeyStoreUtil.unwrap( | ||
| wrappedSecretKey.wrappedKeyData, | ||
| wrappedSecretKey.algorithm, | ||
| keyPair, | ||
| cipherParamsSpec.transformation, | ||
| cipherParamsSpec.algorithmParameterSpec | ||
| ) | ||
| span.setStatus(StatusCode.OK) | ||
| key | ||
| } | ||
| } catch (exception: Exception) { | ||
| Logger.error(methodTag, "Failed to wrap secret key", exception) | ||
| span.setStatus(StatusCode.ERROR) | ||
| span.recordException(exception) | ||
| throw exception | ||
| } finally { | ||
| span.end() | ||
| } | ||
| } |
There was a problem hiding this comment.
Span removed but operation still needs to be tracked. The previous implementation created a dedicated span for the unwrap operation with proper lifecycle management (status codes, exception recording, span.end()). The refactored code removes the span entirely, leaving only an attribute set on the current span. This reduces observability for this cryptographic operation. Consider either: (1) restoring a dedicated span for unwrap operations to maintain detailed tracing, or (2) adding status code management and exception handling to properly track this operation within the parent span context.
| SpanExtension.current().apply { | ||
| setAttribute( | ||
| AttributeName.key_pair_supported_paddings.name, | ||
| supportedPaddings.toString() | ||
| ) | ||
| setAttribute( | ||
| AttributeName.available_cipher_specs.name, | ||
| availableCipherSpecs.toString() | ||
| ) | ||
| } |
There was a problem hiding this comment.
High-cardinality sensitive data exposed in telemetry. The attributes key_pair_supported_paddings and available_cipher_specs are being set with full .toString() output of collections (lines 365, 369), which may contain unbounded, high-cardinality data that could overwhelm telemetry systems. Additionally, the full spec details could potentially expose device-specific cryptographic capabilities that should be privacy-classified. Consider: (1) limiting to count or a bounded summary (e.g., first 3 items), (2) hashing or bucketing the values, or (3) omitting these entirely and relying on lower-cardinality indicators like counts or boolean flags for specific capabilities.
| errorMessageBuilder.append("${spec.print()}: ${exception.message};") | ||
| } | ||
| SpanExtension.current().setAttribute( | ||
| AttributeName.keypair_gen_exception.name, |
There was a problem hiding this comment.
New telemetry attribute key_pair_gen_failure_history is never set. The attribute was added to the enum (line 363 in AttributeName.java) but the code uses keypair_gen_exception (line 525) instead. This appears to be a mismatch - either the new attribute name should be used here, or if keypair_gen_exception is the correct attribute, then key_pair_gen_failure_history is unused and should be removed from the enum to avoid confusion.
| AttributeName.keypair_gen_exception.name, | |
| AttributeName.key_pair_gen_failure_history.name, |
common/src/main/java/com/microsoft/identity/common/crypto/KeyStoreBackedSecretKeyProvider.kt
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/opentelemetry/AttributeName.java
Outdated
Show resolved
Hide resolved
| SpanExtension.current().apply { | ||
| setAttribute( | ||
| AttributeName.key_pair_supported_paddings.name, | ||
| supportedPaddings.toString() | ||
| ) | ||
| setAttribute( | ||
| AttributeName.available_cipher_specs.name, | ||
| availableCipherSpecs.toString() | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing test coverage for new telemetry attribute setting in getKeyPairCompatibleCipherSpecs(). The method now sets attributes key_pair_supported_paddings and available_cipher_specs (lines 362-371). Tests should verify that these attributes are correctly set on the current span with the expected values from the supportedPaddings and availableCipherSpecs collections.
| logAndBuildErrorHistory(failures) | ||
| span.setStatus(StatusCode.OK) | ||
| return@use keyPair |
There was a problem hiding this comment.
Attribute set after successful key pair generation should occur before the return statement. The call to logAndBuildErrorHistory(failures) on line 426 happens after the span status is set to OK (line 427) but this is backwards from the recommended pattern. The error history should be logged before setting the success status. Additionally, if there were no failures (failures map is empty), this will set an empty or trivial string as the attribute value. Consider moving the call before line 427 and only calling it if failures is not empty.
| private fun handleAllFailures(failures: Map<IKeyGenSpec, Throwable>): Nothing { | ||
| val methodTag = "$TAG:handleAllFailures" | ||
| require(failures.isNotEmpty()) { | ||
| "No failures encountered, but no key pair generated. This should not happen." | ||
| } | ||
| val errorMessages = failures.joinToString(separator = "; ") { exception -> | ||
| "${exception.javaClass.simpleName}: ${exception.message ?: "Unknown error"}" | ||
| } | ||
|
|
||
| failures.forEach { exception -> | ||
| Logger.error( | ||
| methodTag, | ||
| "Key pair generation failed with: ${exception.message}", | ||
| exception | ||
| ) | ||
| } | ||
| SpanExtension.current() | ||
| .setAttribute(AttributeName.keypair_gen_exception.name, errorMessages) | ||
|
|
||
| val finalError = failures.last() | ||
| val finalError = failures.values.last() | ||
| Logger.error(methodTag, | ||
| "All key generation attempts failed. Total failures: ${failures.size}", | ||
| finalError | ||
| ) | ||
| SpanExtension.current().setStatus(StatusCode.ERROR) | ||
| SpanExtension.current().recordException(finalError) | ||
| throw ExceptionAdapter.clientExceptionFromException(finalError) | ||
| } |
There was a problem hiding this comment.
The refactored error handling loses important diagnostic information. The original code logged each individual failure with its full exception (lines 503-508 in the removed code), which was valuable for debugging. The new implementation only logs a single summary message with the count of failures and records only the last exception to the span (lines 504-509). This means intermediate failures and their stack traces are lost. Consider restoring the individual error logging within logAndBuildErrorHistory to preserve diagnostic detail while still building the consolidated history string for telemetry.
| private fun logAndBuildErrorHistory(failures: Map<IKeyGenSpec, Throwable>) { | ||
| val errorMessageBuilder = StringBuilder() | ||
| for( (spec, exception) in failures) { | ||
| errorMessageBuilder.append("${spec.print()}: ${exception.message};") | ||
| } | ||
| SpanExtension.current().setAttribute( | ||
| AttributeName.keypair_gen_exception.name, | ||
| errorMessageBuilder.toString() | ||
| ) | ||
| } |
There was a problem hiding this comment.
Missing test coverage for new logAndBuildErrorHistory() method. This new method (lines 519-528) constructs an error history string from a map of failures and sets it as a telemetry attribute. Tests should verify: (1) correct formatting of the error history string with spec details and exception messages, (2) attribute is set on the current span, (3) behavior with empty map, single failure, and multiple failures.
| keyPair: KeyPair | ||
| ): WrappedSecretKey { | ||
| val methodTag = "$TAG:wrapSecretKey" | ||
| val span = OTelUtility.createSpanFromParent( | ||
| SpanName.SecretKeyWrapping.name, | ||
| SpanExtension.current().spanContext | ||
| val cipherParamsSpec = getKeyPairCompatibleCipherSpecs(keyPair).firstOrNull() | ||
| ?: throw ClientException( | ||
| ClientException.UNKNOWN_CRYPTO_ERROR, | ||
| "No compatible cipher specs found for key pair: $keyPair" | ||
| ) | ||
| SpanExtension.current().setAttribute( | ||
| AttributeName.secret_key_wrapping_transformation.name, //elected | ||
| cipherParamsSpec.transformation | ||
| ) | ||
| Logger.info(methodTag, "Wrapping secret key with cipher spec: $cipherParamsSpec") | ||
| val wrappedKey = AndroidKeyStoreUtil.wrap( | ||
| secretKey, | ||
| keyPair, | ||
| cipherParamsSpec.transformation, | ||
| cipherParamsSpec.algorithmParameterSpec | ||
| ) | ||
| return WrappedSecretKey( | ||
| wrappedKeyData = wrappedKey, | ||
| algorithm = secretKey.algorithm, | ||
| cipherTransformation = cipherParamsSpec.transformation | ||
| ) | ||
|
|
||
| return try { | ||
| SpanExtension.makeCurrentSpan(span).use { _ -> | ||
| span.setAttribute(AttributeName.secret_key_wrapping_operation.name, "WRAP") | ||
| val cipherParamsSpec = getKeyPairCompatibleCipherSpecs(keyPair).firstOrNull() | ||
| ?: throw ClientException( | ||
| ClientException.UNKNOWN_CRYPTO_ERROR, | ||
| "No compatible cipher specs found for key pair: $keyPair" | ||
| ) | ||
| span.setAttribute( | ||
| AttributeName.secret_key_wrapping_transformation.name, | ||
| cipherParamsSpec.transformation | ||
| ) | ||
| Logger.info(methodTag, "Wrapping secret key with cipher spec: $cipherParamsSpec") | ||
| val wrappedKey = AndroidKeyStoreUtil.wrap( | ||
| secretKey, | ||
| keyPair, | ||
| cipherParamsSpec.transformation, | ||
| cipherParamsSpec.algorithmParameterSpec | ||
| ) | ||
| span.setStatus(StatusCode.OK) | ||
| WrappedSecretKey( | ||
| wrappedKeyData = wrappedKey, | ||
| algorithm = secretKey.algorithm, | ||
| cipherTransformation = cipherParamsSpec.transformation | ||
| ) | ||
| } | ||
| } catch (exception: Exception) { | ||
| Logger.error(methodTag, "Failed to wrap secret key", exception) | ||
| span.setStatus(StatusCode.ERROR) | ||
| span.recordException(exception) | ||
| throw exception | ||
| } finally { | ||
| span.end() | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Span removed but operation still needs to be tracked. The previous implementation created a dedicated span for the wrap operation with proper lifecycle management (status codes, exception recording, span.end()). The refactored code removes the span entirely, leaving only an attribute set on the current span. This reduces observability for this cryptographic operation. Consider either: (1) restoring a dedicated span for wrap operations to maintain detailed tracing, or (2) adding status code management and exception handling to properly track this operation within the parent span context.
…izer_id for consistency in telemetry attributes
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.