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 A26601FF141 for ; Mon, 13 Apr 2026 13:45:13 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8F85820D6F; Mon, 13 Apr 2026 13:46:01 +0200 (CEST) Message-ID: Date: Mon, 13 Apr 2026 13:45:26 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup v2 19/27] ui: define and expose encryption key management menu item and windows To: Thomas Lamprecht References: <20260411085154.1961287-1-t.lamprecht@proxmox.com> <20260411085154.1961287-8-t.lamprecht@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20260411085154.1961287-8-t.lamprecht@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776080652858 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.068 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. [key.data] Message-ID-Hash: LP3SNKAK574XNBRAGO26Q6VXILMX4MUK X-Message-ID-Hash: LP3SNKAK574XNBRAGO26Q6VXILMX4MUK X-MailFrom: c.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pbs-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 4/11/26 10:50 AM, Thomas Lamprecht wrote: > Am 10.04.26 um 18:54 schrieb Christian Ebner: >> diff --git a/www/config/EncryptionKeysView.js b/www/config/EncryptionKeysView.js >> @@ -0,0 +1,324 @@ >> >> + archiveEncryptionKey: function () { >> + ... >> + if (selection[0].data.type === 'tape') { >> + Ext.Msg.alert(gettext('Error'), gettext('cannot archive tape key')); > > Missing `return;` here - the code falls through to the API2Request below even > after showing the error alert for tape keys. The enableFn on the button should > prevent reaching this in practice, but it is still wrong. fixed for the next version of the patches, thanks. > >> + } > >> + reload: async function () { >> + let [syncKeys, tapeKeys] = await Promise.all([syncKeysFuture, tapeKeysFuture]); > > Might profit from wrapping this in a try/catch - if the user lacks permissions > for one of the two endpoints the entire panel dies with an unhandled promise > rejection instead of showing partial results. True, will wrap these as suggested. > > nit: Type column with width 50 px might be rather narrow for icon + label, but > did not checked out the result yet, so YMMV. will double check. > nit: the synthetic tape key IDs are not super nice/telling on their own. The > tape keys have a hint field that would be more meaningful to display (in > addition?). Currently the hint field is already being displayed as column in the grid, so this should already cover your idea? Just to give a bit more context here: I refrained from using the hint field to be part of the generate id, since this is not as restrictive, allowing e.g. white-spaces, not allowed otherwise. Also, the hint is not unique. The fingerprint should in theory be unique, however it is to long as valid id field value, even if encoded differently (e.g. some form of baseXX encoding). Also, that makes it even worse for handling. Since the likelihood of creating the same key at the same second should be almost 0, a combination of time and fingerprint seemed a good fit. Currently the id for tape keys are only used for display and nowhere enforced, so a migration to a different derivation method should still be possible also in the future if config files should be merged. > >> diff --git a/www/window/EncryptionKeysEdit.js b/www/window/EncryptionKeysEdit.js >> @@ -0,0 +1,383 @@ >> >> + if (key.data === undefined) { >> + return 'Does not seems like a valid Proxmox Backup key!'; > > nit: missing gettect for both validator strings, also: s/seems/seem/ acked, will adapt. >> + { >> + xtype: 'displayfield', >> + name: 'crypt-key-fp', >> + fieldLabel: gettext('Key Source'), >> + padding: '2 0', >> + }, > > Is this set anywhere? I could not find a matching field name in the API > response nor this getting manually set. In fact, `rg -i 'crypt.?key.?fp'` shows > only this lone instance here. No, this should only be a simple text label for the radio-fields below, will adapt.