WIP: Add prompt implementation for service methods lock & unlock #173

warusadura · 2024-12-09T14:19:32Z

Please see the commits.

bilelmoussaoui

also needs a rebase

client/src/crypto/native.rs

bilelmoussaoui · 2024-12-09T14:22:49Z

client/src/crypto/native.rs

+
+    let mut common_secret_bytes = common_secret.to_bytes_be();
+    let mut common_secret_padded = vec![0; 192 - common_secret_bytes.len()];
+    // inefficient, but ok for now


what is inefficient about it

client/src/crypto/native.rs

client/src/crypto/openssl.rs

server/src/secret_exchange.rs

bilelmoussaoui · 2024-12-09T14:31:47Z

server/src/service.rs

-        let (unlocked, _not_unlocked) = self.set_locked(false, &objects).await?;
+        let (unlocked, not_unlocked) = self.set_locked(false, &objects).await?;
+        if !not_unlocked.is_empty() {
+            let prompt = Prompt::new(self.clone()).await;


The prompt also needs a way to pass it a future that would be executed when the prompt is done

warusadura · 2024-12-09T14:45:18Z

Thanks for the prompt review @bilelmoussaoui

bilelmoussaoui · 2025-01-02T17:01:11Z

client/src/crypto/native.rs

@@ -1,6 +1,6 @@
 use std::{
    ops::{Mul, Rem, Shr},
-    sync::LazyLock,
+    sync::{LazyLock, OnceLock},


This lacks an openssl implementation as well as that is what most distros will likely use the daemon with

Will do, thanks!

Co-authored-by: Dhanuka Warusadura <[email protected]> Signed-off-by: Dhanuka Warusadura <[email protected]>

warusadura · 2025-01-10T08:29:02Z

Sorry for the delay @bilelmoussaoui

I have removed the prompt implementation for now. Just wanted get the crypto part and secret_exchange impl cleared before moving into the Prompt impl. I have removed a lot of unwraps, but I'm still not sure this is any good. Very much like to hear what you think of the current impl :)

server/src/gnome/secret_exchange.rs

bilelmoussaoui · 2025-01-10T08:44:01Z

server/src/gnome/secret_exchange.rs

+const CIPHER_TEXT_LEN: usize = 16;
+
+#[derive(Debug, Zeroize)]
+pub struct SecretExchange {


I am still not sure how the SecretExchange is supposed to be used. The current API, even if used internally only, is not really nice. why there is a need for a SecretExchange struct at all? who would call begin/create_shared_secret? and what about retrieve_secret? I would need to see how it is used in a real-case scenario to provide any meaningful feedback

Yeah, thanks!

server/src/gnome/prompter.rs

bilelmoussaoui · 2025-01-13T10:36:52Z

server/src/gnome/prompter.rs

+
+impl PrompterCallback {
+    pub async fn new(service: Service) -> Self {
+        let index = service.prompt_index().await;


You should create a PrompterProxy here and keepe it around for the functions below.

Good idea. But I can't access self inside tokio::spawn. All these following functions are associated functions.

server/src/prompt.rs

bilelmoussaoui · 2025-01-13T10:50:48Z

server/src/gnome/prompter.rs

+                    let objects = prompt.objects().clone();
+                    let result = Value::new(&objects).try_to_owned().unwrap();
+
+                    tokio::spawn(async move {
+                        let _ = service.set_locked(true, &objects, true).await;
+                    });


Instead of passing the objects, this should pass a future directly that you can spawn here. Keeping the logic that is supposed to be in Service::set_locked, stay there.

bilelmoussaoui · 2025-01-13T10:52:02Z

server/src/service.rs

+    pub async fn prompt(&self) -> Option<Prompt> {
+        let prompts = self.prompts.lock().await;
+
+        prompts.last().cloned()
+    }
+
+    pub async fn remove_prompt(&self) {
+        // prompts should always contain one item during a prompt related operation and
+        // it should be cleaned up afterwards.
+        self.prompts.lock().await.pop();
+    }


If there would always be one single prompt (which i don't believe is true, except for maybe GNOME specifically) as the API clearly allows modal keyring dialogs so every app could have an ongoing prompt operation.

bilelmoussaoui · 2025-01-13T10:52:16Z

server/src/service.rs

@@ -318,6 +336,7 @@ impl Service {
        &self,
        locked: bool,
        objects: &[OwnedObjectPath],
+        prompt: bool,


why is this needed?

bilelmoussaoui · 2025-01-13T11:00:44Z

server/src/gnome/prompter.rs

+                    tokio::spawn(PrompterCallback::perform_prompt(
+                        connection, path, "confirm", properties, exchange,
+                    ));


Not sure why this has to be in PrompterCallback,

SecretExchange allows exchange of secrets between two processes on the same system without exposing those secrets. See https://gnome.pages.gitlab.gnome.org/gcr/gcr-4/class.SecretExchange.html Signed-off-by: Dhanuka Warusadura <[email protected]>

server/src/gnome/prompter.rs

bilelmoussaoui · 2025-01-13T17:21:33Z

server/src/gnome/prompter.rs

+        match prompt.role() {
+            PromptRole::Lock => {
+                if properties.is_empty() {
+                    // First PromptReady call


Shouldn't most of the prompting code be exactly the same no matter the PromptRole? the only thing that i see being different is the Properties and the future to execute once the prompt is done

I think this is correct. I have just pushed the unlock commit (wip) and the code looks mostly the same with a couple of changes.

server/src/gnome/prompter.rs

warusadura · 2025-01-19T10:55:21Z

server/src/gnome/prompter.rs

+#[derive(Debug, DeserializeDict, SerializeDict, Type)]
+#[zvariant(signature = "a{sv}")]
+// System prompt properties
+pub struct Properties {


With this approach, during the second PromptReady call return I'm getting the following error, Unexpected non-0 padding byte 1. I can't figure out why. I think this is related to this. Then again all the fields here are properly renamed using #[zvariant(rename = "")

Check what gets send over the bus with busctl and see what is going on? Probably a mismatched type

Will do, thanks.

I tried everything I can think of. I think this is happening due to a bug in zbus. Probably related to zbus issues #1193 and #320.

Can you write a minimal reproducer of the issue? I am using the exact same thing in ashpd and it works fine.

Can you confirm this is caused by the properties and not the the reply param of PromptReady?

Sorry I missed the notification.

Yes I'm 100% certain this is caused by Properties struct. Everything works with the HashMap<&str, zvariant::OwnedValue> approach.

git diff:

diff --git a/server/src/gnome/prompter.rs b/server/src/gnome/prompter.rs index a139fba..b4104e5 100644 --- a/server/src/gnome/prompter.rs +++ b/server/src/gnome/prompter.rs @@ -1,11 +1,13 @@ // org.gnome.keyring.Prompter // https://gitlab.gnome.org/GNOME/gcr/-/blob/main/gcr/org.gnome.keyring.Prompter.xml +use std::collections::HashMap; + use clap::error::Result; use oo7::dbus::ServiceError; use serde::{Deserialize, Serialize}; use zbus::{ - interface, proxy, + interface, proxy, zvariant, zvariant::{DeserializeDict, OwnedObjectPath, OwnedValue, SerializeDict, Type, Value}, }; @@ -138,7 +140,7 @@ pub trait Prompter { &self, callback: OwnedObjectPath, type_: PromptType, - properties: Properties, + properties: HashMap<&str, OwnedValue>, exchange: &str, ) -> Result<(), ServiceError>; @@ -157,7 +159,7 @@ impl PrompterCallback { pub async fn prompt_ready( &self, reply: Reply, - _properties: Properties, + _properties: HashMap<&str, OwnedValue>, exchange: &str, #[zbus(connection)] connection: &zbus::Connection, ) -> Result<(), ServiceError> { @@ -181,6 +183,21 @@ impl PrompterCallback { })?; let exchange = secret_exchange.begin(); + let mut p: HashMap<&str, zvariant::OwnedValue> = HashMap::new(); + p.insert("continue-label", Value::new("Lock").try_to_owned().unwrap()); + p.insert( + "description", + Value::new("Confirm locking \"login\", Keyring.") + .try_to_owned() + .unwrap(), + ); + p.insert( + "message", + Value::new("Lock Keyring").try_to_owned().unwrap(), + ); + p.insert("caller-window", Value::new("").try_to_owned().unwrap()); + p.insert("cancel-label", Value::new("Cancel").try_to_owned().unwrap()); + let properties = Properties::for_lock(); let path = self.path.clone(); @@ -188,7 +205,7 @@ impl PrompterCallback { connection.clone(), path, PromptType::Confirm, - properties, + p, exchange, )); } @@ -261,6 +278,8 @@ impl PrompterCallback { })?; self.service.set_secret_exchange_key(aes_key).await; + let p: HashMap<&str, zvariant::OwnedValue> = HashMap::new(); + let properties = Properties::for_unlock(); let path = self.path.clone(); @@ -268,7 +287,7 @@ impl PrompterCallback { connection.clone(), path, PromptType::Confirm, - properties, + p, daemon_exchange, )); } @@ -373,7 +392,7 @@ impl PrompterCallback { connection: zbus::Connection, path: OwnedObjectPath, prompt_type: PromptType, - properties: Properties, + properties: HashMap<&str, OwnedValue>, exchange: String, ) -> Result<(), ServiceError> { let prompter = PrompterProxy::new(&connection).await?;

Then keep the Properties struct there and add a todo comment about it so we can investigate it further.

got it, thanks! I think I'll have to keep this diff as well. I can't test anything without it :)

Just locally.

bilelmoussaoui · 2025-01-19T13:37:31Z

server/src/gnome/prompter.rs

+    description: Option<String>,
+    message: Option<String>,
+    #[zvariant(rename = "caller-window")]
+    caller_window: Option<String>,


This should be a WindowIdentifierType from ashpd

Don't know anything about ashpd. I'll look into it :)

You don't have to do anything special here, just use that type.

bilelmoussaoui · 2025-01-19T13:38:04Z

server/src/gnome/prompter.rs

+pub enum Reply {
+    No,
+    Yes,
+    Empty,


What does empty corresponds to? Please link in the code to where those answers are defined

Empty corresponds to: ''. The first PromptReady call's reply argument is always '' and also the properties dict is empty.

Check if you can use the zvariant::Optional type here and drop the Empty variant.

Will do, thanks!

hey. I tried reply: Optional<Reply> approach and I think the current approach with Empty variant is bit better or shorter. Because all we have to do is impl Serialize and Deserialize. When using Optional, I had to impl NoneValue and bunch of String, &str traits. I just stopped at: trait FromStr is not implemented for Reply. Should I keep Empty variant or continue with Optional?

Use zvariant::Optional please. and you shouldn't have to implement that many traits but if you are struggling with it, you can leave it as is and i will take care of it before merging.

Ok, thanks :)

bilelmoussaoui · 2025-01-19T13:38:17Z

server/src/gnome/prompter.rs

+    Empty,
+}
+
+const NO: &str = "no";


No need for those consts

bilelmoussaoui · 2025-01-19T13:39:35Z

server/src/gnome/prompter.rs

+#[derive(Deserialize, Serialize, Debug, Type)]
+#[serde(rename_all = "lowercase")]
+#[zvariant(signature = "s")]
+// Possible values for PerformPrompt type parameter


Should this implement the serialisation traits or the derived ones are correct?

Yes, the derived ones are correct. I had to implement the serialisation and de-serialisation for Reply because of the Empty variant.

bilelmoussaoui · 2025-01-19T13:40:37Z

server/src/gnome/prompter.rs

+    pub async fn prompt_ready(
+        &self,
+        reply: Reply,
+        _properties: Properties,


Why is this unused? Already mentioned it before but you marked the thread as resolved except not really...

I'll take another look.

bilelmoussaoui · 2025-01-19T13:42:32Z

server/src/prompt.rs

+        #[zbus(connection)] connection: &zbus::Connection,
+        #[zbus(object_server)] object_server: &zbus::ObjectServer,
+    ) -> Result<(), ServiceError> {
+        let callback = PrompterCallback::new(self.service.clone()).await;


The prompt callback creates the prompt path again, just pass it here instead

bilelmoussaoui · 2025-01-19T17:12:39Z

cc @ueno, could you review & ack the crypto code please?

bilelmoussaoui · 2025-01-21T08:01:00Z

client/src/key.rs

@@ -47,6 +47,14 @@ impl Key {
        Ok(Self::new(crypto::generate_public_key(private_key)?))
    }

+    pub fn generate_public_key_for_secret_exchange(


Those new methods needs tests, similar to the previous ones to ensure we have the same behaviour in both native & openssl crypto.

bilelmoussaoui · 2025-01-21T10:08:45Z

client/src/crypto/native.rs

@@ -86,6 +86,16 @@ pub(crate) fn generate_public_key(private_key: impl AsRef<[u8]>) -> Result<Vec<u
    Ok(public_key_uint.to_bytes_be())
 }

+pub(crate) fn generate_public_key_for_secret_exchange(


As those APIs are only useful for the secret exchange, which is only used by the GNOME implementation I don't think we should have them here and instead move them to the server side under /gnome. Especially that we have a Key::new

bilelmoussaoui · 2025-01-23T08:23:36Z

server/src/gnome/prompter.rs

+            title: Some("Unlock Keyring".to_string()),
+            choice_label: None,
+            description: Some(
+                "An application wants access to the keyring 'login', but it is locked".to_string(),


This assumes the keyring used. Shouldn't be hardcoded but rather passed as a parameter

bilelmoussaoui · 2025-01-23T08:25:10Z

server/src/gnome/prompter.rs

+                            exchange,
+                            &self.service.secret_exchange_key().await,
+                        ) {
+                            match oo7::file::Keyring::open("login", secret).await {


Same, this assumes the keyring to open

bilelmoussaoui · 2025-01-23T08:26:05Z

server/src/service.rs

    ) -> Result<(Vec<OwnedObjectPath>, OwnedObjectPath), ServiceError> {
-        let (unlocked, _not_unlocked) = self.set_locked(false, &objects, false).await?;
+        let (unlocked, not_unlocked) = self.set_locked(false, &objects, false).await?;


same as i mentioned earlier, Prompt should take a Future. The current code is way too complex just to avoid passing a future which would make everything much easier to follow

Even though gnome-keyring-daemon doesn't implement/show a prompt during the service method lock. The secret service spec says a prompt can be used during lock. So, we're adding this to the new daemon. Signed-off-by: Dhanuka Warusadura <[email protected]>

Signed-off-by: Dhanuka Warusadura <[email protected]>

bilelmoussaoui reviewed Dec 9, 2024

View reviewed changes

bilelmoussaoui reviewed Jan 2, 2025

View reviewed changes

client: Use DH with the 1536 ike modp group for secret exchange

01e5ee1

Co-authored-by: Dhanuka Warusadura <[email protected]> Signed-off-by: Dhanuka Warusadura <[email protected]>

warusadura force-pushed the secret_exchange branch 3 times, most recently from db19706 to 49e0912 Compare January 10, 2025 08:25

warusadura force-pushed the secret_exchange branch from 49e0912 to e79dfe6 Compare January 10, 2025 08:31

bilelmoussaoui reviewed Jan 10, 2025

View reviewed changes

server/src/gnome/secret_exchange.rs Outdated Show resolved Hide resolved

bilelmoussaoui reviewed Jan 10, 2025

View reviewed changes

warusadura force-pushed the secret_exchange branch from cf14fbe to 1b96455 Compare January 13, 2025 08:48

bilelmoussaoui reviewed Jan 13, 2025

View reviewed changes

warusadura force-pushed the secret_exchange branch from 1b96455 to 90ca8c6 Compare January 13, 2025 16:55

warusadura changed the title ~~WIP: Add prompt implementation for service method unlock~~ WIP: Add prompt implementation for service methods lock & unlock Jan 13, 2025

bilelmoussaoui reviewed Jan 13, 2025

View reviewed changes

warusadura force-pushed the secret_exchange branch from 90ca8c6 to 5c626cb Compare January 19, 2025 10:45

warusadura commented Jan 19, 2025

View reviewed changes

bilelmoussaoui reviewed Jan 19, 2025

View reviewed changes

warusadura force-pushed the secret_exchange branch 2 times, most recently from 2399214 to 23067f3 Compare January 19, 2025 14:25

bilelmoussaoui reviewed Jan 21, 2025

View reviewed changes

bilelmoussaoui reviewed Jan 23, 2025

View reviewed changes

warusadura added 2 commits January 23, 2025 14:09

Add prompt implementation for service method unlock

0575dac

Signed-off-by: Dhanuka Warusadura <[email protected]>

warusadura force-pushed the secret_exchange branch from 23067f3 to 0575dac Compare January 23, 2025 08:43

WIP: Add prompt implementation for service methods lock & unlock #173

Are you sure you want to change the base?

WIP: Add prompt implementation for service methods lock & unlock #173

Conversation

warusadura commented Dec 9, 2024 • edited Loading

bilelmoussaoui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warusadura commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warusadura commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warusadura Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bilelmoussaoui commented Jan 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warusadura commented Dec 9, 2024 •

edited

Loading

warusadura Jan 23, 2025 •

edited

Loading