Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add linux kernel keyring based credential helper (carry) #235

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

quick rebase of #214, and initial work on addressing some issues

@thaJeztah thaJeztah force-pushed the keyctl_helper_carry branch 2 times, most recently from cc2ca30 to 651f225 Compare August 21, 2022 15:11
go.mod Outdated Show resolved Hide resolved
keyctl/keyctl.go Outdated

// getDefaultCredsStore is a helper function to get the default credsStore keyring
func (k Keyctl) getDefaultCredsStore() (keyctl.NamedKeyring, error) {
if persistent == 1 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering why this check was (as persistent is a const, and always 1)

@crazy-max
Copy link
Member

https://github.com/docker/docker-credential-helpers/runs/7939788183?check_suite_focus=true#step:5:556

#37 57.52 vendor/github.com/jsipprell/keyctl/sys_linux.go:97:35: undefined: syscall_keyctl

Might need to install dev keyctl cross pkg with xx

@thaJeztah
Copy link
Member Author

Ah, yes; let me have a look later. Thought I'd give this one a quick go to see if it all worked, but definitely need to have a better look 😅

@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2022

Codecov Report

Patch coverage has no change and project coverage change: +3.89 🎉

Comparison is base (9ff5b61) 55.55% compared to head (5071773) 59.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
+ Coverage   55.55%   59.45%   +3.89%     
==========================================
  Files           9        8       -1     
  Lines         666      582      -84     
==========================================
- Hits          370      346      -24     
+ Misses        253      199      -54     
+ Partials       43       37       -6     

see 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 21, 2022

Unrelated:

Some warnings we can probably fix:

#0 0.099 gpg-connect-agent: no running gpg-agent - starting '/usr/bin/gpg-agent'
#19 0.101 gpg-connect-agent: waiting for the agent to come up ... (5s)
#19 0.103 gpg-connect-agent: connection to agent established
#19 0.104 OK
#19 0.106 gpg: WARNING: unsafe permissions on homedir '/root/.gnupg'
#19 0.107 gpg: keybox '/root/.gnupg/pubring.kbx' created
#19 0.109 gpg: /root/.gnupg/trustdb.gpg: trustdb created
#19 0.110 gpg: key 0x7D851EB72D73BDA0: public key "Joe Tester <joe@foo.bar>" imported
#19 0.113 gpg: key 0x7D851EB72D73BDA0: secret key imported
#19 0.113 gpg: Total number processed: 1
#19 0.114 gpg:               imported: 1
#19 0.114 gpg:       secret keys read: 1
#19 0.114 gpg:   secret keys imported: 1
#19 0.116 gpg: WARNING: unsafe permissions on homedir '/root/.gnupg'
#19 0.121 OK
#19 0.122 S KEYINFO 3E2D1142AA59E08E16B7E2C64BA6DDC773B1A627 D - - 1 P - - -
#19 0.122 OK
#19 0.124 OK
#19 0.125 S KEYINFO BA83FC8947213477F28ADC019F6564A956456163 D - - 1 P - - -
#19 0.125 OK
#19 0.135 created directory: '/root/.password-store/'
#19 0.136 Password store initialized for 7D851EB72D73BDA0
#19 0.139 gpg: WARNING: unsafe permissions on homedir '/root/.gnupg'

Some tests that are skipped that still mention travis CI (not sure we'll be able to run those tests, as it requires a Gnome session IIRC) edit: never mind; this is the sandboxed tests, so expected.

#19 6.949 === RUN   TestSecretServiceHelper
#19 6.949     secretservice_linux_test.go:11: test requires gnome-keyring but travis CI doesn't have it
#19 6.949 --- SKIP: TestSecretServiceHelper (0.00s)
#19 6.949 === RUN   TestMissingCredentials
#19 6.949     secretservice_linux_test.go:83: test requires gnome-keyring but travis CI doesn't have it
#19 6.949 --- SKIP: TestMissingCredentials (0.00s)

@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 21, 2022

This one is failing both sandboxed, and non-sandboxed; slightly different error though;

Non-sandboxed (ubuntu 20.04);

=== RUN   TestKeyctlHelper
    keyctl_linux_test.go:17: failed to list credentials: cannot read default credStore: cannot run keyctl command to created credstore keyring (/usr/bin/keyctl newring keyctlCredsStore 963514388): add_key: Permission denied
         : exit status 1
--- FAIL: TestKeyctlHelper (0.04s)

Sandboxed: perhaps capabilities or seccomp?

#19 2.088 === RUN   TestKeyctlHelper
#19 2.088     keyctl_linux_test.go:17: failed to list credentials: cannot read default credStore: default persistent keyring cannot be created: cannot run keyctl command to create persistent keyring: keyctl_get_persistent: Operation not permitted
#19 2.088         : exit status 1
#19 2.088 --- FAIL: TestKeyctlHelper (0.00s)

https://man7.org/linux/man-pages/man3/keyctl_get_persistent.3.html

DESCRIPTION 

       keyctl_get_persistent() gets the persistent keyring for the
       specified user ID.  Unlike the session and user keyrings, this
       keyring will persist once all login sessions have been deleted
       and can thus be used to carry authentication tokens for processes
       that run without user interaction, such as programs started by
       cron.

       The persistent keyring will be created by the kernel if it does
       not yet exist.  Each time this function is called, the persistent
       keyring will have its expiration timeout reset to the value in:

              /proc/sys/kernel/keys/persistent_keyring_expiry

       (by default three days).  Should the timeout be reached, the
       persistent keyring will be removed and everything it pins can
       then be garbage collected.

       If uid is -1 then the calling process's real user ID will be
       used.  If uid is not -1 then error EPERM will be given if the
       user ID requested does not match either the caller's real or
       effective user IDs or if the calling process does not have SetUid
       capability.

       If successful, a link to the persistent keyring will be added
       into keyring.

@thaJeztah
Copy link
Member Author

Slightly improved the errors to provide more details;

Non-sandboxed (ubuntu 20.04);

=== RUN   TestKeyctlHelper
    keyctl_linux_test.go:17: failed to list credentials: cannot read default credStore: cannot run keyctl command to create credstore keyring (/usr/bin/keyctl newring keyctlCredsStore 263788617): add_key: Permission denied
         : exit status 1
--- FAIL: TestKeyctlHelper (0.03s)

Sandboxed:

#19 1.934 === RUN   TestKeyctlHelper
#19 1.934     keyctl_linux_test.go:17: failed to list credentials: cannot read default credStore: default persistent keyring cannot be created: cannot run keyctl command (/bin/keyctl get_persistent @u 0) to create persistent keyring: keyctl_get_persistent: Operation not permitted
#19 1.934         : exit status 1
#19 1.934 --- FAIL: TestKeyctlHelper (0.00s)

alakesh and others added 9 commits May 27, 2023 16:37
Implement kernel kerying based credential helper for storing and
retrieving secrets.

Signed-off-by: Alakesh Haloi <alakeshh@amazon.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
While pkg/errors is a great package, it's probably not needed for
how it's used in this project, so let's replace with Go's native
error wrapping.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
full diff: jsipprell/keyctl@v1.0.0...v1.0.3

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@sctb512
Copy link

sctb512 commented Aug 4, 2023

Hello, @thaJeztah. I am intrigued by this PR. But I have a question. Can we retrieve the credential if the machine reboots?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants