jborean93 / pypsrp

PowerShell Remoting Protocol for Python
MIT License
328 stars 49 forks source link

Add low-level value tagging option #128

Closed malthe closed 2 years ago

malthe commented 2 years ago

This is a stop-gap solution waiting for #81 – it provides a low-level interface to marking a value with a tag which is mapped during serialization to a specific type, e.g. "SS" for SecureString.

codecov[bot] commented 2 years ago

Codecov Report

Merging #128 (3819e06) into master (39f8bfc) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #128   +/-   ##
=======================================
  Coverage   99.66%   99.66%           
=======================================
  Files          13       13           
  Lines        3290     3297    +7     
=======================================
+ Hits         3279     3286    +7     
  Misses         11       11           
Flag Coverage Δ
py3.10 99.66% <100.00%> (+<0.01%) :arrow_up:
py3.6 99.66% <100.00%> (+<0.01%) :arrow_up:
py3.7 99.66% <100.00%> (+<0.01%) :arrow_up:
py3.8 99.63% <100.00%> (+<0.01%) :arrow_up:
py3.9 99.63% <100.00%> (+<0.01%) :arrow_up:
ubuntu 98.39% <100.00%> (+<0.01%) :arrow_up:
windows 99.66% <100.00%> (+<0.01%) :arrow_up:
x64 99.66% <100.00%> (+<0.01%) :arrow_up:
x86 99.66% <100.00%> (+<0.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pypsrp/serializer.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 39f8bfc...3819e06. Read the comment docs.

malthe commented 2 years ago

@jborean93 an alternative might be adding serializer as an optional keyword argument to RunspacePool such that it's possible to extend the serializer to handle something like a tagged value.

In Airflow, I'd like to make it possible for a DAG author to wrap a string as a value tagged for serialization as a secure string, e.g. {{ conn.password | securestring }}.

For this to work I need to be able to tag the value accordingly.

jborean93 commented 2 years ago

If using the low level API it should be possible to do this today with the serialize function. You would do something like

from pypsrp.complex_objects import ObjectMeta
from pypsrp.powershell import PowerShell, RunspacePool
from pypsrp.wsman import WSMan

with WSMan("server") as wsman, RunspacePool(wsman) as rp:
    rp.exchange_keys()
    sec_string = rp.serialize("Hello World", ObjectMeta("SS"))

    ps = PowerShell(rp)
    ps.add_script("$args[0]").add_argument(sec_string)
    ps.invoke()

Granted this needs to be called after the keys are exchanged and when building the command to invoke. Will this work for you or will you still need something like what is proposed by your PR?

malthe commented 2 years ago

The issue is that I would prefer it to be declarative because at the time of definition, the runspace pool has not been set up.

It’s closer to the proposed PSSecureString wrapper in this sense (as far as I understand).

søn. 12. dec. 2021 kl. 08.28 skrev Jordan Borean @.***>:

If using the low level API it should be possible to do this today with the serialize https://github.com/jborean93/pypsrp/blob/39f8bfca403a5dd915cc5f1ee1d7dce22fa36975/pypsrp/powershell.py#L598 function. You would do something like

from pypsrp.complex_objects import ObjectMetafrom pypsrp.powershell import PowerShell, RunspacePoolfrom pypsrp.wsman import WSMan with WSMan("server") as wsman, RunspacePool(wsman) as rp: rp.exchange_keys() sec_string = rp.serialize("Hello World", ObjectMeta("SS"))

ps = PowerShell(rp)
ps.add_script("$args[0]").add_argument(sec_string)
ps.invoke()

Granted this needs to be called after the keys are exchanged and when building the command to invoke. Will this work for you or will you still need something like what is proposed by your PR?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jborean93/pypsrp/pull/128#issuecomment-991848191, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAGOJLNPHNGBKLXMDP7UU3UQRFJLANCNFSM5J3OVPTQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

jborean93 commented 2 years ago

That's fair and your proposal is a lot simpler for others to implement so I'm happy to accept the changes. It will just need the tests to be fixed.

It’s closer to the proposed PSSecureString wrapper in this sense (as far as I understand).

Just as an FYI the changes in https://github.com/jborean93/pypsrp/pull/81 have mostly been superseded by https://github.com/jborean93/pypsrp/pull/99. Essentially the PSSecureString mechanism is similar and 99 incorporates the changes in 81 and refines it futher. I've been lagging behind with getting it up to date but I'm hoping to get it ready sometime early next year.

malthe commented 2 years ago

@jborean93 thanks – tests pass now.

It seems like some code is missing from that branch which tracks the #99 changes (like the psrp.dotnet module), but I suppose that – looking toward the future – it would be more ideal if TaggedValue wasn't exposed publicly since it will be replaced by the wrappers in the dotnet module.

But I'm happy to have this present pull request merged too if you'd rather have more time to work on these bigger changes. I'm just looking for a reasonable solution that can be released soonish.

jborean93 commented 2 years ago

Thanks for the changes and fixing up the tests.

It seems like some code is missing from that branch which tracks the #99 changes (like the psrp.dotnet module), but I suppose that – looking toward the future – it would be more ideal if TaggedValue wasn't exposed publicly since it will be replaced by the wrappers in the dotnet module.

Ultimately all the serializer stuff has been moved to https://github.com/jborean93/psrpcore and I've got some documentation around the type setup at https://psrpcore.readthedocs.io/en/latest/types.html. I haven't made a release yet as I'm trying to make sure what I have there will suite the changes in #99. There are probably some other changes I have done locally (I think I've rewritten all this stuff about 5 times so far) so I can't guarantee it works in it's present state :). As for making TaggedValue less public I think it's fine as it is. When #99 comes in the plan is to deprecate the whole pypsrp namespace in favour of the psrp one which is where the new serializer will be used. This will allow people using their existing scripts to not have a breaking change and migrate to the newer code once it's available rather than me just pulling the rug out from their feet.

One thing that has bugged me is trying to make the serializer more pluggable allowing people to provide their own mechanism if they wish. The other benefit would be to make it easier to change these details with a new version without breaking backwards compatibility. So far this isn't possible which makes #99 a bit more difficult to implement. That's a problem for another day though, just thought I should share it.

malthe commented 2 years ago

Love the direction; moving to psrp will definitely help the transition because there is less confusion about the interface then.

jborean93 commented 2 years ago

Thanks for the PR, 0.7.0 is live on PyPI.