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 BAEAF1FF16B for ; Tue, 29 Jul 2025 09:27:10 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C8C3D8C19; Tue, 29 Jul 2025 09:28:32 +0200 (CEST) Message-ID: Date: Tue, 29 Jul 2025 09:27:57 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox VE development discussion , Gabriel Goller References: <20250724141730.468243-1-g.goller@proxmox.com> <20250724141730.468243-2-g.goller@proxmox.com> Content-Language: en-US From: Thomas Lamprecht In-Reply-To: <20250724141730.468243-2-g.goller@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1753774068475 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.033 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. [rfc-editor.org, sdn.pm] Subject: Re: [pve-devel] [PATCH network v2 1/5] sdn: add global lock for configuration X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Am 24.07.25 um 16:18 schrieb Gabriel Goller: > From: Stefan Hanreich > > Add a new cluster-wide lock for SDN that prevents any changes to the > configuration if the generated lock-secret is not provided. It works > by generating and storing a secret in sdn/.lock which gets checked by > lock_sdn_config on every invocation. If the lock file exists, then the > lock secret has to be supplied in order to make changes to the SDN > configuration. > > Lock using the domain lock (`PVE::Cluster::cfs_lock_domain`) and "sdn" > string. > > This is mainly a preparation for PDM, where PDM can take the lock and > prevent concurrent modifications to the SDN configuration from other > sources, even across multiple API calls. > > Co-authored-by: Gabriel Goller > Signed-off-by: Stefan Hanreich > --- > src/PVE/Network/SDN.pm | 53 ++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 49 insertions(+), 4 deletions(-) > > diff --git a/src/PVE/Network/SDN.pm b/src/PVE/Network/SDN.pm > index 0e7d1dfd239c..efee21543387 100644 > --- a/src/PVE/Network/SDN.pm > +++ b/src/PVE/Network/SDN.pm > @@ -13,7 +13,7 @@ use PVE::Cluster qw(cfs_read_file cfs_write_file cfs_lock_file); > use PVE::INotify; > use PVE::RESTEnvironment qw(log_warn); > use PVE::RPCEnvironment; > -use PVE::Tools qw(extract_param dir_glob_regex run_command); > +use PVE::Tools qw(file_get_contents file_set_contents extract_param dir_glob_regex run_command); > > use PVE::Network::SDN::Vnets; > use PVE::Network::SDN::Zones; > @@ -48,6 +48,8 @@ my $write_running_cfg = sub { > > PVE::Cluster::cfs_register_file($running_cfg, $parse_running_cfg, $write_running_cfg); > > +my $LOCK_SECRET_FILE = "/etc/pve/sdn/.lock"; > + > # improve me : move status code inside plugins ? > > sub ifquery_check { > @@ -197,14 +199,57 @@ sub commit_config { > cfs_write_file($running_cfg, $cfg); > } > > +sub generate_lock_secret { nit: might be better to avoid the "secret" terminology here? As this is not really a secret but rather something like a token, handle or maybe even cookie. I.e., this hasn't to stay secret, as it does not provide access on it's own, it's just for ensuring orderly locking by identifying the locker. I'm mostly mentioning this as such method and variable names tend to leak into docs and other communications, and especially secrets are a bit delicate topic, for me that's the biggest reason why it would be better to avoid the term here. Could be fixed up though, if you agree with changing this and have an opinion on what variant (handle, token, cookie, ...?) would be best. > + my $min = ord('!'); # first printable ascii > + > + my $rand_bytes = Crypt::OpenSSL::Random::random_bytes(32); > + die "failed to generate lock secret!\n" if !$rand_bytes; > + > + my $str = join('', map { chr((ord($_) & 0x3F) + $min) } split('', $rand_bytes)); hmm, might have overlooked when checking the v1, but would it be a better option to decode the $rand_bytes as base64? That keeps the full entropy and ensures we got an easy to handle character-set. Another option might be to use a UUIDv7 [0], as that version includes the milliseconds since the unix expoch in the first 48 bits, that would also give some hints for when the handle was created, that info could be even used for expiring a lock handle. [0]: https://www.rfc-editor.org/rfc/rfc9562.html#name-uuid-version-7 As the users of this should not really expect any specific format, we could still change that after applying though, so just tell me what you think/prefer. > + return $str; > +} > + > +sub create_global_lock { > + my $secret = generate_lock_secret(); > + PVE::Tools::file_set_contents($LOCK_SECRET_FILE, $secret); > + return $secret; > +} > + > +sub delete_global_lock { > + unlink $LOCK_SECRET_FILE if -e $LOCK_SECRET_FILE; > +} > + > sub lock_sdn_config { > - my ($code, $errmsg) = @_; > + my ($code, $errmsg, $lock_secret_user) = @_; > > - cfs_lock_file($running_cfg, undef, $code); > + my $lock_wrapper = sub { > + my $lock_secret = undef; > + if (-e $LOCK_SECRET_FILE) { > + $lock_secret = PVE::Tools::file_get_contents($LOCK_SECRET_FILE); > + } > + > + if ( > + defined($lock_secret) > + && (!defined($lock_secret_user) || $lock_secret ne $lock_secret_user) > + ) { > + die "invalid lock secret provided!"; > + } > + > + return $code->(); > + }; > > - if (my $err = $@) { > + return lock_sdn_domain($lock_wrapper, $errmsg); > +} > + > +sub lock_sdn_domain { > + my ($code, $errmsg) = @_; > + > + my $res = PVE::Cluster::cfs_lock_domain("sdn", undef, $code); > + my $err = $@; > + if ($err) { > $errmsg ? die "$errmsg: $err" : die $err; > } > + return $res; > } > > sub get_local_vnets { _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel