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

Ability to provide username/password for remote administrator account #58

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mmooney
Copy link
Member

@mmooney mmooney commented Nov 4, 2013

This is a second (better) attempt at #56.

It's more complete, plus I had screwed up the other pull request and included some code from another feature.

The core change is to add an option WMI username/password to the WmiHelper.Connect method, so that you can impersonate an administrator account on the remote machine.

This is necessary in scenarios like Amazon EC2 where you don't have domain users who have access to multiple boxes. Instead you usually have local administrator accounts on each machine.

What's included:

  • The WinService methods have been expanded to take a WMI user name and password. If provided, they will use the WmiService class instead of ServiceController to manage the service. If those credentials are not provided, it will use the existing ServiceController code (obviously it would be better to use the same code for both situations, but I was trying to reduce the chance of introducing any issues)
  • There is a OpenFolderShareAuthentication task, which will takes a UNC and user account credentials, and will open a share with those credentials. After that point, anything accessing the remote system (like XmlPoke, CopyDirectory, SecurityOptions.ForPath, etc) will automatically use that net share session.

What's not included (yet):

  • IIS management - Since ServerManager doesn't take a username and password, and I'm pretty sure LogonUser/ImpersonateUser won't work, another (more complicated approach) would be required. In the meantime, since the IIS setup is usually a one-time thing, I think the current enhancements have a lot of value even with the IIS stuff.
  • SecurityOptions.LocalPolicy - I tried to find a way to pass credentials to the LsaUtility/LsaSecurity stuff, which not very well documented, but was unsuccessful. Again, this is usually one-time setup, I think these changes are valuable (at least to me) without the local policy stuff.

Testing:

  • I copied the existing WinService tests to test them against a remote VM. The first time you run the tests, it will give you an error that you need to create a settings file (with the machine name, user name, and password). For my testing, I spun up a dumb Amazon VM for this purpose, which I'd be happy to give you guys access to if you wanted to test it out. Otherwise, I'll spin it down and save the AMI in a public place so it could be used for future testing. ALSO, setting up the remote WMI access in Amazon is no trivial task, so I wrote up the steps in a blog post: http://mooneyblog.mmdbsolutions.com/index.php/2013/11/01/accessing-an-amazon-vm-through-wmi/
  • In the comments of the original PR, I had mentioned pulling the static WMI classes apart into some instance objects, so that they could be testable/mockable/etc. I got pretty far into this, but it was REALLY invasive, rewriting large chunks of the WMI code, and it was almost certainly going to introduce more bugs that it was fixing. So instead, I put in the some integration tests
  • Also my main goal is to get this working so that my Team City build server (in Amazon) can publish an application to another Amazon EC2 instance. Coincidentally the application it is deploying is a automated deployment system (like Octopus, but with some differences). Anyhow, I've been dogfooding it by using this branch to deploy that application. If you wanted to see it in action, the code is at https://github.com/mmooney/Sriracha.Deploy, and the DropkicK code is in the Sriracha.Deploy.SelfDeploy project.

Thanks,
Mike

@@ -31,7 +31,7 @@ public override DeploymentResult VerifyCanRun()
}
else
{
result.AddAlert(string.Format("Can't find service '{0}'", ServiceName));
result.AddError(string.Format("Can't find service '{0}'", ServiceName));
Copy link
Member

Choose a reason for hiding this comment

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

@drusellers you good with this change? I don't remember why we do alerts versus errors in verifycanrun

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I wasn't real clear on the Alert/Note distinction. I had switched that to get to get it to return ContainsError()=true (which checks for Error but not Alert).

But in hindsight, that would probably be a breaking change for someone. Sure, it probably should be considered an failing error (you can't very well start a service that doesn't exist), but I'm sure that will hose someone's existing deployment. Not sure how much backwards compatibility you guys need,

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with this change. My guess is that Alert has to do with log level later. I would say that for most of us a failing to start is worth an error.

Copy link
Member

Choose a reason for hiding this comment

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

I'm good with this then. :)

@ferventcoder
Copy link
Member

So I reviewed this and so far looks pretty good. Here's what I'm not digging. You are branching on whether the username/password are null. I think it could be handled in a more elegant way that doesn't include repeating code again and again. As in deciding what to do when those are null further down the pipe. I'm also not really digging wmiUserName/wmiPassword - I'd rather see some sort of context privileges. Also is dropkick designed in such a way that really required that many items to be changed to allow for you to specify wmi privileges? It seems very intrusive to have it required this many times.

@mmooney
Copy link
Member Author

mmooney commented Nov 8, 2013

Thanks Rob. It definitely was a pain to be peppering the WMI username/password everywhere, but I was assuming that it would keep the functionality of the individual tasks isolated. But yeah, it would make so do it higher up in the stack. Were you thinking something like an extension to the ProtoServer itself to define the remote server credentials, and then any tasks after that that used WMI or UNC would pick those up? Or something high up than that?

Not sure what you mean with the context privileges. We need to have username/password somewhere, the use case for this is two different machines not on the same domain or anything like that, and the target machine doesn't care anything about the user on the source machine, so we need the username/password to create an admin context on the target machine.

Internally, the WMI stuff is in some static objects, so they take all of the parameters necessary to perform the specific action. So far it was just machine name and then action-specific parameters like service name, but with this it now has to pass the username/password along with the machine name all over the place. Yeah it's a little ugly.

But then if we move it up higher, and define the WMI credentials with an extension to the ProtoServer, then we could store that in some sort of ambient context somewhere (not sure where, but we could find something), and then have WmiService/WmiHelper/etc use that context where available. Heck, it could probably even store the ManagementScope there, so we could only have to create it once per role, and that could help speed it up a little. Of course, if the context was not there, it would revert to the existing functionality; that last thing I'd want to do is bust up existing deployments for people.

@ferventcoder
Copy link
Member

@mmooney Yes, definitely an extension where the wmi user/pass is passed at a higher level and then used when appropriate. I'm trying to think of a case where you would need to pass differing credentials to a single role deployment to a server and while there might be some advanced scenarios, I think that can be overcome by running a second time pushing a different role with credentials for it.

So yeah, I'd like to see somewhere that this is defined at the top level and then used.

@kiquenet
Copy link

@ferventcoder
Copy link
Member

Not dead. Already answered on #60

@mmooney
Copy link
Member Author

mmooney commented May 12, 2014

Oh wow, I never got back to resubmitting this with the latest changes. I'll will try to get to that ASAP. Thanks for the reminder @kiquenet! :)

@mmooney
Copy link
Member Author

mmooney commented May 28, 2014

So I have this change just about done, you can call a WithAuthentication extension on the ProtoServer, and that will store the WMI username and password for later, and then later when the WmiHelper class is called it will look whether those values have been set, so it doesn't need to get passed around so much. That was a great idea, it felt good to remove all of that unnecessary code.

My only question is the best place to put the username and password. I saw the HUB object which stores the settings, but was also marked with a "YUCK". I was figuring to put a pair of ThreadStatic (or regular static) fields on the WmiHelper class, and the WithAuthentication method would just pass those values down to the WmiHelper class to be used later. I think WmiHelper is the only thing that would actually use those values, so I figure it makes sense to store them there.

Thoughts?

@mmooney
Copy link
Member Author

mmooney commented May 28, 2014

I just updated to to store the values as ThreadStatic fields in WmiHelper, let me know if you want it moved to the HUB class or somewhere else.

Thanks

@drusellers
Copy link
Member

I would really prefer to find a way to not use a static - can it not be passed in?

@mmooney
Copy link
Member Author

mmooney commented May 29, 2014

Hi @drusellers, yeah that was the first version, it was passed along to each task using a WithAuthentication extension method on each, but @ferventcoder asked that it be changed to something that is specified in the beginning and then can be used by any task. #58 (comment) (unless I missed the point which of course is always possible :) ). It did simplify the code a lot, pulling out all of the store-and-pass-along logic that I had to add to all of the various tasks, instead it was just set into the WmiHelper and then would just automatically get used regardless of the specific task being executed.

I was trying to find something was an ambient context that could be set once and then used by all of the tasks, without being a explicit static. The ThreadStatic seemed like a less-bad way than a normal static, because at least it was scoped to the current thread instead of being a global static. I searched through the dropkick code for something similar, but the closest I could find was the HUB class, which of course is commented with a "YUCK". Let me know if there is another place I should be storing that info, or if I should change it back to the passing it into each task.

Thanks,
Mike

@drusellers
Copy link
Member

Yeah, I need to add the ambient concept to DK - ok - thank you for the explanation. :) @ferventcoder you ok with all of this?

@ferventcoder
Copy link
Member

So far yes but I didn't dig into details. We need new maintainers on dk. ;)

On Thursday, May 29, 2014, Dru Sellers [email protected] wrote:

Yeah, I need to add the ambient concept to DK - ok - thank you for the
explanation. :) @ferventcoder https://github.com/ferventcoder you ok
with all of this?


Reply to this email directly or view it on GitHub
#58 (comment).

Rob
"Be passionate in all you do"

http://devlicio.us/blogs/rob_reynolds
http://ferventcoder.com
http://twitter.com/ferventcoder

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.

4 participants