Skip to content

Add storage backend for configs based on OS provided secret store #3333

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Sep 29, 2022
Merged

Add storage backend for configs based on OS provided secret store #3333

merged 6 commits into from
Sep 29, 2022

Conversation

@anjannath
Copy link
Member

@anjannath anjannath commented Sep 5, 2022 •
edited
Loading

Fixes #3312

@openshift-ci openshift-ci bot requested review from cfergeau and gbraad September 5, 2022 10:22
cfergeau
cfergeau reviewed Sep 5, 2022
View reviewed changes
Copy link
Contributor

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

Adding a second implementation of the Config interface to handle secrets is an interesting approach! One thing to have in mind though is that this 'secret' keyring will not always be available, I don't have access to gnome-keyring when I ssh into a linux box (but maybe this is a matter of configuration?)

@anjannath
Copy link
Member Author

anjannath commented Sep 5, 2022

Adding a second implementation of the Config interface to handle secrets is an interesting approach! One thing to have in mind though is that this 'secret' keyring will not always be available, I don't have access to gnome-keyring when I ssh into a linux box (but maybe this is a matter of configuration?)

ahh, didn't know, i don't think i ever tried this, but always assumed the cli secret-tool works via ssh

@cfergeau
Copy link
Contributor

cfergeau commented Sep 6, 2022

ahh, didn't know, i don't think i ever tried this, but always assumed the cli secret-tool works via ssh

My observations are only based on the pull secret keyring code, it always outputs a warning on my test machine, I never checked if it can be fixed.

@anjannath
Copy link
Member Author

anjannath commented Sep 6, 2022

ahh, didn't know, i don't think i ever tried this, but always assumed the cli secret-tool works via ssh

My observations are only based on the pull secret keyring code, it always outputs a warning on my test machine, I never checked if it can be fixed.

seems like the issue is with dbus not being accessible/available, its failing in CI for ubuntu :(

@anjannath anjannath force-pushed the secret-config branch 3 times, most recently from 0e54395 to 2a344cc Compare September 6, 2022 10:30
@anjannath anjannath mentioned this pull request Sep 7, 2022
Merged
@anjannath anjannath changed the title [wip] Fixes #3312 Add config storage backend for os keyring Fixes #3312 Add config storage backend for os keyring Sep 7, 2022
@anjannath anjannath changed the title Fixes #3312 Add config storage backend for os keyring Add storage backend for configs based on OS provided secret store Sep 7, 2022
@anjannath
Copy link
Member Author

anjannath commented Sep 7, 2022

@cfergeau i've tried to add a check to see if the keyring is accessible at the beginning when the secret store is initialised and not error out and crash the whole thing when we try to access a secret config store that doesn't have access to the keychain, PTALA :)

anjannath
anjannath commented Sep 13, 2022
View reviewed changes
anjannath
anjannath commented Sep 13, 2022
View reviewed changes
@anjannath anjannath force-pushed the secret-config branch 2 times, most recently from 9ad38e9 to dc7c612 Compare September 15, 2022 08:17
anjannath
anjannath commented Sep 19, 2022
View reviewed changes
@anjannath anjannath added the status/peer review required Peer review by assignee is required before being merged; label Sep 21, 2022
cfergeau
cfergeau reviewed Sep 23, 2022
View reviewed changes
Copy link
Contributor

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

I need to take a closer look at Add a Secret type to register secret settings, but overall this looks nice!

cfergeau
cfergeau reviewed Sep 26, 2022
View reviewed changes
@gbraad gbraad added this to the 2022 Q3 milestone Sep 27, 2022
@gbraad gbraad linked an issue Sep 27, 2022 that may be closed by this pull request
CIFS/SMB based file sharing for windows #3230
Closed
@anjannath
Add storage backend for configs based on OS provided secret store
10da087 
this implements the RawStorage interface by using the OS provided
keyring/secret store as the persistent store of config settings

currently config settings are peristed to disk by writing to a JSON
file in ~/.crc/crc.json this becomes an issue if we want to have  a
config setting that stores a password or anyother secret as we dont
want it to get written to disk unencrypted

creating an instance of Config with SecretStore as the storage  the
the settings/secrets are written to the OS keyring instead
@anjannath
Add Secret storage to Config struct along with Viper storage
f61af1c 
extends the crc Config struct to also contain an instance of Secret
storage same as the Viper storage, which will store secret  configs

this doesn't add a way to register secret config yet so the  secret
store is not being used and only added to the struct and config.New
method is modified to take a second RawStorage which meant updating
the tests and other places where config.New was called
@anjannath anjannath force-pushed the secret-config branch 3 times, most recently from e6db0e7 to 52bbb0b Compare September 27, 2022 10:07
cfergeau
cfergeau approved these changes Sep 28, 2022
View reviewed changes
pkg/crc/config/config.go Outdated

if err := c.storage.Unset(key); err != nil {
return "", err
switch setting.defaultValue.(type) {
Copy link
Contributor

@cfergeau cfergeau Sep 28, 2022

Choose a reason for hiding this comment

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

Maybe use setting.isSecret here too?

@anjannath
Add a Secret type to register secret settings
7335240 
this adds a Secret type which is just a string but can be used  to
register a setting as a Secret setting, by using this type for the
default empty value of the config

this adds a new private field 'isSecret' to 'Setting' struct which
is used by AddSetting to mark a Setting as secret

based on this field it is decided which storage is used to persist
or fetch the config setting
@anjannath
Add a way to easily identify a secret setting value
6ca0fba 
this adds a bool field 'IsSecret' to SettingValue struct

we'll use this field to filter out secret config  values
in the o/p of 'crc config view' command
@anjannath
Add boolean flag 'show-secrets' to config sub command 'view'
97025ac 
this new flag is added to control the revealing of secrets for the
'crc config view' command, by default the flag is set to false and
keeps the secrets, secret.

this is only added to the 'view' sub command as for 'get' and 'set'
commands users have to also mention the config name and are  aware
that they are working with a secret config

for 'view' to reveal the secrets we'll need to pass '--show-secrets'
flag, 'crc config view --show-secrets'
@anjannath
Don't include Secret configs in the response from /config http api
c64123d 
we don't expose these configs in the tray UI the only consumer of the
http api
@openshift-ci openshift-ci bot removed the lgtm label Sep 29, 2022
@openshift-ci openshift-ci bot added the lgtm label Sep 29, 2022
@openshift-ci
Copy link

openshift-ci bot commented Sep 29, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@anjannath anjannath merged commit 105ad05 into crc-org:main Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cfergeau cfergeau cfergeau approved these changes

@gbraad gbraad Awaiting requested review from gbraad

Assignees

@cfergeau cfergeau

Labels

approved lgtm status/peer review required Peer review by assignee is required before being merged;

Projects

None yet

Milestone

2022 Q3

Development

Successfully merging this pull request may close these issues.

Add a secret config type to store secrets in the OS provided secret store CIFS/SMB based file sharing for windows

3 participants

@anjannath @cfergeau @gbraad