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

chore: Make keyring non-interactive #3026

Conversation

fredcarle
Copy link
Collaborator

@fredcarle fredcarle commented Sep 17, 2024

Relevant issue(s)

Resolves #2995

Description

This PR makes the keyring interactions non-interactive by requiring the keyring password to be set as an environment variable secret. It also adds support for that secret to be stored in a .env file in the working directory or in a file at a path defined by the --secret-file flag.

Making the keyring non-interactive is necessary to support automated deployments.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

Also tested manually starting Defra with a .env file to make sure that the secret was picked up.

Specify the platform(s) on which this was tested:

  • MacOS

@fredcarle fredcarle added the area/cli Related to the CLI binary label Sep 17, 2024
@fredcarle fredcarle added this to the DefraDB v0.14 milestone Sep 17, 2024
@fredcarle fredcarle requested a review from a team September 17, 2024 23:46
@fredcarle fredcarle self-assigned this Sep 17, 2024
@fredcarle fredcarle force-pushed the fredcarle/feat/2995-support-for-non-interactive-keyring branch from b951027 to a91136a Compare September 17, 2024 23:53
Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 52.94118% with 24 lines in your changes missing coverage. Please review.

Project coverage is 79.36%. Comparing base (0e91a49) to head (fcae1de).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
cli/start.go 16.67% 20 Missing ⚠️
cli/config.go 33.33% 1 Missing and 1 partial ⚠️
cli/utils.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3026      +/-   ##
===========================================
+ Coverage    79.34%   79.36%   +0.01%     
===========================================
  Files          333      333              
  Lines        25822    25837      +15     
===========================================
+ Hits         20488    20503      +15     
- Misses        3865     3869       +4     
+ Partials      1469     1465       -4     
Flag Coverage Δ
all-tests 79.36% <52.94%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cli/errors.go 0.00% <ø> (ø)
cli/keyring_export.go 81.82% <100.00%> (+2.87%) ⬆️
cli/keyring_generate.go 78.72% <100.00%> (+1.45%) ⬆️
cli/keyring_import.go 80.95% <100.00%> (+3.17%) ⬆️
cli/root.go 97.73% <100.00%> (+0.11%) ⬆️
keyring/file.go 72.73% <100.00%> (+6.06%) ⬆️
cli/config.go 80.77% <33.33%> (-2.90%) ⬇️
cli/utils.go 76.34% <50.00%> (+1.85%) ⬆️
cli/start.go 38.61% <16.67%> (-2.70%) ⬇️

... and 9 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e91a49...fcae1de. Read the comment docs.

cli/config.go Outdated Show resolved Hide resolved
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

might want to also ignore the .env file in the .gitignore

Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

I like the approach, just had some questions:

question: Where is the loading of the environment variables from '.env' file being done?

question: When you say root directory what do you mean i.e. what are the paths that this .env file will be picked from (and the order of priority if many paths)?

question: Have you tested this works when .env flow?

readPassword = func(_ *cobra.Command, _ string) ([]byte, error) {
return []byte("secret"), nil
}
err := os.Setenv("DEFRA_KEYRING_SECRET", "password")
Copy link
Contributor

Choose a reason for hiding this comment

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

question: don't we want to keep at least one test that uses the old prompt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The prompt has been removed entirely.

@fredcarle fredcarle force-pushed the fredcarle/feat/2995-support-for-non-interactive-keyring branch from b8937c7 to 23745f5 Compare September 18, 2024 12:09
@fredcarle
Copy link
Collaborator Author

question: Where is the loading of the environment variables from '.env' file being done?

@shahzadlone, it's done from where Defra is started.

question: When you say root directory what do you mean i.e. what are the paths that this .env file will be picked from (and the order of priority if many paths)?

Currently there is only one path and it's ./.env. Eventually I would like to add a flag to let users set the path.

question: Have you tested this works when .env flow?

Yes I've tested this manually.

@fredcarle fredcarle force-pushed the fredcarle/feat/2995-support-for-non-interactive-keyring branch 2 times, most recently from 2216e9c to aa381ee Compare September 18, 2024 19:24
@fredcarle
Copy link
Collaborator Author

question: When you say root directory what do you mean i.e. what are the paths that this .env file will be picked from (and the order of priority if many paths)?

Currently there is only one path and it's ./.env. Eventually I would like to add a flag to let users set the path.

@shahzadlone I've added a flag to let the user define the path and updated the documentation to say working directory instead of root directory.

Copy link
Member

@nasdf nasdf left a comment

Choose a reason for hiding this comment

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

A few thoughts and questions but nothing blocking.

cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/keyring_generate.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

LGTM

@fredcarle fredcarle force-pushed the fredcarle/feat/2995-support-for-non-interactive-keyring branch from 436738c to aab3f33 Compare September 18, 2024 21:30
@fredcarle fredcarle merged commit 5b58c19 into sourcenetwork:develop Sep 18, 2024
42 of 44 checks passed
@fredcarle fredcarle deleted the fredcarle/feat/2995-support-for-non-interactive-keyring branch September 18, 2024 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Related to the CLI binary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for non-interactive use of the keyring
4 participants