Merge pull request #1127 from SteveL-MSFT/secure-before

Fix how `secureString` and `secureObject` is propagated

Author: SteveL-MSFT

dsc/examples/secure_parameters.dsc.yaml

@@ -4,12 +4,27 @@ parameters:
     type: secureString
   myObject:
     type: secureObject
+  showSecrets:
+    type: bool
+    defaultValue: false
 resources:
-  - name: Echo 1
+  - name: SecureString
     type: Microsoft.DSC.Debug/Echo
     properties:
       output: "[parameters('myString')]"
-  - name: Echo 2
+      showSecrets: "[parameters('showSecrets')]"
+  - name: SecureObject
     type: Microsoft.DSC.Debug/Echo
     properties:
       output: "[parameters('myObject').myProperty]"
+      showSecrets: "[parameters('showSecrets')]"
+  - name: SecureArray
+    type: Microsoft.DSC.Debug/Echo
+    properties:
+      output: "[parameters('myObject').myArray]"
+      showSecrets: "[parameters('showSecrets')]"
+  - name: SecureArrayIndexed
+    type: Microsoft.DSC.Debug/Echo
+    properties:
+      output: "[parameters('myObject').myArray[1]]"
+      showSecrets: "[parameters('showSecrets')]"

dsc/examples/secure_parameters.parameters.yaml

@@ -2,3 +2,6 @@ parameters:
   myString: mySecret
   myObject:
     myProperty: mySecretProperty
+    myArray:
+      - item1
+      - item2

dsc/examples/secure_parameters_shown.parameters.yaml

@@ -0,0 +1,8 @@
+parameters:
+  myString: mySecret
+  myObject:
+    myProperty: mySecretProperty
+    myArray:
+      - item1
+      - item2
+  showSecrets: true

dsc/tests/dsc_extension_secret.tests.ps1

@@ -144,11 +144,12 @@ Describe 'Tests for the secret() function and extensions' {
             type: Microsoft.DSC.Debug/Echo
             properties:
               output: "[parameters('myString')]"
+              showSecrets: true
 '@
       $out = dsc -l trace config get -i $configYaml 2> $TestDrive/error.log | ConvertFrom-Json
       $LASTEXITCODE | Should -Be 0
       $out.results.Count | Should -Be 1
-      $out.results[0].result.actualState.Output | Should -BeExactly 'Hello'
+      $out.results[0].result.actualState.Output.secureString | Should -BeExactly 'Hello'
     }
 
     It 'Allows to pass in secret() through variables' {

dsc/tests/dsc_parameters.tests.ps1

@@ -274,11 +274,61 @@ Describe 'Parameters tests' {
       $out.results[0].result.inDesiredState | Should -BeTrue
     }
 
-    It 'secure types can be passed as objects to resources' {
-      $out = dsc config -f $PSScriptRoot/../examples/secure_parameters.parameters.yaml get -f $PSScriptRoot/../examples/secure_parameters.dsc.yaml | ConvertFrom-Json
+    It 'secure types can be passed as objects to resources but redacted in output: <operation> <property>' -TestCases @(
+        @{ operation = 'get'; property = 'actualState' }
+        @{ operation = 'set'; property = 'beforeState' }
+        @{ operation = 'set'; property = 'afterState' }
+        @{ operation = 'test'; property = 'desiredState' }
+        @{ operation = 'test'; property = 'actualState' }
+        @{ operation = 'export'; property = $null }
+    ) {
+      param($operation, $property)
+
+      $out = dsc -l trace config -f $PSScriptRoot/../examples/secure_parameters.parameters.yaml $operation -f $PSScriptRoot/../examples/secure_parameters.dsc.yaml 2> $TestDrive/error.log | ConvertFrom-Json
       $LASTEXITCODE | Should -Be 0
-      $out.results[0].result.actualState.output | Should -BeExactly 'mySecret'
-      $out.results[1].result.actualState.output | Should -BeExactly 'mySecretProperty'
+      if ($operation -eq 'export') {
+        $out.resources.Count | Should -Be 4
+        $out.resources[0].properties.output | Should -BeExactly '<secureValue>'
+        $out.resources[1].properties.output | Should -BeExactly '<secureValue>'
+        $out.resources[2].properties.output[0] | Should -BeExactly '<secureValue>'
+        $out.resources[2].properties.output[1] | Should -BeExactly '<secureValue>'
+        $out.resources[3].properties.output | Should -BeExactly '<secureValue>'
+      } else {
+        $out.results.Count | Should -Be 4 -Because ($out | ConvertTo-Json -Dep 10 | Out-String)
+        $out.results[0].result.$property.output | Should -BeExactly '<secureValue>' -Because ($out | ConvertTo-Json -Dep 10 | Out-String)
+        $out.results[1].result.$property.output | Should -BeExactly '<secureValue>'
+        $out.results[2].result.$property.output[0] | Should -BeExactly '<secureValue>'
+        $out.results[2].result.$property.output[1] | Should -BeExactly '<secureValue>'
+        $out.results[3].result.$property.output | Should -BeExactly '<secureValue>'
+      }
+    }
+
+    It 'secure types can be passed as objects to resources: <operation> <property>' -TestCases @(
+        # `set` beforeState is redacted in output, `test` desiredState is redacted in output so those test cases are not included here
+        @{ operation = 'get'; property = 'actualState' }
+        @{ operation = 'set'; property = 'afterState' }
+        @{ operation = 'test'; property = 'actualState' }
+        @{ operation = 'export'; property = $null }
+    ) {
+      param($operation, $property)
+
+      $out = dsc config -f $PSScriptRoot/../examples/secure_parameters_shown.parameters.yaml $operation -f $PSScriptRoot/../examples/secure_parameters.dsc.yaml | ConvertFrom-Json
+      $LASTEXITCODE | Should -Be 0
+      if ($operation -eq 'export') {
+        $out.resources.Count | Should -Be 4 -Because ($out | ConvertTo-Json -Dep 10 | Out-String)
+        $out.resources[0].properties.output.secureString | Should -BeExactly 'mySecret'
+        $out.resources[1].properties.output.secureString | Should -BeExactly 'mySecretProperty'
+        $out.resources[2].properties.output[0].secureString | Should -BeExactly 'item1'
+        $out.resources[2].properties.output[1].secureString | Should -BeExactly 'item2'
+        $out.resources[3].properties.output.secureObject.secureString | Should -BeExactly 'item2'
+      } else {
+        $out.results.Count | Should -Be 4
+        $out.results[0].result.$property.output.secureString | Should -BeExactly 'mySecret' -Because ($out | ConvertTo-Json -Dep 10 | Out-String)
+        $out.results[1].result.$property.output.secureString | Should -BeExactly 'mySecretProperty'
+        $out.results[2].result.$property.output[0].secureString | Should -BeExactly 'item1'
+        $out.results[2].result.$property.output[1].secureString | Should -BeExactly 'item2'
+        $out.results[3].result.$property.output.secureObject.secureString | Should -BeExactly 'item2'
+      }
     }
 
     It 'parameter types are validated for <type>' -TestCases @(

dsc_lib/locales/en-us.toml

@@ -544,6 +544,7 @@ parsingIndexAccessor = "Parsing index accessor '%{index}'"
 indexNotFound = "Index value not found"
 invalidAccessorKind = "Invalid accessor kind: '%{kind}'"
 functionResult = "Function results: %{results}"
+functionResultSecure = "Function result is secure"
 evalAccessors = "Evaluating accessors"
 memberNameNotFound = "Member '%{member}' not found"
 accessOnNonObject = "Member access on non-object value"

dsc_lib/src/configure/parameters.rs

@@ -4,18 +4,63 @@
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
-use std::collections::HashMap;
+use std::{collections::HashMap, fmt::Display};
 
 #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
 pub struct Input {
     pub parameters: HashMap<String, Value>,
 }
 
+pub const SECURE_VALUE_REDACTED: &str = "<secureValue>";
+
+#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
+pub struct SecureString {
+    #[serde(rename = "secureString")]
+    pub secure_string: String,
+}
+
+impl Display for SecureString {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{SECURE_VALUE_REDACTED}")
+    }
+}
+
+#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
+pub struct SecureObject {
+    #[serde(rename = "secureObject")]
+    pub secure_object: Value,
+}
+
+impl Display for SecureObject {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{SECURE_VALUE_REDACTED}")
+    }
+}
+
+/// Check if a given JSON value is a secure value (either `SecureString` or `SecureObject`).
+///
+/// # Arguments
+///
+/// * `value` - The JSON value to check.
+///
+/// # Returns
+///
+/// `true` if the value is a secure value, `false` otherwise.
+#[must_use]
+pub fn is_secure_value(value: &Value) -> bool {
+    if let Some(obj) = value.as_object() {
+        if obj.len() == 1 && (obj.contains_key("secureString") || obj.contains_key("secureObject")) {
+            return true;
+        }
+    }
+    false
+}
+
 #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)]
 #[serde(deny_unknown_fields, untagged)]
 pub enum SecureKind {
     #[serde(rename = "secureString")]
-    SecureString(String),
+    SecureString(SecureString),
     #[serde(rename = "secureObject")]
-    SecureObject(Value),
+    SecureObject(SecureObject),
 }

dsc_lib/src/dscresources/command_resource.rs

@@ -9,7 +9,7 @@ use serde_json::{Map, Value};
 use std::{collections::HashMap, env, process::Stdio};
 use crate::configure::{config_doc::ExecutionKind, config_result::{ResourceGetResult, ResourceTestResult}};
 use crate::dscerror::DscError;
-use super::{dscresource::get_diff, invoke_result::{ExportResult, GetResult, ResolveResult, SetResult, TestResult, ValidateResult, ResourceGetResponse, ResourceSetResponse, ResourceTestResponse, get_in_desired_state}, resource_manifest::{ArgKind, InputKind, Kind, ResourceManifest, ReturnKind, SchemaKind}};
+use super::{dscresource::{get_diff, redact}, invoke_result::{ExportResult, GetResult, ResolveResult, SetResult, TestResult, ValidateResult, ResourceGetResponse, ResourceSetResponse, ResourceTestResponse, get_in_desired_state}, resource_manifest::{ArgKind, InputKind, Kind, ResourceManifest, ReturnKind, SchemaKind}};
 use tracing::{error, warn, info, debug, trace};
 use tokio::{io::{AsyncBufReadExt, AsyncWriteExt, BufReader}, process::Command};
 
@@ -120,8 +120,9 @@ pub fn invoke_set(resource: &ResourceManifest, cwd: &str, desired: &str, skip_te
         };
 
         if in_desired_state && execution_type == &ExecutionKind::Actual {
+            let before_state = redact(&serde_json::from_str(desired)?);
             return Ok(SetResult::Resource(ResourceSetResponse{
-                before_state: serde_json::from_str(desired)?,
+                before_state,
                 after_state: actual_state,
                 changed_properties: None,
             }));
@@ -152,7 +153,7 @@ pub fn invoke_set(resource: &ResourceManifest, cwd: &str, desired: &str, skip_te
     else {
         return Err(DscError::Command(resource.resource_type.clone(), exit_code, stderr));
     };
-    let pre_state = if pre_state_value.is_object() {
+    let mut pre_state = if pre_state_value.is_object() {
         let mut pre_state_map: Map<String, Value> = serde_json::from_value(pre_state_value)?;
 
         // if the resource is an adapter, then the `get` will return a `result`, but a full `set` expects the before state to be `resources`
@@ -200,6 +201,7 @@ pub fn invoke_set(resource: &ResourceManifest, cwd: &str, desired: &str, skip_te
 
             // for changed_properties, we compare post state to pre state
             let diff_properties = get_diff( &actual_value, &pre_state);
+            pre_state = redact(&pre_state);
             Ok(SetResult::Resource(ResourceSetResponse{
                 before_state: pre_state,
                 after_state: actual_value,
@@ -241,6 +243,7 @@ pub fn invoke_set(resource: &ResourceManifest, cwd: &str, desired: &str, skip_te
                 }
             };
             let diff_properties = get_diff( &actual_state, &pre_state);
+            pre_state = redact(&pre_state);
             Ok(SetResult::Resource(ResourceSetResponse {
                 before_state: pre_state,
                 after_state: actual_state,
@@ -286,7 +289,7 @@ pub fn invoke_test(resource: &ResourceManifest, cwd: &str, expected: &str) -> Re
         return Ok(TestResult::Group(group_test_response));
     }
 
-    let expected_value: Value = serde_json::from_str(expected)?;
+    let mut expected_value: Value = serde_json::from_str(expected)?;
     match test.returns {
         Some(ReturnKind::State) => {
             let actual_value: Value = match serde_json::from_str(&stdout){
@@ -297,6 +300,7 @@ pub fn invoke_test(resource: &ResourceManifest, cwd: &str, expected: &str) -> Re
             };
             let in_desired_state = get_desired_state(&actual_value)?;
             let diff_properties = get_diff(&expected_value, &actual_value);
+            expected_value = redact(&expected_value);
             Ok(TestResult::Resource(ResourceTestResponse {
                 desired_state: expected_value,
                 actual_state: actual_value,
@@ -315,6 +319,7 @@ pub fn invoke_test(resource: &ResourceManifest, cwd: &str, expected: &str) -> Re
                 return Err(DscError::Command(resource.resource_type.clone(), exit_code, t!("dscresources.commandResource.testNoDiff").to_string()));
             };
             let diff_properties: Vec<String> = serde_json::from_str(diff_properties)?;
+            expected_value = redact(&expected_value);
             let in_desired_state = get_desired_state(&actual_value)?;
             Ok(TestResult::Resource(ResourceTestResponse {
                 desired_state: expected_value,
@@ -339,6 +344,7 @@ pub fn invoke_test(resource: &ResourceManifest, cwd: &str, expected: &str) -> Re
                 }
             };
             let diff_properties = get_diff( &expected_value, &actual_state);
+            expected_value = redact(&expected_value);
             Ok(TestResult::Resource(ResourceTestResponse {
                 desired_state: expected_value,
                 actual_state,
@@ -717,9 +723,9 @@ async fn run_process_async(executable: &str, args: Option<Vec<String>>, input: O
 #[allow(clippy::implicit_hasher)]
 #[tokio::main]
 pub async fn invoke_command(executable: &str, args: Option<Vec<String>>, input: Option<&str>, cwd: Option<&str>, env: Option<HashMap<String, String>>, exit_codes: Option<&HashMap<i32, String>>) -> Result<(i32, String, String), DscError> {
-    debug!("{}", t!("dscresources.commandResource.commandInvoke", executable = executable, args = args : {:?}));
+    trace!("{}", t!("dscresources.commandResource.commandInvoke", executable = executable, args = args : {:?}));
     if let Some(cwd) = cwd {
-        debug!("{}", t!("dscresources.commandResource.commandCwd", cwd = cwd));
+        trace!("{}", t!("dscresources.commandResource.commandCwd", cwd = cwd));
     }
 
     match run_process_async(executable, args, input, cwd, env, exit_codes).await {

dsc_lib/src/dscresources/dscresource.rs

@@ -1,7 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
-use crate::{configure::{config_doc::{Configuration, ExecutionKind, Resource}, Configurator, context::ProcessMode}, dscresources::resource_manifest::Kind};
+use crate::{configure::{Configurator, config_doc::{Configuration, ExecutionKind, Resource}, context::ProcessMode, parameters::{SECURE_VALUE_REDACTED, is_secure_value}}, dscresources::resource_manifest::Kind};
 use crate::dscresources::invoke_result::{ResourceGetResponse, ResourceSetResponse};
 use dscerror::DscError;
 use jsonschema::Validator;
@@ -311,7 +311,7 @@ impl Invoke for DscResource {
             let TestResult::Resource(ref resource_result) = result.results[0].result else {
                 return Err(DscError::Operation(t!("dscresources.dscresource.invokeReturnedWrongResult", operation = "test", resource = self.type_name).to_string()));
             };
-            let desired_state = resource_result.desired_state
+            let mut desired_state = resource_result.desired_state
                 .as_object().ok_or(DscError::Operation(t!("dscresources.dscresource.propertyIncorrectType", property = "desiredState", property_type = "object").to_string()))?
                 .get("resources").ok_or(DscError::Operation(t!("dscresources.dscresource.propertyNotFound", property = "resources").to_string()))?
                 .as_array().ok_or(DscError::Operation(t!("dscresources.dscresource.propertyIncorrectType", property = "resources", property_type = "array").to_string()))?[0]
@@ -324,6 +324,7 @@ impl Invoke for DscResource {
                 .as_object().ok_or(DscError::Operation(t!("dscresources.dscresource.propertyIncorrectType", property = "result", property_type = "object").to_string()))?
                 .get("properties").ok_or(DscError::Operation(t!("dscresources.dscresource.propertyNotFound", property = "properties").to_string()))?.clone();
             let diff_properties = get_diff(&desired_state, &actual_state);
+            desired_state = redact(&desired_state);
             let test_result = TestResult::Resource(ResourceTestResponse {
                 desired_state,
                 actual_state,
@@ -346,7 +347,7 @@ impl Invoke for DscResource {
                 let resource_manifest = import_manifest(manifest.clone())?;
                 if resource_manifest.test.is_none() {
                     let get_result = self.get(expected)?;
-                    let desired_state = serde_json::from_str(expected)?;
+                    let mut desired_state = serde_json::from_str(expected)?;
                     let actual_state = match get_result {
                         GetResult::Group(results) => {
                             let mut result_array: Vec<Value> = Vec::new();
@@ -360,6 +361,7 @@ impl Invoke for DscResource {
                         }
                     };
                     let diff_properties = get_diff( &desired_state, &actual_state);
+                    desired_state = redact(&desired_state);
                     let test_result = TestResult::Resource(ResourceTestResponse {
                         desired_state,
                         actual_state,
@@ -491,6 +493,36 @@ pub fn get_well_known_properties() -> HashMap<String, Value> {
     ])
 }
 
+/// Checks if the JSON value is sensitive and should be redacted
+///
+/// # Arguments
+///
+/// * `value` - The JSON value to check
+///
+/// # Returns
+///
+/// Original value if not sensitive, otherwise a redacted value
+pub fn redact(value: &Value) -> Value {
+    if is_secure_value(value) {
+        return Value::String(SECURE_VALUE_REDACTED.to_string());
+    }
+
+    if let Some(map) = value.as_object() {
+        let mut new_map = Map::new();
+        for (key, val) in map {
+            new_map.insert(key.clone(), redact(val));
+        }
+        return Value::Object(new_map);
+    }
+
+    if let Some(array) = value.as_array() {
+        let new_array: Vec<Value> = array.iter().map(redact).collect();
+        return Value::Array(new_array);
+    }
+
+    value.clone()
+}
+
 #[must_use]
 /// Performs a comparison of two JSON Values if the expected is a strict subset of the actual
 ///
@@ -524,6 +556,11 @@ pub fn get_diff(expected: &Value, actual: &Value) -> Vec<String> {
         }
 
         for (key, value) in &*map {
+            if is_secure_value(value) {
+                // skip secure values as they are not comparable
+                continue;
+            }
+
             if value.is_object() {
                 let sub_diff = get_diff(value, &actual[key]);
                 if !sub_diff.is_empty() {