Remove BinaryFormatter use in PSRP serialization #17133

jborean93 · 2022-04-10T23:23:46Z

PR Summary

Removes the usage of BinaryFormatter in PSRP connections.

PR Context

The BinaryFormatter class is marked as obsolete and is planned to be removed in a future .NET version https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/binaryformatter-obsoletion.md. One location where it is used in pwsh is when serializing the client's time zone. On the server end PowerShell has ignored this property since v3 and even on v2 it has an exception handler to ignore invalid values. This means the client can either omit the property altogether or send an empty value and the server will still be fine.

One minor difference between a missing property and an empty property value is that $PSSenderInfo.ClientTimeZone is $null when it's missing and set to [TimeZone]::Current when it's an empty value. So to preserve as much existing behaviour an empty value was used instead.

The only concern I have with this approach that I haven't tested it with an Exchange endpoint to see if it has any special behaviour with this property.

Partially addressed #14054

PR Checklist

awakecoding · 2022-04-10T23:40:30Z

Thanks a lot @jborean93 for this pull request, I really appreciate it!

PaulHigin · 2022-04-11T16:21:01Z

I would love to get rid of the Client Timezone property. It is part of the PowerShell remoting protocol (PSRP), but is listed as an optional property. So it is Ok to not sent it, but not Ok to provide an incorrect value.

My main concern is how it might affect Exchange endpoints. I was not on the team when this property was included, but I seem to recall it was included for Exchange endpoints. One possible solution could be to serialize this object ourselves 'safely' so that it can be deserialized and used on older endpoints. But I don't know how important it is and if it is really needed.

Marking for committee review.

PaulHigin · 2022-04-11T16:22:53Z

src/System.Management.Automation/engine/remoting/fanin/PSPrincipal.cs

-            get;
-            internal set;
-        }
+        public TimeZoneInfo ClientTimeZone => TimeZoneInfo.Local;


This violates the remoting protocol (PSRP). We either need to provide the actual client timezone or nothing.

sending nothing would be the most straightforward solution then

How does it violate the protocol? PowerShell, since v3, has ignored whatever was sent to it from the client and did TimeZone.CurrentTimeZone as per

PowerShell/src/System.Management.Automation/engine/remoting/common/WireDataFormat/EncodeAndDecode.cs

Lines 2376 to 2392 in 7cc9c87

if (dataAsPSObject.Properties[RemoteDataNameStrings.TimeZone] != null)

{

// Binary deserialization of timezone info via BinaryFormatter is unsafe,

// so don't deserialize any untrusted client data using this API.

//

// In addition, the binary data being sent by the client doesn't represent

// the client's current TimeZone unless they somehow accessed the

// StandardName and DaylightName. These properties are initialized lazily

// by the .NET Framework, and would be populated by the server with local

// values anyways.

//

// So just return the CurrentTimeZone.

#if !CORECLR // TimeZone Not In CoreCLR

result.TimeZone = TimeZone.CurrentTimeZone;

#endif

}

.

The TimeZone class is deprecated and further along PowerShell just changes it to TimeZoneInfo.Local

I can make this null and just always return that if you wish, it just seems like having it always set to this value is more aligned with the current behaviour.

Yes, please leave it null, as that conforms to the protocol (which makes this capability field optional). Also I notice that currently this field is null now, with this previous code:

#if !CORECLR // TimeZone Not In CoreCLR result.TimeZone = TimeZone.CurrentTimeZone; #endif

jborean93 · 2022-04-11T19:09:22Z

but not Ok to provide an incorrect value.

I can change the code to omit the field altogether if you wish, the only reason why I send an empty byte value is that

If the serialization of the TZ fails on the client this is what is sent today
the server populates the $PSSenderInfo.ClientTimeZone differently based on whether the property was set or not
- If missing the value is alwasy null
- If set, v2 will use the provided value and null if the value is empty/invalid
- If set, v3 will use TimeZone.CurrentTimeZone

I tested both a missing and empty value and decided on the latter to try and preserve as much existing behaviour as I could ($PSSenderInfo.ClientTimeZone not being null). Considering currently PowerShell can already send an empty byte array when serialization fails (for whatever reason) it seems like the behaviour is already vaguely defined.

My main concern is how it might affect Exchange endpoints. I was not on the team when this property was included, but I seem to recall it was included for Exchange endpoints. One possible solution could be to serialize this object ourselves 'safely' so that it can be deserialized and used on older endpoints. But I don't know how important it is and if it is really needed.

Yea I can understand this concern and ultimately was hoping that you, or someone else in the pwsh team, can get them to review this change. I know for sure that Exchange works with the property not present altogether as my Python client doesn't add this field at all, I'm just not sure if it will be able to handle an empty value that this PR has today.

SteveL-MSFT · 2022-04-20T22:49:59Z

@PowerShell/powershell-committee reviewed this, we agree that given dotnet's deprecation and removal plan, we should remove binaryformatter completely and not replace the optional protocol property with incorrect information. We can get customer feedback on impact through 7.3, however, the only viable mitigation we can give them is to use 5.1 or 7.2 instead.

daxian-dbw · 2022-04-27T00:00:35Z

@jborean93 and @PaulHigin, can you please help me understand if additional changes are needed given the committee's comments?

PaulHigin · 2022-04-27T00:37:11Z

The committee Ok'd removing the ClientTimeZone message since it is optional in the protocol. We want to see if there are any adverse affects. However, I believe this PR will have to change to not substitute the ClientTimeZone on the server with the current local timezone, as that does not reflect the actual client timezone and violates the protocol.

jborean93 · 2022-04-27T00:51:14Z

I believe this PR will have to change to not substitute the ClientTimeZone on the server with the current local timezone, as that does not reflect the actual client timezone and violates the protocol.

This is currently what happens today, the server (since pwsh v3) will ignore the ClientTimeZone value and always use the current time zone value as per

PowerShell/src/System.Management.Automation/engine/remoting/common/WireDataFormat/EncodeAndDecode.cs

Lines 2376 to 2392 in 7cc9c87

    
                       if (dataAsPSObject.Properties[RemoteDataNameStrings.TimeZone] != null) 
        
                       { 
        
                           // Binary deserialization of timezone info via BinaryFormatter is unsafe, 
        
                           // so don't deserialize any untrusted client data using this API. 
        
                           // 
        
                           // In addition, the binary data being sent by the client doesn't represent 
        
                           // the client's current TimeZone unless they somehow accessed the 
        
                           // StandardName and DaylightName. These properties are initialized lazily 
        
                           // by the .NET Framework, and would be populated by the server with local 
        
                           // values anyways. 
        
                           // 
        
                           // So just return the CurrentTimeZone. 
        
           #if !CORECLR // TimeZone Not In CoreCLR 
        
                           result.TimeZone = TimeZone.CurrentTimeZone; 
        
           #endif 
        
                       }

.

Only PowerShell 2 actually read this value and parsed it. I have no idea what Exchange does though and whether they use this value for anything. I know it doesn't care if the property doesn't exist as per the docs it is optional but I'm unsure what it does with an empty value like <BA N="ClientTimeZone"></BA>

PaulHigin · 2022-04-27T15:21:45Z

@jborean93

Only PowerShell 2 actually read this value and parsed it

How do you know? Do you have access to the code base?

In any case, this is contrary to what the protocol states. So either the protocol must change or the code. This PR should change the code and we can evaluate its effects in preview.

jborean93 · 2022-04-27T21:19:18Z

How do you know? Do you have access to the code base?

The only way I can, through tools like dotpeek and dnsspy. On Windows 7 with pwsh 2 installed the method there will actually deserialize the .NET binary blob. On pwsh v3 and newer (since 2012) it has the same comment you see today in the code.

In any case, this is contrary to what the protocol states. So either the protocol must change or the code. This PR should change the code and we can evaluate its effects in preview.

That's fine with me, I don't know where you keep the docs, I'm just trying to remove a component that is going to be removed from .NET. My goal is to try and keep the same behaviour as is today.

PaulHigin · 2022-04-27T21:53:06Z

@jborean93
Makes sense, thanks.

It is not just updating protocol docs (which requires approval) but also updating the protocol version since it will be (or has been, since the code is now doing this) changed (ClientTimeZone information is actually ServerTimeZone). That protocol change makes no sense and is not worth doing in my opinion and as the committee recommends, this PR should simply remove it, since it is optional. I am thinking the 'ClientTimeZone' property should be set to null.

jborean93 · 2022-04-27T21:57:29Z

Considering the change happened in pwsh v3 the protocol version was already bumped from 2.1 to 2.2 I'm not sure it really warrants a new bump for something that has already happened.

I'm happy to just omit the value altogether but it is strictly a breaking change; $PSSenderInfo.ClientTimeZone is now $null compared to the time zone. I highly doubt people are relying on this property but considering how change adverse this project is I assumed you preferred it to continue to be the server time zone, what it has been since v3. If you still want me to change the PR so it's never sent I can but I just want to make sure you are aware of the potential breaking change it can cause.

daxian-dbw · 2022-04-27T23:16:14Z

Since the property has been set to the server time zone since PowerShell v3, I think we should keep it that way to avoid potentially breaking anything.

PaulHigin · 2022-04-27T23:18:55Z

@daxian-dbw We cannot keep this behavior, regardless of how long it has been there, as it violates the protocol and we are non-compliant. We either need to change the protocol (which doesn't make sense) or adhere to the protocol which allows us to not send the message (since it is optional).

daxian-dbw · 2022-04-27T23:24:36Z

OK, so it looks to me that means the server side timezone needs to be fixed regardless of the rest changes in this PR.

PaulHigin · 2022-04-28T16:04:17Z

@daxian-dbw
No, the server side timezone needs to be fixed as part of this PR. We cannot remove the TimeZone field from the client session capability message and report an invalid timezone on the server side.

PaulHigin · 2022-04-28T16:40:07Z

src/System.Management.Automation/engine/remoting/fanin/PSPrincipal.cs

-            get;
-            internal set;
-        }
+        public TimeZoneInfo ClientTimeZone => TimeZoneInfo.Local;


Yes, please leave it null, as that conforms to the protocol (which makes this capability field optional). Also I notice that currently this field is null now, with this previous code:

#if !CORECLR // TimeZone Not In CoreCLR result.TimeZone = TimeZone.CurrentTimeZone; #endif

daxian-dbw · 2022-05-09T18:17:37Z

@jborean93 can you please address @PaulHigin's comments?

daxian-dbw · 2022-05-16T22:39:50Z

@PaulHigin I just pushed a commit to address you comment (leave ClientTimeZone to be null). Please review again and see if we can merge this PR. Thanks!

jborean93 · 2022-05-16T23:25:56Z

Sorry I've been slack in following it up, don't you also want to remove the line https://github.com/PowerShell/PowerShell/pull/17133/files#diff-24dd8a004527ebae1b7fb5796191281dc116c2f41ed925f4f12b38cab99a71dfR1604 so it doesn't add the property at all in the CLIXML. I believe that was desired rather than an empty byte array.

GitHub
Remove BinaryFormatter use in PSRP serialization by jborean93 · Pull Request #17133 · PowerShell/PowerShell
PR Summary Removes the usage of BinaryFormatter in PSRP connections. PR Context The BinaryFormatter class is marked as obsolete and is planned to be removed in a future .NET version https://github....

daxian-dbw · 2022-05-16T23:54:54Z

@PaulHigin Can you comment on @jborean93's message above?

PaulHigin · 2022-05-17T00:45:16Z

Hmm, yes thanks @jborean93. I think it is best to just not include the TimeZone data item in the message stream.

pull-request-quantifier-deprecated · 2022-05-17T01:13:00Z

This PR has 42 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Small
Size       : +1 -41
Percentile : 16.8%

Total files changed: 5

Change summary by file extension:
.cs : +1 -41

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

daxian-dbw · 2022-05-17T01:13:40Z

OK, that line was removed. Can both of you take another look? I will merge if all look good.
BTW, is it still a breaking change? I guess not, since we actually never do this assignment result.TimeZone = TimeZone.CurrentTimeZone; on PowerShell Core.

jborean93 · 2022-05-17T01:40:54Z

The only breaking change I know off is that now is when using a client with this change, the $PSSenderInfo.ClientTimeZone value will be $null on any sessions created. The same thing would occur if a client then connected to a server running with this change as well. Technically the value before wasn't 100% correct as it was set to the time zone of the server. All in all I don't think it's really a problem IMO.

PaulHigin

LGTM

daxian-dbw · 2022-05-17T16:19:23Z

Thank you both. I will merge this PR today.

ghost · 2022-05-23T16:22:44Z

🎉v7.3.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

Avoid BinaryFormatter in PSRP serialization

c5d8e60

jborean93 requested a review from PaulHigin as a code owner April 10, 2022 23:23

ghost assigned daxian-dbw Apr 10, 2022

pull-request-quantifier-deprecated bot added the Extra Small label Apr 10, 2022

PaulHigin reviewed Apr 11, 2022

View reviewed changes

PaulHigin added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Apr 11, 2022

JamesWTruher added the CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log label Apr 20, 2022

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Apr 20, 2022

daxian-dbw added the Breaking-Change breaking change that may affect users label Apr 27, 2022

PaulHigin added the Backport-5.1-Consider Consider to backport to Windows PowerShell 5.1 due to impact label Apr 27, 2022

PaulHigin suggested changes Apr 28, 2022

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 28, 2022

Address Paul's comment

b2b012f

PaulHigin approved these changes May 16, 2022

View reviewed changes

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 16, 2022

Address one more comment

e8704bb

PaulHigin approved these changes May 17, 2022

View reviewed changes

daxian-dbw merged commit 3d1d052 into PowerShell:master May 17, 2022

jborean93 deleted the SessionCapability-Binary branch May 17, 2022 18:47

daxian-dbw mentioned this pull request Jun 8, 2022

Does this library not support remote PowerShell commands dotnet/interactive#2041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove BinaryFormatter use in PSRP serialization #17133

Remove BinaryFormatter use in PSRP serialization #17133

jborean93 commented Apr 10, 2022

awakecoding commented Apr 10, 2022

PaulHigin commented Apr 11, 2022

PaulHigin Apr 11, 2022

awakecoding Apr 11, 2022

jborean93 Apr 11, 2022

PaulHigin Apr 28, 2022

jborean93 commented Apr 11, 2022

SteveL-MSFT commented Apr 20, 2022 •

edited

daxian-dbw commented Apr 27, 2022

PaulHigin commented Apr 27, 2022

jborean93 commented Apr 27, 2022 •

edited

PaulHigin commented Apr 27, 2022

jborean93 commented Apr 27, 2022

PaulHigin commented Apr 27, 2022

jborean93 commented Apr 27, 2022

daxian-dbw commented Apr 27, 2022

PaulHigin commented Apr 27, 2022

daxian-dbw commented Apr 27, 2022

PaulHigin commented Apr 28, 2022

PaulHigin Apr 28, 2022

daxian-dbw commented May 9, 2022

daxian-dbw commented May 16, 2022

jborean93 commented May 16, 2022 •

edited by unfurl-links bot

daxian-dbw commented May 16, 2022

PaulHigin commented May 17, 2022

pull-request-quantifier-deprecated bot commented May 17, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

daxian-dbw commented May 17, 2022 •

edited

jborean93 commented May 17, 2022

PaulHigin left a comment

daxian-dbw commented May 17, 2022

ghost commented May 23, 2022

	if (dataAsPSObject.Properties[RemoteDataNameStrings.TimeZone] != null)
	{
	// Binary deserialization of timezone info via BinaryFormatter is unsafe,
	// so don't deserialize any untrusted client data using this API.
	//
	// In addition, the binary data being sent by the client doesn't represent
	// the client's current TimeZone unless they somehow accessed the
	// StandardName and DaylightName. These properties are initialized lazily
	// by the .NET Framework, and would be populated by the server with local
	// values anyways.
	//
	// So just return the CurrentTimeZone.

	#if !CORECLR // TimeZone Not In CoreCLR
	result.TimeZone = TimeZone.CurrentTimeZone;
	#endif
	}

Remove BinaryFormatter use in PSRP serialization #17133

Remove BinaryFormatter use in PSRP serialization #17133

Conversation

jborean93 commented Apr 10, 2022

PR Summary

PR Context

PR Checklist

awakecoding commented Apr 10, 2022

PaulHigin commented Apr 11, 2022

PaulHigin Apr 11, 2022

Choose a reason for hiding this comment

awakecoding Apr 11, 2022

Choose a reason for hiding this comment

jborean93 Apr 11, 2022

Choose a reason for hiding this comment

PaulHigin Apr 28, 2022

Choose a reason for hiding this comment

jborean93 commented Apr 11, 2022

SteveL-MSFT commented Apr 20, 2022 • edited

daxian-dbw commented Apr 27, 2022

PaulHigin commented Apr 27, 2022

jborean93 commented Apr 27, 2022 • edited

PaulHigin commented Apr 27, 2022

jborean93 commented Apr 27, 2022

PaulHigin commented Apr 27, 2022

jborean93 commented Apr 27, 2022

daxian-dbw commented Apr 27, 2022

PaulHigin commented Apr 27, 2022

daxian-dbw commented Apr 27, 2022

PaulHigin commented Apr 28, 2022

PaulHigin Apr 28, 2022

Choose a reason for hiding this comment

daxian-dbw commented May 9, 2022

daxian-dbw commented May 16, 2022

jborean93 commented May 16, 2022 • edited by unfurl-links bot

daxian-dbw commented May 16, 2022

PaulHigin commented May 17, 2022

pull-request-quantifier-deprecated bot commented May 17, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

daxian-dbw commented May 17, 2022 • edited

jborean93 commented May 17, 2022

PaulHigin left a comment

Choose a reason for hiding this comment

daxian-dbw commented May 17, 2022

ghost commented May 23, 2022

SteveL-MSFT commented Apr 20, 2022 •

edited

jborean93 commented Apr 27, 2022 •

edited

jborean93 commented May 16, 2022 •

edited by unfurl-links bot

daxian-dbw commented May 17, 2022 •

edited