Attenuate tokens for remote builds #3443
Conversation
saleemrashid
force-pushed
the
saleem/build-tokens
branch
from
5052215
to
53b9168
Compare
| // We need the build context to get a build token | ||
| buildCtx, cancel := context.WithCancelCause(ap.ctx) | ||
| // Cancel the build token request when the gRPC context is cancelled | ||
| stop := context.AfterFunc(ctx, func() { |
There was a problem hiding this comment.
TIL context.AfterFunc. This is a neat way to combine two contexts.
| var ( | ||
| client = fly.ClientFromContext(ctx) | ||
| io = iostreams.FromContext(ctx) | ||
| cfg = appconfig.ConfigFromContext(ctx) | ||
| ) | ||
|
|
||
| daemonType := imgsrc.NewDockerDaemonType(!flag.GetBool(ctx, "build-remote-only"), !flag.GetBool(ctx, "build-local-only"), env.IsCI(), flag.GetBool(ctx, "build-nixpacks")) | ||
| resolver := imgsrc.NewResolver(daemonType, client, appName, io, flag.GetWireguard(ctx)) | ||
| resolver := imgsrc.NewResolver(daemonType, client, app, io, flag.GetWireguard(ctx)) | ||
| defer imgsrc.RevokeBuildTokens(context.WithoutCancel(ctx), app) |
There was a problem hiding this comment.
Instead of having to know when/where to call RevokeBuildTokens, we could do
task.FromContext(ctx).RunFinalizer(func(ctx){
revokeToken(...)
})
as a part of getBuildToken().
Also TIL context.WithoutCancel(). You know all the cool new APIs!
There was a problem hiding this comment.
Oh! I looked for an API like that and couldn't find it :) Thank you!
| var token string | ||
| if len(tokens.UserTokens) > 0 { | ||
| token, err = getBuildTokenFromUser(ctx, orgID, app.Organization) | ||
| } else { | ||
| token, err = getBuildTokenFromMacaroons(orgID, tokens.Macaroons()) | ||
| } |
There was a problem hiding this comment.
I guess when we have oauth+macaroons, we can't easily make a build token from the macaroons because the discharge tokens are short lived?
There was a problem hiding this comment.
Yeah, I'm favouring user tokens to copy the existing behaviour from registry, and because the temporary deploy token has fewer moving parts.
That being said, this should be fairly resilient to token expiry? We should be calling getBuildToken right before we use the token, and I think tokens.Macaroons() will return tokens that won't expire for at least a minute (from what I understand of the pruning/fetching logic). There's maybe some contrived edge case if you disable BuildKit and have a long multi-stage build and use FROM registry.fly.io.
There was a problem hiding this comment.
I don't have the whole flow in my head. Flyctl call rchab and then rchab uses the tokens to talk to registry, right? I was worried about tokens expiring before rchab has a chance to use them.
There was a problem hiding this comment.
rchab is basically just a proxy to dockerd. So "build and push" is flyctl asking rchab to do a build, then flyctl asking rchab to push the image it just created (so the build and push will each do a fresh getBuildToken call). And in a BuildKit world, we don't send tokens with the initial build request, BuildKit makes a request back to flyctl when it needs them (6af24e1). So rchab should be using the tokens ~immediately after getting them.
The edge case is a non-BuildKit, legacy multi-stage build where flyctl sends tokens at the start of the build, but Docker doesn't pull the images for later stages until much later, because I think legacy builds are fully sequential.
There was a problem hiding this comment.
Makes sense. Thanks for the context. There's a lot more going on than I realized.
| delete(cfg.CachedBuildTokens, orgID) | ||
|
|
||
| apiClient := fly.ClientFromContext(ctx) | ||
| return apiClient.RevokeLimitedAccessToken(ctx, cachedToken.ID) |
There was a problem hiding this comment.
Another option that wouldn't require keeping track of the ID is the logOut mutation.
|
|
||
| // Append all the relevant discharge tokens | ||
| for _, cav := range macaroon.GetCaveats[*macaroon.Caveat3P](&m.UnsafeCaveats) { | ||
| toks = append(toks, dischargeByTicket[hex.EncodeToString(cav.Ticket)]) |
There was a problem hiding this comment.
Should we return an error here if we don't have the discharge?
There was a problem hiding this comment.
I don't think we want to return an error, just skip this permission token entirely so we don't fail if there was another one which we did have all the discharges for. (If there weren't, we'd hit the len(toks) == 0 below).
I was putting off refactoring that to whenever I rip out the discharge collection code into a macaroon helper function. That being said, it should probably be wrapped in if dis, ok := dischargeByTicket[hex.EncodeToString(cav.Ticket)]; ok.
There was a problem hiding this comment.
Makes sense. Right now, we're adding the permission token even if we don't have the discharges for it though, right?
There was a problem hiding this comment.
That's right. Does that cause issues, apart from being a bit wasteful?
This attenuates tokens sent to remote builders and registry to have the minimum permissions required:
This depends on superfly/fly-go#36.