From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <t.lamprecht@proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id F10B28ABBC for <pve-devel@lists.proxmox.com>; Fri, 21 Oct 2022 13:30:14 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D190C217B7 for <pve-devel@lists.proxmox.com>; Fri, 21 Oct 2022 13:29:44 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for <pve-devel@lists.proxmox.com>; Fri, 21 Oct 2022 13:29:43 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 08A1B44B0E for <pve-devel@lists.proxmox.com>; Fri, 21 Oct 2022 13:29:43 +0200 (CEST) Message-ID: <04a8f643-cb3f-daf8-480b-240726cec255@proxmox.com> Date: Fri, 21 Oct 2022 13:29:42 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:106.0) Gecko/20100101 Thunderbird/106.0 Content-Language: en-GB To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>, Dominik Csapak <d.csapak@proxmox.com> References: <20221021083117.1239396-1-d.csapak@proxmox.com> <20221021083117.1239396-2-d.csapak@proxmox.com> From: Thomas Lamprecht <t.lamprecht@proxmox.com> Cc: Wolfgang Bumiller <w.bumiller@proxmox.com> In-Reply-To: <20221021083117.1239396-2-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.034 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH access-control v2 1/3] authenticate_2nd_new: only lock tfa config for recovery keys X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> X-List-Received-Date: Fri, 21 Oct 2022 11:30:15 -0000 Can we *please* get sane commit subject for *human* consumption?! The worst one, that really triggers me: > authenticate_user: pass undef instead of empty $tfa_challenge to authenticate_2nd_new Instead of a high level human overview it gives basically another almost machine readable diff of the lower level changes. So there's close to zero of useful higher level and/or _new_ info in there, but sure makes one stop to parse on seeing this in some log. Copying the method name (nor file module name!) is basically never a good idea, especially as tag and especially for (admin) user facing changes - it can naturally be OK for library stuff (i.e., dev-only facing changes), but even then seldom as start `<tag>:`... following would allow for a quicker to read, and (granted, slightly subjective) better high level understanding of the commits, thus better categorizing for when skimming through the log, e.g., in search of relevant changes due to some new/questionable/.. behavior; besides that it makes d/changelog writing easier. * tfa: only lock config for recovery keys on authentication * tfa: rename outdated $otp variable to $tfa_response (this is internal and won't ever make it in the d/changelog so only dev readability matters) * auth: drop passing bogus challenge variable when checking first factor (also internally, dev oriented) This should be also verified and either amened or commented on by the maintainer (planning) pushing a commit/series - while its obviously 1) hard and 2) a lot overhead to have a very strict rule book that's fine for a partially subjective thing like this, it should be that hard to detect the obvious cases, at least for user facing changes. May look like a small thing to "rant" on, but I spend a lot of time in git log et al. and copy pasted method/file names without simple concise tagging and for-human info can make it much harder with no benefit for anyone. Naturally applies to all devs/maintainers not just those in To/Cc here.