Am 24.07.25 um 16:18 schrieb Gabriel Goller:
> From: Stefan Hanreich <s.hanre...@proxmox.com>
>
> 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 <g.gol...@proxmox.com>
> Signed-off-by: Stefan Hanreich <s.hanre...@proxmox.com>
> ---
> 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
