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.

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

Add keyring-util #50

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Add keyring-util #50

merged 1 commit into from
Dec 19, 2023

Conversation

A6GibKm
Copy link
Collaborator

@A6GibKm A6GibKm commented Nov 15, 2023

Fixes: #48

warusadura
warusadura reviewed Nov 16, 2023
View reviewed changes
keyring-util/Cargo.toml Show resolved Hide resolved
keyring-util/src/main.rs Show resolved Hide resolved
keyring-util/src/main.rs Show resolved Hide resolved
@A6GibKm
Copy link
Collaborator Author

A6GibKm commented Nov 16, 2023 •
edited
Loading

Regarding search and lookup, one could do get and list instead.

@A6GibKm A6GibKm force-pushed the keyring-util branch 2 times, most recently from 7165572 to dd66e26 Compare November 25, 2023 18:45
@A6GibKm A6GibKm changed the title WIP: Add keyring-util Add keyring-util Nov 25, 2023
@A6GibKm A6GibKm changed the title Add keyring-util WIP: Add keyring-util Nov 25, 2023
@A6GibKm A6GibKm changed the title WIP: Add keyring-util Add keyring-util Nov 25, 2023
@bilelmoussaoui
Copy link
Owner

Looks good to me but still lacks CI bits :)

warusadura
warusadura reviewed Dec 5, 2023
View reviewed changes
Copy link
Collaborator

@warusadura warusadura left a comment

Choose a reason for hiding this comment

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

Looks good to me!
I have one suggestion though and I'm not sure it's possible to implement. It would be great if keyring-util can handle a scenario like the following,

 ./target/debug/keyring-util store 'My password' key1 value1
Error: The collection 'default' doesn't exists

So, we need a new command to create a keyring or collection. What do you think @A6GibKm ?

keyring-util/Cargo.toml Show resolved Hide resolved
@bilelmoussaoui
Copy link
Owner

Looks good to me! I have one suggestion though and I'm not sure it's possible to implement. It would be great if keyring-util can handle a scenario like the following,

 ./target/debug/keyring-util store 'My password' key1 value1
Error: The collection 'default' doesn't exists

So, we need a new command to create a keyring or collection. What do you think @A6GibKm ?

I would check first what is the behaviour of secret-tool in this case. Collections are not widely used so maybe we should automatically create the default collection if it doesn't exists?

bilelmoussaoui
bilelmoussaoui reviewed Dec 6, 2023
View reviewed changes
keyring-util/src/main.rs Outdated
}

async fn collection<'a>() -> Result<Collection<'a>, Error> {
let service = Service::new(Algorithm::Encrypted)
Copy link
Owner

Choose a reason for hiding this comment

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

Unrelated but as we have the same constructor that fallsback to a plain connection, we should add such constructor to the API itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are two common patterns we use repeatedly fallback the encrypted to a plain service, and load or create+load the default keyring, both used here (after the last force-push).

Regarding the first one, I am not sure how to call such method. Regarding the second, I don't think it is justified to create a helper for this, this might not be something users of the library should be doing often.

keyring-util/Cargo.toml Show resolved Hide resolved
keyring-util/src/main.rs
async fn store(label: &str, attributes: HashMap<&str, &str>) -> Result<(), Error> {
let collection = collection().await?;

print!("Type a secret: ");
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be translatable ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure how I feel about translating CLI utils, maybe it makes sense here since we request an input. In any case this requires to have meson.

@A6GibKm A6GibKm force-pushed the keyring-util branch 3 times, most recently from 45eafbf to a6600e4 Compare December 14, 2023 21:23
@A6GibKm
Copy link
Collaborator Author

A6GibKm commented Dec 14, 2023

Looks good to me! I have one suggestion though and I'm not sure it's possible to implement. It would be great if keyring-util can handle a scenario like the following,

 ./target/debug/keyring-util store 'My password' key1 value1
Error: The collection 'default' doesn't exists

So, we need a new command to create a keyring or collection. What do you think @A6GibKm ?

I would check first what is the behaviour of secret-tool in this case. Collections are not widely used so maybe we should automatically create the default collection if it doesn't exists?

For the moment I will just create one if needed.

@A6GibKm
Copy link
Collaborator Author

A6GibKm commented Dec 15, 2023 •
edited
Loading

Looks good to me but still lacks CI bits :)

Could you please take care of this? I have yet to understand how github CI works. Figured it out already.

@A6GibKm A6GibKm force-pushed the keyring-util branch 7 times, most recently from bfb47d0 to 0f5a4f7 Compare December 15, 2023 21:39
bilelmoussaoui
bilelmoussaoui reviewed Dec 19, 2023
View reviewed changes
Copy link
Owner

@bilelmoussaoui bilelmoussaoui left a comment

Choose a reason for hiding this comment

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

otherwise looks good! thanks

.github/workflows/CI.yml Outdated
@@ -40,6 +46,9 @@ jobs:
- name: Build and test (OpenSSL)
run: |
dbus-run-session -- cargo test --no-default-features --features async-std --features openssl_crypto
- name: Build and test keyring-util
Copy link
Owner

Choose a reason for hiding this comment

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

we don't have tests for keyring-util, so just drop this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to Build and used cargo build instead.

Cargo.toml Outdated Show resolved Hide resolved
keyring-util/Cargo.toml Outdated Show resolved Hide resolved
keyring-util/Cargo.toml Outdated Show resolved Hide resolved
@A6GibKm A6GibKm force-pushed the keyring-util branch 2 times, most recently from ccba964 to e8c9a47 Compare December 19, 2023 18:09
@A6GibKm A6GibKm force-pushed the keyring-util branch 2 times, most recently from 00cb0e6 to 7387b62 Compare December 19, 2023 18:22
@bilelmoussaoui bilelmoussaoui merged commit 9414a57 into bilelmoussaoui:main Dec 19, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@warusadura warusadura warusadura left review comments

@sophie-h sophie-h sophie-h left review comments

@bilelmoussaoui bilelmoussaoui bilelmoussaoui approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Re-implement secret-tool
4 participants
@A6GibKm @bilelmoussaoui @sophie-h @warusadura