Discard placeholder DMI serial numbers

pkg/pillar/hardware/model.go

@@ -172,19 +172,187 @@
 	return strings.TrimSuffix(getOverride(log, softSerialFile), "\n")
 }
 
-func GetProductSerial(log *base.LogObject) string {
-	serial, err := base.Exec(log, "dmidecode", "-s", "system-serial-number").Output()
+// dmiPlaceholders holds values firmware reports for a field it never
+// programmed. They appear as literal filler ("To be filled by O.E.M."), as an
+// echo of the field's own name ("System Serial Number"), or as a counting
+// pattern ("0123456789"). Every unit of an affected model reports the same
+// string, so accepting one as a serial number makes distinct devices
+// indistinguishable to a controller. Keys are lower-case; lookups trim and
+// fold case first.
+//
+// The set is the union of the placeholder lists these projects apply to
+// serial-number and asset-tag fields. Paths and symbol names rather than line
+// numbers, so the references survive upstream edits:
+//
+//	glpi-project/glpi			src/Blacklist.php (getDefaults)
+//	saltstack/salt				salt/grains/core.py (_clean_value)
+//	saltstack/salt				salt/modules/smbios.py (_dmi_isclean)
+//	lpereira/hardinfo			hardinfo/dmi_util.c (ignore_placeholder_strings)
+//	reactos/reactos				dll/cpl/sysdm/smbios.c (IsGenericSystemName)
+//	memtest86plus/memtest86plus		system/smbios.c (dmi_string_is_junk)
+//	osquery/osquery				osquery/core/system.cpp (kPlaceholderHardwareUUIDList)
+//	freebsd/freebsd-src			libexec/rc/rc.d/hostid (valid_hostid)
+//	linuxhw/hw-probe			hw-probe.pl (emptyVal)
+//	openshift/assisted-installer-agent	src/scanners/machine_uuid_scanner.go
+//	sergelogvinov/proxmox-csi-plugin	pkg/utils/node/smbios.go
+//
+// "Not Present" and "Not Settable" are dmidecode's own renderings of an all-FF
+// and an all-zero field respectively (mirror/dmidecode dmidecode.c), so they
+// reach a caller as literal text.
+var dmiPlaceholders = map[string]bool{
+	"default":                  true,
+	"default string":           true,
+	"system serial number":     true,
+	"chassis serial number":    true,
+	"base board serial number": true,
+	"systemserialnumb":         true,
+	"oem_serial":               true,
+	"not specified":            true,
+	"not available":            true,
+	"not applicable":           true,
+	"not defined":              true,
+	"not present":              true,
+	"not settable":             true,
+	"none":                     true,
+	"(none)":                   true,
+	"n/a":                      true,
+	"na":                       true,
+	"null":                     true,
+	"(null string)":            true,
+	"no string":                true,
+	"empty":                    true,
+	"unknown":                  true,
+	"unknow":                   true,
+	"uknown":                   true,
+	"undefined":                true,
+	"invalid":                  true,
+	"inva":                     true,
+	"out of spec":              true,
+	"reserved":                 true,
+	"eval":                     true,
+	"0123456789":               true,
+	"123456789":                true,
+	"1234567890":               true,
+	"sys-1234567890":           true,
+	"mb-1234567890":            true,
+	"asset-1234567890":         true,
+	"sn-12345":                 true,
+	"nnnnnnn":                  true,
+}
+
+// isRepeatedRune reports whether s is one rune repeated, such as "0000000000"
+// or "xxxx". The empty string is not repetition, and reporting false for it
+// keeps the first-rune access below in range for every input.
+func isRepeatedRune(s string) bool {
+	if s == "" {
+		return false
+	}
+	runes := []rune(s)
+	for _, r := range runes[1:] {
+		if r != runes[0] {
+			return false
+		}
+	}
+	return true
+}
+
+// isDMIPlaceholder reports whether s carries no device-specific information:
+// an empty string, a known placeholder, an unprogrammed-field marker beginning
+// "to be filled", or a single repeated rune. dmidecode itself prints
+// "Not Present" and "Not Settable" for an all-FF and all-zero SMBIOS UUID, and
+// those spellings reach this path too.
+func isDMIPlaceholder(s string) bool {
+	s = strings.TrimSpace(s)
+	if s == "" {
+		return true
+	}
+	lower := strings.ToLower(s)
+	if dmiPlaceholders[lower] || strings.HasPrefix(lower, "to be filled") {
+		return true
+	}
+	return isRepeatedRune(s)
+}
+
+// serialSource names one origin of a device serial number and reads it.
+type serialSource struct {
+	name string
+	read func(*base.LogObject) string
+}
+
+// dmidecodeString returns the value dmidecode reports for keyword, or "" when
+// dmidecode fails or the platform has no SMBIOS.
+func dmidecodeString(log *base.LogObject, keyword string) string {
+	out, err := base.Exec(log, "dmidecode", "-s", keyword).Output()
 	if err != nil {
-		log.Errorf("GetProductSerial system-serial-number failed %s\n",
-			err)
-		serial = []byte{}
+		log.Errorf("GetProductSerial %s failed %s\n", keyword, err)
+		return ""
 	}
-	strserial := strings.TrimSuffix(string(serial), "\n")
-	if strserial != "" && strserial != "Not Specified" {
-		return strserial
-	} else {
-		return getCPUSerial(log)
+	return string(out)
+}
+
+// productSerialSources are consulted in order. The SMBIOS system serial comes
+// first because it is the value a vendor prints on the chassis label and quotes
+// on an invoice, so it is the one an operator can pre-register. The baseboard
+// serial follows: a board sold to an integrator often carries the only
+// programmed serial on the unit, and some vendors write the label value there
+// rather than into the system field. The CPU or SoC serial is last, being the
+// only source on ARM platforms and normally absent on x86.
+var productSerialSources = []serialSource{
+	{
+		name: "system-serial-number",
+		read: func(log *base.LogObject) string {
+			return dmidecodeString(log, "system-serial-number")
+		},
+	},
+	{
+		name: "baseboard-serial-number",
+		read: func(log *base.LogObject) string {
+			return dmidecodeString(log, "baseboard-serial-number")
+		},
+	},
+	{
+		name: "cpu-serial",
+		read: getCPUSerial,
+	},
+}
+
+// firstUsableSerial returns the first source value that is not a placeholder,
+// together with the name of the source it came from, and logs which source was
+// used so an operator can tell where a reported serial originated. Both results
+// are "" when no source carries a device-specific value.
+func firstUsableSerial(log *base.LogObject, sources []serialSource) (serial, source string) {
+	for i, src := range sources {
+		value := strings.TrimSpace(src.read(log))
+		if isDMIPlaceholder(value) {
+			if value != "" {
+				log.Warnf("GetProductSerial: ignoring placeholder %s %q\n",
+					src.name, value)
+			}
+			continue
+		}
+		if i == 0 {
+			log.Functionf("GetProductSerial: using %s\n", src.name)
+		} else {
+			log.Noticef("GetProductSerial: falling back to %s\n", src.name)
+		}
+		return value, src.name
 	}
+	log.Warnf("GetProductSerial: no source carries a device-specific serial\n")
+	return "", ""
+}
+
+// GetProductSerial returns the device's hardware serial number, taking the
+// first of the SMBIOS system serial, the SMBIOS baseboard serial and the
+// CPU/SoC serial that is not a firmware placeholder. The result is "" when none
+// of them carries a device-specific value; a caller must treat "" as "unknown"
+// and never as an identifier.
+//
+// Because the baseboard serial is not disclosed to a purchaser by any vendor,
+// a value sourced from it can identify a device to a controller but cannot be
+// pre-registered from the paperwork. The source is logged for that reason.
+func GetProductSerial(log *base.LogObject) string {
+	serial, _ := firstUsableSerial(log, productSerialSources)
+	return serial
 }
 
 // Returns productManufacturer, productName, productVersion, productSerial, productUuid

pkg/pillar/hardware/model_test.go

@@ -6,6 +6,7 @@
 import (
 	"testing"
 
+	"github.com/lf-edge/eve/pkg/pillar/base"
 	"github.com/sirupsen/logrus"
 )
 
@@ -21,3 +22,130 @@
 	}
 	logrus.Infof("TestCompatible: DONE\n")
 }
+
+// TestIsDMIPlaceholder pins the split between serials that identify a device
+// and values every unit of a model reports identically. The "real" cases are
+// serials observed on actual hardware, including a Supermicro system, baseboard
+// and chassis serial from one machine and an HPE serial recovered from a Type 1
+// UUID; none may ever be discarded. The "placeholder" cases are the values
+// firmware ships for an unprogrammed field.
+func TestIsDMIPlaceholder(t *testing.T) {
+	deviceSerials := []string{
+		"S279678X8335734",
+		"WM179S000284",
+		"CE101AG41A10040",
+		"ZM143S051601",
+		"ZM16AS024713",
+		"CM144S013179",
+		"MXQ93102WL",
+		"0123456789A",
+		"AB",
+	}
+	for _, s := range deviceSerials {
+		if isDMIPlaceholder(s) {
+			t.Errorf("isDMIPlaceholder(%q) = true, want false", s)
+		}
+	}
+
+	placeholders := []string{
+		"",
+		"   ",
+		"To be filled by O.E.M.",
+		"To Be Filled By O.E.M.",
+		"TO BE FILLED BY O.E.M",
+		"Default string",
+		"Not Specified",
+		"Not Present",
+		"Not Settable",
+		"System Serial Number",
+		"Chassis Serial Number",
+		"SystemSerialNumb",
+		"OEM_Serial",
+		"None",
+		"N/A",
+		"Unknow",
+		"INVALID",
+		"0123456789",
+		"1234567890",
+		"SYS-1234567890",
+		"0000000000",
+		"1111",
+		"-",
+		"0",
+		"xxxxxxxxxxx",
+		"  0123456789  ",
+		"\t0000000000\n",
+	}
+	for _, s := range placeholders {
+		if !isDMIPlaceholder(s) {
+			t.Errorf("isDMIPlaceholder(%q) = false, want true", s)
+		}
+	}
+}
+
+// TestIsRepeatedRuneEmpty pins the contract that isDMIPlaceholder relies on but
+// cannot reach: the empty string is not repetition. Returning true here would
+// be harmless today only because the caller short-circuits first.
+func TestIsRepeatedRuneEmpty(t *testing.T) {
+	if isRepeatedRune("") {
+		t.Error(`isRepeatedRune("") = true, want false`)
+	}
+}
+
+// TestFirstUsableSerial exercises the source ordering: the first source whose
+// value is not a placeholder wins, placeholders are skipped rather than
+// returned, and an exhausted chain yields "" so a caller cannot mistake filler
+// for an identifier. Source names are reported so an operator can tell whether
+// a serial came from the chassis label field or from the board.
+func TestFirstUsableSerial(t *testing.T) {
+	log := base.NewSourceLogObject(logrus.StandardLogger(), t.Name(), 0)
+
+	src := func(name, value string) serialSource {
+		return serialSource{name: name, read: func(*base.LogObject) string { return value }}
+	}
+
+	tests := []struct {
+		name       string
+		sources    []serialSource
+		wantSerial string
+		wantSource string
+	}{{
+		name:       "system serial wins when real",
+		sources:    []serialSource{src("system", "S279678X8335734"), src("board", "WM179S000284")},
+		wantSerial: "S279678X8335734",
+		wantSource: "system",
+	}, {
+		name:       "placeholder system falls through to board",
+		sources:    []serialSource{src("system", "0123456789"), src("board", "WM179S000284")},
+		wantSerial: "WM179S000284",
+		wantSource: "board",
+	}, {
+		name:       "empty system falls through to board",
+		sources:    []serialSource{src("system", ""), src("board", "WM179S000284")},
+		wantSerial: "WM179S000284",
+		wantSource: "board",
+	}, {
+		name:       "skips two placeholders to reach the third source",
+		sources:    []serialSource{src("system", "To be filled by O.E.M."), src("board", "0000000000"), src("cpu", "abc123")},
+		wantSerial: "abc123",
+		wantSource: "cpu",
+	}, {
+		name:       "all placeholders yields no serial",
+		sources:    []serialSource{src("system", "0123456789"), src("board", "Not Specified"), src("cpu", "")},
+		wantSerial: "",
+		wantSource: "",
+	}, {
+		name:       "surrounding whitespace is trimmed",
+		sources:    []serialSource{src("system", "  WM179S000284\r\n")},
+		wantSerial: "WM179S000284",
+		wantSource: "system",
+	}}
+
+	for _, tc := range tests {
+		serial, source := firstUsableSerial(log, tc.sources)
+		if serial != tc.wantSerial || source != tc.wantSource {
+			t.Errorf("%s: got (%q, %q), want (%q, %q)",
+				tc.name, serial, source, tc.wantSerial, tc.wantSource)
+		}
+	}
+}