localjwtauthority: Prep for rotation support - #1313
Conversation
ea0bf18 to
0bc0692
Compare
ad3a0c1 to
40e8d65
Compare
Max Thompson (thompsonmax)
left a comment
Max Thompson (thompsonmax)
left a comment
There was a problem hiding this comment.
Mostly LGTM, just some nits and potential testing gap
| actorIDCAPool, err := localca.NewRefreshingPool(*actorIDCAPoolFile) | ||
| if err != nil { | ||
| serverboot.Fatal(ctx, "while loading the Actor ID CA", err) | ||
| serverboot.Fatal(ctx, "while loading the Actor ID certificate authority pool: %w", err) |
There was a problem hiding this comment.
[nit] %w is printed verbatim because we're calling Fatal rather than Fatalf
There was a problem hiding this comment.
Done.
| actorIdentitySrv := actoridentity.New(actorIdentityJWTIssuer, *actorIDJWTPoolFile, actorIDCAPool, persistence, workerCache) | ||
| actorIDJWTAuthorityPool, err := localjwtauthority.NewRefreshingPool(*actorIDJWTPoolFile) | ||
| if err != nil { | ||
| serverboot.Fatal(ctx, "while loading the Actor ID JWT authority pool: %w", err) |
There was a problem hiding this comment.
[nit] %w is printed verbatim because we're calling Fatal rather than Fatalf
There was a problem hiding this comment.
Done.
| return "", fmt.Errorf("selected authority %q not present", p.ActiveForSigning) | ||
| } | ||
| } else { | ||
| // Fall back to first entry. |
There was a problem hiding this comment.
Should we have some TODO on removing this fall back at some point? (similar to the fallback on the CA pool impl)
There was a problem hiding this comment.
Done.
| return "", fmt.Errorf("while marshaling payload: %w", err) | ||
| } | ||
|
|
||
| // TODO(ahmedtd): Select authority |
There was a problem hiding this comment.
[nit] stale TODO? Seems like you're selecting the authority here :)
There was a problem hiding this comment.
Whoops, done.
| if err != nil { | ||
| t.Fatalf("MarshalECPrivateKey(): %v", err) | ||
| pool1 := &ConcretePool{ | ||
| Authorities: []*Authority{ca1}, |
There was a problem hiding this comment.
There's no tests here that exercise a pool that has multiple authorities set, unless I'm missing something. Should we add unit tests for that?
There was a problem hiding this comment.
I've made this test case simulate a full rotation.
| // designated as active for signing. The rest are inactive, but are still | ||
| // trusted for verifying JWTs. | ||
| // | ||
| // The active/inactive desngination allows a Pool to be seamlessly rotated. |
There was a problem hiding this comment.
[typo] 'desngination' -> 'designation'
There was a problem hiding this comment.
Done.
* Add a RefreshingPool abstraction like localca.RefreshingPool. This provides caching (so we don't read the keys fresh from the filesystem) for each signing operation. * Extend ConcretePool so that it tracks which key is currently active for signing, instead of always picking the first key. A followup PR will add admin commands for actually rotating the JWT pool.
40e8d65 to
112ecba
Compare
|
Approved, waiting on CI tests to merge. |
daf5a13
into
agent-substrate:main
2 participants
A followup PR will add admin commands for actually rotating the JWT pool.