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.

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?

I've refactored this quite a bit now. Not only is there a timeout mechanism when waiting for the remote process to exit (and go 1.20 introduces a nicer way to do this), there is also an optional watchdog mechanism that exercises the command processing / response sending code on the remote side, so that the parent knows that it's eventually going to get a response and can terminate the operation if this fails.

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.

I've removed this function (and the corresponding SetIsArgon2RemoteProcess entirely now.

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.

…pass

pedronis

did a pass, there is quite a few moving parts here, probably a good idea for @valentindavid @bboozzoo and @ZeyadYasser to take a look for awareness and possibly review as well

pedronis · 2024-12-02T16:19:01Z

argon2.go

@@ -119,14 +130,15 @@ func (o *Argon2Options) kdfParams(keyLen uint32) (*kdfParams, error) {

 	mode := o.Mode
 	if mode == Argon2Default {
+		//


forgot the comment?

I did. I've added a comment now.

pedronis · 2024-12-02T16:24:20Z

argon2_out_of_process_support_sync.go

+				// file we have opened. Try again with a 10ms backoff time,
+				// unless the request timeout fires first.
+				select {
+				case <-time.NewTimer(10 * time.Millisecond).C:


isn't this a bit too aggressive? how long does it take usually to compute the actual KDF?

At this point, we have no idea how long it will be before we can acquire the lock, so it's a bit of a trade-off between being aggressive and waiting longer than is necessary, where the lock ends up unheld for longer for a period. I'm not sure what time to use for the backoff here tbh - the other option is to make the call to flock blocking, but then there's no way for it to be able to timeout.

I've changed this to 100ms.

pedronis · 2024-12-02T16:26:02Z

argon2_out_of_process_support_sync.go

+			select {
+			case <-timeoutTimer.C:
+				// The timeout has expired.
+				return nil, errArgon2OutOfProcessHandlerSystemLockTimeout


this case is not reached by tests

It's a bit hit and miss whether the timeout on line 154 gets tested, or this one gets tested - it depends where the code lands before the timer expires. I'm not sure of a better way of doing it but I'll try and think of a way to make it a bit more reproducible.

I've refactored this code to make it more reproducible where the code lands during tests.

pedronis · 2024-12-02T16:35:45Z

argon2_out_of_process_support.go

+				var startupWg sync.WaitGroup
+				startupWg.Add(1) // Mark the WaitGroup as waiting for a single event.


any reason not to use a channel for this?

I don't mind, I just picked sync.WaitGroup because it seems like it's designed for this use case.

I've changed this to a channel now.

These tests keep passing locally and failing in github, and memory usage is the only thing I can think of that might be causing the failures.

Also abort WaitForAndRunArgon2OutOfProcessRequest on an error response, as the underlying RunArgon2OutOfProcessRequest function can't be called again anyway.

…sts more reproducible

bboozzoo · 2024-12-03T12:40:38Z

argon2.go

 		benchmarkParams := &argon2.BenchmarkParams{
 			MaxMemoryCostKiB: 1 * 1024 * 1024, // the default maximum memory cost is 1GiB.
 			TargetDuration:   2 * time.Second, // the default target duration is 2s.
 		}

 		if o.MemoryKiB != 0 {
+			// The memory cost has been spcified expli


Suggested change

// The memory cost has been spcified expli

// The memory cost has been specified explicitly

I've fixed this.

bboozzoo · 2024-12-03T13:00:25Z

argon2_out_of_process_support.go

+	str := "cannot process request: " + string(e.ErrorType)
+	if e.ErrorString != "" {
+		str += " ("
+		str += e.ErrorString
+		str += ")"
+	}


you can use strings.Builder(), it's available from 1.10+

I'm using that now.

bboozzoo · 2024-12-03T13:23:32Z

argon2_out_of_process_support.go

+	return actualRsp, nil
+}
+
+func (k *outOfProcessArgon2KDFImpl) Derive(passphrase string, salt []byte, mode Argon2Mode, params *Argon2CostParams, keyLen uint32) (key []byte, err error) {


I know it's part of the existing interface, but if params is always expected to be non nil, this could pass value not a pointer.

That seems reasonable, but given that this interface has been around for a while, shall we leave this one for another PR?

yet, we can do a followup, or leave a TODO

argon2_out_of_process_support.go

bboozzoo · 2024-12-03T13:53:41Z

argon2_out_of_process_support.go

+			// eventually returning an error to the caller.
+			var req *Argon2OutOfProcessRequest
+			dec := json.NewDecoder(in)
+			dec.DisallowUnknownFields()


means that the both ends need to be in sync now, which hopefully is true most of the time, but should there be some format, version field in requests/responses to indicate protocol version?

Just to be clear, I would not want to introduce any form to a handshake, as this would complicate the code even more, but a clear version check would help with debugging.

I've removed the calls to DisallowUnknownFields everywhere it was used.

bboozzoo · 2024-12-03T13:56:51Z

argon2_out_of_process_support.go

+				startupCh := make(chan struct{})
+
+				tmb.Go(func() error {
+					startupCh <- struct{}{} // Unblock the main routine.


should have the same effect

Suggested change

startupCh <- struct{}{} // Unblock the main routine.

close(startupCh)

I've made this change now.

argon2_out_of_process_support.go

This updates the x/crypto repository to the latest version we possibly can whilst still maintaining compatibility with go 1.18.

…ponse channel

This ended up catching a bug that resulted in a deadlock if the locking function had to perform a retry.

…gle request

bboozzoo

Some comments, I need to do another pass with a clear head

bboozzoo · 2024-12-11T16:12:16Z

internal/paths/paths.go

+	// Argon2OutOfProcessHandlerSystemLockPath is the lock file path used to
+	// serialize KDF requests system-wide. All process's that use the system-wide
+	// lock participate in the lock/unlock contract described above.
+	Argon2OutOfProcessHandlerSystemLockPath = filepath.Join(RunDir, "snapd/argon2.lock")


if secbot was consumed by other project than snapd, how could they set a different path given they would not be able to import secboot's internal package?

Note, the path already includes a directory which is not guaranteed to exist.

per discussion with @chrisccoulson we'll use a snapd agnostic path, or consider making the lock file location an argument

I've updated this to /run/secboot_argon2.lock

bboozzoo · 2024-12-11T16:17:41Z

argon2_out_of_process_support.go

+// Note that Argon2 requests are serialized using a system-wide lock, which this function does not
+// explicitly release. If the lock is acquired, it returns a callback that the caller may choose


if I were to run 2 out of process instances, requests to each instance would be serialized on the same shared lock. Why not a per instance lock?

per discussion with @chrisccoulson Conceptually follows the same approach as cryptsetup, trying to keep the lock as close to the process as possible.

bboozzoo · 2024-12-11T16:20:48Z

argon2_out_of_process_support.go

+
+	costParams := &Argon2CostParams{
+		Time:      request.Time,
+		MemoryKiB: request.MemoryKiB,


can memory be 0?

per discussion with @chrisccoulson 0 is valid, the code will use a minimum of 32kB.

@chrisccoulson Can you add a comment?

I've added a comment explaining why we don't test MemoryKiB here.

argon2_out_of_process_support.go

bboozzoo · 2024-12-11T16:26:21Z

argon2_out_of_process_support.go

+// future, which might require replacing the existing library we use for Argon2.
+func RunArgon2OutOfProcessRequest(request *Argon2OutOfProcessRequest) (response *Argon2OutOfProcessResponse, lockRelease func()) {
+	// Perform checks of arguments that are common to call requests
+	switch request.Mode {


would it make sense to move the request validation to separate helper, eg:

func validateOutOfProcessRequest(req *Argon2OutOfProcessRequest ) error { .. }

just cosmetics, not high priority

I might look at this in a follow-up

bboozzoo · 2024-12-11T16:28:55Z

argon2_out_of_process_support.go

+//
+// The implementation is expected to be paired with an equivalent implementation of
+// [Argon2OutOfProcessWatchdogMonitor] in the parent process.
+type Argon2OutOfProcessWatchdogHandler = func(challenge []byte) (response []byte, err error)


unless this needs to be a type alias?

Suggested change

type Argon2OutOfProcessWatchdogHandler = func(challenge []byte) (response []byte, err error)

type Argon2OutOfProcessWatchdogHandler func(challenge []byte) (response []byte, err error)

These are no longer type aliases

bboozzoo · 2024-12-11T16:43:09Z

argon2_out_of_process_support.go

+// Note that Argon2 requests are serialized using a system-wide lock, which this function does not
+// explicitly release. If the lock is acquired, it returns a callback that the caller may choose
+// to execute in order to explicitly release the lock, or the caller can just leave it to be
+// implicitly released on process exit. If the lock is explicitly released, the caller must be
+// sure that the large amount of memory allocated for the Argon2 operation has been freed and
+// returned back to the OS, else this defeats the point of having a system-wide lock (to avoid
+// having multiple processes with high physical memory requirements running at the same time). If
+// the lock wasn't acquired, no release callback will be returned.


What if the locking exercise was left to the caller in the parent process? it feels like this is imposing a significant limitation but without visibility by the users of secboot.

bboozzoo · 2024-12-11T16:54:56Z

argon2_out_of_process_support_sync.go

+		}
+
+		// Attempt to open the lock file for writing.
+		lockFile, err = os.OpenFile(paths.Argon2OutOfProcessHandlerSystemLockPath, os.O_RDWR|os.O_CREATE, 0600)


| syscall.O_NOFOLLOW | syscall.O_CLOEXEC ?

CLOEXEC may be set by default, let's double check

I've added O_NOFOLLOW now, and verified that O_CLOEXEC is set b the go runtime.

argon2_out_of_process_support_sync.go

The gopkg.in/tomb.v2 package does not handle routines that panic very well - the active routine count will not decrement and it won't put the tomb into a dying state. In fact, it looks like it makes it impossible for the tomb to ever reach a dead state. If the Argon2 crypto package panics, the KDF routine will disappear but the child process will carry on processing watchdog requests despite the fact that the parent is never going to get an answer. We recover panics on the KDF routine and turn them into errors that put the tomb into a dying state. This begins the teardown of the child process and halts the processing of watchdog requests. For this reason, the parent may see 1 of 2 errors - either an error indicating that the child process exited with a non-zero exit code, or an error indicating a watchdog timeout. The test added for this handles both cases. This also introduces a small package - internal/testenv - with a single function (testenv.IsTestBinary) that can be used in production code and requires some special linker flags to enable. This permits us to export a function from the core secboot library to mock an internal function that should only be accessible in unit tests (and should panic otherwise).

…t parameter

I was going to also pass O_CLOEXEC, but the go runtime already does this.

chrisccoulson · 2025-01-17T19:18:10Z

This is ready for (hopefully the final 🙂) review again, although the unit tests currently fail because #363 needs to land

bboozzoo

LGTM

pedronis

thank you, one comment about the lock filename

nitpicks for possibly later:

I think remote is strange terminology for this, I'm happy we fixed the code name, but the comments should probably use external/separate more instead of remote
maybe the _support.go file should be split into invocation and external code, with common bits in the invocation .go file

pedronis · 2025-01-22T09:42:24Z

internal/paths/paths.go

+	// Argon2OutOfProcessHandlerSystemLockPath is the lock file path used to
+	// serialize KDF requests system-wide. All process's that use the system-wide
+	// lock participate in the lock/unlock contract described above.
+	Argon2OutOfProcessHandlerSystemLockPath = filepath.Join(RunDir, "secboot_argon2.lock")


nitpick but secboot-argon2.lock would seem more in line with other naming

That's fine with me - I've pushed an update to rename this.

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

chrisccoulson added 3 commits September 18, 2024 16:00

[WIP] address review comments

844021f

Merge branch 'master' into remote-argon2-kdf

fd31de5

Argon2 remoting cleanups

fe291c5

chrisccoulson requested a review from pedronis December 2, 2024 09:38

chrisccoulson added 3 commits December 2, 2024 10:23

Fix a couplke of data races

a097997

argon2: increase timeout for github

ef2c8e4

reduce memory usage of tests that fail in github to see if they will …

dce2200

…pass

pedronis reviewed Dec 2, 2024

View reviewed changes

chrisccoulson added 5 commits December 2, 2024 21:00

argon2: add a missing comment

8f57b45

argon2: replace the use of sync.WaitGroup with a channel

3c64825

argon2: reduce memory usage for github

35fd6f1

These tests keep passing locally and failing in github, and memory usage is the only thing I can think of that might be causing the failures.

reduce memory usage in a couple more argon2 tests

8b5c902

Also abort WaitForAndRunArgon2OutOfProcessRequest on an error response, as the underlying RunArgon2OutOfProcessRequest function can't be called again anyway.

Change the backoff time to 100ms and refactor the code to make the te…

9d6d0e4

…sts more reproducible

chrisccoulson requested a review from pedronis December 3, 2024 12:19

bboozzoo reviewed Dec 3, 2024

View reviewed changes

chrisccoulson added 3 commits December 4, 2024 14:58

close the channel to unblock the receiving routine

d1f97b3

complete a doc comment

fabe880

Update x/crypto

0c869e8

This updates the x/crypto repository to the latest version we possibly can whilst still maintaining compatibility with go 1.18.

chrisccoulson added 13 commits December 6, 2024 15:08

Remove a bit of unnecessary test code

d38ab1c

Test WaitForAndRunArgon2OutOfProcessRequest closes its end of the res…

093b6ee

…ponse channel

Improve the argon2 remoting system-lock tests

c8d71ba

This ended up catching a bug that resulted in a deadlock if the locking function had to perform a retry.

Make use of strings.Builder

a1e15a7

remove some unnecessary channel usage

9c9d5ca

Try running some tests with a memory consumption of 2GiB again

327f235

Simplify the HMACArgon2OutOfProcessWatchdogMonitor function

afca4f4

Move all tests that run Argon2 to the expensive suites

ba7c52f

Add some debugging fmt.Printf statements

6270d98

Reenable the watchdog timeout again

88d8658

try disabling the GC during the failing test

d6cabb3

remove debug fmt.Printfs

4cdec5f

Ensure WaitForAndRunArgon2OutOfProcessRequest will only process a sin…

57a5694

…gle request

pedronis requested a review from bboozzoo December 10, 2024 08:53

bboozzoo reviewed Dec 11, 2024

View reviewed changes

chrisccoulson added 8 commits January 17, 2025 10:42

argon2: Add a comment about not launching sub-processes

586bc6b

internal/paths: Use a snapd agnostic path for the Argon2 lock file

625db26

argon2: Add a note explaining why we don't validate the MemoryKiB cos…

8883d04

…t parameter

argon2: Don't use type aliases for the watchdog function templates

e094dfa

argon2: Pass O_NOFOLLOW when opening the lock file

a264de8

I was going to also pass O_CLOEXEC, but the go runtime already does this.

argon2: make sure the lock file is a regular file

0e6e386

Merge branch 'master' into remote-argon2-kdf

71a9050

chrisccoulson requested a review from bboozzoo January 17, 2025 19:15

bboozzoo approved these changes Jan 20, 2025

View reviewed changes

Merge branch 'master' into remote-argon2-kdf

d11584e

pedronis approved these changes Jan 23, 2025

View reviewed changes

internal/paths: update the Argon2 lock filename

77dcb36

chrisccoulson merged commit 5fbde5b into canonical:master Jan 23, 2025
2 checks passed

		var startupWg sync.WaitGroup
		startupWg.Add(1) // Mark the WaitGroup as waiting for a single event.

	// The memory cost has been spcified expli
	// The memory cost has been specified explicitly

	startupCh <- struct{}{} // Unblock the main routine.
	close(startupCh)

		// Note that Argon2 requests are serialized using a system-wide lock, which this function does not
		// explicitly release. If the lock is acquired, it returns a callback that the caller may choose

	type Argon2OutOfProcessWatchdogHandler = func(challenge []byte) (response []byte, err error)
	type Argon2OutOfProcessWatchdogHandler func(challenge []byte) (response []byte, err error)

argon2: Add helpers for running the KDF remotely #328

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

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

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

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

bboozzoo 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

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

chrisccoulson commented Jan 17, 2025

bboozzoo left a comment

Choose a reason for hiding this comment

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

chrisccoulson commented Sep 3, 2024 •

edited

Loading