auth: fetch tokens from client side #1660

tonistiigi · 2020-08-31T05:52:47Z

Instead of sharing credentials with the daemon use session to request temporary tokens requested from the cli side. The tokens are still reused with resolvers like before and efficiency shouldn't suffer. @sipsma

~~Marking wip to add client-side progress injection making token requests visible. This turned out to be more complicated and I'm moving code from buildx to buildkit to achieve it.~~

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

tonistiigi · 2020-08-31T05:55:57Z

session/auth/authprovider/tokenseed.go

+ ts.mu.Lock()
+ defer ts.mu.Unlock()
+
+ fp := filepath.Join(ts.dir, ".token_seed")


Was hoping to avoid this but couldn't think of a better solution. Afaics if we don't do something like this we always have a possibility for a chosen-plaintext attack from the daemon side. Another possible way would be to use pbkdf/scrypt for the verification but didn't want to take the 100ms+ performance hit for this and waste cpu.

Worth leaving a comment that describes this?

fyi @justincormack

tonistiigi · 2020-09-03T06:14:43Z

util/progress/progresswriter/multiwriter.go

+ "golang.org/x/sync/errgroup"
+)
+
+type MultiWriter struct {


These new progress handling functions are copied from buildx with slight modification. Buildx versions will be removed on next vendor.

sipsma · 2020-09-14T00:11:54Z

session/auth/authprovider/tokenseed.go

+ return nil, err
+ }
+
+ if err := os.MkdirAll(filepath.Dir(fp), 0701); err != nil {


Just double checking, we want anyone to have access to paths inside the dir, but not be able to list the dir? Or is this supposed to be 0700?

On my machine it is 0755 actually, but cli does not seem to be consistent. The search bit should be set though as not all things in .docker are secret. Eg. it may contain plugins that need to be executable.

FYI @thaJeztah

In most cases, this would be inside the user's home-directory, which would likely not be accessible to the "world" already, correct? 😅

but cli does not seem to be consistent

Can you elaborate what's not consistent? Something that needs to be fixed?

@thaJeztah

eg. https://github.com/docker/cli/blob/master/cli/context/store/metadatastore.go#L31
https://github.com/docker/cli/blob/master/cli/config/configfile/file.go#L187

There's at least one example for both cases

updated to 0755

Opened docker/cli#2727 for tracking on the CLI side.

Would 0755 be problematic if a custom config-directory is specified through --config (potentially outside of ~/ ?

Not really problematic as the file itself is 0600 and this is just the parent directory. But if you want to be more safe I can put it back to 0700. In 99% of the cases the directory already exists and all this will be ignored.

sipsma · 2020-09-14T00:45:34Z

session/auth/authprovider/tokenseed.go

+ return nil, err
+ }
+
+ if err := ioutil.WriteFile(fp, dt, 0600); err != nil {


WriteFile opens the file w/ O_TRUNC, so if the daemon crashed at the exact wrong time, I think this file could end up existing but be empty, which would cause the json.Unmarshal above to always return an error. Obviously pretty obscure, but if that happened the only recourse would be for the user to manually delete the file.

Maybe if the json unmarshal fails you can just assume the file is empty and start anew?

Would you prefer a file lock?

Sure lock would work, or you could write to a new temp file opened w/ O_EXCL and rename(2) it to the final one once done, whichever is easiest

Added file lock

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

mkasprzyk · 2020-10-13T08:18:33Z

This change broke configurations in Kubernetes like this one:

  containers:
  - name: buildkit
    image: moby/buildkit:master-rootless
    env:
      - name: BUILDKITD_FLAGS
        value: --oci-worker-no-process-sandbox
      - name: DOCKER_CONFIG
        value: /tmp/.docker
    tty: true
    command:
    - cat
    volumeMounts:
      - name: jenkins-docker
        mountPath: /tmp/.docker

It's a common practise that credentials are mount to this directory with Kube secrets.

ERROR: rpc error: code = Unknown desc = open /tmp/.docker/.token_seed.lock: read-only file system

thaJeztah · 2020-10-13T08:28:17Z

Hmm... I guess it's a bit of an odd situation where the temp-dir is read-only; is that common? (are temp-dirs always expected to be writable?)

^^ @tonistiigi looks because of https://github.com/moby/buildkit/pull/1660/files#r479909291

Are there paths that are required to be writable? XDG_CACHE_HOME, XDG_RUNTIME_DIR or one of the other XDG dirs (or their equivalent)?

tonistiigi commented Aug 31, 2020

View reviewed changes

tonistiigi force-pushed the token-fetch branch from 259df14 to fa6ce06 Compare September 3, 2020 06:12

tonistiigi marked this pull request as ready for review September 3, 2020 06:12

tonistiigi commented Sep 3, 2020

View reviewed changes

tonistiigi mentioned this pull request Sep 4, 2020

[discuss] client auth protocol for push/pull moby/moby#38591

Open

tonistiigi force-pushed the token-fetch branch from fa6ce06 to 715fe26 Compare September 5, 2020 00:27

sipsma reviewed Sep 14, 2020

View reviewed changes

tonistiigi force-pushed the token-fetch branch from 715fe26 to 08b152a Compare September 15, 2020 20:29

auth: fetch tokens from client side

1f94445

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi force-pushed the token-fetch branch from 08b152a to 1f94445 Compare September 15, 2020 20:40

thaJeztah mentioned this pull request Sep 16, 2020

Inconsistent directory permissions for ~/.docker directory docker/cli#2727

Open

tonistiigi requested a review from sipsma October 4, 2020 07:41

sipsma approved these changes Oct 6, 2020

View reviewed changes

tonistiigi merged commit 4177e7c into moby:master Oct 12, 2020

tonistiigi mentioned this pull request Oct 22, 2020

authprovider: allow readonly config dir #1745

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth: fetch tokens from client side #1660

auth: fetch tokens from client side #1660

tonistiigi commented Aug 31, 2020 •

edited

Loading

tonistiigi Aug 31, 2020

thaJeztah Sep 14, 2020

tonistiigi Sep 15, 2020

tonistiigi Sep 15, 2020

tonistiigi Sep 3, 2020

sipsma Sep 14, 2020

tonistiigi Sep 14, 2020 •

edited

Loading

thaJeztah Sep 14, 2020

tonistiigi Sep 14, 2020

tonistiigi Sep 15, 2020

thaJeztah Sep 16, 2020

tonistiigi Sep 16, 2020

sipsma Sep 14, 2020

tonistiigi Sep 14, 2020

sipsma Sep 14, 2020

tonistiigi Sep 15, 2020

mkasprzyk commented Oct 13, 2020 •

edited

Loading

thaJeztah commented Oct 13, 2020

auth: fetch tokens from client side #1660

auth: fetch tokens from client side #1660

Conversation

tonistiigi commented Aug 31, 2020 • edited Loading

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

tonistiigi Sep 14, 2020 • edited Loading

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

mkasprzyk commented Oct 13, 2020 • edited Loading

thaJeztah commented Oct 13, 2020

tonistiigi commented Aug 31, 2020 •

edited

Loading

tonistiigi Sep 14, 2020 •

edited

Loading

mkasprzyk commented Oct 13, 2020 •

edited

Loading