sdague / amt

Python tools for interacting with Intel's AMT hardware control interfaces
Apache License 2.0
64 stars 30 forks source link

Add initial VNC specific password functionality #25

Open jyundt opened 6 years ago

jyundt commented 6 years ago

Without making changes to the existing argparse config, I wasn't sure of the best way to add a "prompt" flag for the vnc enable command.

This current PR will detect if the AMT password can't be used as a VNC password, and if so, will prompt the user for a VNC specific password.

@sdague lemme know your thoughts on this. I'm not really sold on ^this^ idea but I figured it would be a starting point at least.

Ref #24


This change is Reviewable

sdague commented 6 years ago

Ok. First, I apologize a lot for not getting to this patch over the holidays. You patience is really welcomed.

In trying to figure out how to get this added more as an argument, I ended up adding a new optional -V to the amtctrl set command to handle VNC password differently from regular password. It is still missing validation and better error handling, but at least this creates the structure to get this in and use that value instead of the main password.

Can you take upstream master and see if it's working for you? And then feel free to enhance with the better validation you had in this patch. Thanks much for bringing up the issue and taking a first stab here, sorry again for this taking so long, and me having to do some structural changes out from under you to get the code ready for this.

jyundt commented 6 years ago

No need to apologize!

I tried upstream but was getting some 401s when trying to perform on/off/status operations.

I think the addition of vncpassword was clobbering the username parameter. I made a quick change and I think it looks ok. Can you verify?

+++ b/amt/client.py
@@ -70,8 +70,8 @@ class Client(object):
     Manage interactions with AMT host.
     """
     def __init__(self, address, password,
-                 username='admin', protocol='http',
-                 vncpasswd=None):
+                 vncpassword=None, username='admin',
+                 protocol='http'):
         port = AMT_PROTOCOL_PORT_MAP[protocol]
         path = '/wsman'
         self.uri = "%(protocol)s://%(address)s:%(port)s%(path)s" % {
@@ -81,7 +81,7 @@ class Client(object):
             'path': path}
         self.username = username
         self.password = password
-        self.vncpassword = vncpasswd
+        self.vncpassword = vncpassword

     def post(self, payload, ns=None):
         resp = requests.post(self.uri,
sdague commented 6 years ago

Ah, good catch. Man do I need more real testing in this code.... :)

The way this really should be corrected is by changing how the Client is constructed. Adding the vncpasswd parameter where I did was intentional, because it means the API is backwards compatible. We just need to add kwarg calling for the new parameter when the client is constructed.

sdague commented 6 years ago

Ok I think this is fixed in master now.