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

feat(crypto/keyring): add Linux's keyctl support #17607

Closed
wants to merge 13 commits into from

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Sep 1, 2023

For more info, please see: https://docs.kernel.org/security/keys/core.html

The feature is built only when GOARCH=linux.

Summary by CodeRabbit

  • New Feature: Enhanced the keyring creation process with a more intuitive function, newKeyringGeneric, improving error handling by returning both the Keyring and any potential error.
  • New Feature: Added support for the 'keyctl' backend in the keyring package, providing more flexibility and options when creating a new keyring.
  • Improvement: Introduced a convenient wrapper function, 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.

@alessio alessio requested a review from a team as a code owner September 1, 2023 14:21
@github-prbot github-prbot requested review from a team, facundomedica and likhita-809 and removed request for a team September 1, 2023 14:21
@julienrbrt julienrbrt changed the title keyring: add Linux's keyctl support feat(crypto/keyring): add Linux's keyctl support Sep 1, 2023
@julienrbrt
Copy link
Member

Can we get a changelog under unreleased improvements?

Copy link
Collaborator

@odeke-em odeke-em left a 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.

crypto/keyring/keyring_linux.go Outdated Show resolved Hide resolved
crypto/keyring/keyring_linux.go Outdated Show resolved Hide resolved
@alessio alessio requested a review from odeke-em September 23, 2023 17:31
@alessio
Copy link
Contributor Author

alessio commented Sep 25, 2023

I am reviewing your changes @odeke-em and am learning something new. Thanks :)

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 20, 2023


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai help to get help.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits Files that changed from the base of the PR and between 5cd5003 and 066dcfe.
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 the New 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 around newKeyringGeneric. 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 to newKeyringGeneric. 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.

crypto/keyring/keyring_linux.go Show resolved Hide resolved
@yihuang
Copy link
Collaborator

yihuang commented Nov 1, 2023

keyctl store secrets in volatile memory right, which is very different from the other backends, I'm curious about the usage for keyctl.

@julienrbrt julienrbrt self-requested a review November 1, 2023 07:44
Copy link
Member

@julienrbrt julienrbrt left a 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:

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.

@tac0turtle
Copy link
Member

closing due to staleness

@tac0turtle tac0turtle closed this Nov 12, 2023
@alessio
Copy link
Contributor Author

alessio commented Sep 11, 2024

This way it is obvious that keyctl is supported on linux.
Same as yihuang curious about the use case. Could you comment on that?

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.

@alessio
Copy link
Contributor Author

alessio commented Sep 11, 2024

lgtm!

Could you make a variable of the supported backend in keyring_linux and keyring_other, and use that here:

Could you elaborate further, please?

@alessio
Copy link
Contributor Author

alessio commented Sep 11, 2024

I opened a new PR #21653

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.

8 participants