From 520017bdece9798f07ac4cc0d276dff1ac718486 Mon Sep 17 00:00:00 2001 From: Gleb Chesnokov Date: Wed, 14 Oct 2026 20:18:24 +0300 Subject: [PATCH] scstadmin: Omit dynamic attribute values from update messages Dynamic attributes can contain CHAP secrets. Administrative update, duplicate, removal and debug messages currently print their values, exposing them in captured command output. Report the operation and attribute name without the value in both the CLI and Perl library. Keep metadata-driven discovery and explicit privileged queries unchanged. Add a Ginkgo case that checks add, duplicate, remove, missing-value and debug output through the real scstadmin. Query the newly created attribute explicitly to confirm that the synthetic secret was stored before checking that the update output omits it. Link: https://github.com/SCST-project/scst/issues/91 --- scstadmin/scstadmin.sysfs/man1/scstadmin.1 | 5 ++-- .../scst-1.0.0/lib/SCST/SCST.pm | 8 +++--- scstadmin/scstadmin.sysfs/scstadmin | 24 +++++++--------- tests/ginkgo/iscsi/credentials_test.go | 28 +++++++++++++++++++ 4 files changed, 45 insertions(+), 20 deletions(-) diff --git a/scstadmin/scstadmin.sysfs/man1/scstadmin.1 b/scstadmin/scstadmin.sysfs/man1/scstadmin.1 index 9defbdf8a..663d8afe1 100644 --- a/scstadmin/scstadmin.sysfs/man1/scstadmin.1 +++ b/scstadmin/scstadmin.sysfs/man1/scstadmin.1 @@ -82,8 +82,9 @@ requires root access and a live SCST sysfs hierarchy. Save the current configuration to the specified file. The file may contain authentication secrets and is created or restricted to mode 0600 before writing. An existing output must be a regular file owned by -the effective user; symbolic links are rejected. Saved configurations still -contain the original authentication secrets. +the effective user; symbolic links are rejected. Dynamic attribute values +are omitted from update and debug messages, but explicit attribute queries +and saved configurations still contain their original values. .TP .B -clear_config Remove all configured devices, targets that do not correspond to a physical diff --git a/scstadmin/scstadmin.sysfs/scst-1.0.0/lib/SCST/SCST.pm b/scstadmin/scstadmin.sysfs/scst-1.0.0/lib/SCST/SCST.pm index af4738ddf..d8dc75345 100644 --- a/scstadmin/scstadmin.sysfs/scst-1.0.0/lib/SCST/SCST.pm +++ b/scstadmin/scstadmin.sysfs/scst-1.0.0/lib/SCST/SCST.pm @@ -1016,7 +1016,7 @@ sub addDriverDynamicAttribute { my $io = new IO::File $path, O_WRONLY; if ($io) { if ($self->{'debug'}) { - print "DBG($$): $path -> $cmd\n"; + print "DBG($$): $path -> add_attribute $attribute [redacted]\n"; } else { $bytes = _syswrite($io, $cmd, length($cmd)); } @@ -1054,7 +1054,7 @@ sub removeDriverDynamicAttribute { my $io = new IO::File $path, O_WRONLY; if ($io) { if ($self->{'debug'}) { - print "DBG($$): $path -> $cmd\n"; + print "DBG($$): $path -> del_attribute $attribute [redacted]\n"; } else { $bytes = _syswrite($io, $cmd, length($cmd)); } @@ -1260,7 +1260,7 @@ sub addTargetDynamicAttribute { my $io = new IO::File $path, O_WRONLY; if ($io) { if ($self->{'debug'}) { - print "DBG($$): $path -> $cmd\n"; + print "DBG($$): $path -> add_target_attribute $target $attribute [redacted]\n"; } else { $bytes = _syswrite($io, $cmd, length($cmd)); } @@ -1303,7 +1303,7 @@ sub removeTargetDynamicAttribute { my $io = new IO::File $path, O_WRONLY; if ($io) { if ($self->{'debug'}) { - print "DBG($$): $path -> $cmd\n"; + print "DBG($$): $path -> del_target_attribute $target $attribute [redacted]\n"; } else { $bytes = _syswrite($io, $cmd, length($cmd)); } diff --git a/scstadmin/scstadmin.sysfs/scstadmin b/scstadmin/scstadmin.sysfs/scstadmin index 85e32b052..1106db933 100755 --- a/scstadmin/scstadmin.sysfs/scstadmin +++ b/scstadmin/scstadmin.sysfs/scstadmin @@ -4553,7 +4553,7 @@ sub setDriverAttributes { removeDriverDynamicAttribute($driver, $attribute, $value); $changes++; } else { - print "\t-> Driver dynamic attribute '$attribute' with value '$value' ". + print "\t-> Driver dynamic attribute '$attribute' ". "is not in configuration. Use -force to remove it.\n"; } } @@ -4575,8 +4575,7 @@ sub addDriverDynamicAttributes { foreach my $attribute (keys %{$attributes}) { if (defined($$existing{$attribute}->{$$attributes{$attribute}})) { - my $value = $$attributes{$attribute}; - print "\t-> Attribute/value '$attribute/$value' already exists for driver '$driver'.\n"; + print "\t-> Value for dynamic attribute '$attribute' already exists for driver '$driver'.\n"; next; } @@ -4591,7 +4590,7 @@ sub addDriverDynamicAttribute { my $attribute = shift; my $value = shift; - print "\t-> Adding driver attribute '$attribute' with value '$value' for driver '$driver': "; + print "\t-> Adding driver attribute '$attribute' for driver '$driver': "; my $rc = $SCST->addDriverDynamicAttribute($driver, $attribute, $value); print "done.\n"; @@ -4612,8 +4611,7 @@ sub removeDriverDynamicAttributes { foreach my $attribute (keys %{$attributes}) { if (!defined($$existing{$attribute}->{$$attributes{$attribute}})) { - my $value = $$attributes{$attribute}; - print "\t-> Attribute/value '$attribute/$value' doesn't exist for driver '$driver'.\n"; + print "\t-> Value for dynamic attribute '$attribute' doesn't exist for driver '$driver'.\n"; next; } @@ -4630,7 +4628,7 @@ sub removeDriverDynamicAttribute { return TRUE if (!$SCST->driverIsVirtualCapable($driver)); - print "\t-> Removing dynamic attribute '$attribute' with value '$value' for driver '$driver': "; + print "\t-> Removing dynamic attribute '$attribute' for driver '$driver': "; my $rc = $SCST->removeDriverDynamicAttribute($driver, $attribute, $value); print "done.\n"; @@ -4731,7 +4729,7 @@ sub setTargetAttributes { removeTargetDynamicAttribute($driver, $target, $attribute, $value); $changes++; } else { - print "\t-> Target dynamic attribute '$attribute' with value '$value' ". + print "\t-> Target dynamic attribute '$attribute' ". "is not in configuration. Use -force to remove it.\n"; } } @@ -4754,8 +4752,7 @@ sub addTargetDynamicAttributes { foreach my $attribute (keys %{$attributes}) { if (defined($$existing{$attribute}->{$$attributes{$attribute}})) { - my $value = $$attributes{$attribute}; - print "\t-> Attribute/value '$attribute/$value' already exists for driver/target ". + print "\t-> Value for dynamic attribute '$attribute' already exists for driver/target ". "'$driver/$target'.\n"; next; } @@ -4772,7 +4769,7 @@ sub addTargetDynamicAttribute { my $attribute = shift; my $value = shift; - print "\t-> Adding target attribute '$attribute' with value '$value' ". + print "\t-> Adding target attribute '$attribute' ". "for driver/target '$driver/$target': "; my $rc = $SCST->addTargetDynamicAttribute($driver, $target, $attribute, $value); print "done.\n"; @@ -4795,8 +4792,7 @@ sub removeTargetDynamicAttributes { foreach my $attribute (keys %{$attributes}) { if (!defined($$existing{$attribute}->{$$attributes{$attribute}})) { - my $value = $$attributes{$attribute}; - print "\t-> Attribute/value '$attribute/$value' doesn't exist for driver/target ". + print "\t-> Value for dynamic attribute '$attribute' doesn't exist for driver/target ". "'$driver/$target'.\n"; next; } @@ -4815,7 +4811,7 @@ sub removeTargetDynamicAttribute { return TRUE if (!$SCST->driverIsVirtualCapable($driver)); - print "\t-> Removing dynamic attribute '$attribute' with value '$value' for driver/target '$driver/$target': "; + print "\t-> Removing dynamic attribute '$attribute' for driver/target '$driver/$target': "; my $rc = $SCST->removeTargetDynamicAttribute($driver, $target, $attribute, $value); print "done.\n"; diff --git a/tests/ginkgo/iscsi/credentials_test.go b/tests/ginkgo/iscsi/credentials_test.go index 923cf1b8e..f29b834fd 100644 --- a/tests/ginkgo/iscsi/credentials_test.go +++ b/tests/ginkgo/iscsi/credentials_test.go @@ -99,6 +99,34 @@ var _ = Describe("CHAP credential confidentiality", Label("basic", "iscsi", "cha Entry("existing private file", os.FileMode(0600)), ) + It("does not print secrets during dynamic attribute updates or debug output", func(ctx SpecContext) { + f := newFixture(ctx) + secret, r := addTestCHAPUser(ctx, f) + // Inspect only the management operation's output, not argv in the + // command journal or the explicit privileged attribute query. + Expect(r.Stdout + "\n" + r.Stderr).NotTo(ContainSubstring(secret)) + for _, operation := range []struct { + name string + debug bool + }{ + {"-add_tgt_attr", false}, // Duplicate value. + {"-rem_tgt_attr", true}, // Debug must not remove the value. + {"-rem_tgt_attr", false}, + {"-rem_tgt_attr", false}, // Missing value. + {"-add_tgt_attr", true}, + } { + args := []string{operation.name, f.Target, "-driver", "iscsi", "-attributes", "IncomingUser=tester " + secret} + if operation.debug { + args = append(args, "-debug") + } + r, err := cli.Execute(ctx, args...) + Expect(err).NotTo(HaveOccurred(), r.String()) + Expect(r.ExitCode).To(BeZero(), r.String()) + Expect(r.ReportedError()).To(BeFalse(), r.String()) + Expect(r.Stdout + "\n" + r.Stderr).NotTo(ContainSubstring(secret)) + } + }) + It("rejects a symlink output without changing its referent", Label("config"), func(ctx SpecContext) { f := newFixture(ctx) dir := filepath.Join(artifactDir, f.Device+"-output-link")