-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(crypto/keyring): add Linux's keyctl support #17607
Conversation
Can we get a changelog under unreleased improvements? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thank you and nice to catch you here after ages @alessio! I've added some feedback. Also kindly provide a test if possible.
I am reviewing your changes @odeke-em and am learning something new. Thanks :) |
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Files ignored due to filter (1)
- go.mod
Files selected for processing (3)
- crypto/keyring/keyring.go (1 hunks)
- crypto/keyring/keyring_linux.go (1 hunks)
- crypto/keyring/keyring_other.go (1 hunks)
Additional comments (Suppressed): 5
crypto/keyring/keyring_other.go (2)
6-10: The import statement for
"io"
and"github.com/cosmos/cosmos-sdk/codec"
are not used in theNew
function. Please ensure that these imports are necessary for other parts of the code not shown in this hunk. If not, consider removing them to avoid unused imports.12-16: The
New
function is a simple wrapper aroundnewKeyringGeneric
. This is a good practice as it provides a clean and simple interface for creating a new keyring. It also allows for future expansion or changes in the underlying implementation without affecting the callers of this function.crypto/keyring/keyring_linux.go (2)
28-41: The
New
function now supports the "keyctl" backend. Ensure that all calls to this function throughout the codebase have been updated to handle the new error return value. Also, verify that the "keyctl" backend is available and functional in the deployment environment.35-38: Error handling is done correctly here. If
keyring.Open
fails, the error is returned immediately, preventing further execution.crypto/keyring/keyring.go (1)
- 176-181: The function signature has been changed from
New
tonewKeyringGeneric
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the change in function visibility (from public to private) does not affect any external packages that might be using this function.
keyctl store secrets in volatile memory right, which is very different from the other backends, I'm curious about the usage for keyctl. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Could you make a variable of the supported backend in keyring_linux and keyring_other, and use that here:
cosmos-sdk/client/flags/flags.go
Line 153 in c73e823
flags.String(FlagKeyringBackend, DefaultKeyringBackend, "Select keyring's backend (os|file|kwallet|pass|test|memory)") |
This way it is obvious that keyctl is supported on linux.
Same as yihuang curious about the use case. Could you comment on that?
Additionally, you need to run make lint-fix
on your branch.
closing due to staleness |
I stumbled upon use cases where a user wanted to use a keyring for as long as the process is alive w/o being prompted for passwords at every turn. keyctl allows cryptographical material to be stored in memory and is kept private to the process. |
Could you elaborate further, please? |
I opened a new PR #21653 |
For more info, please see: https://docs.kernel.org/security/keys/core.html
The feature is built only when GOARCH=linux.
Summary by CodeRabbit
newKeyringGeneric
, improving error handling by returning both the Keyring and any potential error.New
, for creating a new keyring, simplifying the user experience.These changes aim to improve the flexibility, error handling, and user experience when working with keyrings.