From 21acb5cdd6fa629078cb5760b63e1b75c2b81c26 Mon Sep 17 00:00:00 2001 From: tegwick Date: Mon, 7 Sep 2026 23:22:55 +0200 Subject: [PATCH] Make snapshot attribute validation enforce a real rule Closes gap G06. checkNoUnknownAttributes was named for a rule it did not implement: it rejected blank keys, which is not an allow-list, so a snapshot carrying malformed or shadowing attribute names passed a check whose name said otherwise. A rule that passes for the wrong reason is worse than an absent one, because the report is read as evidence. raw_attributes_well_formed requires each key to be a valid LDAP attribute descriptor, forbids shadowing an attribute the canonical model owns (uid, cn, mail) in any casing, and rejects keys differing only by case, which LDAP treats as one attribute. spec/ldap-schema.yaml carries the same wording. Two corrections to the gap's description rather than implementations of it. An allow-list is not derivable: ldapAttributes is defined as what the canonical model does not cover, so the schema cannot enumerate what may appear there. And the reference constraint already existed -- checkValidGroupMemberships only checks emptiness, but the semantic rule checkReferencedUsersExist resolves every member against the user set. Neutering the rule fails four of the five new tests. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NV9oijZukGyGbRQGGKnK4P Assistant: claude-code Assistant-Model: opus Assistant-Process: 713576@bnt-lap001 Assistant-Session: 384c511d-9bce-4cb8-a676-2aef6c0c8df6 --- SCOPE.md | 6 +- ...26-09-05-011726-scope-intent-assessment.md | 20 ++++ spec/ldap-schema.yaml | 9 +- src/internal/validator/validator.go | 103 ++++++++++++++++-- src/internal/validator/validator_test.go | 89 ++++++++++++--- ...Y-WP-0021-snapshot-attribute-validation.md | 97 +++++++++++++++++ 6 files changed, 298 insertions(+), 26 deletions(-) create mode 100644 workplans/KEY-WP-0021-snapshot-attribute-validation.md diff --git a/SCOPE.md b/SCOPE.md index 737ee21..03f184e 100644 --- a/SCOPE.md +++ b/SCOPE.md @@ -56,7 +56,11 @@ Keycloak interchangeability are not established. attribute. The output is a reviewed artifact, not a proven migration; that proof needs a live provider swap and is not established. - Snapshot validation is a limited Go rule set, not full machine-readable schema - enforcement. The canonical YAML model and discovery metadata now match the + enforcement. Raw LDAP attribute keys are checked for descriptor validity, + canonical-mapping shadowing and case-only duplicates (KEY-WP-0021); attribute + values are not validated against a directory schema, and no allow-list of + permitted attribute names exists, since that field carries what the canonical + model does not name. The canonical YAML model and discovery metadata now match the runtime client-registration surface and are held there by a conformance check (KEY-WP-0017); the Go model is the runtime authority and the YAML the reviewed contract. That is narrower than schema enforcement in general. diff --git a/history/2026-09-05-011726-scope-intent-assessment.md b/history/2026-09-05-011726-scope-intent-assessment.md index 5c63948..39103bf 100644 --- a/history/2026-09-05-011726-scope-intent-assessment.md +++ b/history/2026-09-05-011726-scope-intent-assessment.md @@ -263,6 +263,26 @@ identity existence. It does not derive its checks from the YAML schema. the normative contract to the actual checks; add invalid-snapshot cases that prove each stated rule. SCOPE now calls this limited snapshot validation. +**Status 2026-09-07 (KEY-WP-0021): closed.** The placeholder is replaced by +`raw_attributes_well_formed`, which requires each key to be a valid LDAP +attribute descriptor (RFC 4512 descr or numeric OID), forbids shadowing a +mapping the canonical model owns (uid, cn, mail) in any casing, and rejects two +keys differing only by case. `spec/ldap-schema.yaml` carries the same wording, so +the rule's name no longer overpromises. Invalid-snapshot cases cover every +constraint plus accepting legitimate raw attributes; neutering the rule fails +four of them. + +Two corrections to this gap's own description. First, an allow-list of permitted +attribute names is not derivable: `ldapAttributes` is defined as the attributes +the canonical model does *not* cover, so the schema cannot enumerate what may +appear there — the constraints above are what is actually checkable. Second, the +reference constraint was already implemented: `checkValidGroupMemberships` does +only check emptiness, but the semantic rule `checkReferencedUsersExist` resolves +every member against the user set and fails on an unknown one. The rule existed +in a different function than the one this assessment examined. + +Still not claimed: validation of attribute *values* against a directory schema. + ### G07 — Optional tenant-role support is not wired into the server **Priority: medium. Kind: integration gap.** diff --git a/spec/ldap-schema.yaml b/spec/ldap-schema.yaml index 56e1543..41cc357 100644 --- a/spec/ldap-schema.yaml +++ b/spec/ldap-schema.yaml @@ -76,8 +76,13 @@ validation_rules: description: "All DNs must conform to the base_dn and OU layout above." - name: required_attributes_present description: "Every entry must carry all required attributes for its OU." - - name: no_unknown_attributes - description: "No attributes outside the allowed set may appear." + - name: raw_attributes_well_formed + description: > + Raw ldapAttributes keys must be valid LDAP attribute descriptors, must + not shadow an attribute the canonical model owns (uid, cn, mail), and + must not repeat one attribute under different casing. This is not an + allow-list: ldapAttributes carries what the canonical model does not + name, so no permitted-name set can be derived from the schema. - name: valid_group_memberships description: "All member values must be non-empty valid DNs." semantic: diff --git a/src/internal/validator/validator.go b/src/internal/validator/validator.go index 268248e..0d41105 100644 --- a/src/internal/validator/validator.go +++ b/src/internal/validator/validator.go @@ -34,7 +34,7 @@ func runStructural(snap Snapshot) []RuleResult { return []RuleResult{ checkValidDNStructure(snap), checkRequiredAttributesPresent(snap), - checkNoUnknownAttributes(snap), + checkRawAttributesWellFormed(snap), checkValidGroupMemberships(snap), } } @@ -91,22 +91,109 @@ func checkRequiredAttributesPresent(snap Snapshot) RuleResult { return r } -// checkNoUnknownAttributes is a placeholder for attribute allow-list enforcement. -// In v0.1 with the canonical Go model all fields are known by type; this rule -// checks that no LDAPAttributes keys are empty strings. -func checkNoUnknownAttributes(snap Snapshot) RuleResult { - r := RuleResult{Rule: "no_unknown_attributes", Passed: true} +// canonicalLDAPMappings are the directory attributes the canonical model already +// owns: User.username maps to uid, User.displayName to cn, User.email to mail, +// and Group.name to cn. A raw attribute restating one of these creates a second +// source for a value the model defines, which is how an export and the directory +// it came from drift apart. +var canonicalLDAPMappings = map[string]string{ + "uid": "username", + "cn": "displayName", + "mail": "email", +} + +// checkRawAttributesWellFormed validates the raw LDAP attributes a snapshot +// carries alongside the canonical fields (KEY-WP-0021). +// +// `ldapAttributes` is defined as the attributes the canonical model does *not* +// cover, so there is no schema-derived allow-list of permitted names to check +// against — the field exists to carry exactly what the schema does not name. +// What can be enforced is that each name is writable to a directory, does not +// shadow a mapping the model owns, and is not the same attribute twice. +func checkRawAttributesWellFormed(snap Snapshot) RuleResult { + r := RuleResult{Rule: "raw_attributes_well_formed", Passed: true} for _, u := range snap.Users { - for k := range u.LDAPAttributes { - if strings.TrimSpace(k) == "" { + // LDAP attribute descriptors are case-insensitive, so keys differing + // only by case are one attribute supplied twice, not two attributes. + seen := make(map[string]string, len(u.LDAPAttributes)) + for key := range u.LDAPAttributes { + switch { + case strings.TrimSpace(key) == "": r.Passed = false r.Message = appendMsg(r.Message, fmt.Sprintf("user %q has blank LDAP attribute key", u.ID)) + continue + case !validAttributeDescriptor(key): + r.Passed = false + r.Message = appendMsg(r.Message, fmt.Sprintf( + "user %q has LDAP attribute %q that is not a valid attribute descriptor", u.ID, key)) + continue } + folded := strings.ToLower(key) + if field, canonical := canonicalLDAPMappings[folded]; canonical { + r.Passed = false + r.Message = appendMsg(r.Message, fmt.Sprintf( + "user %q carries LDAP attribute %q as a raw attribute; it is owned by the canonical %s field", + u.ID, key, field)) + continue + } + if first, duplicate := seen[folded]; duplicate { + r.Passed = false + r.Message = appendMsg(r.Message, fmt.Sprintf( + "user %q has LDAP attributes %q and %q that differ only by case", u.ID, first, key)) + continue + } + seen[folded] = key } } return r } +// validAttributeDescriptor reports whether name is an LDAP attribute descriptor +// per RFC 4512: either a numeric OID, or a letter followed by letters, digits +// and hyphens. Options after a semicolon (e.g. "description;lang-de") are not +// accepted, since a canonical snapshot carries values, not transfer encodings. +func validAttributeDescriptor(name string) bool { + if name == "" { + return false + } + if name[0] >= '0' && name[0] <= '9' { + return validNumericOID(name) + } + for i := 0; i < len(name); i++ { + c := name[i] + switch { + case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z': + case c >= '0' && c <= '9', c == '-': + if i == 0 { + return false + } + default: + return false + } + } + return true +} + +// validNumericOID reports whether name is a dotted-decimal OID with at least two +// arcs and no empty or leading-zero arc. +func validNumericOID(name string) bool { + arcs := strings.Split(name, ".") + if len(arcs) < 2 { + return false + } + for _, arc := range arcs { + if arc == "" || (len(arc) > 1 && arc[0] == '0') { + return false + } + for i := 0; i < len(arc); i++ { + if arc[i] < '0' || arc[i] > '9' { + return false + } + } + } + return true +} + // checkValidGroupMemberships verifies that every member ID listed in a group // is non-empty. func checkValidGroupMemberships(snap Snapshot) RuleResult { diff --git a/src/internal/validator/validator_test.go b/src/internal/validator/validator_test.go index 34426c1..afb55dd 100644 --- a/src/internal/validator/validator_test.go +++ b/src/internal/validator/validator_test.go @@ -1,6 +1,7 @@ package validator_test import ( + "strings" "testing" "keycape/internal/domain" @@ -105,27 +106,85 @@ func TestRequiredAttributesPresent_MissingDisplayName(t *testing.T) { } } -// --- structural: no_unknown_attributes --- +// --- structural: raw_attributes_well_formed --- -func TestNoUnknownAttributes_Pass(t *testing.T) { +// attributeRule runs the snapshot and returns the raw-attribute rule result. +func attributeRule(t *testing.T, attributes map[string]string) validator.RuleResult { + t.Helper() u := makeUser("u1", "alice", "Alice", "alice@example.com") - u.LDAPAttributes = map[string]string{"sn": "Example"} - snap := validator.Snapshot{Users: []domain.User{u}} - r := validator.Validate(snap, validator.ModeCI) - result := findRule(r.Structural, "no_unknown_attributes") - if !result.Passed { - t.Errorf("expected pass, got: %s", result.Message) + u.LDAPAttributes = attributes + r := validator.Validate(validator.Snapshot{Users: []domain.User{u}}, validator.ModeCI) + return findRule(r.Structural, "raw_attributes_well_formed") +} + +// The rule must be selective, not merely strict: legitimate raw attributes are +// exactly what this field exists to carry. +func TestRawAttributes_AcceptsLegitimateAttributes(t *testing.T) { + for name, attributes := range map[string]map[string]string{ + "simple descriptor": {"sn": "Example"}, + "hyphenated and digits": {"employee-id2": "42"}, + "numeric oid": {"1.3.6.1.4.1.1466.115.121.1.15": "value"}, + "distinct attributes": {"sn": "Example", "givenName": "Alice"}, + "no attributes": nil, + } { + t.Run(name, func(t *testing.T) { + if result := attributeRule(t, attributes); !result.Passed { + t.Errorf("expected pass, got: %s", result.Message) + } + }) } } -func TestNoUnknownAttributes_BlankKey(t *testing.T) { - u := makeUser("u1", "alice", "Alice", "alice@example.com") - u.LDAPAttributes = map[string]string{"": "value"} - snap := validator.Snapshot{Users: []domain.User{u}} - r := validator.Validate(snap, validator.ModeCI) - result := findRule(r.Structural, "no_unknown_attributes") +func TestRawAttributes_RejectsMalformedDescriptors(t *testing.T) { + for name, key := range map[string]string{ + "blank": "", + "whitespace only": " ", + "leading digit": "2fa", + "leading hyphen": "-sn", + "underscore": "employee_id", + "space inside": "given Name", + "transfer option": "description;lang-de", + "single arc oid": "1", + "empty oid arc": "1..3", + "leading zero oid": "1.03", + "non numeric oid arc": "1.3.x", + } { + t.Run(name, func(t *testing.T) { + if result := attributeRule(t, map[string]string{key: "value"}); result.Passed { + t.Errorf("expected fail for attribute key %q", key) + } + }) + } +} + +// A raw attribute must not restate a value the canonical model owns, in any +// casing, since LDAP attribute descriptors are case-insensitive. +func TestRawAttributes_RejectsCanonicalMappings(t *testing.T) { + for _, key := range []string{"uid", "cn", "mail", "UID", "Mail", "cN"} { + t.Run(key, func(t *testing.T) { + if result := attributeRule(t, map[string]string{key: "value"}); result.Passed { + t.Errorf("expected fail for canonical mapping %q", key) + } + }) + } +} + +func TestRawAttributes_RejectsCaseOnlyDuplicates(t *testing.T) { + result := attributeRule(t, map[string]string{"givenName": "Alice", "givenname": "Alice"}) if result.Passed { - t.Error("expected fail for blank attribute key") + t.Error("expected fail for attributes differing only by case") + } +} + +// The rule reports the offending user and attribute: a report saying only that +// something failed cannot be acted on. +func TestRawAttributes_MessageNamesUserAndAttribute(t *testing.T) { + result := attributeRule(t, map[string]string{"employee_id": "42"}) + if result.Passed { + t.Fatal("expected fail") + } + if !strings.Contains(result.Message, "u1") || !strings.Contains(result.Message, "employee_id") { + t.Errorf("message does not identify the problem: %s", result.Message) } } diff --git a/workplans/KEY-WP-0021-snapshot-attribute-validation.md b/workplans/KEY-WP-0021-snapshot-attribute-validation.md new file mode 100644 index 0000000..2f93ffe --- /dev/null +++ b/workplans/KEY-WP-0021-snapshot-attribute-validation.md @@ -0,0 +1,97 @@ +--- +id: KEY-WP-0021 +type: workplan +title: "Make snapshot attribute validation enforce a real rule" +domain: infotech +repo: key-cape +status: finished +owner: claude +topic_slug: snapshot-attribute-validation +created: "2026-09-07" +updated: "2026-09-07" +--- + +Closes gap G06 of `history/2026-09-05-011726-scope-intent-assessment.md`: +`checkNoUnknownAttributes` is named for a rule it does not implement. It rejects +blank keys, which is not an allow-list, so a snapshot carrying arbitrary or +malformed LDAP attribute names passes a check whose name says otherwise. A rule +that passes for the wrong reason is worse than an absent one, because a report +listing `no_unknown_attributes: passed` is read as evidence. + +One part of the assessment's description needs correcting rather than +implementing: it says group membership validation checks nonempty member IDs +rather than complete referenced identity existence. `checkValidGroupMemberships` +does only check emptiness, but the semantic rule `checkReferencedUsersExist` +already resolves every member against the user set and fails on an unknown one. +The reference constraint exists; it lives in a different rule than the one the +assessment looked at. + +## Enforce real attribute constraints + +```task +id: KEY-WP-0021-T01 +status: done +priority: medium +``` + +`ldapAttributes` is defined by the canonical model as raw attributes *not covered +by* the model, so an allow-list of permitted names cannot be derived from the +schema — the field exists precisely to carry what the schema does not name. Two +constraints are derivable and worth enforcing: + +- the attribute name must be a syntactically valid LDAP attribute descriptor + (RFC 4512: a letter followed by letters, digits or hyphens, or a numeric OID), + since a name that cannot be written to a directory cannot round-trip; and +- it must not shadow a canonical field's documented LDAP mapping (`uid`, `cn`, + `mail`), because two sources for one value is how an export and a directory + come to disagree, and the model already owns those. + +Attribute descriptors are case-insensitive in LDAP, so keys differing only by +case are a collision, not two attributes. Keep the blank-key check. Rename the +rule to describe what it actually enforces rather than leaving a name that +overpromises. + +Implemented as `raw_attributes_well_formed`, replacing `no_unknown_attributes` +in both `validator.go` and `spec/ldap-schema.yaml`, with the schema entry stating +plainly that this is not an allow-list and why one cannot be derived. + +## Prove each rule with an invalid snapshot + +```task +id: KEY-WP-0021-T02 +status: done +priority: medium +``` + +The assessment asks for invalid-snapshot cases proving each stated rule. Add one +per constraint — malformed descriptor, shadowed canonical mapping, case-only +duplicate, blank key — plus passing cases for legitimate raw attributes and a +valid numeric OID, so the rule is shown to be selective rather than merely +strict. + +Eleven malformed-descriptor cases, six canonical-mapping cases across casings, a +case-only duplicate, a message-content case, and five accepted-attribute cases. +Confirmed they test the rule rather than merely passing: making the check return +early fails four of the five tests. + +## Reconcile the records + +```task +id: KEY-WP-0021-T03 +status: done +priority: medium +``` + +Update `SCOPE.md` and G06's status. State what the rule now enforces and, just as +importantly, what it still does not: this is not schema-derived validation of +every attribute against a directory schema, and the canonical model cannot +provide one for a field defined as everything the model does not cover. Correct +the assessment's reference-constraint premise rather than restating it. + +`history/…-scope-intent-assessment.md` was being edited by a concurrent session +when this workplan was written; make this edit only against a clean tree. + +Done against a clean tree after the other session committed. G06's status records +both corrections: no allow-list is derivable for a field defined as what the +model does not cover, and the reference constraint was already implemented in +`checkReferencedUsersExist` rather than missing.