Implement per-log bastions by niels-moller · Pull Request #35 · FiloSottile/torchwood

niels-moller · 2025-11-10T15:47:58Z

No description provided.

niels-moller · 2025-11-10T15:52:30Z

-{"origin":"example.com/foo","size":0,"root_hash":"47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=","keys":["sigsum.org/v1/tree/2ef59132082631d13e353b5ae49b22bc51a9bd59f41a2d570960a9658c1ed151+e2137795+ATp+37IPHc3SbPGzFMyZmPTOUlClk6PYPH+Ce5JiCb/h"]}
-{"origin":"sigsum.org/v1/tree/f48d4a1d0c6370ec189dc537f648ef3bb347b012fbd3c899a630a4cd2e9b8702","size":0,"root_hash":"47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=","keys":["sigsum.org/v1/tree/f48d4a1d0c6370ec189dc537f648ef3bb347b012fbd3c899a630a4cd2e9b8702+9bc54c7f+AVv6q3xDaHxI2aTemqEb7W6ZcbO7QbTqTr20thOqfqsw"]}
-{"origin":"sigsum.org/v1/tree/fae7fd8f084f9e7a1482162da8a3e52b08e6c1bac74ab831d00eb5c983b84120","size":0,"root_hash":"47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=","keys":["sigsum.org/v1/tree/fae7fd8f084f9e7a1482162da8a3e52b08e6c1bac74ab831d00eb5c983b84120+7f693d84+AUlxeri80AO7/4j/+OGo+5M2Sud0ktFg34uZl2fZnjJT"]}
+{"origin":"example.com/foo","size":0,"root_hash":"47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=","keys":["sigsum.org/v1/tree/2ef59132082631d13e353b5ae49b22bc51a9bd59f41a2d570960a9658c1ed151+e2137795+ATp+37IPHc3SbPGzFMyZmPTOUlClk6PYPH+Ce5JiCb/h"],"bastions":[null]}


It's weird that I get "bastions":[null] rather than "bastions":[] or maybe "bastions":null. Maybe a log with no keys also has this behavior? I've found one hint of a potential workaround at https://sqlite.org/forum/info/5804b03c6ad78238c5a86323332c1a7ad38ccbc431b08b502fbc530d52ec5fdd, but I haven't tried it out.

This would annoy me if I tried to process the resulting json, but I'd suggest leaving this as is for now (unless someone can suggest an easy fix).

Fixed with an explicit COALESCE.

niels-moller · 2025-11-10T15:55:03Z

 	}
 	e := make(chan error, 1)
+	// Handle log-specific bastions.
+	logBastions, err := w.AllBastions()


This only looks up configured bastions at startup, changes made with witnessctl won't take effect until next restart. I wonder if that is good enough, or if we need something more dynamic?

There seems to be an sqlite3_update_hook function, but unclear to me if that is usable for this purpose.

I think it's highly desirable to make this a bit more dynamic, in one way or the otherm but I think that can be improved in later PRs.

niels-moller · 2025-11-10T15:57:20Z

+	const bastionInitialRetryDelay = 30 * time.Second
+	const bastionMaxRetryDelay = time.Hour


These seem not too unreasonable to me, but it means that if a bastion is consistently down, the witness will exit and need restart about every hour.

Maybe just keep trying with the max value instead of exiting would make sense?

niels-moller · 2025-11-13T15:46:52Z

I've added a basic integration-level test. And I wasted an hour or two before realizing the reason for a strange failure was that the testscript package always runs its tests in parallel, and I added a second test that wanted to run a bastion on the default port localhost:8443...

rgdd · 2025-11-14T13:08:33Z

FWIW -- I'm now running this successfully on rgdd.se/poc-witness. Notes:

This seems to be backwards-compatible: I swapped out litewitness and witnessctl binaries without changing any configuration, and everything worked (I'm running with -bastion option on command line).
It also worked to remove "-bastion" on cmdline and instead specify the same bastion host for all logs in my configuration using the add-bastion command
It also worked to remove configured bastion with del-bastion.
Then I moved on to setup a 2nd bastion host (bastion.testonly.rgdd.se), and configured barreleye to use this with help from elias. So right now I'm running barreleye on one bastion host, and all other logs on bastion.glasklar.is. Seems to be working!

Only thing that maybe stood out when I was testing:

Looks like it is possible to add >1 bastion per log. Spotted this when I was surprised that del-bastion needed to know which bastion to remove for the log; and then I realized it's possible to e.g. configure both bastion.glasklar.is and bastion.testonly.rgdd.se for barreleye. I assume the semantics are similar to when this is done with the -bastion flag? (IIRC: try the first one on startup, and if that fails try the next one; die if all fail).
I'd be OK if we only supported a single bastion host per log, and later cross the bridge of having a list per log if requested with a good motivation.
(Maybe only supporting a single bastion per log also gets rid of the odd [null] thing, i.e., because then we're no longer dealing with a list?)

niels-moller · 2025-11-14T13:24:03Z

Thanks for testing.

* Looks like it is possible to add >1 bastion per log. Spotted this when I was surprised that del-bastion needed to know which bastion to remove for the log; and then I realized it's possible to e.g. configure both bastion.glasklar.is and bastion.testonly.rgdd.se for barreleye. I assume the semantics are similar to when this is done with the -bastion flag? (IIRC: try the first one on startup, and if that fails try the next one; die if all fail).

With a separate table for bastions, allowing multiple bastions for a log seemed easier than enforcing at most one. And the connection logic doesn't care about the mapping, it gets all the bastions (SELECT DISTINCT bastion FROM bastion), and tries to keep an open connection to each of them.

When a request is received, the list of bastions for that origin line is looked up, and compared to what's attached to the request context.

I could see some use for multiple bastions, either for redundancy, or for migration from one bastion to another.

rgdd · 2025-11-14T14:19:13Z

OK, thanks for explaining how you were thinking when picking direction! SGTM. I just wanted to make sure we weren't inheriting behavior from -bastion cmdline which we might want to deprecate. Keeping the connection open to all the bastions that are specified seems like a clean behavior (rather than picking the first one that works on startup -- the current cmdline behavior).

I'll try the "dual bastion" behavior on Monday when I'm migrating back to bastion.glasklar.is.

niels-moller · 2025-11-17T09:54:19Z

Some of the things left out of this MR, which could be follow up work:

Make litewitness react to changes to the bastion table.
Temporarily disconnect bastions if they exceed rate limits or misbehave in some other way.
Improve reconnect/restart logic.
Deprecate/delete providing bastions on the command line.
Figure out the weird [null] issue in the json loglist output.

niels-moller · 2025-11-20T13:42:35Z

 			e <- srv.ListenAndServe()
 		}()
+	} else if len(logBastions) == 0 {
+		fatal("configured to not open a listening port, but no bastions configured")


Not sure if this should be a fatal error, or a warning or something configurable.

Given the explicit use of -no-listen, presumably it's a new witness where logs with bastions will be added later. Changed to a warning.

rgdd · 2025-11-20T16:31:33Z

Updated testing notes (after a few minor tinkering rounds with nisse):

Reconnect logic with expo. backoff works (starting on 5s; i think the "since" logic to reset the backoff is off by a factor of two, but maybe it's just a difference of how to think of it. But current behavior seems to be, latest backoff I did was 20s. And then I will need to be connected successfully for at least 2*20s for the reset logic to kick in.)
Add bastion + SIGHUP -> works
Delete bastion + SIGHUP -> works

(Note: I have not done any detailed code review - just blackbox testing. AFAICT this PR does what I want.)

(There's the same configuration nit UX here wrt. accepting the same bastion host multiple times as in #41. Does not cause any issues AFAICT, so maybe can fix that minor nit separately for both add-keys and add-bastions later.)

@FiloSottile -- can you take a look and ensure we get this merged from here on? Let me know if you want me to help with any more testing (or review) tomorrow.

FiloSottile

Love how clean this turned out. I pushed fixes for a few issues.

FiloSottile · 2025-11-23T12:35:33Z

-{"origin":"example.com/foo","size":0,"root_hash":"47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=","keys":["sigsum.org/v1/tree/2ef59132082631d13e353b5ae49b22bc51a9bd59f41a2d570960a9658c1ed151+e2137795+ATp+37IPHc3SbPGzFMyZmPTOUlClk6PYPH+Ce5JiCb/h"]}
-{"origin":"sigsum.org/v1/tree/f48d4a1d0c6370ec189dc537f648ef3bb347b012fbd3c899a630a4cd2e9b8702","size":0,"root_hash":"47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=","keys":["sigsum.org/v1/tree/f48d4a1d0c6370ec189dc537f648ef3bb347b012fbd3c899a630a4cd2e9b8702+9bc54c7f+AVv6q3xDaHxI2aTemqEb7W6ZcbO7QbTqTr20thOqfqsw"]}
-{"origin":"sigsum.org/v1/tree/fae7fd8f084f9e7a1482162da8a3e52b08e6c1bac74ab831d00eb5c983b84120","size":0,"root_hash":"47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=","keys":["sigsum.org/v1/tree/fae7fd8f084f9e7a1482162da8a3e52b08e6c1bac74ab831d00eb5c983b84120+7f693d84+AUlxeri80AO7/4j/+OGo+5M2Sud0ktFg34uZl2fZnjJT"]}
+{"origin":"example.com/foo","size":0,"root_hash":"47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=","keys":["sigsum.org/v1/tree/2ef59132082631d13e353b5ae49b22bc51a9bd59f41a2d570960a9658c1ed151+e2137795+ATp+37IPHc3SbPGzFMyZmPTOUlClk6PYPH+Ce5JiCb/h"],"bastions":[null]}


Fixed with an explicit COALESCE.

FiloSottile · 2025-11-23T12:37:10Z

 			e <- srv.ListenAndServe()
 		}()
+	} else if len(logBastions) == 0 {
+		fatal("configured to not open a listening port, but no bastions configured")


Given the explicit use of -no-listen, presumably it's a new witness where logs with bastions will be added later. Changed to a warning.

FiloSottile · 2025-11-23T12:51:12Z

+	if len(bastions) > 0 {
+		// Accept requests only via these bastions.
+		if bastion == "" {
+			l.Debug("rejected request not using bastion")


Hmm, I think requests coming to the public listen endpoint (if enabled) should be allowed for all logs. Witnesses that are worried about receiving too much traffic are going to use -no-listen anyway, and rejecting them would make it hard to migrate a log from direct connection to a bastion, because as soon as the witness operator adds the bastion, the requests will start failing until the log switches to using the bastion.

FiloSottile · 2025-11-23T13:38:34Z

+			}
+			time.Sleep(delay)
+			delay *= 2
+		}


I have reworked this logic to

cap the retry at one minute, exponential backoff is useful to avoid thundering herds, but even 100 witnesses doing 1 minute retries will be 1.7qps, totally manageable; on the other hand, a 1 hour backoff extends an outage by 30 minutes on average

return if ctx is cancelled, the previous logic would keep trying to reconnect (and fail due to the cancelled ctx) after deleting a bastion

not kill the process if a per-log bastion is unavailable for a long time, the witness serves multiple logs and one log should not affect the others (this also combined with (2) to kill the process ~2h after a bastion was deleted)

reset the delay if the connection is alive for a fixed amount of time, not for an amount of time dependent on the latest delay (the two are not related)

make the delays easier to think about by expanding them into a table

FiloSottile · 2025-11-23T13:42:52Z

 		return fmt.Errorf("connecting to bastion: %v", err)
 	}
+	// Ensure that the connection is closed when our context is cancelled.
+	go func() {


The function can return even if the context is not cancelled, leaking the goroutine.

FiloSottile · 2025-11-23T16:15:57Z

+
 	slog.Info("connected to bastion", "bastion", bastion)
+	if logSpecific {
+		ctx = witness.ContextWithBastion(ctx, bastion)


This write to the ctx variable races with using ctx in the goroutine above.

The function can return even if the context is not cancelled, leaking the goroutine.

niels-moller · 2025-11-25T08:08:18Z

+exec witnessctl add-bastion -origin=sigsum.org/v1/tree/4d6d8825a6bb689d459628312889dfbb0bcd41b5211d9e1ce768b0ff0309e562 -bastion=localhost:9443
+
+# start bastion
+exec litebastion -listen localhost:9443 -testcert -backends=backends.txt &litebastion&


With your change to make "no bastion configured" into a warning rather than error, this invocation can now use -no-listen.

niels-moller · 2025-11-25T08:14:49Z

+	// Ensure that the connection is closed when our context is cancelled.
+	ctx, cancel = context.WithCancel(ctx)
+	defer cancel()
+	go func(ctx context.Context) {
+		// TODO: gracefully complete in-flight requests.
+		<-ctx.Done()
+		conn.Close()
+	}(ctx)
+
 	slog.Info("connected to bastion", "bastion", bastion)
+	if logSpecific {
+		ctx = witness.ContextWithBastion(ctx, bastion)


Note that it is unclear to me why this is needed; it would make sense to me if one could have ServeConn close the connection when the context passed down to that machinery is cancelled.

I also first tried to move creation of this goroutine later, after the ctx = witness.ContextWithBastion(...) call, but then it seemed to have no effect. It's as if the use of context.WithValue breaks parent/child relation, which is weird.

niels-moller commented Nov 10, 2025

View reviewed changes

niels-moller mentioned this pull request Nov 10, 2025

Support log-specific bastions in litewitness #33

Closed

niels-moller marked this pull request as ready for review November 13, 2025 15:43

niels-moller force-pushed the nisse/per-log-bastion branch 2 times, most recently from 8d209b3 to f784a5d Compare November 20, 2025 08:36

niels-moller commented Nov 20, 2025

View reviewed changes

niels-moller force-pushed the nisse/per-log-bastion branch from 0d9928f to c983b2f Compare November 20, 2025 15:39

FiloSottile reviewed Nov 23, 2025

View reviewed changes

FiloSottile force-pushed the nisse/per-log-bastion branch from dbafaa1 to 205b357 Compare November 23, 2025 14:01

FiloSottile reviewed Nov 23, 2025

View reviewed changes

Niels Möller and others added 14 commits November 23, 2025 19:04

litewitness: Support log-specific bastions

c029f22

witnessctl: Add bastion config

4188cd4

witnessctl: Update loglist test

df81e69

Add integration test

9bfa804

witness: Respond with 409 (Forbidden) for unexpected bastion usage

24af86b

Improve bastion retry logic

f22d6c1

litewitness: Reduce initial retry delay to 5s, fix doubling logic

2b3e803

litewitness: Log message on SIGHUP

b323254

litewitness: Add integration test for SIGHUP

071217f

Add workaround to explicitly close bastion connection on cancel.

2cc1032

Document new flags in cmd/litewitness/README.md

b981aab

Rework per-log bastion reconnection backoff strategy

271d5d3

Minor logging and comment improvements

805207f

Allow running with -no-listen and no bastions configured

5fef782

FiloSottile added 6 commits November 23, 2025 19:04

Avoid goroutine leaks on closed bastion connections

b7a0eb6

The function can return even if the context is not cancelled, leaking the goroutine.

Add tests for wrong bastion rejection

4e25cd2

Fix "bastions":[null] list-logs output

695fad0

Allow non-bastion requests to all logs

60f44a9

Fix internal/witness/witness_test.go compilation errors

b2c55b0

Fix race with "ctx = witness.ContextWithBastion" write

94baa7d

FiloSottile force-pushed the nisse/per-log-bastion branch from f911b25 to 94baa7d Compare November 23, 2025 18:05

FiloSottile merged commit 4af1bff into FiloSottile:main Nov 23, 2025
8 checks passed

niels-moller commented Nov 25, 2025

View reviewed changes

		const bastionInitialRetryDelay = 30 * time.Second
		const bastionMaxRetryDelay = time.Hour

Conversation

niels-moller commented Nov 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niels-moller commented Nov 13, 2025

Uh oh!

rgdd commented Nov 14, 2025

Uh oh!

niels-moller commented Nov 14, 2025

Uh oh!

rgdd commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niels-moller commented Nov 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgdd commented Nov 20, 2025

Uh oh!

FiloSottile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rgdd commented Nov 14, 2025 •

edited

Loading