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.