Merge pull request #7059 from habitat-sh/win_default_user

do not use hab as default user on windows

.expeditor/end_to_end.pipeline.yml

@@ -97,6 +97,17 @@ steps:
             - BUILD_PKG_TARGET=x86_64-windows
             - BUILDKITE_AGENT_ACCESS_TOKEN
 
+  - label: "[:windows: hab-svc-load-with-hab-user]"
+    command:
+      - powershell .expeditor/scripts/end_to_end/run_e2e_test.ps1 DEV test_supervisor_load_with_hab_user
+    expeditor:
+      executor:
+        docker:
+          host_os: windows
+          environment:
+            - BUILD_PKG_TARGET=x86_64-windows
+            - BUILDKITE_AGENT_ACCESS_TOKEN
+
   - label: "[:linux: test_launcher_checks_supervisor_version]"
     command:
       - .expeditor/scripts/end_to_end/setup_environment.sh DEV

components/common/src/templating/package.rs

@@ -19,7 +19,7 @@ use std::{collections::{BTreeMap,
           path::PathBuf,
           result};
 
-const DEFAULT_USER: &str = "hab";
+pub const DEFAULT_USER: &str = "hab";
 const DEFAULT_GROUP: &str = "hab";
 
 const PATH_KEY: &str = "PATH";
@@ -193,19 +193,21 @@ fn get_user_and_group(pkg_install: &PackageInstall) -> Result<(String, String)>
 }
 
 /// check and see if a user/group is specified in package metadata.
-/// if not, we'll try and use hab/hab.
-/// If hab/hab doesn't exist, try to use (current username, current group).
+/// if not, we'll use the current user that the process is running under.
+/// If hab/hab (default) is specified but doesn't exist, use the current username.
 /// If that doesn't work, then give up.
-/// Windows will also check if hab exists if it was the given user name
-/// If it does not exist then fall back to the current username
-/// This is because historically windows plans defaulted to
-/// the hab pkg_svc_user even if not explicitly provided
+/// Note that in all releases through 0.88.0, hab packaged a svc_user value
+/// of 'hab' unless specified otherwise in a plan. So for all packages built
+/// by those releases, a svc_user should always be specified, but as already
+/// stated, we do check to see if the user exists. This turns out to do more
+/// harm than good on windows especially if there is a hab user on the system
+/// that was not intended to run habitat services.
 #[cfg(windows)]
 fn get_user_and_group(pkg_install: &PackageInstall) -> Result<(String, String)> {
     match get_pkg_user_and_group(&pkg_install)? {
         Some((ref user, ref _group)) if user == DEFAULT_USER => Ok(default_user_and_group()?),
         Some((user, group)) => Ok((user, group)),
-        _ => Ok(default_user_and_group()?),
+        _ => Ok(current_user_and_group()?),
     }
 }
 
@@ -233,19 +235,23 @@ fn default_user_and_group() -> Result<(String, String)> {
         (Some(_), Some(_)) => Ok((DEFAULT_USER.to_string(), DEFAULT_GROUP.to_string())),
         _ => {
             debug!("hab:hab does NOT exist");
-            let user = users::get_current_username();
-            let group = users::get_current_groupname();
-            match (user, group) {
-                (Some(user), Some(group)) => {
-                    debug!("Running as {}/{}", user, group);
-                    Ok((user, group))
-                }
-                _ => {
-                    Err(Error::PermissionFailed("Can't determine current \
-                                                 user:group"
-                                                            .to_string()))
-                }
-            }
+            current_user_and_group()
+        }
+    }
+}
+
+fn current_user_and_group() -> Result<(String, String)> {
+    let user = users::get_current_username();
+    let group = users::get_current_groupname();
+    match (user, group) {
+        (Some(user), Some(group)) => {
+            debug!("Running as {}/{}", user, group);
+            Ok((user, group))
+        }
+        _ => {
+            Err(Error::PermissionFailed("Can't determine current \
+                                         user:group"
+                                                    .to_string()))
         }
     }
 }

components/hab/habitat/plan.ps1

@@ -82,3 +82,7 @@ function Invoke-Install {
     Copy-Item "$(Get-HabPackagePath "xz")/bin/*.dll" "$pkg_prefix/bin"
     Copy-Item "$(Get-HabPackagePath "visual-cpp-redist-2015")/bin/*.dll" "$pkg_prefix/bin"
 }
+
+function Invoke-Clean {
+    if(!$env:HAB_SKIP_CLEAN) { Invoke-DefaultClean }
+}

components/plan-build-ps1/bin/hab-plan-build.ps1

@@ -110,9 +110,13 @@ $script:pkg_expose = @()
 # An associative array representing configuration data which should be gossiped to peers.
 $script:pkg_exports = @{}
 # The user to run the service as
-$script:pkg_svc_user = "hab"
-# The group to run the service as
-$script:pkg_svc_group = "$pkg_svc_user"
+$script:pkg_svc_user = ""
+# svc_group is not actually used on Windows but it needs to exist.
+# The Pkg struct in the common crate expects both user and group to be populated
+# otherwise it handles neither. Ideally we would have a windows only implementation
+# that just ignored the group. That would be trivial to add but risks backward
+# compatibility with new packages without SVC_GROUP and older supervisors
+$script:pkg_svc_group = "hab"
 
 # Initially set $pkg_svc_* variables. This happens before the Plan is sourced,
 # meaning that `$pkg_name` is not yet set. However, `$pkg_svc_run` wants

components/sup/habitat/plan.ps1

@@ -87,3 +87,7 @@ function Invoke-Install {
     Copy-Item "$(Get-HabPackagePath "zeromq")/bin/*.dll" "$pkg_prefix/bin"
     Copy-Item "$(Get-HabPackagePath "visual-cpp-redist-2015")/bin/*.dll" "$pkg_prefix/bin"
 }
+
+function Invoke-Clean {
+    if(!$env:HAB_SKIP_CLEAN) { Invoke-DefaultClean }
+}

components/sup/src/manager/service.rs

@@ -53,13 +53,15 @@ pub use habitat_common::templating::{config::{Cfg,
                                                PkgProxy}};
 use habitat_common::{outputln,
                      templating::{config::CfgRenderer,
-                                  hooks::Hook}};
+                                  hooks::Hook,
+                                  package::DEFAULT_USER}};
 use habitat_core::{crypto::hash,
                    fs::{atomic_write,
                         svc_hooks_path,
                         SvcDir,
                         FS_ROOT_PATH},
-                   os::process::ShutdownTimeout,
+                   os::{process::ShutdownTimeout,
+                        users},
                    package::{metadata::Bind,
                              PackageIdent,
                              PackageInstall},
@@ -242,7 +244,7 @@ impl Service {
                     -> Result<Service> {
         spec.validate(&package)?;
         let all_pkg_binds = package.all_binds()?;
-        let pkg = Pkg::from_install(&package)?;
+        let pkg = Self::resolve_pkg(&package, &spec)?;
         let spec_file = manager_fs_cfg.specs_path.join(spec.file());
         let service_group = ServiceGroup::new(spec.application_environment.as_ref(),
                                               &pkg.name,
@@ -285,6 +287,34 @@ impl Service {
                      shutdown_timeout: spec.shutdown_timeout })
     }
 
+    // And now prepare yourself for a little horribleness...Ready?
+    // In releases 0.88.0 and prior, we would run hooks under
+    // the hab user account on windows if it existed and no other
+    // svc_user was specified just like we do on linux. That is problematic
+    // and not a ubiquitous pattern for windows. The default user is now
+    // always the current user. However, packages built on those older
+    // versions included a SVC_USER metafile with the 'hab' user by default.
+    // So to protect for scenarios where a user has launched an older package,
+    // is on windows and has a 'hab' account on the system BUT never intended
+    // to run hooks under that account and therefore has not passed a
+    // '--password' argument to 'hab svc load', we will revert the user to
+    // the current user.
+    #[cfg(windows)]
+    fn resolve_pkg(package: &PackageInstall, spec: &ServiceSpec) -> Result<Pkg> {
+        let mut pkg = Pkg::from_install(&package)?;
+        if spec.svc_encrypted_password.is_none() && pkg.svc_user == DEFAULT_USER {
+            if let Some(user) = users::get_current_username() {
+                pkg.svc_user = user;
+            }
+        }
+        Ok(pkg)
+    }
+
+    #[cfg(unix)]
+    fn resolve_pkg(package: &PackageInstall, spec: &ServiceSpec) -> Result<Pkg> {
+        Ok(Pkg::from_install(&package)?)
+    }
+
     /// Returns the config root given the package and optional config-from path.
     fn config_root(package: &Pkg, config_from: Option<&PathBuf>) -> PathBuf {
         config_from.and_then(|p| Some(p.as_path()))

components/win-users/src/account.rs

@@ -67,7 +67,7 @@ fn lookup_account(name: &str, system_name: Option<String>) -> Option<Account> {
         ERROR_INSUFFICIENT_BUFFER => {}
         ERROR_NONE_MAPPED => return None,
         _ => {
-            error!("Error while looking up account for {}: {}",
+            debug!("Error while looking up account for {}: {}",
                    name,
                    Error::last_os_error());
             return None;

test/end-to-end/test_supervisor_load_service_with_password.ps1

@@ -8,7 +8,7 @@ $lsa_wrapper.SetRight($username, "SeServiceLogonRight")
 
 $env:HAB_ORIGIN = "ci"
 hab origin key generate ci
-hab pkg build test/fixtures/dummy_windows_plan
+hab pkg build test/fixtures/windows_plans/dummy_svc_user
 . .\results\last_build.ps1
 hab pkg install .\results\$pkg_artifact
 

test/end-to-end/test_supervisor_load_with_hab_user.ps1

@@ -0,0 +1,69 @@
+# This endures that given a 'hab' user, a plan with no pkg_svc_user will
+# not produce a service that will be run by the hab user. This used to be
+# the case but we are no longer using the hab account by default on windows.
+# see https://github.com/habitat-sh/habitat/issues/6847
+$username = "hab"
+$password = "Pass@word1"
+net user /add $username $password
+net localgroup administrators $username /add
+Add-Type -TypeDefinition (Get-Content ".expeditor\scripts\end_to_end\LsaWrapper.cs" | Out-String)
+$lsa_wrapper = New-Object -type LsaWrapper
+$lsa_wrapper.SetRight($username, "SeServiceLogonRight")
+
+$env:HAB_ORIGIN = "ci"
+hab origin key generate ci
+
+hab pkg install core/windows-service
+hab pkg exec core/windows-service install
+Start-Service Habitat
+Wait-Supervisor -Timeout 45
+
+Describe "with no svc_user" {
+    hab pkg build test/fixtures/windows_plans/dummy
+    . .\results\last_build.ps1
+    hab pkg install .\results\$pkg_artifact
+    $loadOut = hab svc load ci/dummy
+    Start-Sleep -Seconds 5
+
+    It "does not create a SVC_USR metafile" {
+        Test-Path c:\hab\pkgs\$pkg_ident\SVC_USER | Should -Be $false
+    }
+    It "Succesfully loads service" {
+        $loadOut | Should -Be "The ci/dummy service was successfully loaded"
+    }
+    It "Reports service on HTTP Gateway as UP" {
+        ((Invoke-WebRequest "http://localhost:9631/services/dummy/default" -UseBasicParsing).content | ConvertFrom-Json).process.state | Should -Be "up"
+    }
+    It "runs hook as current user" {
+        # the dummy run hook simply runs ping continuously
+        $proc = Get-Process ping -IncludeUserName
+        $proc.UserName | Should -Be "NT AUTHORITY\SYSTEM"
+    }
+    AfterAll {
+        hab svc unload ci/dummy
+        Start-Sleep -Seconds 5 # ping needs to be forcefully shutdown
+    }
+}
+
+Describe "with svc_user" {
+    hab pkg build test/fixtures/windows_plans/dummy_hab_svc_user
+    . .\results\last_build.ps1
+    hab pkg install .\results\$pkg_artifact
+    $loadOut = hab svc load ci/dummy-hab-user --password $password
+    Start-Sleep -Seconds 5
+
+    It "does create a SVC_USR metafile" {
+        Test-Path c:\hab\pkgs\$pkg_ident\SVC_USER | Should -Be $true
+    }
+    It "Succesfully loads service" {
+        $loadOut | Should -Be "The ci/dummy-hab-user service was successfully loaded"
+    }
+    It "Reports service on HTTP Gateway as UP" {
+        ((Invoke-WebRequest "http://localhost:9631/services/dummy-hab-user/default" -UseBasicParsing).content | ConvertFrom-Json).process.state | Should -Be "up"
+    }
+    It "runs hook as current user" {
+        # the dummy run hook simply runs ping continuously
+        $proc = Get-Process ping -IncludeUserName
+        $proc.UserName | Should -Be "$env:computername\hab"
+    }
+}

test/fixtures/windows_plans/dummy/plan.ps1

@@ -0,0 +1,3 @@
+$pkg_name="dummy"
+$pkg_origin="ci"
+$pkg_version="0.1.0"

test/fixtures/windows_plans/dummy_hab_svc_user/hooks/init

@@ -0,0 +1 @@
+write-host "I am initializing"

test/fixtures/windows_plans/dummy_hab_svc_user/hooks/install

@@ -0,0 +1 @@
+write-host "I am installing"

test/fixtures/windows_plans/dummy_hab_svc_user/hooks/run

@@ -0,0 +1,2 @@
+Write-host "I am running"
+ping -t localhost

test/fixtures/windows_plans/dummy_hab_svc_user/plan.ps1

@@ -0,0 +1,4 @@
+$pkg_name="dummy-hab-user"
+$pkg_origin="ci"
+$pkg_version="0.1.0"
+$pkg_svc_user="hab"

test/fixtures/windows_plans/dummy_svc_user/hooks/init

@@ -0,0 +1 @@
+write-host "I am initializing"

test/fixtures/windows_plans/dummy_svc_user/hooks/install

@@ -0,0 +1 @@
+write-host "I am installing"

test/fixtures/windows_plans/dummy_svc_user/hooks/run

@@ -0,0 +1,2 @@
+Write-host "I am running"
+ping -t localhost