From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 1AB961FF16F for ; Tue, 19 Aug 2025 15:55:02 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D6BF61678C; Tue, 19 Aug 2025 15:56:46 +0200 (CEST) Mime-Version: 1.0 Date: Tue, 19 Aug 2025 15:56:42 +0200 Message-Id: From: "Lukas Wagner" To: "Dominik Csapak" , X-Mailer: aerc 0.20.1-0-g2ecb8770224a References: <20250516133611.3499075-1-d.csapak@proxmox.com> <49f1e7de-52c3-4118-ac9d-5849bdda1cc3@proxmox.com> In-Reply-To: <49f1e7de-52c3-4118-ac9d-5849bdda1cc3@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1755611760392 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.022 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [lib.rs, connection.rs, mod.rs, remotes.rs] Subject: Re: [pdm-devel] [PATCH datacenter-manager 00/21] improve remote wizard X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" On Tue Aug 19, 2025 at 2:52 PM CEST, Dominik Csapak wrote: > > > On 8/19/25 14:09, Lukas Wagner wrote: >> On Fri May 16, 2025 at 3:35 PM CEST, Dominik Csapak wrote: >>> # Summary >>> >>> This series improves the remote wizard in various points: >>> * improved wording and texts >>> * moved seperate buttons on pages into the next button >>> * probing of entered nodes >>> * confirmation dialog for missing fingerprints >>> * better realm selector >>> >>> # Notes >>> >>> I'm not super sure about the API call structure. I think two seperate >>> API calls (one for tls probing, one for scanning) might make more sense. >>> If that's wanted, I'll do so in a v2 of the series. >> >> Yeah, I think two separate API endpoints make a lot of sense, the `scan` >> endpoint is a bit weird right now. >> > thanks, i also thought so, but only after i did this implementation > already. will change the v3 to include a separate api call for tls > probing. Doing 2 api calls from the ui should be fine > >>> >>> ## Probing on the server side >>> >>> I tried to determine if we need a fingerprint for nodes inside the >>> API call, by probing each node. Since we currently only get the >>> nodenames and not FQDNs in the nodelist, this will currently not >>> result in a valid connection in most cases and return the fingerprint. >>> >>> My plan would be to include FQDNs of the nodes on the PVE side API call, >>> so we can return here a list of nodename, ip and FQDNs, which the user >>> then can select from. (which of those I'd probe on first check is yet >>> to be determined) >>> >>> ## Fingerprint confirmation dialogs >>> >>> Not sure if we want to be able to let the user confirm the fingerprint >>> so easily. On one hand it's very convenient, but maybe leads to users >>> simply clicking yes without understanding what's happening. >>> >>> If it's deemed too dangerous, I'd rework the series without this. >> >> Good question. IMO something like this can be okay for a bit more >> convenience, but maybe some other people have an opinion on this as >> well. >> >> One thing I'm not sure about is the fact that we spawn a dialog from a >> dialog here, which (I think) is a first for us. >> > > yes it's a first indeed, but especially on the nodes part of the wizard > i found no good way to show that info otherwise. I can try to integrate > the info in the panel itself, but since it requires confirmation > i would change that to a checkbox ("Trust these/this fingerprint(s)") ? > > on the nodes part, it would involve scrolling to possibly show all > the fingerprints of the nodes, (which also can happen on the dialog if > there are many). > Mhmmm, given the security implications I think the dialog is actually the best choice for now (as it actively seeks the user's attention), so IMO this can stay as is for now. >> >> Some other minor things I've noticed while testing this: >> >> - Entering the remote name on the second stage of the wizard seemed a >> bit odd to me (can't really explain why, just a gut feeling I had when >> the second stage was shown). But I guess it could make sense in case >> we want to autofill the clustername there. > > i get what you mean, but yes exactly, the idea initially was to autofill > the cluster name here (so the first panel would not work). > > For now I'd leave it on the second page (it's already that way) if > that's ok for you. (We can still move it around later) Fine for me :) > >> >> - When you go from "Summary" from "Probe Remote" and then to "Settings" >> (all via clicking the tab bar), the 'Trust fingerprint' dialog is >> shown again. Maybe we can detect that the IP/host has not been changed >> and/or cache which fingerprints we already marked as trusted? > > Should be possible, I'll see that I do it that way. Could you just add the accepted fingerprint into the "fingerprint" field in the first page? I think this would be a quite simple fix for this. > >> >> - 'Realm' field should be greyed out when "Use existing Token" has been >> selected > > oops, that's a bug^^ > >> >> All just minor things, all it all these changes looked quite solid to >> me! >> >>> >>> # Future work >>> >>> The next step for the wizard is to have some kind of quick copy&paste >>> info. After discussing off-list with Fabian a bit, I think it would be >>> best for this to contain the hostname (FQDN?) + fingerprint (if just a >>> self-signed certificate) + a list of nodes with their respective >>> nodename + FQDNs (maybe requires api change on PVE side to generate >>> this). The user would then still have to do most of the steps currently >>> necessary in the wizard, except the manual copy & pasting of >>> fingerprints and maybe entering of FQDNs. >>> >>> >>> Dominik Csapak (21): >>> server/ui: pve: change 'realm list' api call to GET >>> api types: RemoteType: put default port info to the type >>> server: connection: add probe_tls_connection helper >>> server/ui: pve api: extend 'scan' so it can probe the tls connection >>> pdm-client: add scan_remote and probe_tls methods >>> ui: remotes: node url list: add placeholder and clear trigger >>> ui: rmeotes: node url list: make column header clearer >>> ui: remotes: node url list: handle changing default >>> ui: pve wizard: rename 'realm' variable to 'info' >>> ui: pve wizard: summary: add default text for fingerprint >>> ui: pve wizard: nodes: improve info text >>> ui: pve wizard: nodes: probe hosts to verify fingerprint settings >>> ui: pve wizard: info: use pdm_client for scanning >>> ui: pve wizard: info: detect hostname and fingerprint >>> ui: pve wizard: info: remove manual scan button >>> ui: widget: add pve realm selector >>> ui: pve wizard: info: use pve realm selector >>> ui: pve wizard: connect: factor out normalize_hostname >>> ui: pve wizard: connect: move connection logic to next button >>> ui: pve wizard: connect: use scan api endpoint instead of realms >>> ui: pve wizard: connect: add certificate confirmation dialog >>> >>> lib/pdm-api-types/Cargo.toml | 1 + >>> lib/pdm-api-types/src/lib.rs | 2 + >>> lib/pdm-api-types/src/remotes.rs | 16 ++ >>> lib/pdm-client/src/lib.rs | 45 ++++ >>> server/src/api/pve/mod.rs | 62 ++++-- >>> server/src/connection.rs | 87 +++++++- >>> ui/Cargo.toml | 1 + >>> ui/src/remotes/add_wizard.rs | 8 +- >>> ui/src/remotes/node_url_list.rs | 33 ++- >>> ui/src/remotes/wizard_page_connect.rs | 308 ++++++++++++++++---------- >>> ui/src/remotes/wizard_page_info.rs | 127 ++++++----- >>> ui/src/remotes/wizard_page_nodes.rs | 241 +++++++++++++++++++- >>> ui/src/remotes/wizard_page_summary.rs | 5 +- >>> ui/src/widget/mod.rs | 3 + >>> ui/src/widget/pve_realm_selector.rs | 125 +++++++++++ >>> 15 files changed, 853 insertions(+), 211 deletions(-) >>> create mode 100644 ui/src/widget/pve_realm_selector.rs >> _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel