Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make exception serialization settings used by Remoting secure by default #398

Merged
merged 4 commits into from
Feb 28, 2025

Conversation

olegsych
Copy link
Member

  • Deprecate the ExceptionDeserializationTechnique property of the FabricTransportRemotingSettings and change its default value from Fallback to Default.
  • Deprecate the ExceptionSerializationTechnique property of the FabricTransportListenerSettings and change its default value from BinaryFormatter to Default.

@olegsych olegsych requested a review from Copilot February 28, 2025 19:21

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR updates exception serialization and deserialization settings to enhance security by deprecating the legacy BinaryFormatter/Fallback defaults. The changes remove hard-coded default values in production code, mark the deprecated properties with Obsolete attributes using DeprecationMessage.RemotingV1, and update unit tests to use the new defaults.

  • Removed the old branching logic in exception serialization to avoid using BinaryFormatter by default.
  • Annotated deprecated enums and properties and updated log messages.
  • Adjusted unit tests to remove usage of the deprecated default settings.

Reviewed Changes

File Description
src/Microsoft.ServiceFabric.Services.Remoting/V2/Runtime/ExceptionConversionHandler.cs Changed exception serialization logic to use secure defaults.
src/Microsoft.ServiceFabric.Services.Remoting/V2/FabricTransport/Runtime/FabricTransportServiceRemotingListener.cs Wrapped telemetry calls with pragma disables for deprecated properties.
test/unittests/Microsoft.ServiceFabric.Services.Remoting.Tests/V2/ExceptionConvertors/CompatScenarioTest.cs Updated handler initialization to remove explicit deprecated property usage.
src/Microsoft.ServiceFabric.Services.Remoting/V2/Client/ExceptionConversionHandler.cs Updated deserialization logic and adjusted logging severity.
src/Microsoft.ServiceFabric.Services.Remoting/RemotingListenerVersion.cs and RemotingClientVersion.cs Improved deprecation messages for V1 components.
src/Microsoft.ServiceFabric.Services.Remoting/FabricTransport/Runtime/FabricTransportRemotingListenerSettings.cs Removed default assignment for exception serialization and added Obsolete attributes.
test/unittests/* Updated unit tests to align with the revised default settings.
src/Microsoft.ServiceFabric.Services.Remoting/FabricTransport/FabricTransportRemotingSettings.cs Removed default assignment for exception deserialization, marked as obsolete, and updated log messages.

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

src/Microsoft.ServiceFabric.Services.Remoting/FabricTransport/Runtime/FabricTransportRemotingListenerSettings.cs:303

  • Typo in log message: 'ExceptionSerializationTechinique' should be 'ExceptionSerializationTechnique'.
                "HeaderBufferCount {7} , ExceptionSerializationTechinique {8} , RemotingExceptionDepth {9}",

src/Microsoft.ServiceFabric.Services.Remoting/FabricTransport/FabricTransportRemotingSettings.cs:285

  • Typo in log message: 'ExceptionSerializationTechinique' should be 'ExceptionSerializationTechnique'.
                "HeaderBufferSize {6}, HeaderBufferCount {7}, ExceptionSerializationTechinique {8}",

src/Microsoft.ServiceFabric.Services.Remoting/V2/Client/ExceptionConversionHandler.cs:120

  • The change in log severity (using WriteInfo instead of WriteWarning) in this deserialization branch should be verified to ensure it accurately reflects the error condition.
                    ServiceTrace.Source.WriteInfo(
@olegsych olegsych enabled auto-merge (squash) February 28, 2025 19:23
@olegsych olegsych requested a review from egaribay77 February 28, 2025 20:05
@olegsych olegsych merged commit 52c83f4 into develop Feb 28, 2025
2 checks passed
@olegsych olegsych deleted the user/olegsych/ExceptionSerializationTechnique branch February 28, 2025 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants