Make snapshot attribute validation enforce a real rule
All checks were successful
Build and Publish Container Image / build-and-push (push) Successful in 40s
All checks were successful
Build and Publish Container Image / build-and-push (push) Successful in 40s
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 <noreply@anthropic.com> 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
This commit is contained in:
parent
f366df814a
commit
21acb5cdd6
6 changed files with 298 additions and 26 deletions
6
SCOPE.md
6
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.
|
||||
|
|
|
|||
|
|
@ -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.**
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
97
workplans/KEY-WP-0021-snapshot-attribute-validation.md
Normal file
97
workplans/KEY-WP-0021-snapshot-attribute-validation.md
Normal file
|
|
@ -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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue