Add keyring package #619

QiWang19 · 2019-04-25T11:52:38Z

Add package from https://github.com/jsipprell/keyctl to store authentication for podman login to kernel user keyring.
close #2167

rhatdan · 2019-04-25T12:47:21Z

@QiWang19 Looks like you are missing some vendors.

vrothberg · 2019-04-25T15:13:32Z

https://github.com/zalando/go-keyring requires dbus which does not seem very portable. Imagine this running in a container.

vrothberg · 2019-05-13T11:27:05Z

https://github.com/zalando/go-keyring requires dbus which does not seem very portable. Imagine this running in a container.

@rhatdan WDYT about the dbus requirement? I think we can use the zalando lib for OS X and Windows, but for Linux I would prefer using the syscalls directly to avoid a dbus dependency. We need only a very limited set of the entire keyrings functionality, so I'd go for writing a package here in containers/image.

Also adding @mtrmac to share thoughts.

rhatdan · 2019-05-13T14:11:50Z

Yes I am not interested in the dbus bindings, I would rather to it directly.

mtrmac · 2019-05-13T19:10:35Z

More importantly, containers/podman#2167 asks for support of the kernel keyring (keyctl(2) etc.), which is completely different from the per-user userspace keyring (FreeDesktop secrets) supported by https://github.com/zalando/go-keyring ; it’s not just a matter of using/not using D-Bus, it’s a completely different store.

As far as the implementation goes, it’s not obvious to me that this should be a hard-coded silently-used default like this; using the credHelpers configuration, or even more likely, a global credsStore option, would clearly express users’ preference where to store the credentials. (E.g., if the user wanted to use the kernel keyring, and expected the credentials never to hit the disk, and some permission error caused the use of the kernel keyring to fail, the code automatically falls back to files.)

And then we would talk about whether to implement the helpers out-of-process using that protocol, or in-process by specially recognizing some keywords; both is plausible.

QiWang19 · 2019-05-13T19:10:55Z

still need this PR using https://github.com/zalando/go-keyring to support OS X and Windows?

mtrmac · 2019-05-13T19:15:14Z

OS X and Windows don’t have keyctl(2), so containers/podman#2167 is fundamentally impossible for them.

(The kernel keyring is fundamentally ephemeral, a reboot, or probably a log out, clears it; the keyrings supported by go-keyring are persistent and intended for ~single-sign-on (logging into the computer makes the credentials available).

Sure, we can also have a conversation about which one of these is more valuable (and I personally lean towards ephemeral login state being too much hassle for not that much security), but if this PR is supposed to support the kernel keyring or something equivalent, and it just doesn’t.)

vrothberg · 2019-05-15T12:11:56Z

still need this PR using https://github.com/zalando/go-keyring to support OS X and Windows?

We can defer that to another PR and focus just on Linux here. I think it would nice to also support the OS X keyring (and whatever Windows does) but that might be out of scope.

@QiWang19, I suggest to first add the code to support the kernel keyring. There are go libraries in the wild that offer bindings for keyctl but I would prefer to have a dedicated (and minimal) package somewhere here in containers/image. Once that's in place, we can check the details how we can integrate this into the stack (see @mtrmac's comment #619 (comment)).

QiWang19 · 2019-05-15T14:09:51Z

yes, I think I can add a package to use kernel keyring first and then decide how to apply it.

QiWang19 · 2019-05-20T12:55:16Z

Grab files from https://github.com/jsipprell/keyctl to add session keyring. Is this kind of keyring that we should use?

rhatdan · 2019-05-20T12:55:56Z

Yes I believe so.

QiWang19 · 2019-05-21T15:45:12Z

I saw containers/image already supported the credHelpers configuration using API from github.com/docker/docker-credential-helpers/client.

image/pkg/docker/config/config.go

Lines 42 to 46 in 2c0349c

    
           return modifyJSON(sys, func(auths *dockerConfigFile) (bool, error) { 
        
           	if ch, exists := auths.CredHelpers[registry]; exists { 
        
           		return false, setAuthToCredHelper(ch, registry, username, password) 
        
           	}

I don't want kernel keyring to be configured under credHelpers. Can I use the kernel keyring as a default choice, if credHelpers is not configured, then add username &password to kernel keyring, if an error occurs they will be added to the authfile?

rhatdan · 2019-05-22T06:59:54Z

I would prefer this to be primary and not rely on external helpers.

mtrmac · 2019-05-22T16:29:54Z

I don't want kernel keyring to be configured under credHelpers. Can I use the kernel keyring as a default choice, if credHelpers is not configured, then add username &password to kernel keyring, if an error occurs they will be added to the authfile?

I am, hypothetically, a bit worried that this “if an error occurs” state could cause the user to end up with two credential locations at the same time, and unclear rules about when data is recorded in which one and how to manage it — but that is hypothetical and strongly depends on the actual implementation; arguably kernel facilities should very rarely fail, and if they do, they should fail with a clear reason.

QiWang19 · 2019-05-22T19:11:49Z

@mtrmac PTAL

mtrmac

Very minimal initial impressions:

I’d really much prefer vendoring the original keyctl* wrappers to maintaining a separate fork in our repo.
The key name (“description”) should include an unambiguous indication that these are container credentials, to prevent accidental clashes with other software that could use the hostname as a key name for some other purpose.

mtrmac · 2019-05-22T19:27:54Z

This code must be automatically disabled on non-Linux platforms; AFAICS the current implementation won’t compile.

rhatdan · 2019-05-22T20:13:09Z

@mtrmac - @vrothberg Asked her to copy the limited functions into this Repo rather then vendoring in the entire repo. (I believe)

vrothberg · 2019-05-23T07:11:06Z

@mtrmac - @vrothberg Asked her to copy the limited functions into this Repo rather then vendoring in the entire repo. (I believe)

Yes, I asked to have a minimal package here. The required code for our purpose is small in size and complexity. Having it here allows for changing it to our needs if needed and removes the burden of creating new source packages for downstream packaging (mostly Debian).

vrothberg

Great work, @QiWang19! I tested it locally and it's working as expected. Most comments relate to adding comments in the code and logging errors. The only remaining issue is how we encode user/pw (see comments below).

pkg/docker/config/config_linux.go

pkg/docker/config/config.go

pkg/docker/config/config_linux.go

QiWang19 · 2019-06-25T19:30:25Z

@vrothberg 🙂 thanks for reviewing. Fixed the comments above. PTAL

vrothberg · 2019-07-08T15:26:17Z

Apologies, @QiWang19. This one slipped through my emails! I will have another look tomorrow.

vrothberg

Only minor nits, comments and clean-ups. The tests need more coverage. Other than that, LGTM. Great work, @QiWang19!

@rhatdan PTAL as well. It can easily be tested with Podman (go mod edit -replace github.com/containers/image=github.com/QiWang19/image@key, make vendor, ...).

pkg/docker/config/config.go

pkg/docker/config/config_linux.go

pkg/keyctl/sys_linux.go

pkg/keyctl/keyring.go

pkg/keyctl/keyring_test.go

vrothberg · 2019-07-09T08:47:03Z

@rhatdan, one thing that still worries me a bit is that the keyrings are not yet namespaced. So they can leak into and from a container. Just tried that with Podman.

rhatdan · 2019-07-09T12:27:04Z

@vrothberg Were you able to see the keyring from inside of podman, or was it blocked by SELinux and other security mechanisms?

vrothberg · 2019-07-09T13:47:46Z

@vrothberg Were you able to see the keyring from inside of podman, or was it blocked by SELinux and other security mechanisms?

It's blocked by SELinux and SECCOMP, which I consider to be "good enough" for a general use but I am no security expert and leave the decision up to you :^)

rhatdan · 2019-07-09T14:00:29Z

Well we already have this issue, so this just adds a little to the problem. Hopefully the patches to the kernel will finally get merged which will at least allow us to isolate Keyring via User Namespace in the future.
I think it is good enough.

vrothberg

Very last clean ups and then I am good to merge. @QiWang19, please ping me when once the PR is updated.

vrothberg · 2019-07-16T07:35:44Z

pkg/docker/config/config.go


+		err := setAuthToKernelKeyring(registry, username, password)
+		if err == nil {
+			logrus.Debugf("credentials for (%s, %s) was stored in the kernel keyring\n", registry, username)


s/was/were/

pkg/docker/config/config.go

pkg/keyctl/sys_linux.go

QiWang19 · 2019-07-17T01:10:21Z

@vrothberg PTAL

pkg/keyctl/sys_linux.go

vrothberg

@QiWang19, we need the contraints at other places as well (following the references and callers).

pkg/keyctl/keyring_test.go

vrothberg · 2019-07-17T13:51:55Z

pkg/keyctl/keyring.go

+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build linux


Constraints here.

pkg/keyctl/key.go

pkg/docker/config/config_unsupported.go

vrothberg

LGTM, just needs a rebase

Add package from https://github.com/jsipprell/keyctl to store authentication for podman login to kernel user keyring. close containers#2167 Signed-off-by: Qi Wang <[email protected]>

QiWang19 · 2019-07-18T19:13:34Z

@vrothberg PTAL 🙂

vrothberg

LGTM, thanks @QiWang19 👍

@rhatdan @TomSweeneyRedHat @mtrmac PTAL, let's get this in.

rhatdan · 2019-07-20T10:26:24Z

LGTM

QiWang19 force-pushed the key branch from 1db3cde to 8fc1f7f Compare April 25, 2019 15:10

QiWang19 force-pushed the key branch from 8fc1f7f to b268113 Compare May 20, 2019 12:50

QiWang19 changed the title ~~Add keyring storage~~ Add keyring pacakge May 20, 2019

QiWang19 force-pushed the key branch 5 times, most recently from b1d92db to a5a3a64 Compare May 20, 2019 21:54

QiWang19 force-pushed the key branch from a5a3a64 to 9104616 Compare May 22, 2019 19:05

mtrmac reviewed May 22, 2019

View reviewed changes

rhatdan changed the title ~~Add keyring pacakge~~ Add keyring package May 22, 2019

QiWang19 force-pushed the key branch from 9104616 to 2215b13 Compare May 22, 2019 20:12

vrothberg reviewed Jun 25, 2019

View reviewed changes

QiWang19 force-pushed the key branch 3 times, most recently from 5c8434b to a838f90 Compare June 25, 2019 19:28

vrothberg reviewed Jul 9, 2019

View reviewed changes

QiWang19 force-pushed the key branch 2 times, most recently from 437d4e3 to f91da40 Compare July 10, 2019 14:38

vrothberg reviewed Jul 16, 2019

View reviewed changes

QiWang19 force-pushed the key branch from f91da40 to f546688 Compare July 16, 2019 17:39

vrothberg reviewed Jul 17, 2019

View reviewed changes

pkg/keyctl/sys_linux.go Show resolved Hide resolved

vrothberg reviewed Jul 17, 2019

View reviewed changes

QiWang19 force-pushed the key branch from f546688 to a07edf2 Compare July 17, 2019 17:53

vrothberg reviewed Jul 18, 2019

View reviewed changes

QiWang19 force-pushed the key branch from a07edf2 to 773e435 Compare July 18, 2019 13:58

Add user registry auth to kernel keyring

94e0f0e

Add package from https://github.com/jsipprell/keyctl to store authentication for podman login to kernel user keyring. close containers#2167 Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the key branch from 773e435 to 94e0f0e Compare July 18, 2019 18:42

vrothberg reviewed Jul 19, 2019

View reviewed changes

rhatdan merged commit 231d782 into containers:master Jul 20, 2019

mtrmac mentioned this pull request Jul 22, 2019

keyring ignores --authfile #662

Closed

mtrmac mentioned this pull request Aug 2, 2019

Update keyctl for other arches #675

Merged

wking mentioned this pull request Sep 24, 2019

types: Add SystemContext.GetAuth #588

Closed

Add keyring package #619

Add keyring package #619

Uh oh!

Conversation

QiWang19 commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhatdan commented Apr 25, 2019

Uh oh!

vrothberg commented Apr 25, 2019

Uh oh!

vrothberg commented May 13, 2019

Uh oh!

rhatdan commented May 13, 2019

Uh oh!

mtrmac commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QiWang19 commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vrothberg commented May 15, 2019

Uh oh!

QiWang19 commented May 15, 2019

Uh oh!

QiWang19 commented May 20, 2019

Uh oh!

rhatdan commented May 20, 2019

Uh oh!

QiWang19 commented May 21, 2019

Uh oh!

rhatdan commented May 22, 2019

Uh oh!

mtrmac commented May 22, 2019

Uh oh!

QiWang19 commented May 22, 2019

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac commented May 22, 2019

Uh oh!

rhatdan commented May 22, 2019

Uh oh!

vrothberg commented May 23, 2019

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

QiWang19 commented Jun 25, 2019

Uh oh!

vrothberg commented Jul 8, 2019

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vrothberg commented Jul 9, 2019

QiWang19 commented Apr 25, 2019 •

edited

Loading

mtrmac commented May 13, 2019 •

edited

Loading

QiWang19 commented May 13, 2019 •

edited

Loading

mtrmac commented May 13, 2019 •

edited

Loading