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

Add initial VNC specific password functionality #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jyundt
Copy link

@jyundt jyundt commented Dec 10, 2017

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

Copy link
Owner

@sdague sdague left a comment

Choose a reason for hiding this comment

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

I'm staring at this, and at the mess I've made of my code previously (it's been a while since I had all this in my head). I think I have some ideas about this to get it into the db and enable from there. Let me give it a shot and see what you think.

@@ -61,6 +63,11 @@ def parse_args_rm():
help='')
return parser.parse_args()

def check_valid_vnc_password(passwd):
if re.search(r'^(?=.*?[A-Z])(?=.*?[a-z])(?=.*?[0-9])(?=.*?[#?!@$%^&*-]).{8}\b$', passwd):
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather break this up into separate conditionals just to make it easier to understand later. I can do that post merge.

@sdague
Copy link
Owner

sdague commented Jan 12, 2018

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
Copy link
Author

jyundt commented Jan 13, 2018

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
Copy link
Owner

sdague commented Jan 14, 2018

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
Copy link
Owner

sdague commented Jan 14, 2018

Ok I think this is fixed in master now.

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