Support running SPIRE as a Windows service #3625

amartinezfayo · 2022-11-21T16:24:10Z

Add support in SPIRE to run both SPIRE Server and SPIRE Agent as a Windows service.

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

guilhermocc · 2022-11-22T14:14:46Z

pkg/common/entrypoint/entrypoint_windows_test.go

+
+			status := <-testCase.sc.statusCh
+			assert.Equal(t, svc.Running, status.State)
+


Hey @amartinezfayo, what do you think about making an interrogation here so we can cover this path asserting it returns the current state?

Suggested change

// Interrogate the service, which should return the current status.

testCase.sc.changeRequestCh <- svc.ChangeRequest{Cmd: svc.Interrogate}

status = <-testCase.sc.statusCh

MarcosDY · 2022-11-25T17:08:01Z

cmd/spire-agent/main.go

 )

 func main() {
-	os.Exit(new(cli.CLI).Run(os.Args[1:]))
+	os.Exit(entrypoint.NewEntryPoint(new(cli.CLI).Run).Main())


I'm worried that with this approach, any command will try to verify if our binary is running as a service,
but that is only true for "run" commands

We discussed this and we agreed that in order to avoid blocking SPIRE to run only because the svc.IsWindowsService() fails, we will fallback to run SPIRE as a regular process if IsWindowsService() returns an error.

MarcosDY · 2022-11-25T17:18:32Z

cmd/spire-server/cli/run/run.go

+	ctx := cmd.ctx
+	if ctx == nil {
+		ctx = context.Background()
+	}


may this initialization be moved to newRunCommand or NewRunCommand?
I know this is a double check... but sounds like if we require a context it must be set on struct initialization

I'm not sure if there is a real benefit of doing that. Currently, the context is being initialized here also, there is no change with that (https://github.com/spiffe/spire/blob/main/cmd/spire-server/cli/run/run.go#L244).
The cost of moving this to newRunCommand would be that if the struct is being initialized without calling the newRunCommand function (like we currently do in unit tests), we would need to set ctx there. The intention of having it here is that a context is always provided if not specified.
What do you think?

MarcosDY · 2022-11-25T17:19:53Z

doc/spire_agent.md

@@ -166,6 +166,13 @@ the following flags are available:
 | `-trustBundleUrl`                | URL to download the SPIRE server CA bundle                                          |                       |
 | `-trustDomain`                   | The trust domain that this agent belongs to (should be no more than 255 characters) |                       |

+#### Running SPIRE Agent as a Windows service
+
+On Windows platform, SPIRE Agent can optionally be run as a Windows service. When running as a Windows service, the only command supported is the `run` command.


how is this validation done? may we prevent it programmatically? what happens if a user try to create a service that run ./bin/spire-server bundle show (it is unrealistic example)

I'll add a validation to fail with ERROR_BAD_ARGUMENTS if an argument indicating a command different to "run" is used when running as a service.

MarcosDY · 2022-11-25T17:22:30Z

doc/spire_agent.md

+On Windows platform, SPIRE Agent can optionally be run as a Windows service. When running as a Windows service, the only command supported is the `run` command.
+
+_Note: SPIRE does not automatically create the service in the system, it must be created by the user.
+When starting the service, all the arguments to execute SPIRE Agent with the `run` command must be passed as service arguments._


it is something useful to provide an example here of a configuration that allows this?

Sounds good. I'll add an example of how the service can be created and how the service can be run.

MarcosDY · 2022-11-25T17:42:24Z

pkg/common/entrypoint/entrypoint_windows.go

+		handler: &service{
+			RunFn: func(ctx context.Context, stop context.CancelFunc, args []string) int {
+				retCode := runCmdFn(ctx, args[1:])
+				stop()


may we move stop to a defer?

MarcosDY · 2022-11-25T17:52:29Z

pkg/common/entrypoint/entrypoint_windows_test.go

+				changeRequestCh:  make(chan svc.ChangeRequest, 1),
+				statusCh:         make(chan svc.Status, 1),


NIT: those channels can be created on test body, when isWindowsService == true

MarcosDY · 2022-11-25T17:55:19Z

pkg/common/entrypoint/entrypoint_windows_test.go

+				ep := newEntryPoint(func(ctx context.Context, args []string) int {
+					return testCase.retCode
+				}, testCase.sc)
+				retCodeCh <- ep.Main()


when main is closed? may we add a select with this + ctx.Done()?

MarcosDY · 2022-11-25T18:00:00Z

pkg/common/entrypoint/entrypoint_windows_test.go

+			// This is running as a service.
+			// Check if we expect a failure running the service.
+			if testCase.retCode != 0 {
+				assert.Equal(t, testCase.retCode, <-retCodeCh)


how about adding a select here too? so we can be sure we return something before our ctx is finished (ctx with timeout)

MarcosDY · 2022-11-25T18:05:10Z

pkg/common/entrypoint/entrypoint_windows_test.go

+				// Final status should be Stopped.
+				assert.Equal(t, svc.Stopped, (<-testCase.sc.statusCh).State)
+
+				assert.Equal(t, false, testCase.sc.svcSpecificEC)


assert.IsFalse

MarcosDY · 2022-11-25T18:11:47Z

pkg/common/entrypoint/service_windows.go

+	}
+
+	status <- svc.Status{State: svc.StopPending}
+	stop()


defer after creation?

This needs to be here to prevent races with the retCode.

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

MarcosDY

looks great a couple comments on docs to pass lints

MarcosDY · 2023-01-03T21:03:57Z

doc/spire_agent.md

@@ -173,6 +173,18 @@ On Windows platform, SPIRE Agent can optionally be run as a Windows service. Whe
 _Note: SPIRE does not automatically create the service in the system, it must be created by the user.
 When starting the service, all the arguments to execute SPIRE Agent with the `run` command must be passed as service arguments._

+##### Example to create the SPIRE Agent Windows service
+
+```


you will need to add type here

Type is already added, I think that this was commented from a previous commit.

MarcosDY · 2023-01-03T21:05:21Z

doc/spire_agent.md

+##### Example to create the SPIRE Agent Windows service
+
+```
+sc.exe create spire-agent binpath=c:\spire\bin\spire-agent.exe


since it is a shell may we add $ at the start?

> probably fits better for a Windows terminal / command prompt?

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

MarcosDY

LGTM!!!

* Support running SPIRE as a Windows service Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Support running SPIRE as a Windows service

91676fa

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

amartinezfayo added this to the 1.5.2 milestone Nov 21, 2022

amartinezfayo requested review from evan2645, azdagron, MarcosDY and rturner3 as code owners November 21, 2022 16:24

guilhermocc reviewed Nov 22, 2022

View reviewed changes

azdagron assigned MarcosDY Nov 22, 2022

MarcosDY reviewed Nov 25, 2022

View reviewed changes

amartinezfayo modified the milestones: 1.5.2, 1.5.3 Dec 1, 2022

amartinezfayo added 2 commits January 3, 2023 12:25

Address PR comments

43b918a

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Fix linter

3d5a483

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

MarcosDY reviewed Jan 4, 2023

View reviewed changes

Address PR comments

0186320

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

MarcosDY approved these changes Jan 4, 2023

View reviewed changes

Merge branch 'main' into win-service-support

7634851

amartinezfayo merged commit a7df8a5 into spiffe:main Jan 4, 2023

amartinezfayo deleted the win-service-support branch March 1, 2023 17:59

stevend-uber pushed a commit to stevend-uber/spire that referenced this pull request Oct 16, 2023

Support running SPIRE as a Windows service (spiffe#3625)

c39878a

* Support running SPIRE as a Windows service Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support running SPIRE as a Windows service #3625

Support running SPIRE as a Windows service #3625

amartinezfayo commented Nov 21, 2022

guilhermocc Nov 22, 2022

MarcosDY Nov 25, 2022

amartinezfayo Dec 23, 2022

MarcosDY Nov 25, 2022

amartinezfayo Dec 21, 2022

MarcosDY Nov 25, 2022

amartinezfayo Dec 23, 2022

MarcosDY Nov 25, 2022

amartinezfayo Jan 3, 2023

MarcosDY Nov 25, 2022

MarcosDY Nov 25, 2022

MarcosDY Nov 25, 2022

MarcosDY Nov 25, 2022

MarcosDY Nov 25, 2022

MarcosDY Nov 25, 2022

amartinezfayo Jan 3, 2023

MarcosDY left a comment

MarcosDY Jan 3, 2023

amartinezfayo Jan 4, 2023

MarcosDY Jan 3, 2023

amartinezfayo Jan 4, 2023

MarcosDY left a comment


		status := <-testCase.sc.statusCh
		assert.Equal(t, svc.Running, status.State)

+// Interrogate the service, which should return the current status.
+testCase.sc.changeRequestCh <- svc.ChangeRequest{Cmd: svc.Interrogate}
+status = <-testCase.sc.statusCh

		changeRequestCh: make(chan svc.ChangeRequest, 1),
		statusCh: make(chan svc.Status, 1),

Support running SPIRE as a Windows service #3625

Support running SPIRE as a Windows service #3625

Conversation

amartinezfayo commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosDY left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosDY left a comment

Choose a reason for hiding this comment