argon2: Add helpers for running the KDF remotely #328

chrisccoulson · 2024-09-03T21:11:08Z

As Argon2 is memory intensive, it's not suitable for multiple invocations in long-lived
garbage collected processes where multiple invocations may result in the process being
unable to perform new allocations, or even worse, triggering the kernel’s OOM killer.
For this reason, Argon2 is currently abstracted with an interface (Argon2KDF), of which
the application sets a global version of this which is intended to proxy KDF requests to
a short-lived remote process which uses the real InProcessArgon2KDF.

This adds some functionality to facilitate this.

Then there are JSON serializable types Argon2OutOfProcessRequest and
Argon2OutOfProcessResponse. The request can be fed directly to RunArgon2OutOfProcessRequest
on the remote side, but this is a fairly low-level API - the application still has to
deal with the transport.

There is a higher level API - NewOutOfProcessArgon2KDF, for use in the application process,
and which returns an implementation of Argon2KDF which proxies requests to a short-lived
remote handler process. The caller supplies a function to construct an appropriate exec.Cmd
instance for this. This function configures the exec.Cmd so that the handler process receives
a request on stdin and it expects a response on stdout. The handler process is expected to pass
both os.Stdin and os.Stdout to WaitForAndRunArgon2OutOfProcessRequest, although it doesn't
hardcode these descriptors for implementations that want to construct their own processes with
transports that don't rely on stdin and stdout.

Once a handler process has completed a request, it should exit cleanly. Neither
RunArgon2OutOfProcessRequest or WaitForAndRunArgon2OutOfProcessRequest support being called
more than once in the same process, which is enforced by an unexported Argon2KDF implementation
that becomes the global Argon2KDF implementation in the remote handler process after calling
SetIsArgon2HandlerProcess. Note that calling this function is required before using
RunArgon2OutOfProcessRequest and WaitForAndRunArgon2OutOfProcessRequest, else they will just
respond with an error.

The code in cmd/run_argon2 provides an example handler process, although this is mainly useful for
unit testing (where it is currently used). It is envisaged that the handler process will be a
special mode of snapd and snap-bootstrap in order to avoid adding an additional go binary just for
this.

As Argon2 is memory intensive, it's not suitable for multiple invocations in long-lived garbage collected processes. For this reason, Argon2 is abstracted with an interface (Argon2KDF), of which the application sets a global version of this which is intended to proxy KDF requests to a short-lived remote process which uses the real InProcessArgon2KDF. This adds some functionality to facilitate this. First of all, InProcessArgon2KDF is no longer a variable - it's a function. By default, it's methods return an error unless the application code has called SetIsArgon2RemoteProcess, which unlocks the real in-process KDF. Then there are JSON serializable types "Argon2RemoteInput" and "Argon2RemoteOutput". The input can be fed directly to RunArgon2RequestInRemoteProcess on the remote side, but this is a fairly low-level API. There is a higher level API - NewRemoteArgon2KDF, for use in the application process, and which returns an implementation of Argon2KDF which proxies requests to a short-lived remote helper process. The caller supplied a function to construct an appropriate exec.Cmd instance for this. This function is configured so that the remote process recieves a request on stdin and it expects a response on stdout. The remote process passes both os.Stdin and os.Stdout to WaitAndRunArgon2RequestInRemoteProcess, although it doesn't hardcode these descriptors for implementations that want to construct their own transport that doesn't rely on stdin and stdout. Once a remote process has completed a request, it should exit cleanly. Neither RunArgon2RequestInRemoteProcess or WaitAndRunArgon2RequestInRemoteProcess support being called more than once in the same process. The code in cmd/run_argon2 provides an example remote process, although this is mainly useful for unit testing (where it is currently used). It is envisaged that the remote process will be a special mode of snapd and snap-bootstrap in order to avoid adding an additional new go binary just for this.

pedronis

did a first pass

pedronis · 2024-09-04T14:21:09Z

argon2_remote_support.go

+	"github.com/snapcore/secboot/internal/argon2"
+)
+
+type nullInProcessArgon2KDFImpl struct{}


null is a bit of a strange name for something that actually fails

This type has gone, although there is still a nullArgon2KDFImpl that existed previously.

pedronis · 2024-09-04T14:24:06Z

argon2_remote_support.go

+	defer func() {
+		// Run Cmd.Wait in a defer so that we shut down on error paths too,
+		// and we capture the Wait error if there was no other error.
+		waitErr := cmd.Wait()


do we need a timeout mechanism around this?

pedronis · 2024-09-04T14:24:33Z

argon2_remote_support.go

+// os.Stdout when using - certainly when using [NewRemoteArgon2KDF] on the parent side.
+// This function can only be called once in a process. Subsequent calls in the same
+// process will result in an error response being returned.
+func WaitAndRunArgon2RequestInRemoteProcess(in io.Reader, out io.Writer) error {


this function code does not seem to be exercised

It gets executed in a sub-process (cmd/run_argon2) by any test that makes use of NewOutOfProcessArgon2KDF, which might be why it doesn't appear in the test coverage.

I've added an in-process test of this function so that it appears in the test coverage.

pedronis · 2024-09-04T14:24:59Z

export_test.go

@@ -148,6 +150,11 @@ func MockHashAlgAvailable() (restore func()) {
 	}
 }

+func ClearIsArgon2RemoteProcess() {
+	atomic.StoreUint32(&argon2RemoteProcessStatus, notArgon2RemoteProcess)
+	runtime.GC()


why the GC here?

I've added a comment to this function to explain why.

pedronis · 2024-09-04T14:25:48Z

argon2_remote_support.go

+		}
+	}
+
+	switch input.Command {


maybe a bit more of the error cases below should be tested

I've added some more test cases.

pedronis · 2024-09-04T14:29:38Z

@chrisccoulson Also OutOfProcess might be a clearer term than Remote, if a bit longer

chrisccoulson · 2024-09-06T12:17:55Z

@chrisccoulson Also OutOfProcess might be a clearer term than Remote, if a bit longer

I've renamed things to OutOfProcess.

This includes some other small refactorigns, eg, the handler process makes use of the global Argon2KDF implementation now in the same way that the parent process does.

pedronis

thanks for the changes, maybe I missed it but it doesn't seem you replied to my question about what to do if the subprocess takes way too long/gets stuck for some reason

chrisccoulson · 2024-09-06T15:32:26Z

thanks for the changes, maybe I missed it but it doesn't seem you replied to my question about what to do if the subprocess takes way too long/gets stuck for some reason

Aha, I'll think of a way to address that. As the process is meant to block for some time, I wonder whether creating some sort of regular watchdog or ping would be more appropriate here. It shouldn't be that hard to add to the protocol.

chrisccoulson requested a review from pedronis September 3, 2024 21:12

chrisccoulson force-pushed the remote-argon2-kdf branch from 4cfd159 to 017287b Compare September 4, 2024 07:50

chrisccoulson removed the request for review from pedronis September 4, 2024 14:24

pedronis reviewed Sep 4, 2024

View reviewed changes

chrisccoulson added 2 commits September 6, 2024 13:19

argon2: address some review comments

bb46af8

This includes some other small refactorigns, eg, the handler process makes use of the global Argon2KDF implementation now in the same way that the parent process does.

argon2: add in-process test for WaitForAndRunArgon2OutOfProcessRequest

93292cd

chrisccoulson requested a review from pedronis September 6, 2024 13:05

pedronis reviewed Sep 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

argon2: Add helpers for running the KDF remotely #328

argon2: Add helpers for running the KDF remotely #328

chrisccoulson commented Sep 3, 2024 •

edited

Loading

pedronis left a comment

pedronis Sep 4, 2024

chrisccoulson Sep 6, 2024

pedronis Sep 4, 2024

pedronis Sep 4, 2024

chrisccoulson Sep 6, 2024

chrisccoulson Sep 6, 2024

pedronis Sep 4, 2024

chrisccoulson Sep 6, 2024

pedronis Sep 4, 2024

chrisccoulson Sep 6, 2024

pedronis commented Sep 4, 2024

chrisccoulson commented Sep 6, 2024

pedronis left a comment

chrisccoulson commented Sep 6, 2024

argon2: Add helpers for running the KDF remotely #328

Are you sure you want to change the base?

argon2: Add helpers for running the KDF remotely #328

Conversation

chrisccoulson commented Sep 3, 2024 • edited Loading

pedronis 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

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

pedronis commented Sep 4, 2024

chrisccoulson commented Sep 6, 2024

pedronis left a comment

Choose a reason for hiding this comment

chrisccoulson commented Sep 6, 2024

chrisccoulson commented Sep 3, 2024 •

edited

Loading