move keyctl to internal & func remove auth from keyring #683

QiWang19 · 2019-08-12T20:36:15Z

Move pkg/keyctl to internal/pkg/keyctl.
Add method removeAllAuthFromKeyring.
Get key describes from keyring using KEYCTL_READ and KEYCTL_DESCRIBE, and remove them from keyring if the description has prefix 'container-registry-login:'.

Signed-off-by: Qi Wang qiwan@redhat.com

QiWang19 · 2019-08-12T20:38:28Z

@mtrmac @vrothberg PTAL 🙂

mtrmac

Just a very quick first look

pkg/docker/config/config_linux.go

mtrmac · 2019-08-12T21:07:54Z

pkg/keyctl/key.go

 }
+
+// Describe returns a string describing the attributes of a specified key
+func Describe(kID ID) (string, error) {


No more public functions in this subpackage, please; we don’t want to maintain it as a stable public API. Make them private in pkg/docker/config.

Maybe we could (should?) move this package into ./internal before the next major bump?

why Unlink() can stay in pkg/keyctl/key.go but not Describe(kID ID)..
I think move keyctk package to ./internal is better?

The pkg/keyctl package was present in a release, so we can’t remove it, or any public functions/methods/types in it, without a major version bump.

Still, it’s not something users are expected to look for in c/image, and we don’t really want to maintain or enhance it for such hypothetical external users — after all, it’s just a pretty small façade for golang.org/x/sys/unix now.

#676 has already marked pkg/keyctl as deprecated: at the time, all callers could use either golang.org/x/sys/unix or github.com/jsipprell/keyctl without involving c/image.

This is a bit less true now that ReadUserKeyring seems to have no direct equivalent in either of those packages; still, c/image is not the obvious place to maintain that as a public utility.

Having the new code directly in pkg/docker/config (or maybe pkg/docker/config/internal or something like pkg/docker/config/internal/keyring) would work well enough, I think; nothing else in the repo needs to care.

Before v4.0.0 we can then either move the current pkg/keyctl to that same location, or just discard it replace its users with direct calls to golang.org/x/sys/unix.

I added the new code to pkg/docker/config_linux.go, should I move it to pkg/docker/config.go?
Do I need to add test readUserKeyring in pkg/docker/config/config_test.go?

removeAllAuthFromKernelKeyring is Linux-specific, so it needs to be in a *_linux.go file (or a differently-named file that uses an explicit build tag).

Both config_linux.go and a separate keyring_linux.go would be fine with me.

If unit tests exist, having them (again, in a Linux-specific file) in the package would be nice, sure. (If they don’t, it’s not quite blocking the PR … but this is not quite trivial code, so tests would be nice.)

pkg/keyctl/keyring.go

internal/keyctl/keyring.go

pkg/docker/config/config.go

pkg/docker/config/config_linux.go

mtrmac · 2019-08-19T12:48:28Z

pkg/docker/config/config_linux.go

+}
+
+// readUserKeyring reads user keyring and returns slice of key id(key_serial_t) representing the IDs of all the keys that are linked to it
+func readUserKeyring() ([]int32, error) {


(A fair amount about this code could be simplified, OTOH I suppose it’s somewhat useful that that it is exactly the same as the loop in Key.Get().)

I can export the id of Key struct and set a Key variable with id KEY_SPEC_USER_KEYRING to call Key.Get() and remove readUserKeyring()

I can export the id of Key struct and set a Key variable with id KEY_SPEC_USER_KEYRING to call Key.Get() and remove readUserKeyring()

Does this sound better?

I think the code as is good enough; not worth adding extra public interfaces to pkg/keyring for me, at least.

@QiWang19 Could you still revert the recent change, keeping the read loop in here so that we don’t make the Key.Id value public?

mtrmac · 2019-08-19T12:50:28Z

pkg/docker/config/config_unsupported.go

 }
+
+func removeAllAuthFromKernelKeyring() error {
+	return ErrNotSupported


(Non-blocking: This will cause pointless debug logs about failures on non-Linux platforms. OTOH it is consistent with the others, so it would be better to clean this up in a separate PR.)

rhatdan · 2019-08-20T16:08:17Z

Is this ready to merge then?

QiWang19 · 2019-08-20T16:44:48Z

revert the code to not export Key.id and create a PR in libpod to test if can pass the CI containers/podman#3860

QiWang19 · 2019-08-26T13:56:38Z

@mtrmac @vrothberg PTAL

rhatdan · 2019-09-11T17:26:30Z

@mtrmac @vrothberg Is this ready to merge. We need to finish this up and get back to enabling the kernel keyring support.

vrothberg

Just a minor nit (absolutely non-blocking). I think that most of the low-level code should go into pkg/keyctl to avoid scattering the low-level code over multiple packages. I would also love to see some tests for it. We already regressed in libpod on it, so I'd prefer to test as early as possible.

vrothberg · 2019-09-12T08:34:48Z

pkg/docker/config/config_linux.go

+			if err != nil {
+				return errors.Wrapf(err, "error unlinking key %d", kID)
+			}
+			logrus.Debugf("unlink key %d:%s", kID, keyAttr)


s/unlink/unlinked/?

vrothberg · 2019-09-12T08:53:23Z

@mtrmac @vrothberg PTAL

Sorry for the very late reply. I must have missed that going through my mails.

QiWang19 · 2019-09-12T12:58:20Z

which part of the low level code? Some code was not added to keyctl because The pkg/keyctl package was present in a release, so we can’t remove it, or any public functions/methods/types in it, without a major version bump.#683 (comment)

vrothberg · 2019-09-12T13:04:02Z

which part of the low level code?

The code of ...

removeAllAuthFromKernelKeyring() error
readUserKeyring() ([]int32, error)
getKeyIDsFromByte(byteKeyIDs []byte) []int32

It's dealing with keyring details that I think should be in the pkg/keyctl package since that's the place dealing with other low-level keyring parts.

Some code was not added to keyctl because The pkg/keyctl package was present in a release, so we can’t remove it, or any public functions/methods/types in it, without a major version bump.#683 (comment)

I am not asking to remove the pkg/keyctl package but to move the code there. I want to avoid low-level keyring code from being scattered over multiple packages.

QiWang19 · 2019-09-12T13:17:21Z

hmm, there's comment mentioned no more public method in keyctl

No more public functions in this subpackage
#683 (comment)

vrothberg · 2019-09-12T13:26:21Z

hmm, there's comment mentioned no more public method in keyctl

No more public functions in this subpackage
#683 (comment)

It does not make sense to me to scatter code. Either we use pkg/keyctl or we remove it (or ideally move it to internal/pkg/keyctl so we continue having a coherent package) but having something in between just complicates maintenance.

#563 will entail a major version bump as well - if that was the motivation for scattering.

QiWang19 · 2019-09-12T17:59:17Z

moved syscalls KEYCTL_READ, KEYCTL_DESCRIBE to keyctl.
removeAllAuthFromKernelKeyring() stays in config_linux.go with get/setAuthFromKernelKeyring()...

mtrmac · 2019-09-12T19:58:38Z

Just a minor nit (absolutely non-blocking). I think that most of the low-level code should go into pkg/keyctl to avoid scattering the low-level code over multiple packages.

We have no ambition to create a general keyring abstraction, and no interest in taking the time to design a good stable long-term API for that; so, nothing about the keyring should be a part of the public API of c/image. (OTOH c/image could link to some other external package that is exists to make the keyring access easy — but then the one we chose ended up being more trouble than benefit.)

So, per #683 (comment), I’d strongly prefer either an internal subpackage, or just separate files in pkg/docker/config (we can stomach a package that contains more than 5 .go files).

WRT scattering the code between pkg/keyctl and the other place; that’s a valid concern.

Assuming we drop pkg/keyctl soonish, it’s a temporary concern; sure, still valid, and “soonish” can easily be forgotten.
The clean solution would be, in that case, to create an internal/keyring subpackage which contains all of the actual code, and have pkg/keyctl just be dumb forwards to that one. But then, pkg/keyctl are, apart from the 30 lines of Key.Get, already just dumb forwards to golang.org/x/sys/unix…

vrothberg · 2019-09-13T08:01:26Z

Thanks for clarifying, @mtrmac! Anything against moving the entire pkg/keyctl into internal/? #563 would entail a major version bump as well (unless we consider all behavioral changes as bug fixes).

rhatdan · 2019-09-13T10:39:45Z

I think we should move it.

QiWang19 · 2019-09-13T15:21:34Z

PTAL @vrothberg @mtrmac

vrothberg

LGTM, thanks @QiWang19!

Could you rebase?

Move pkg/keyctl to internal/pkg/keyctl. Add method removeAllAuthFromKeyring. Get key describes from keyring using KEYCTL_READ and KEYCTL_DESCRIBE, and remove them from keyring if the decription has prefix 'container-registry-login:'. Signed-off-by: Qi Wang <qiwan@redhat.com>

rhatdan · 2019-09-16T19:39:22Z

LGTM

QiWang19 force-pushed the rm_all_keyring branch 2 times, most recently from 8147289 to 0ec0452 Compare August 12, 2019 20:56

mtrmac reviewed Aug 12, 2019

View reviewed changes

QiWang19 force-pushed the rm_all_keyring branch from 0ec0452 to 1595019 Compare August 13, 2019 16:57

mtrmac reviewed Aug 13, 2019

View reviewed changes

internal/keyctl/keyring.go Show resolved Hide resolved

pkg/docker/config/config.go Show resolved Hide resolved

QiWang19 force-pushed the rm_all_keyring branch 2 times, most recently from 93718ea to e75f429 Compare August 15, 2019 18:13

mtrmac reviewed Aug 19, 2019

View reviewed changes

QiWang19 force-pushed the rm_all_keyring branch from e75f429 to cacda0d Compare August 20, 2019 15:57

QiWang19 force-pushed the rm_all_keyring branch from cacda0d to 96fd441 Compare August 20, 2019 16:39

QiWang19 force-pushed the rm_all_keyring branch 4 times, most recently from 2f92651 to 55d4042 Compare August 23, 2019 16:46

QiWang19 force-pushed the rm_all_keyring branch from 55d4042 to 8e1c9e9 Compare September 11, 2019 17:28

vrothberg reviewed Sep 12, 2019

View reviewed changes

QiWang19 force-pushed the rm_all_keyring branch from 8e1c9e9 to 71d6976 Compare September 12, 2019 17:47

QiWang19 force-pushed the rm_all_keyring branch from 71d6976 to 5f88be2 Compare September 13, 2019 15:18

vrothberg reviewed Sep 16, 2019

View reviewed changes

QiWang19 force-pushed the rm_all_keyring branch from 5f88be2 to 0c82bbe Compare September 16, 2019 13:36

QiWang19 changed the title ~~add keyctl read&describe to remove auth from keyring~~ move keyctl to internal & func remove auth from keyring Sep 16, 2019

QiWang19 force-pushed the rm_all_keyring branch from 0c82bbe to d033863 Compare September 16, 2019 17:28

rhatdan merged commit 9bafe00 into containers:master Sep 16, 2019

move keyctl to internal & func remove auth from keyring #683

move keyctl to internal & func remove auth from keyring #683

Uh oh!

Conversation

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

Uh oh!

Uh oh!

QiWang19 commented Aug 12, 2019

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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

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!

Uh oh!

Uh oh!

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!

rhatdan commented Aug 20, 2019

Uh oh!

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

Uh oh!

Uh oh!

QiWang19 commented Aug 26, 2019

Uh oh!

rhatdan commented Sep 11, 2019

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrothberg commented Sep 12, 2019

Uh oh!

QiWang19 commented Sep 12, 2019

Uh oh!

vrothberg commented Sep 12, 2019

Uh oh!

QiWang19 commented Sep 12, 2019

Uh oh!

vrothberg commented Sep 12, 2019

Uh oh!

QiWang19 commented Sep 12, 2019

Uh oh!

mtrmac commented Sep 12, 2019

Uh oh!

vrothberg commented Sep 13, 2019

Uh oh!

rhatdan commented Sep 13, 2019

Uh oh!

QiWang19 commented Sep 13, 2019

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

QiWang19 commented Aug 12, 2019 •

edited

Loading

mtrmac Aug 13, 2019 •

edited

Loading

QiWang19 commented Aug 20, 2019 •

edited

Loading