Erroring out instead of silent override, and bubbling up the error to the caller

Signed-off-by: Manish Ranjan Mahanta <mmahanta@microsoft.com>

Author: Manish Ranjan Mahanta

internal/gcs-sidecar/handlers.go

@@ -526,7 +526,10 @@ func (b *Bridge) modifyServiceSettings(req *request) (err error) {
 					log.G(req.ctx).Tracef("Allowed log sources after policy enforcement: %v", allowedLogSources)
 
 					// Update the allowed log sources in the settings. This will be forwarded to inbox GCS which expects the log sources in a JSON string format with GUIDs for providers included.
-					allowedLogSources = etw.UpdateLogSources(req.ctx, allowedLogSources, false, true)
+					allowedLogSources, err := etw.UpdateLogSources(allowedLogSources, false, true)
+					if err != nil {
+						return fmt.Errorf("failed to update log sources: %w", err)
+					}
 					settings.Settings = allowedLogSources
 				}
 			default:

internal/uvm/log_wcow.go

@@ -4,6 +4,7 @@ package uvm
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/Microsoft/hcsshim/internal/gcs"
 	"github.com/Microsoft/hcsshim/internal/gcs/prot"
@@ -69,15 +70,17 @@ func (uvm *UtilityVM) SetLogSources(ctx context.Context) error {
 		// For confidential WCOw, we skip the adding guids to the log sources as the sidecar-GCS will verify the
 		// allowed log sources against policy and append the necessary GUIDs to the ones allowed. Rest are dropped.
 		// For non-confidential WCOW, we include the GUIDs in the log sources as the hcsshim communicates directly with the inboxGCS.
-		settings := etw.UpdateLogSources(ctx, uvm.logSources, uvm.defaultLogSourcesEnabled, !uvm.HasConfidentialPolicy())
-
+		settings, err := etw.UpdateLogSources(uvm.logSources, uvm.defaultLogSourcesEnabled, !uvm.HasConfidentialPolicy())
+		if err != nil {
+			return fmt.Errorf("failed to parse log sources: %w", err)
+		}
 		req := guestrequest.LogForwardServiceRPCRequest{
 			RPCType:  guestrequest.RPCModifyServiceSettings,
 			Settings: settings,
 		}
-		err := uvm.gc.ModifyServiceSettings(ctx, prot.LogForwardService, req)
+		err = uvm.gc.ModifyServiceSettings(ctx, prot.LogForwardService, req)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to modify service settings: %w", err)
 		}
 	}
 	return nil

internal/vm/vmutils/etw/provider_map.go

@@ -1,13 +1,12 @@
 package etw
 
 import (
-	"context"
 	"encoding/base64"
 	"encoding/json"
+	"fmt"
 	"strings"
 
 	"github.com/Microsoft/go-winio/pkg/guid"
-	"github.com/Microsoft/hcsshim/internal/log"
 )
 
 // Log Sources JSON structure
@@ -94,17 +93,15 @@ func mergeLogSources(resultSources []Source, userSources []Source) []Source {
 }
 
 // decodeAndUnmarshalLogSources decodes a base64-encoded JSON string and unmarshals it into a LogSourcesInfo.
-func decodeAndUnmarshalLogSources(ctx context.Context, base64EncodedJSONLogConfig string) (LogSourcesInfo, error) {
+func decodeAndUnmarshalLogSources(base64EncodedJSONLogConfig string) (LogSourcesInfo, error) {
 	jsonBytes, err := base64.StdEncoding.DecodeString(base64EncodedJSONLogConfig)
 	if err != nil {
-		log.G(ctx).Errorf("Error decoding base64 log config: %v", err)
-		return LogSourcesInfo{}, err
+		return LogSourcesInfo{}, fmt.Errorf("error decoding base64 log config: %w", err)
 	}
 
 	var userLogSources LogSourcesInfo
 	if err := json.Unmarshal(jsonBytes, &userLogSources); err != nil {
-		log.G(ctx).Errorf("Error unmarshalling user log config: %v", err)
-		return LogSourcesInfo{}, err
+		return LogSourcesInfo{}, fmt.Errorf("error unmarshalling user log config: %w", err)
 	}
 	return userLogSources, nil
 }
@@ -119,14 +116,13 @@ func trimGUID(in string) string {
 
 // resolveGUIDsWithLookup normalizes and fills in provider GUIDs from the well-known ETW map
 // for all providers across all sources. Providers with an invalid GUID are warned and skipped.
-func resolveGUIDsWithLookup(ctx context.Context, sources []Source) []Source {
+func resolveGUIDsWithLookup(sources []Source) ([]Source, error) {
 	for i, src := range sources {
 		for j, provider := range src.Providers {
 			if provider.ProviderGUID != "" {
 				guid, err := guid.FromString(trimGUID(provider.ProviderGUID))
 				if err != nil {
-					log.G(ctx).Warningf("Skipping invalid GUID %q for provider %q: %v", provider.ProviderGUID, provider.ProviderName, err)
-					continue
+					return nil, fmt.Errorf("invalid GUID %q for provider %q: %w", provider.ProviderGUID, provider.ProviderName, err)
 				}
 				sources[i].Providers[j].ProviderGUID = strings.ToLower(guid.String())
 			}
@@ -135,77 +131,86 @@ func resolveGUIDsWithLookup(ctx context.Context, sources []Source) []Source {
 			}
 		}
 	}
-	return sources
+	return sources, nil
 }
 
 // stripRedundantGUIDs removes the GUID from providers where both Name and GUID are present and
 // the GUID matches the well-known lookup by name. This ensures sidecar-GCS prefers name-based
-// policy verification. Invalid GUIDs are warned and left as-is after normalization.
-func stripRedundantGUIDs(ctx context.Context, sources []Source) []Source {
+// policy verification. Invalid GUIDs are errored out.
+func stripRedundantGUIDs(sources []Source) ([]Source, error) {
 	for i, src := range sources {
 		for j, provider := range src.Providers {
 			if provider.ProviderName == "" || provider.ProviderGUID == "" {
 				continue
 			}
 			guid, err := guid.FromString(trimGUID(provider.ProviderGUID))
 			if err != nil {
-				log.G(ctx).Warningf("Skipping invalid GUID %q for provider %q: %v", provider.ProviderGUID, provider.ProviderName, err)
-				continue
+				return nil, fmt.Errorf("invalid GUID %q for provider %q: %w", provider.ProviderGUID, provider.ProviderName, err)
 			}
 			if strings.EqualFold(guid.String(), getProviderGUIDFromName(provider.ProviderName)) {
 				sources[i].Providers[j].ProviderGUID = ""
 			} else {
+				// If the GUID doesn't match the well-known GUID for the provider name,
+				// we keep it but ensure it's normalized to lowercase without braces.
+				// However, we remove the provider name to avoid incorrect policy matches
+				// in sidecar-GCS, since the GUID is the source of truth in this case.
+				sources[i].Providers[j].ProviderName = ""
 				sources[i].Providers[j].ProviderGUID = strings.ToLower(guid.String())
 			}
 		}
 	}
-	return sources
+	return sources, nil
 }
 
 // applyGUIDPolicy applies GUID resolution or stripping to all sources depending on the includeGUIDs flag.
 // See resolveGUIDsWithLookup and stripRedundantGUIDs for the respective behaviors.
-func applyGUIDPolicy(ctx context.Context, sources []Source, includeGUIDs bool) []Source {
+func applyGUIDPolicy(sources []Source, includeGUIDs bool) ([]Source, error) {
 	if len(sources) == 0 {
-		return sources
+		return sources, nil
 	}
 	if includeGUIDs {
-		return resolveGUIDsWithLookup(ctx, sources)
+		return resolveGUIDsWithLookup(sources)
 	}
-	return stripRedundantGUIDs(ctx, sources)
+	return stripRedundantGUIDs(sources)
 }
 
 // marshalAndEncodeLogSources marshals the given LogSourcesInfo to JSON and encodes it as a base64 string.
 // On error, it logs and returns the original fallback string.
-func marshalAndEncodeLogSources(ctx context.Context, logCfg LogSourcesInfo, fallback string) (string, error) {
+func marshalAndEncodeLogSources(logCfg LogSourcesInfo) (string, error) {
 	jsonBytes, err := json.Marshal(logCfg)
 	if err != nil {
-		log.G(ctx).Errorf("Error marshalling log config: %v", err)
-		return fallback, err
+		return "", fmt.Errorf("error marshalling log config: %w", err)
 	}
 	return base64.StdEncoding.EncodeToString(jsonBytes), nil
 }
 
 // UpdateLogSources updates the user provided log sources with the default log sources based on the
 // configuration and returns the updated log sources as a base64 encoded JSON string.
 // If there is an error in the process, it returns the original user provided log sources string.
-func UpdateLogSources(ctx context.Context, base64EncodedJSONLogConfig string, useDefaultLogSources bool, includeGUIDs bool) string {
+func UpdateLogSources(base64EncodedJSONLogConfig string, useDefaultLogSources bool, includeGUIDs bool) (string, error) {
 	var resultLogCfg LogSourcesInfo
 	if useDefaultLogSources {
 		resultLogCfg = defaultLogSourcesInfo
 	}
 
 	if base64EncodedJSONLogConfig != "" {
-		userLogSources, err := decodeAndUnmarshalLogSources(ctx, base64EncodedJSONLogConfig)
-		if err == nil {
-			resultLogCfg.LogConfig.Sources = mergeLogSources(resultLogCfg.LogConfig.Sources, userLogSources.LogConfig.Sources)
+		userLogSources, err := decodeAndUnmarshalLogSources(base64EncodedJSONLogConfig)
+		if err != nil {
+			return "", fmt.Errorf("failed to decode and unmarshal user log sources: %w", err)
 		}
+		resultLogCfg.LogConfig.Sources = mergeLogSources(resultLogCfg.LogConfig.Sources, userLogSources.LogConfig.Sources)
+
 	}
 
-	resultLogCfg.LogConfig.Sources = applyGUIDPolicy(ctx, resultLogCfg.LogConfig.Sources, includeGUIDs)
+	var err error
+	resultLogCfg.LogConfig.Sources, err = applyGUIDPolicy(resultLogCfg.LogConfig.Sources, includeGUIDs)
+	if err != nil {
+		return "", fmt.Errorf("failed to apply GUID policy: %w", err)
+	}
 
-	result, err := marshalAndEncodeLogSources(ctx, resultLogCfg, base64EncodedJSONLogConfig)
+	result, err := marshalAndEncodeLogSources(resultLogCfg)
 	if err != nil {
-		return base64EncodedJSONLogConfig
+		return "", fmt.Errorf("failed to marshal and encode log sources: %w", err)
 	}
-	return result
+	return result, nil
 }

internal/vm/vmutils/etw/provider_map_test.go

@@ -1,7 +1,6 @@
 package etw
 
 import (
-	"context"
 	"encoding/base64"
 	"encoding/json"
 	"reflect"
@@ -108,7 +107,10 @@ func TestUpdateLogSources_Combinations(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			defaultLogSourcesInfo = cloneLogSourcesInfo(originalDefaults)
 
-			gotEncoded := UpdateLogSources(context.Background(), tt.base64Input, tt.useDefault, tt.includeGUIDs)
+			gotEncoded, err := UpdateLogSources(tt.base64Input, tt.useDefault, tt.includeGUIDs)
+			if err != nil {
+				t.Fatalf("UpdateLogSources returned error: %v", err)
+			}
 			got := mustDecodeLogSources(t, gotEncoded)
 
 			if !reflect.DeepEqual(got, tt.expectedLogCfg) {
@@ -243,3 +245,142 @@ func mustDecodeLogSources(t *testing.T, encoded string) LogSourcesInfo {
 	}
 	return cfg
 }
+
+func TestUpdateLogSources_ErrorCases(t *testing.T) {
+	originalDefaults := cloneLogSourcesInfo(defaultLogSourcesInfo)
+	t.Cleanup(func() {
+		defaultLogSourcesInfo = cloneLogSourcesInfo(originalDefaults)
+	})
+
+	// Build a config with an invalid GUID to trigger applyGUIDPolicy errors.
+	invalidGUIDConfig := LogSourcesInfo{
+		LogConfig: LogConfig{
+			Sources: []Source{
+				{
+					Type: "ETW",
+					Providers: []EtwProvider{
+						{
+							ProviderName: "SomeProvider",
+							ProviderGUID: "not-a-valid-guid",
+						},
+					},
+				},
+			},
+		},
+	}
+	invalidGUIDBase64 := mustEncodeLogSources(t, invalidGUIDConfig)
+
+	// Build a config with an invalid GUID but no provider name (only GUID set),
+	// to trigger the resolveGUIDsWithLookup path specifically.
+	invalidGUIDOnlyConfig := LogSourcesInfo{
+		LogConfig: LogConfig{
+			Sources: []Source{
+				{
+					Type: "ETW",
+					Providers: []EtwProvider{
+						{
+							ProviderGUID: "zzz-invalid",
+						},
+					},
+				},
+			},
+		},
+	}
+	invalidGUIDOnlyBase64 := mustEncodeLogSources(t, invalidGUIDOnlyConfig)
+
+	tests := []struct {
+		name         string
+		base64Input  string
+		useDefault   bool
+		includeGUIDs bool
+		errContains  string
+	}{
+		{
+			name:         "invalid_base64_input",
+			base64Input:  "not-valid-base64!@#$",
+			useDefault:   false,
+			includeGUIDs: false,
+			errContains:  "failed to decode and unmarshal user log sources",
+		},
+		{
+			name:         "valid_base64_invalid_json",
+			base64Input:  base64.StdEncoding.EncodeToString([]byte("{{not json}}")),
+			useDefault:   false,
+			includeGUIDs: false,
+			errContains:  "failed to decode and unmarshal user log sources",
+		},
+		{
+			name:         "invalid_base64_with_defaults",
+			base64Input:  "!!!bad-base64!!!",
+			useDefault:   true,
+			includeGUIDs: false,
+			errContains:  "failed to decode and unmarshal user log sources",
+		},
+		{
+			name:         "invalid_base64_with_defaults_and_guids",
+			base64Input:  "???",
+			useDefault:   true,
+			includeGUIDs: true,
+			errContains:  "failed to decode and unmarshal user log sources",
+		},
+		{
+			name:         "valid_base64_malformed_json_structure",
+			base64Input:  base64.StdEncoding.EncodeToString([]byte(`{"LogConfig": {"sources": "not_an_array"}}`)),
+			useDefault:   false,
+			includeGUIDs: false,
+			errContains:  "failed to decode and unmarshal user log sources",
+		},
+		{
+			name:         "invalid_guid_with_includeGUIDs_resolveGUIDsWithLookup",
+			base64Input:  invalidGUIDBase64,
+			useDefault:   false,
+			includeGUIDs: true,
+			errContains:  "failed to apply GUID policy",
+		},
+		{
+			name:         "invalid_guid_without_includeGUIDs_stripRedundantGUIDs",
+			base64Input:  invalidGUIDBase64,
+			useDefault:   false,
+			includeGUIDs: false,
+			errContains:  "failed to apply GUID policy",
+		},
+		{
+			name:         "invalid_guid_only_no_name_with_includeGUIDs",
+			base64Input:  invalidGUIDOnlyBase64,
+			useDefault:   false,
+			includeGUIDs: true,
+			errContains:  "failed to apply GUID policy",
+		},
+		{
+			name:         "invalid_guid_with_defaults_and_includeGUIDs",
+			base64Input:  invalidGUIDBase64,
+			useDefault:   true,
+			includeGUIDs: true,
+			errContains:  "failed to apply GUID policy",
+		},
+		{
+			name:         "invalid_guid_with_defaults_without_includeGUIDs",
+			base64Input:  invalidGUIDBase64,
+			useDefault:   true,
+			includeGUIDs: false,
+			errContains:  "failed to apply GUID policy",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			defaultLogSourcesInfo = cloneLogSourcesInfo(originalDefaults)
+
+			got, err := UpdateLogSources(tt.base64Input, tt.useDefault, tt.includeGUIDs)
+			if err == nil {
+				t.Fatalf("expected error containing %q, got nil (result: %q)", tt.errContains, got)
+			}
+			if !strings.Contains(err.Error(), tt.errContains) {
+				t.Fatalf("expected error containing %q, got: %v", tt.errContains, err)
+			}
+			if got != "" {
+				t.Fatalf("expected empty result on error, got %q", got)
+			}
+		})
+	}
+}