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 service commands to system api group #97

Merged
merged 2 commits into from
Oct 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
527 changes: 500 additions & 27 deletions client/api/system/v1alpha1/api.pb.go

Large diffs are not rendered by default.

68 changes: 68 additions & 0 deletions client/api/system/v1alpha1/api.proto
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,15 @@ option go_package = "github.com/kubernetes-csi/csi-proxy/client/api/system/v1alp
service System {
// GetBIOSSerialNumber returns the device's serial number
rpc GetBIOSSerialNumber(GetBIOSSerialNumberRequest) returns (GetBIOSSerialNumberResponse) {}

// StartService starts a Windows service
rpc StartService(StartServiceRequest) returns (StartServiceResponse) {}

// StopService stops a Windows service
rpc StopService(StopServiceRequest) returns (StopServiceResponse) {}

// GetService queries a Windows service state
rpc GetService(GetServiceRequest) returns (GetServiceResponse) {}
}

message GetBIOSSerialNumberRequest {
Expand All @@ -17,3 +26,62 @@ message GetBIOSSerialNumberResponse {
// Serial number
string serial_number = 1;
}

message StartServiceRequest {
// Service name (as listed in System\CCS\Services keys)
string name = 1;
}

message StartServiceResponse {
// Intentionally empty
}

message StopServiceRequest {
// Service name (as listed in System\CCS\Services keys)
string name = 1;

// Forces stopping of services that has dependant services
bool force = 2;
}

message StopServiceResponse {
// Intentionally empty
}


// https://docs.microsoft.com/en-us/windows/win32/api/winsvc/ns-winsvc-service_status#members
enum ServiceStatus {
SERVICE_STATUS_UNKNOWN = 0;
SERVICE_STATUS_STOPPED = 1;
SERVICE_STATUS_START_PENDING = 2;
SERVICE_STATUS_STOP_PENDING = 3;
SERVICE_STATUS_RUNNING = 4;
SERVICE_STATUS_CONTINUE_PENDING = 5;
SERVICE_STATUS_PAUSE_PENDING = 6;
SERVICE_STATUS_PAUSED = 7;
}

// https://docs.microsoft.com/en-us/windows/win32/api/winsvc/nf-winsvc-changeserviceconfiga
enum StartType {
START_TYPE_BOOT = 0;
START_TYPE_SYSTEM = 1;
START_TYPE_AUTOMATIC = 2;
START_TYPE_MANUAL = 3;
START_TYPE_DISABLED = 4;
}

message GetServiceRequest {
// Service name (as listed in System\CCS\Services keys)
string name = 1;
}

message GetServiceResponse {
// Service display name
string display_name = 1;

// Service start type
StartType start_type = 2;

// Service status
ServiceStatus status = 3;
}
12 changes: 12 additions & 0 deletions client/groups/system/v1alpha1/client_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

79 changes: 79 additions & 0 deletions integrationtests/system_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@ package integrationtests

import (
"context"
"encoding/json"
"fmt"
"os/exec"
"strings"
"testing"

"github.com/kubernetes-csi/csi-proxy/client/api/system/v1alpha1"
v1alpha1client "github.com/kubernetes-csi/csi-proxy/client/groups/system/v1alpha1"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand All @@ -31,3 +34,79 @@ func TestGetBIOSSerialNumber(t *testing.T) {
require.True(t, strings.Contains(resultString, response.SerialNumber))
})
}

func TestServiceCommands(t *testing.T) {
t.Run("GetService", func(t *testing.T) {
const ServiceName = "MSiSCSI"
client, err := v1alpha1client.NewClient()
require.Nil(t, err)
defer client.Close()

request := &v1alpha1.GetServiceRequest{Name: ServiceName}
response, err := client.GetService(context.TODO(), request)
require.NoError(t, err)
require.NotNil(t, response)

out, err := runPowershellCmd(fmt.Sprintf(`Get-Service -Name "%s" `+
`| Select-Object DisplayName, Status, StartType | ConvertTo-Json`,
ServiceName))
require.NoError(t, err)

var serviceInfo = struct {
DisplayName string `json:"DisplayName"`
Status uint32 `json:"Status"`
StartType uint32 `json:"StartType"`
}{}

err = json.Unmarshal([]byte(out), &serviceInfo)
require.NoError(t, err, "failed unmarshalling json out=%v", out)

assert.Equal(t, serviceInfo.Status, uint32(response.Status))
assert.Equal(t, serviceInfo.StartType, uint32(response.StartType))
assert.Equal(t, serviceInfo.DisplayName, response.DisplayName)
})

t.Run("Stop/Start Service", func(t *testing.T) {
const ServiceName = "MSiSCSI"
client, err := v1alpha1client.NewClient()
require.Nil(t, err)
defer client.Close()

_, err = runPowershellCmd(`Stop-Service -Name "MSiSCSI"`)
require.NoError(t, err)
assertServiceStopped(t, ServiceName)

startReq := &v1alpha1.StartServiceRequest{Name: ServiceName}
startResp, err := client.StartService(context.TODO(), startReq)

assert.NoError(t, err)
assert.NotNil(t, startResp)
assertServiceStarted(t, ServiceName)

stopReq := &v1alpha1.StopServiceRequest{Name: ServiceName}
stopResp, err := client.StopService(context.TODO(), stopReq)

assert.NoError(t, err)
assert.NotNil(t, stopResp)
assertServiceStopped(t, ServiceName)
})

}

func assertServiceStarted(t *testing.T, serviceName string) {
assertServiceStatus(t, serviceName, "Running")
}

func assertServiceStopped(t *testing.T, serviceName string) {
assertServiceStatus(t, serviceName, "Stopped")
}

func assertServiceStatus(t *testing.T, serviceName string, status string) {
out, err := runPowershellCmd(fmt.Sprintf(`Get-Service -Name "%s" | `+
`Select-Object -ExpandProperty Status`, serviceName))
if !assert.NoError(t, err, "Failed getting service out=%s", out) {
return
}

assert.Equal(t, strings.TrimSpace(out), status)
}
61 changes: 61 additions & 0 deletions internal/os/system/api.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package system

import (
"encoding/json"
"fmt"
"os"
"os/exec"
"strings"
)
Expand All @@ -11,6 +13,17 @@ import (
// internal/server/system/server.go so that logic can be easily unit-tested
// without requiring specific OS environments.

type ServiceInfo struct {
// Service display name
DisplayName string `json:"DisplayName"`

// Service start type
StartType uint32 `json:"StartType"`

// Service status
Status uint32 `json:"Status"`
}

type APIImplementor struct{}

func New() APIImplementor {
Expand All @@ -37,3 +50,51 @@ func (APIImplementor) GetBIOSSerialNumber() (string, error) {
}
return lines[1], nil
}

func (APIImplementor) GetService(name string) (*ServiceInfo, error) {
script := `Get-Service -Name $env:ServiceName | Select-Object DisplayName, Status, StartType | ` +
`ConvertTo-JSON`
cmd := exec.Command("powershell", "/c", script)
cmd.Env = append(os.Environ(), fmt.Sprintf("ServiceName=%s", name))

out, err := cmd.CombinedOutput()
if err != nil {
return nil, fmt.Errorf("error querying service name=%s. cmd: %s, output: %s, error: %v", name, cmd, string(out), err)
}

var serviceInfo ServiceInfo
err = json.Unmarshal(out, &serviceInfo)
if err != nil {
return nil, err
}

return &serviceInfo, nil
}

func (APIImplementor) StartService(name string) error {
script := `Start-Service -Name $env:ServiceName`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to set the servicename parameter through an environment variable here? Why not use sprintf directly here rather than a couple of lines below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, thanks for the review! :)

I've built this upon the comments that were left in the smb api group:

func (APIImplementor) NewSmbGlobalMapping(remotePath, username, password string) error {
// use PowerShell Environment Variables to store user input string to prevent command line injection
// https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_environment_variables?view=powershell-5.1
cmdLine := fmt.Sprintf(`$PWord = ConvertTo-SecureString -String $Env:smbpassword -AsPlainText -Force` +
`;$Credential = New-Object -TypeName System.Management.Automation.PSCredential -ArgumentList $Env:smbuser, $PWord` +
`;New-SmbGlobalMapping -RemotePath $Env:smbremotepath -Credential $Credential`)

I tried to find more data online about Powershell command injection, but I haven't found anyone referring to env variables. I did find this link which might indicate that env variables could prevent injection.
Anyway, if this really solves injection then it's a good idea to add this in all csi-proxy commands

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for pointing out the link! Makes complete sense to use this protection in case the csi-proxy is being used to invoke arbitrary commands in a malicious context on the host.

cmd := exec.Command("powershell", "/c", script)
cmd.Env = append(os.Environ(), fmt.Sprintf("ServiceName=%s", name))

out, err := cmd.CombinedOutput()
if err != nil {
return fmt.Errorf("error starting service name=%s. cmd: %s, output: %s, error: %v", name, cmd, string(out), err)
}

return nil
}

func (APIImplementor) StopService(name string, force bool) error {
script := `Stop-Service -Name $env:ServiceName -Force:$([System.Convert]::ToBoolean($env:Force))`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as before: Is there a reason to set the servicename and force parameters through environment variables here? Why not use sprintf directly rather than a couple of lines below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above, tell me what you think

cmd := exec.Command("powershell", "/c", script)
cmd.Env = append(os.Environ(),
fmt.Sprintf("ServiceName=%s", name),
fmt.Sprintf("Force=%t", force))

out, err := cmd.CombinedOutput()
if err != nil {
return fmt.Errorf("error stopping service name=%s. cmd: %s, output: %s, error: %v", name, cmd, string(out), err)
}

return nil
}
60 changes: 60 additions & 0 deletions internal/server/system/internal/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,63 @@ type GetBIOSSerialNumberRequest struct {
type GetBIOSSerialNumberResponse struct {
SerialNumber string
}

type StartServiceRequest struct {
// Service name (as listed in System\CCS\Services keys)
Name string
}

type StartServiceResponse struct {
// Intentionally empty
}

type StopServiceRequest struct {
// Service name (as listed in System\CCS\Services keys)
Name string

// Forces stopping of services that has dependant services
Force bool
}

type StopServiceResponse struct {
// Intentionally empty
}

type ServiceStatus uint32

const (
SERVICE_STATUS_UNKNOWN = 0
SERVICE_STATUS_STOPPED = 1
SERVICE_STATUS_START_PENDING = 2
SERVICE_STATUS_STOP_PENDING = 3
SERVICE_STATUS_RUNNING = 4
SERVICE_STATUS_CONTINUE_PENDING = 5
SERVICE_STATUS_PAUSE_PENDING = 6
SERVICE_STATUS_PAUSED = 7
)

type Startype uint32

const (
START_TYPE_BOOT = 0
START_TYPE_SYSTEM = 1
START_TYPE_AUTOMATIC = 2
START_TYPE_MANUAL = 3
START_TYPE_DISABLED = 4
)

type GetServiceRequest struct {
// Service name (as listed in System\CCS\Services keys)
Name string
}

type GetServiceResponse struct {
// Service display name
DisplayName string

// Service start type
StartType Startype

// Service status
Status ServiceStatus
}
3 changes: 3 additions & 0 deletions internal/server/system/internal/types_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading