From 3ab32bcb21486a1eb7d4769a4bf17c9cffd12948 Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 10 Jun 2026 10:35:26 +0200 Subject: [PATCH 1/4] feat: redact sensitive northbound values and persist sensitive paths Implement the sensitive-data PRD: key-pruned path markers unioned across intents for merged views, RenderOpts-driven `***` redaction in JSON/XML/proto/ XPath output, BlameConfig and deviation masking, admin bypass via include_sensitive, and atomic TransactionSet persistence of sensitive_paths with a datastore reverse index for O(1) lookup. Co-authored-by: Cursor --- mocks/mocktreeentry/entry.go | 484 +----------------- pkg/datastore/datastore_rpc.go | 61 ++- pkg/datastore/deviations.go | 15 +- pkg/datastore/deviations_test.go | 183 +++++++ pkg/datastore/intent_rpc.go | 22 +- pkg/datastore/sensitive_path_union_test.go | 476 +++++++++++++++++ pkg/datastore/sync.go | 2 +- pkg/datastore/transaction_rpc.go | 82 +-- pkg/datastore/transaction_rpc_test.go | 175 ++++++- pkg/datastore/types/transaction_intent.go | 12 + pkg/server/datastore.go | 2 +- pkg/server/intent.go | 2 +- pkg/tree/api/adapter/entryoutputadapter.go | 17 +- pkg/tree/api/adapter/intentresponse.go | 14 +- pkg/tree/api/leaf_variants.go | 27 +- pkg/tree/entry_test.go | 2 +- .../importer/xml/xml_tree_importer_test.go | 4 +- pkg/tree/ops/getdeviations.go | 4 +- pkg/tree/ops/getdeviations_test.go | 233 +++++++++ pkg/tree/ops/json.go | 29 +- pkg/tree/ops/json_test.go | 91 +++- pkg/tree/ops/proto.go | 16 +- pkg/tree/ops/render_opts.go | 38 ++ pkg/tree/ops/sensitive_test.go | 98 ++++ pkg/tree/ops/xml.go | 65 ++- pkg/tree/ops/xml_test.go | 90 +++- pkg/tree/ops/xpath.go | 11 +- pkg/tree/ops/xpath_test.go | 6 +- pkg/tree/processors/processor_blame_config.go | 34 +- .../processors/processor_blame_config_test.go | 93 ++++ pkg/tree/types/sensitive_path_index.go | 135 +++++ pkg/tree/types/sensitive_path_index_test.go | 101 ++++ pkg/utils/testhelper/utils.go | 38 ++ 33 files changed, 2048 insertions(+), 614 deletions(-) create mode 100644 pkg/datastore/deviations_test.go create mode 100644 pkg/datastore/sensitive_path_union_test.go create mode 100644 pkg/tree/ops/getdeviations_test.go create mode 100644 pkg/tree/ops/render_opts.go create mode 100644 pkg/tree/ops/sensitive_test.go create mode 100644 pkg/tree/types/sensitive_path_index.go create mode 100644 pkg/tree/types/sensitive_path_index_test.go diff --git a/mocks/mocktreeentry/entry.go b/mocks/mocktreeentry/entry.go index c3637e2b..25ef91db 100644 --- a/mocks/mocktreeentry/entry.go +++ b/mocks/mocktreeentry/entry.go @@ -43,48 +43,19 @@ func (m *MockEntry) EXPECT() *MockEntryMockRecorder { return m.recorder } -// AddChild mocks base method. -func (m *MockEntry) AddChild(arg0 context.Context, arg1 api.Entry) error { +// AddOrGetChild mocks base method. +func (m *MockEntry) AddOrGetChild(arg0 context.Context, arg1 api.Entry) (api.Entry, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AddChild", arg0, arg1) - ret0, _ := ret[0].(error) - return ret0 -} - -// AddChild indicates an expected call of AddChild. -func (mr *MockEntryMockRecorder) AddChild(arg0, arg1 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddChild", reflect.TypeOf((*MockEntry)(nil).AddChild), arg0, arg1) -} - -// AddUpdateRecursive mocks base method. -func (m *MockEntry) AddUpdateRecursive(ctx context.Context, relativePath *sdcpb.Path, u *types.Update, flags *types.UpdateInsertFlags) (api.Entry, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AddUpdateRecursive", ctx, relativePath, u, flags) - ret0, _ := ret[0].(api.Entry) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// AddUpdateRecursive indicates an expected call of AddUpdateRecursive. -func (mr *MockEntryMockRecorder) AddUpdateRecursive(ctx, relativePath, u, flags any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddUpdateRecursive", reflect.TypeOf((*MockEntry)(nil).AddUpdateRecursive), ctx, relativePath, u, flags) -} - -// AddUpdateRecursiveInternal mocks base method. -func (m *MockEntry) AddUpdateRecursiveInternal(ctx context.Context, path *sdcpb.Path, idx int, u *types.Update, flags *types.UpdateInsertFlags) (api.Entry, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AddUpdateRecursiveInternal", ctx, path, idx, u, flags) + ret := m.ctrl.Call(m, "AddOrGetChild", arg0, arg1) ret0, _ := ret[0].(api.Entry) ret1, _ := ret[1].(error) return ret0, ret1 } -// AddUpdateRecursiveInternal indicates an expected call of AddUpdateRecursiveInternal. -func (mr *MockEntryMockRecorder) AddUpdateRecursiveInternal(ctx, path, idx, u, flags any) *gomock.Call { +// AddOrGetChild indicates an expected call of AddOrGetChild. +func (mr *MockEntryMockRecorder) AddOrGetChild(arg0, arg1 any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddUpdateRecursiveInternal", reflect.TypeOf((*MockEntry)(nil).AddUpdateRecursiveInternal), ctx, path, idx, u, flags) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddOrGetChild", reflect.TypeOf((*MockEntry)(nil).AddOrGetChild), arg0, arg1) } // CanDelete mocks base method. @@ -115,6 +86,20 @@ func (mr *MockEntryMockRecorder) CanDeleteBranch(keepDefault any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CanDeleteBranch", reflect.TypeOf((*MockEntry)(nil).CanDeleteBranch), keepDefault) } +// ChoicesResolvers mocks base method. +func (m *MockEntry) ChoicesResolvers() api.ChoiceResolvers { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ChoicesResolvers") + ret0, _ := ret[0].(api.ChoiceResolvers) + return ret0 +} + +// ChoicesResolvers indicates an expected call of ChoicesResolvers. +func (mr *MockEntryMockRecorder) ChoicesResolvers() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ChoicesResolvers", reflect.TypeOf((*MockEntry)(nil).ChoicesResolvers)) +} + // DeepCopy mocks base method. func (m *MockEntry) DeepCopy(tc api.TreeContext, parent api.Entry) (api.Entry, error) { m.ctrl.T.Helper() @@ -130,20 +115,6 @@ func (mr *MockEntryMockRecorder) DeepCopy(tc, parent any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeepCopy", reflect.TypeOf((*MockEntry)(nil).DeepCopy), tc, parent) } -// DeleteBranch mocks base method. -func (m *MockEntry) DeleteBranch(ctx context.Context, path *sdcpb.Path, owner string) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteBranch", ctx, path, owner) - ret0, _ := ret[0].(error) - return ret0 -} - -// DeleteBranch indicates an expected call of DeleteBranch. -func (mr *MockEntryMockRecorder) DeleteBranch(ctx, path, owner any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteBranch", reflect.TypeOf((*MockEntry)(nil).DeleteBranch), ctx, path, owner) -} - // DeleteCanDeleteChilds mocks base method. func (m *MockEntry) DeleteCanDeleteChilds(keepDefault bool) { m.ctrl.T.Helper() @@ -156,21 +127,6 @@ func (mr *MockEntryMockRecorder) DeleteCanDeleteChilds(keepDefault any) *gomock. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteCanDeleteChilds", reflect.TypeOf((*MockEntry)(nil).DeleteCanDeleteChilds), keepDefault) } -// FilterChilds mocks base method. -func (m *MockEntry) FilterChilds(keys map[string]string) ([]api.Entry, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "FilterChilds", keys) - ret0, _ := ret[0].([]api.Entry) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// FilterChilds indicates an expected call of FilterChilds. -func (mr *MockEntryMockRecorder) FilterChilds(keys any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FilterChilds", reflect.TypeOf((*MockEntry)(nil).FilterChilds), keys) -} - // FinishInsertionPhase mocks base method. func (m *MockEntry) FinishInsertionPhase(ctx context.Context) error { m.ctrl.T.Helper() @@ -185,21 +141,6 @@ func (mr *MockEntryMockRecorder) FinishInsertionPhase(ctx any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FinishInsertionPhase", reflect.TypeOf((*MockEntry)(nil).FinishInsertionPhase), ctx) } -// GetChild mocks base method. -func (m *MockEntry) GetChild(name string) (api.Entry, bool) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetChild", name) - ret0, _ := ret[0].(api.Entry) - ret1, _ := ret[1].(bool) - return ret0, ret1 -} - -// GetChild indicates an expected call of GetChild. -func (mr *MockEntryMockRecorder) GetChild(name any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChild", reflect.TypeOf((*MockEntry)(nil).GetChild), name) -} - // GetChildMap mocks base method. func (m *MockEntry) GetChildMap() *api.ChildMap { m.ctrl.T.Helper() @@ -228,64 +169,6 @@ func (mr *MockEntryMockRecorder) GetChilds(arg0 any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChilds", reflect.TypeOf((*MockEntry)(nil).GetChilds), arg0) } -// GetDeletes mocks base method. -func (m *MockEntry) GetDeletes(entries types.DeleteEntriesList, aggregatePaths bool) (types.DeleteEntriesList, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetDeletes", entries, aggregatePaths) - ret0, _ := ret[0].(types.DeleteEntriesList) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetDeletes indicates an expected call of GetDeletes. -func (mr *MockEntryMockRecorder) GetDeletes(entries, aggregatePaths any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDeletes", reflect.TypeOf((*MockEntry)(nil).GetDeletes), entries, aggregatePaths) -} - -// GetHighestPrecedence mocks base method. -func (m *MockEntry) GetHighestPrecedence(result api.LeafVariantSlice, onlyNewOrUpdated, includeDefaults, includeExplicitDelete bool) api.LeafVariantSlice { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetHighestPrecedence", result, onlyNewOrUpdated, includeDefaults, includeExplicitDelete) - ret0, _ := ret[0].(api.LeafVariantSlice) - return ret0 -} - -// GetHighestPrecedence indicates an expected call of GetHighestPrecedence. -func (mr *MockEntryMockRecorder) GetHighestPrecedence(result, onlyNewOrUpdated, includeDefaults, includeExplicitDelete any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetHighestPrecedence", reflect.TypeOf((*MockEntry)(nil).GetHighestPrecedence), result, onlyNewOrUpdated, includeDefaults, includeExplicitDelete) -} - -// GetHighestPrecedenceLeafValue mocks base method. -func (m *MockEntry) GetHighestPrecedenceLeafValue(arg0 context.Context) (*api.LeafEntry, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetHighestPrecedenceLeafValue", arg0) - ret0, _ := ret[0].(*api.LeafEntry) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetHighestPrecedenceLeafValue indicates an expected call of GetHighestPrecedenceLeafValue. -func (mr *MockEntryMockRecorder) GetHighestPrecedenceLeafValue(arg0 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetHighestPrecedenceLeafValue", reflect.TypeOf((*MockEntry)(nil).GetHighestPrecedenceLeafValue), arg0) -} - -// GetHighestPrecedenceValueOfBranch mocks base method. -func (m *MockEntry) GetHighestPrecedenceValueOfBranch(filter api.HighestPrecedenceFilter) int32 { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetHighestPrecedenceValueOfBranch", filter) - ret0, _ := ret[0].(int32) - return ret0 -} - -// GetHighestPrecedenceValueOfBranch indicates an expected call of GetHighestPrecedenceValueOfBranch. -func (mr *MockEntryMockRecorder) GetHighestPrecedenceValueOfBranch(filter any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetHighestPrecedenceValueOfBranch", reflect.TypeOf((*MockEntry)(nil).GetHighestPrecedenceValueOfBranch), filter) -} - // GetLeafVariants mocks base method. func (m *MockEntry) GetLeafVariants() *api.LeafVariants { m.ctrl.T.Helper() @@ -314,36 +197,6 @@ func (mr *MockEntryMockRecorder) GetLevel() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLevel", reflect.TypeOf((*MockEntry)(nil).GetLevel)) } -// GetListChilds mocks base method. -func (m *MockEntry) GetListChilds() ([]api.Entry, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetListChilds") - ret0, _ := ret[0].([]api.Entry) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetListChilds indicates an expected call of GetListChilds. -func (mr *MockEntryMockRecorder) GetListChilds() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetListChilds", reflect.TypeOf((*MockEntry)(nil).GetListChilds)) -} - -// GetOrCreateChilds mocks base method. -func (m *MockEntry) GetOrCreateChilds(ctx context.Context, path *sdcpb.Path) (api.Entry, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetOrCreateChilds", ctx, path) - ret0, _ := ret[0].(api.Entry) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetOrCreateChilds indicates an expected call of GetOrCreateChilds. -func (mr *MockEntryMockRecorder) GetOrCreateChilds(ctx, path any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetOrCreateChilds", reflect.TypeOf((*MockEntry)(nil).GetOrCreateChilds), ctx, path) -} - // GetParent mocks base method. func (m *MockEntry) GetParent() api.Entry { m.ctrl.T.Helper() @@ -358,20 +211,6 @@ func (mr *MockEntryMockRecorder) GetParent() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetParent", reflect.TypeOf((*MockEntry)(nil).GetParent)) } -// GetRootBasedEntryChain mocks base method. -func (m *MockEntry) GetRootBasedEntryChain() []api.Entry { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetRootBasedEntryChain") - ret0, _ := ret[0].([]api.Entry) - return ret0 -} - -// GetRootBasedEntryChain indicates an expected call of GetRootBasedEntryChain. -func (mr *MockEntryMockRecorder) GetRootBasedEntryChain() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRootBasedEntryChain", reflect.TypeOf((*MockEntry)(nil).GetRootBasedEntryChain)) -} - // GetSchema mocks base method. func (m *MockEntry) GetSchema() *sdcpb.SchemaElem { m.ctrl.T.Helper() @@ -386,20 +225,6 @@ func (mr *MockEntryMockRecorder) GetSchema() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetSchema", reflect.TypeOf((*MockEntry)(nil).GetSchema)) } -// GetSchemaKeys mocks base method. -func (m *MockEntry) GetSchemaKeys() []string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetSchemaKeys") - ret0, _ := ret[0].([]string) - return ret0 -} - -// GetSchemaKeys indicates an expected call of GetSchemaKeys. -func (mr *MockEntryMockRecorder) GetSchemaKeys() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetSchemaKeys", reflect.TypeOf((*MockEntry)(nil).GetSchemaKeys)) -} - // GetTreeContext mocks base method. func (m *MockEntry) GetTreeContext() api.TreeContext { m.ctrl.T.Helper() @@ -414,20 +239,6 @@ func (mr *MockEntryMockRecorder) GetTreeContext() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTreeContext", reflect.TypeOf((*MockEntry)(nil).GetTreeContext)) } -// HoldsLeafvariants mocks base method. -func (m *MockEntry) HoldsLeafvariants() bool { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "HoldsLeafvariants") - ret0, _ := ret[0].(bool) - return ret0 -} - -// HoldsLeafvariants indicates an expected call of HoldsLeafvariants. -func (mr *MockEntryMockRecorder) HoldsLeafvariants() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HoldsLeafvariants", reflect.TypeOf((*MockEntry)(nil).HoldsLeafvariants)) -} - // IsRoot mocks base method. func (m *MockEntry) IsRoot() bool { m.ctrl.T.Helper() @@ -442,21 +253,6 @@ func (mr *MockEntryMockRecorder) IsRoot() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsRoot", reflect.TypeOf((*MockEntry)(nil).IsRoot)) } -// NavigateSdcpbPath mocks base method. -func (m *MockEntry) NavigateSdcpbPath(ctx context.Context, path *sdcpb.Path) (api.Entry, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "NavigateSdcpbPath", ctx, path) - ret0, _ := ret[0].(api.Entry) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// NavigateSdcpbPath indicates an expected call of NavigateSdcpbPath. -func (mr *MockEntryMockRecorder) NavigateSdcpbPath(ctx, path any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NavigateSdcpbPath", reflect.TypeOf((*MockEntry)(nil).NavigateSdcpbPath), ctx, path) -} - // PathName mocks base method. func (m *MockEntry) PathName() string { m.ctrl.T.Helper() @@ -526,243 +322,3 @@ func (mr *MockEntryMockRecorder) StringIndent(result any) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "StringIndent", reflect.TypeOf((*MockEntry)(nil).StringIndent), result) } - -// MockLeafVariantEntry is a mock of LeafVariantEntry interface. -type MockLeafVariantEntry struct { - ctrl *gomock.Controller - recorder *MockLeafVariantEntryMockRecorder - isgomock struct{} -} - -// MockLeafVariantEntryMockRecorder is the mock recorder for MockLeafVariantEntry. -type MockLeafVariantEntryMockRecorder struct { - mock *MockLeafVariantEntry -} - -// NewMockLeafVariantEntry creates a new mock instance. -func NewMockLeafVariantEntry(ctrl *gomock.Controller) *MockLeafVariantEntry { - mock := &MockLeafVariantEntry{ctrl: ctrl} - mock.recorder = &MockLeafVariantEntryMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockLeafVariantEntry) EXPECT() *MockLeafVariantEntryMockRecorder { - return m.recorder -} - -// GetEntry mocks base method. -func (m *MockLeafVariantEntry) GetEntry() api.Entry { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetEntry") - ret0, _ := ret[0].(api.Entry) - return ret0 -} - -// GetEntry indicates an expected call of GetEntry. -func (mr *MockLeafVariantEntryMockRecorder) GetEntry() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetEntry", reflect.TypeOf((*MockLeafVariantEntry)(nil).GetEntry)) -} - -// MarkDelete mocks base method. -func (m *MockLeafVariantEntry) MarkDelete(onlyIntended bool) *api.LeafEntry { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "MarkDelete", onlyIntended) - ret0, _ := ret[0].(*api.LeafEntry) - return ret0 -} - -// MarkDelete indicates an expected call of MarkDelete. -func (mr *MockLeafVariantEntryMockRecorder) MarkDelete(onlyIntended any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MarkDelete", reflect.TypeOf((*MockLeafVariantEntry)(nil).MarkDelete), onlyIntended) -} - -// String mocks base method. -func (m *MockLeafVariantEntry) String() string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "String") - ret0, _ := ret[0].(string) - return ret0 -} - -// String indicates an expected call of String. -func (mr *MockLeafVariantEntryMockRecorder) String() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "String", reflect.TypeOf((*MockLeafVariantEntry)(nil).String)) -} - -// MockLeafVariantEntries is a mock of LeafVariantEntries interface. -type MockLeafVariantEntries struct { - ctrl *gomock.Controller - recorder *MockLeafVariantEntriesMockRecorder - isgomock struct{} -} - -// MockLeafVariantEntriesMockRecorder is the mock recorder for MockLeafVariantEntries. -type MockLeafVariantEntriesMockRecorder struct { - mock *MockLeafVariantEntries -} - -// NewMockLeafVariantEntries creates a new mock instance. -func NewMockLeafVariantEntries(ctrl *gomock.Controller) *MockLeafVariantEntries { - mock := &MockLeafVariantEntries{ctrl: ctrl} - mock.recorder = &MockLeafVariantEntriesMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockLeafVariantEntries) EXPECT() *MockLeafVariantEntriesMockRecorder { - return m.recorder -} - -// Add mocks base method. -func (m *MockLeafVariantEntries) Add(l *api.LeafEntry) { - m.ctrl.T.Helper() - m.ctrl.Call(m, "Add", l) -} - -// Add indicates an expected call of Add. -func (mr *MockLeafVariantEntriesMockRecorder) Add(l any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Add", reflect.TypeOf((*MockLeafVariantEntries)(nil).Add), l) -} - -// AddExplicitDeleteEntry mocks base method. -func (m *MockLeafVariantEntries) AddExplicitDeleteEntry(owner string, priority int32) *api.LeafEntry { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AddExplicitDeleteEntry", owner, priority) - ret0, _ := ret[0].(*api.LeafEntry) - return ret0 -} - -// AddExplicitDeleteEntry indicates an expected call of AddExplicitDeleteEntry. -func (mr *MockLeafVariantEntriesMockRecorder) AddExplicitDeleteEntry(owner, priority any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddExplicitDeleteEntry", reflect.TypeOf((*MockLeafVariantEntries)(nil).AddExplicitDeleteEntry), owner, priority) -} - -// AddWithStats mocks base method. -func (m *MockLeafVariantEntries) AddWithStats(l *api.LeafEntry, stats *types.ImportStats) { - m.ctrl.T.Helper() - m.ctrl.Call(m, "AddWithStats", l, stats) -} - -// AddWithStats indicates an expected call of AddWithStats. -func (mr *MockLeafVariantEntriesMockRecorder) AddWithStats(l, stats any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddWithStats", reflect.TypeOf((*MockLeafVariantEntries)(nil).AddWithStats), l, stats) -} - -// DeleteByOwner mocks base method. -func (m *MockLeafVariantEntries) DeleteByOwner(owner string) *api.LeafEntry { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteByOwner", owner) - ret0, _ := ret[0].(*api.LeafEntry) - return ret0 -} - -// DeleteByOwner indicates an expected call of DeleteByOwner. -func (mr *MockLeafVariantEntriesMockRecorder) DeleteByOwner(owner any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteByOwner", reflect.TypeOf((*MockLeafVariantEntries)(nil).DeleteByOwner), owner) -} - -// GetByOwner mocks base method. -func (m *MockLeafVariantEntries) GetByOwner(owner string) *api.LeafEntry { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetByOwner", owner) - ret0, _ := ret[0].(*api.LeafEntry) - return ret0 -} - -// GetByOwner indicates an expected call of GetByOwner. -func (mr *MockLeafVariantEntriesMockRecorder) GetByOwner(owner any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetByOwner", reflect.TypeOf((*MockLeafVariantEntries)(nil).GetByOwner), owner) -} - -// GetHighestPrecedence mocks base method. -func (m *MockLeafVariantEntries) GetHighestPrecedence(onlyNewOrUpdated, includeDefaults, includeExplicitDeletes bool) *api.LeafEntry { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetHighestPrecedence", onlyNewOrUpdated, includeDefaults, includeExplicitDeletes) - ret0, _ := ret[0].(*api.LeafEntry) - return ret0 -} - -// GetHighestPrecedence indicates an expected call of GetHighestPrecedence. -func (mr *MockLeafVariantEntriesMockRecorder) GetHighestPrecedence(onlyNewOrUpdated, includeDefaults, includeExplicitDeletes any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetHighestPrecedence", reflect.TypeOf((*MockLeafVariantEntries)(nil).GetHighestPrecedence), onlyNewOrUpdated, includeDefaults, includeExplicitDeletes) -} - -// GetRunning mocks base method. -func (m *MockLeafVariantEntries) GetRunning() *api.LeafEntry { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetRunning") - ret0, _ := ret[0].(*api.LeafEntry) - return ret0 -} - -// GetRunning indicates an expected call of GetRunning. -func (mr *MockLeafVariantEntriesMockRecorder) GetRunning() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRunning", reflect.TypeOf((*MockLeafVariantEntries)(nil).GetRunning)) -} - -// Length mocks base method. -func (m *MockLeafVariantEntries) Length() int { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Length") - ret0, _ := ret[0].(int) - return ret0 -} - -// Length indicates an expected call of Length. -func (mr *MockLeafVariantEntriesMockRecorder) Length() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Length", reflect.TypeOf((*MockLeafVariantEntries)(nil).Length)) -} - -// MarkOwnerForDeletion mocks base method. -func (m *MockLeafVariantEntries) MarkOwnerForDeletion(owner string, onlyIntended bool) *api.LeafEntry { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "MarkOwnerForDeletion", owner, onlyIntended) - ret0, _ := ret[0].(*api.LeafEntry) - return ret0 -} - -// MarkOwnerForDeletion indicates an expected call of MarkOwnerForDeletion. -func (mr *MockLeafVariantEntriesMockRecorder) MarkOwnerForDeletion(owner, onlyIntended any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MarkOwnerForDeletion", reflect.TypeOf((*MockLeafVariantEntries)(nil).MarkOwnerForDeletion), owner, onlyIntended) -} - -// RemoveDeletedByOwner mocks base method. -func (m *MockLeafVariantEntries) RemoveDeletedByOwner(owner string) *api.LeafEntry { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RemoveDeletedByOwner", owner) - ret0, _ := ret[0].(*api.LeafEntry) - return ret0 -} - -// RemoveDeletedByOwner indicates an expected call of RemoveDeletedByOwner. -func (mr *MockLeafVariantEntriesMockRecorder) RemoveDeletedByOwner(owner any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveDeletedByOwner", reflect.TypeOf((*MockLeafVariantEntries)(nil).RemoveDeletedByOwner), owner) -} - -// ResetFlags mocks base method. -func (m *MockLeafVariantEntries) ResetFlags(deleteFlag, newFlag, updatedFlag bool) int { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ResetFlags", deleteFlag, newFlag, updatedFlag) - ret0, _ := ret[0].(int) - return ret0 -} - -// ResetFlags indicates an expected call of ResetFlags. -func (mr *MockLeafVariantEntriesMockRecorder) ResetFlags(deleteFlag, newFlag, updatedFlag any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResetFlags", reflect.TypeOf((*MockLeafVariantEntries)(nil).ResetFlags), deleteFlag, newFlag, updatedFlag) -} diff --git a/pkg/datastore/datastore_rpc.go b/pkg/datastore/datastore_rpc.go index b3337060..024147dd 100644 --- a/pkg/datastore/datastore_rpc.go +++ b/pkg/datastore/datastore_rpc.go @@ -17,6 +17,7 @@ package datastore import ( "context" "errors" + "fmt" "runtime" "sync" "time" @@ -35,7 +36,10 @@ import ( "github.com/sdcio/data-server/pkg/pool" "github.com/sdcio/data-server/pkg/schema" "github.com/sdcio/data-server/pkg/tree" + "github.com/sdcio/data-server/pkg/tree/ops" "github.com/sdcio/data-server/pkg/tree/processors" + treetypes "github.com/sdcio/data-server/pkg/tree/types" + tree_persist "github.com/sdcio/sdc-protos/tree_persist" ) type Datastore struct { @@ -69,6 +73,10 @@ type Datastore struct { syncTree *tree.RootEntry syncTreeMutex *sync.RWMutex + // sensitivePathIndex is the in-memory reverse index of sensitive paths. + // It is populated at startup and updated on every intent write or delete. + sensitivePathIndex *treetypes.SensitivePathIndex + taskPool *pool.SharedTaskPool } @@ -104,17 +112,18 @@ func New(ctx context.Context, c *config.DatastoreConfig, sc schema.Client, cc ca ccb := cache.NewCacheClientBound(c.Name, cc) ds := &Datastore{ - config: c, - schemaClient: scb, - ctx: ctx, - cfn: cancel, - cacheClient: ccb, - m: &sync.RWMutex{}, - dmutex: &sync.Mutex{}, - deviationClients: make(map[sdcpb.DataServer_WatchDeviationsServer]string), - syncTree: syncTreeRoot, - syncTreeMutex: &sync.RWMutex{}, - taskPool: pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)), + config: c, + schemaClient: scb, + ctx: ctx, + cfn: cancel, + cacheClient: ccb, + m: &sync.RWMutex{}, + dmutex: &sync.Mutex{}, + deviationClients: make(map[sdcpb.DataServer_WatchDeviationsServer]string), + syncTree: syncTreeRoot, + syncTreeMutex: &sync.RWMutex{}, + sensitivePathIndex: treetypes.NewSensitivePathIndex(), + taskPool: pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)), } ds.transactionManager = types.NewTransactionManager(NewDatastoreRollbackAdapter(ds)) @@ -122,6 +131,12 @@ func New(ctx context.Context, c *config.DatastoreConfig, sc schema.Client, cc ca // this is a blocking call ds.initCache(ctx) + // populate the sensitive-path index from all existing intents in cache. + if err := populateSensitivePathIndex(ctx, ds.sensitivePathIndex, ccb); err != nil { + cancel() + return nil, fmt.Errorf("failed to populate sensitive path index: %w", err) + } + go func() { // init sbi, this is a blocking call err := ds.connectSBI(ctx, opts...) @@ -228,7 +243,7 @@ func (d *Datastore) Stop(ctx context.Context) error { return nil } -func (d *Datastore) BlameConfig(ctx context.Context, includeDefaults bool) (*sdcpb.BlameTreeElement, error) { +func (d *Datastore) BlameConfig(ctx context.Context, includeDefaults, exposeSensitive bool) (*sdcpb.BlameTreeElement, error) { log := logger.FromContext(ctx).WithName("BlameConfig") ctx = logger.IntoContext(ctx, log) @@ -239,13 +254,19 @@ func (d *Datastore) BlameConfig(ctx context.Context, includeDefaults bool) (*sdc if err != nil { return nil, err } - // load all intents + // load all intents into the tree for blame; sensitive paths come from the index. _, err = d.LoadAllButRunningIntents(ctx, root) if err != nil { return nil, err } - bcp := processors.NewBlameConfigProcessor(&processors.BlameConfigProcessorParams{IncludeDefaults: includeDefaults}) + bcp := processors.NewBlameConfigProcessor(&processors.BlameConfigProcessorParams{ + IncludeDefaults: includeDefaults, + RenderOpts: ops.RenderOpts{ + IncludeSensitive: exposeSensitive, + SensitivePathSet: d.sensitivePathIndex, + }, + }) bte, err := bcp.Run(ctx, root.Entry, d.taskPool) // set the root level elements name to the target name @@ -272,3 +293,15 @@ func (dra *DatastoreRollbackAdapter) TransactionRollback(ctx context.Context, tr // Assure the types.RollbackInterface is implemented by the DatastoreRollbackAdapter var _ types.RollbackInterface = &DatastoreRollbackAdapter{} + +// populateSensitivePathIndex scans all intents in cache and loads their +// sensitive_paths into s. It is called once during Datastore startup, before +// the first northbound read is served. +func populateSensitivePathIndex(ctx context.Context, s *treetypes.SensitivePathIndex, cc cache.CacheClientBound) error { + return forEachIntent(ctx, cc, nil, func(intent *tree_persist.Intent) error { + if len(intent.GetSensitivePaths()) > 0 { + s.Set(intent.GetIntentName(), intent.GetSensitivePaths()) + } + return nil + }) +} diff --git a/pkg/datastore/deviations.go b/pkg/datastore/deviations.go index 964817ec..17d2f675 100644 --- a/pkg/datastore/deviations.go +++ b/pkg/datastore/deviations.go @@ -138,11 +138,16 @@ func (d DeviationsStats) LogValue() map[string]map[string]int { return stats } +// SendDeviations reads deviation entries from ch and forwards them to all +// active deviation clients. Sensitivity redaction is applied upstream in the +// ops layer (GetDeviations with IncludeSensitive=false), so no masking is done +// here. func (d *Datastore) SendDeviations(ctx context.Context, ch <-chan *treetypes.DeviationEntry, deviationClients map[string]sdcpb.DataServer_WatchDeviationsServer) { log := logf.FromContext(ctx) deviationsStats := make(DeviationsStats) for deviation := range ch { deviationsStats.Add(deviation) + for clientIdentifier, dc := range deviationClients { if dc.Context().Err() != nil { continue @@ -212,7 +217,15 @@ func (d *Datastore) calculateDeviations(ctx context.Context) (<-chan *treetypes. deviationChan <- treetypes.NewDeviationEntry(n, treetypes.DeviationReasonIntentExists, nil) } - err := ops.GetDeviations(ctx, deviationTree.Entry, &ops.GetDeviationParams{Ch: deviationChan}, d.taskPool) + // IncludeSensitive=false: redaction is applied in the ops layer so that + // sensitive values never leave the tree as plaintext. + err := ops.GetDeviations(ctx, deviationTree.Entry, &ops.GetDeviationParams{ + Ch: deviationChan, + RenderOpts: ops.RenderOpts{ + IncludeSensitive: false, + SensitivePathSet: d.sensitivePathIndex, + }, + }, d.taskPool) if err != nil { log.Error(err, "failed to run deviation processor") } diff --git a/pkg/datastore/deviations_test.go b/pkg/datastore/deviations_test.go new file mode 100644 index 00000000..fb87c216 --- /dev/null +++ b/pkg/datastore/deviations_test.go @@ -0,0 +1,183 @@ +package datastore + +import ( + "context" + "runtime" + "sync" + "testing" + + "github.com/openconfig/ygot/ygot" + "github.com/sdcio/data-server/mocks/mockcacheclient" + "github.com/sdcio/data-server/pkg/config" + "github.com/sdcio/data-server/pkg/pool" + schemaClientPkg "github.com/sdcio/data-server/pkg/datastore/clients/schema" + treetypes "github.com/sdcio/data-server/pkg/tree/types" + "github.com/sdcio/data-server/pkg/utils/testhelper" + sdcio_schema "github.com/sdcio/data-server/tests/sdcioygot" + tree_persist "github.com/sdcio/sdc-protos/tree_persist" + sdcpb "github.com/sdcio/sdc-protos/sdcpb" + "go.uber.org/mock/gomock" + "google.golang.org/grpc/metadata" +) + +// fakeDeviationStream is a minimal in-memory DataServer_WatchDeviationsServer. +// Only Send and Context are called by SendDeviations; everything else is a no-op. +type fakeDeviationStream struct { + ctx context.Context + mu sync.Mutex + received []*sdcpb.WatchDeviationResponse +} + +func newFakeDeviationStream(ctx context.Context) *fakeDeviationStream { + return &fakeDeviationStream{ctx: ctx} +} + +func (f *fakeDeviationStream) Send(r *sdcpb.WatchDeviationResponse) error { + f.mu.Lock() + defer f.mu.Unlock() + f.received = append(f.received, r) + return nil +} + +func (f *fakeDeviationStream) updateResponses() []*sdcpb.WatchDeviationResponse { + f.mu.Lock() + defer f.mu.Unlock() + var out []*sdcpb.WatchDeviationResponse + for _, r := range f.received { + if r.GetEvent() == sdcpb.DeviationEvent_UPDATE { + out = append(out, r) + } + } + return out +} + +func (f *fakeDeviationStream) Context() context.Context { return f.ctx } +func (f *fakeDeviationStream) SetHeader(metadata.MD) error { return nil } +func (f *fakeDeviationStream) SendHeader(metadata.MD) error { return nil } +func (f *fakeDeviationStream) SetTrailer(metadata.MD) {} +func (f *fakeDeviationStream) SendMsg(any) error { return nil } +func (f *fakeDeviationStream) RecvMsg(any) error { return nil } + +var _ sdcpb.DataServer_WatchDeviationsServer = (*fakeDeviationStream)(nil) + +// deviationEntry builds a DeviationEntry with string-valued ExpectedValue and CurrentValue. +func deviationEntry(path *sdcpb.Path, expected, current string) *treetypes.DeviationEntry { + return treetypes.NewDeviationEntry("test-intent", treetypes.DeviationReasonNotApplied, path). + SetExpectedValue(&sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: expected}}). + SetCurrentValue(&sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: current}}) +} + +// simplePath builds a root-based sdcpb.Path with a single element. +func simplePath(name string) *sdcpb.Path { + return &sdcpb.Path{ + IsRootBased: true, + Elem: []*sdcpb.PathElem{{Name: name}}, + } +} + +// TestSendDeviations_PassthroughUnredacted verifies that SendDeviations forwards +// deviation entries to clients without modifying values. Sensitivity redaction is +// now the responsibility of the ops layer (GetDeviations), not the datastore layer. +func TestSendDeviations_PassthroughUnredacted(t *testing.T) { + ctx := context.Background() + + ds := &Datastore{ + config: &config.DatastoreConfig{Name: "test-ds", Validation: config.NewValidationConfig()}, + } + + path := simplePath("plain-leaf") + ch := make(chan *treetypes.DeviationEntry, 1) + ch <- deviationEntry(path, "expected-value", "current-value") + close(ch) + + stream := newFakeDeviationStream(ctx) + ds.SendDeviations(ctx, ch, map[string]sdcpb.DataServer_WatchDeviationsServer{"fake": stream}) + + updates := stream.updateResponses() + if len(updates) != 1 { + t.Fatalf("expected 1 UPDATE response, got %d", len(updates)) + } + if got := updates[0].GetExpectedValue().GetStringVal(); got != "expected-value" { + t.Errorf("ExpectedValue = %q, want %q", got, "expected-value") + } + if got := updates[0].GetCurrentValue().GetStringVal(); got != "current-value" { + t.Errorf("CurrentValue = %q, want %q", got, "current-value") + } +} + +// TestWatchDeviations_SensitivePathMasking is the integration acceptance test +// for Issue 05. +// +// Scenario: one intent sets "patterntest" to "running-secret"; a second (marker) +// intent marks /patterntest in its sensitive_paths. The sync tree has no value +// for patterntest, so the deviation engine sees a mismatch. Both ExpectedValue +// and CurrentValue in the emitted deviation must be "***". +func TestWatchDeviations_SensitivePathMasking(t *testing.T) { + ctx := context.Background() + + sc, schema, err := testhelper.InitSDCIOSchema() + if err != nil { + t.Fatal(err) + } + // Plain schema client — patterntest is NOT schema-sensitive. + scb := schemaClientPkg.NewSchemaClientBound(schema, sc) + + runningDevice := &sdcio_schema.Device{Patterntest: ygot.String("running-secret")} + dataIntent := buildTestIntent(t, ctx, scb, runningDevice, "data-intent", 10, nil) + markerIntent := buildTestIntent(t, ctx, scb, nil, "marker-intent", 5, []*sdcpb.Path{{Elem: []*sdcpb.PathElem{{Name: "patterntest"}}, IsRootBased: true}}) + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + ccb := mockcacheclient.NewMockCacheClientBound(ctrl) + ccb.EXPECT(). + IntentGetAll(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, _ []string, intentChan chan<- *tree_persist.Intent, errChan chan<- error) { + intentChan <- dataIntent + intentChan <- markerIntent + close(intentChan) + close(errChan) + }) + + // Pre-populate index with marker-intent's sensitive paths. + idx := treetypes.NewSensitivePathIndex() + idx.Set("marker-intent", markerIntent.GetSensitivePaths()) + + // Empty sync tree → running device has no patterntest → triggers deviation. + syncRoot := buildEmptySyncTree(t, ctx, scb) + + ds := &Datastore{ + config: &config.DatastoreConfig{Name: "test-ds", Validation: config.NewValidationConfig()}, + syncTree: syncRoot, + syncTreeMutex: &sync.RWMutex{}, + taskPool: pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)), + cacheClient: ccb, + schemaClient: scb, + sensitivePathIndex: idx, + } + + deviationChan, calcErr := ds.calculateDeviations(ctx) + if calcErr != nil { + t.Fatalf("calculateDeviations() error: %v", calcErr) + } + + stream := newFakeDeviationStream(ctx) + ds.SendDeviations(ctx, deviationChan, map[string]sdcpb.DataServer_WatchDeviationsServer{"fake": stream}) + + found := false + for _, r := range stream.updateResponses() { + elems := r.GetPath().GetElem() + if len(elems) > 0 && elems[len(elems)-1].GetName() == "patterntest" { + found = true + if got := r.GetExpectedValue().GetStringVal(); got != "***" { + t.Errorf("patterntest ExpectedValue = %q, want %q", got, "***") + } + if got := r.GetCurrentValue().GetStringVal(); got != "***" { + t.Errorf("patterntest CurrentValue = %q, want %q", got, "***") + } + } + } + if !found { + t.Error("no deviation found for patterntest — check that the deviation engine emits it") + } +} diff --git a/pkg/datastore/intent_rpc.go b/pkg/datastore/intent_rpc.go index 974384d5..6b70d971 100644 --- a/pkg/datastore/intent_rpc.go +++ b/pkg/datastore/intent_rpc.go @@ -26,6 +26,7 @@ import ( "github.com/sdcio/data-server/pkg/tree/api/adapter" "github.com/sdcio/data-server/pkg/tree/consts" "github.com/sdcio/data-server/pkg/tree/importer/proto" + "github.com/sdcio/data-server/pkg/tree/ops" "github.com/sdcio/data-server/pkg/tree/types" "github.com/sdcio/data-server/pkg/utils" logf "github.com/sdcio/logger" @@ -58,8 +59,10 @@ func (d *Datastore) applyIntent(ctx context.Context, source targettypes.TargetSo return rsp, nil } -func (d *Datastore) GetIntent(ctx context.Context, intentName string) (GetIntentResponse, error) { - // serve running from synctree +func (d *Datastore) GetIntent(ctx context.Context, intentName string, exposeSensitive bool) (GetIntentResponse, error) { + // serve running from synctree; sensitive paths are the cross-intent union + // (running has no own markers — its values may have been echoed back by + // the device verbatim, so any intent's classification must apply). if intentName == consts.RunningIntentName { d.syncTreeMutex.RLock() defer d.syncTreeMutex.RUnlock() @@ -75,12 +78,18 @@ func (d *Datastore) GetIntent(ctx context.Context, intentName string) (GetIntent Orphan: false, NonRevertive: false, ExplicitDeletes: nil, + RenderOpts: ops.RenderOpts{ + IncludeSensitive: exposeSensitive, + SensitivePathSet: d.sensitivePathIndex, + }, } return result, nil } - // otherwise consult cache + // For a regular intent GET, sensitive-path redaction is scoped to that + // intent's own markers only. Another intent's classification does not + // affect how this intent's data is presented (see ADR 0004). root, err := tree.NewTreeRoot(ctx, tree.NewTreeContext(d.schemaClient, d.taskPool)) if err != nil { return nil, err @@ -102,6 +111,9 @@ func (d *Datastore) GetIntent(ctx context.Context, intentName string) (GetIntent return nil, err } + intentSensitivePaths := types.NewSensitivePathIndex() + intentSensitivePaths.Add(tp.GetSensitivePaths()...) + result := &adapter.IntentResponseAdapter{ Entry: root.Entry, IntentName: tp.GetIntentName(), @@ -109,6 +121,10 @@ func (d *Datastore) GetIntent(ctx context.Context, intentName string) (GetIntent Orphan: tp.GetOrphan(), NonRevertive: tp.GetNonRevertive(), ExplicitDeletes: tp.GetExplicitDeletes(), + RenderOpts: ops.RenderOpts{ + IncludeSensitive: exposeSensitive, + SensitivePathSet: intentSensitivePaths, + }, } return result, nil } diff --git a/pkg/datastore/sensitive_path_union_test.go b/pkg/datastore/sensitive_path_union_test.go new file mode 100644 index 00000000..452bae95 --- /dev/null +++ b/pkg/datastore/sensitive_path_union_test.go @@ -0,0 +1,476 @@ +package datastore + +import ( + "context" + "runtime" + "sync" + "testing" + + "github.com/openconfig/ygot/ygot" + "github.com/sdcio/data-server/mocks/mockcacheclient" + "github.com/sdcio/data-server/pkg/config" + schemaClient "github.com/sdcio/data-server/pkg/datastore/clients/schema" + "github.com/sdcio/data-server/pkg/pool" + "github.com/sdcio/data-server/pkg/tree" + "github.com/sdcio/data-server/pkg/tree/consts" + "github.com/sdcio/data-server/pkg/tree/ops" + treetypes "github.com/sdcio/data-server/pkg/tree/types" + "github.com/sdcio/data-server/pkg/utils/testhelper" + sdcio_schema "github.com/sdcio/data-server/tests/sdcioygot" + sdcpb "github.com/sdcio/sdc-protos/sdcpb" + tree_persist "github.com/sdcio/sdc-protos/tree_persist" + "go.uber.org/mock/gomock" +) + +// buildTestIntent builds a tree_persist.Intent from a device struct using +// TreeExport, then stamps the provided sensitive_paths onto it. If device is +// nil the Root will be empty (useful for marker-only intents). +func buildTestIntent(t *testing.T, ctx context.Context, scb schemaClient.SchemaClientBound, device *sdcio_schema.Device, intentName string, prio int32, sensitivePaths []*sdcpb.Path) *tree_persist.Intent { + t.Helper() + + flagsNew := treetypes.NewUpdateInsertFlags() + flagsNew.SetNewFlag() + + tc := tree.NewTreeContext(scb, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0))) + root, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + + if device != nil { + _, err = testhelper.LoadYgotStructIntoTreeRoot(ctx, device, root.Entry, intentName, prio, false, flagsNew) + if err != nil { + t.Fatal(err) + } + } + if err := root.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + + if device == nil { + return &tree_persist.Intent{ + IntentName: intentName, + Priority: prio, + SensitivePaths: sensitivePaths, + } + } + + intent, err := ops.TreeExport(root.Entry, intentName, prio, false) + if err != nil { + t.Fatalf("TreeExport failed: %v", err) + } + intent.SensitivePaths = sensitivePaths + return intent +} + +// buildEmptySyncTree returns a RootEntry with an empty (finished) syncTree, +// suitable for use as the datastore syncTree in tests. +func buildEmptySyncTree(t *testing.T, ctx context.Context, scb schemaClient.SchemaClientBound) *tree.RootEntry { + t.Helper() + tc := tree.NewTreeContext(scb, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0))) + root, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + if err := root.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + return root +} + +// TestBlameConfig_CrossIntentSensitivePathRedaction verifies the cross-intent +// rule for BlameConfig: if any intent marks /patterntest in its sensitive_paths, +// BlameConfig redacts the value even when the schema does not mark that leaf +// sensitive and even when the owning intent did not declare it sensitive. +// +// Acceptance criteria covered: +// - BlameConfig: cross-intent sensitive path causes "***" redaction +// - include_sensitive=true bypasses path-marker redaction +// - Intent with no sensitive_paths contributes nothing to union +func TestBlameConfig_CrossIntentSensitivePathRedaction(t *testing.T) { + ctx := context.Background() + + sc, schema, err := testhelper.InitSDCIOSchema() + if err != nil { + t.Fatal(err) + } + // Plain schema client — patterntest is NOT schema-sensitive. + scb := schemaClient.NewSchemaClientBound(schema, sc) + + // data-intent: holds the actual patterntest value, no sensitive_paths. + dataDevice := &sdcio_schema.Device{Patterntest: ygot.String("secret-value")} + dataIntent := buildTestIntent(t, ctx, scb, dataDevice, "data-intent", 10, nil) + + // marker-intent: no data, but marks /patterntest sensitive. + markerIntent := buildTestIntent(t, ctx, scb, nil, "marker-intent", 5, []*sdcpb.Path{{Elem: []*sdcpb.PathElem{{Name: "patterntest"}}, IsRootBased: true}}) + + tests := []struct { + name string + exposeSensitive bool + wantValue string + }{ + { + name: "redacts path-marker sensitive value when include_sensitive=false", + exposeSensitive: false, + wantValue: "***", + }, + { + name: "reveals path-marker sensitive value when include_sensitive=true", + exposeSensitive: true, + wantValue: "secret-value", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + ccb := mockcacheclient.NewMockCacheClientBound(ctrl) + // LoadAllButRunningIntents streams via IntentGetAll (excludes running). + ccb.EXPECT(). + IntentGetAll(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, _ []string, intentChan chan<- *tree_persist.Intent, errChan chan<- error) { + intentChan <- dataIntent + intentChan <- markerIntent + close(intentChan) + close(errChan) + }) + + // Pre-populate the index with marker-intent's sensitive paths — + // BlameConfig reads the union from the index. + idx := treetypes.NewSensitivePathIndex() + idx.Set("marker-intent", markerIntent.GetSensitivePaths()) + + syncTree := buildEmptySyncTree(t, ctx, scb) + ds := &Datastore{ + config: &config.DatastoreConfig{Name: "test-ds", Validation: config.NewValidationConfig()}, + syncTree: syncTree, + syncTreeMutex: &sync.RWMutex{}, + taskPool: pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)), + cacheClient: ccb, + schemaClient: scb, + sensitivePathIndex: idx, + } + + bte, err := ds.BlameConfig(ctx, false, tt.exposeSensitive) + if err != nil { + t.Fatalf("BlameConfig() error: %v", err) + } + + // Walk the blame tree to find "patterntest". + var patterntestValue string + found := false + for _, child := range bte.GetChilds() { + if child.GetName() == "patterntest" { + found = true + patterntestValue = child.GetValue().GetStringVal() + break + } + } + if !found { + t.Fatal("patterntest not found in blame tree") + } + if patterntestValue != tt.wantValue { + t.Errorf("patterntest value = %q, want %q", patterntestValue, tt.wantValue) + } + }) + } +} + +// TestGetIntent_CrossIntentPathsDoNotRedact verifies the scoped semantics for +// GetIntent(regular): another intent's sensitive_paths markers do NOT cause +// redaction in the fetched intent's response. Only the fetched intent's own +// markers apply (see ADR 0004). +// +// Acceptance criteria covered: +// - GetIntent(regular): cross-intent path markers are ignored +// - include_sensitive flag does not change this — the value is already plain +func TestGetIntent_CrossIntentPathsDoNotRedact(t *testing.T) { + ctx := context.Background() + + sc, schema, err := testhelper.InitSDCIOSchema() + if err != nil { + t.Fatal(err) + } + // Plain schema client — patterntest is NOT schema-sensitive. + scb := schemaClient.NewSchemaClientBound(schema, sc) + + // data-intent: holds the actual patterntest value, no sensitive_paths. + dataDevice := &sdcio_schema.Device{Patterntest: ygot.String("secret-value")} + dataIntent := buildTestIntent(t, ctx, scb, dataDevice, "data-intent", 10, nil) + + // marker-intent marks the path sensitive, but data-intent is the one being fetched. + markerSensitivePaths := []*sdcpb.Path{{Elem: []*sdcpb.PathElem{{Name: "patterntest"}}, IsRootBased: true}} + + tests := []struct { + name string + exposeSensitive bool + }{ + { + name: "cross-intent marker does not redact when include_sensitive=false", + exposeSensitive: false, + }, + { + name: "cross-intent marker does not redact when include_sensitive=true", + exposeSensitive: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + ccb := mockcacheclient.NewMockCacheClientBound(ctrl) + // GetIntent loads the specific intent only — no IntentGetAll. + ccb.EXPECT(). + IntentGet(gomock.Any(), "data-intent"). + Return(dataIntent, nil) + + // Index has marker-intent's paths but data-intent is NOT asking for them. + idx := treetypes.NewSensitivePathIndex() + idx.Set("marker-intent", markerSensitivePaths) + + syncTree := buildEmptySyncTree(t, ctx, scb) + ds := &Datastore{ + config: &config.DatastoreConfig{Name: "test-ds", Validation: config.NewValidationConfig()}, + syncTree: syncTree, + syncTreeMutex: &sync.RWMutex{}, + taskPool: pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)), + cacheClient: ccb, + schemaClient: scb, + sensitivePathIndex: idx, + } + + resp, err := ds.GetIntent(ctx, "data-intent", tt.exposeSensitive) + if err != nil { + t.Fatalf("GetIntent() error: %v", err) + } + + updates, err := resp.ToProtoUpdates(ctx) + if err != nil { + t.Fatalf("ToProtoUpdates() error: %v", err) + } + + for _, u := range updates { + elems := u.GetPath().GetElem() + if len(elems) > 0 && elems[len(elems)-1].GetName() == "patterntest" { + got := u.GetValue().GetStringVal() + if got != "secret-value" { + t.Errorf("patterntest value = %q, want %q (cross-intent redaction must not apply)", got, "secret-value") + } + return + } + } + t.Errorf("patterntest update not found in GetIntent response; all updates: %v", updates) + }) + } +} + +// TestGetIntent_OwnSensitivePaths_Redacted verifies that an intent's own +// sensitive_paths markers DO cause redaction in GetIntent for that intent. +func TestGetIntent_OwnSensitivePaths_Redacted(t *testing.T) { + ctx := context.Background() + + sc, schema, err := testhelper.InitSDCIOSchema() + if err != nil { + t.Fatal(err) + } + scb := schemaClient.NewSchemaClientBound(schema, sc) + + // data-intent: holds patterntest value AND marks it sensitive itself. + dataDevice := &sdcio_schema.Device{Patterntest: ygot.String("my-secret")} + ownSensitivePaths := []*sdcpb.Path{{Elem: []*sdcpb.PathElem{{Name: "patterntest"}}, IsRootBased: true}} + dataIntent := buildTestIntent(t, ctx, scb, dataDevice, "data-intent", 10, ownSensitivePaths) + + tests := []struct { + name string + exposeSensitive bool + wantValue string + }{ + { + name: "own sensitive path is redacted when include_sensitive=false", + exposeSensitive: false, + wantValue: "***", + }, + { + name: "own sensitive path is revealed when include_sensitive=true", + exposeSensitive: true, + wantValue: "my-secret", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + ccb := mockcacheclient.NewMockCacheClientBound(ctrl) + ccb.EXPECT(). + IntentGet(gomock.Any(), "data-intent"). + Return(dataIntent, nil) + + syncTree := buildEmptySyncTree(t, ctx, scb) + ds := &Datastore{ + config: &config.DatastoreConfig{Name: "test-ds", Validation: config.NewValidationConfig()}, + syncTree: syncTree, + syncTreeMutex: &sync.RWMutex{}, + taskPool: pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)), + cacheClient: ccb, + schemaClient: scb, + sensitivePathIndex: treetypes.NewSensitivePathIndex(), + } + + resp, err := ds.GetIntent(ctx, "data-intent", tt.exposeSensitive) + if err != nil { + t.Fatalf("GetIntent() error: %v", err) + } + updates, err := resp.ToProtoUpdates(ctx) + if err != nil { + t.Fatalf("ToProtoUpdates() error: %v", err) + } + + for _, u := range updates { + elems := u.GetPath().GetElem() + if len(elems) > 0 && elems[len(elems)-1].GetName() == "patterntest" { + got := u.GetValue().GetStringVal() + if got != tt.wantValue { + t.Errorf("patterntest value = %q, want %q", got, tt.wantValue) + } + return + } + } + t.Errorf("patterntest update not found in GetIntent response") + }) + } +} + +// TestGetIntent_NoSensitivePaths_NoRedaction verifies that an intent with no +// sensitive_paths does not cause redaction via the path-marker mechanism +// (schema-flag redaction is tested separately in the ops package). +func TestGetIntent_NoSensitivePaths_NoRedaction(t *testing.T) { + ctx := context.Background() + + sc, schema, err := testhelper.InitSDCIOSchema() + if err != nil { + t.Fatal(err) + } + scb := schemaClient.NewSchemaClientBound(schema, sc) + + // data-intent: holds patterntest, no sensitive_paths. + dataDevice := &sdcio_schema.Device{Patterntest: ygot.String("plain-value")} + dataIntent := buildTestIntent(t, ctx, scb, dataDevice, "data-intent", 10, nil) + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + ccb := mockcacheclient.NewMockCacheClientBound(ctrl) + ccb.EXPECT(). + IntentGet(gomock.Any(), "data-intent"). + Return(dataIntent, nil) + + syncTree := buildEmptySyncTree(t, ctx, scb) + ds := &Datastore{ + config: &config.DatastoreConfig{Name: "test-ds", Validation: config.NewValidationConfig()}, + syncTree: syncTree, + syncTreeMutex: &sync.RWMutex{}, + taskPool: pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)), + cacheClient: ccb, + schemaClient: scb, + sensitivePathIndex: treetypes.NewSensitivePathIndex(), + } + + resp, err := ds.GetIntent(ctx, "data-intent", false) + if err != nil { + t.Fatalf("GetIntent() error: %v", err) + } + updates, err := resp.ToProtoUpdates(ctx) + if err != nil { + t.Fatalf("ToProtoUpdates() error: %v", err) + } + + for _, u := range updates { + elems := u.GetPath().GetElem() + if len(elems) > 0 && elems[len(elems)-1].GetName() == "patterntest" { + got := u.GetValue().GetStringVal() + if got != "plain-value" { + t.Errorf("patterntest value = %q, want %q (no redaction expected)", got, "plain-value") + } + return + } + } + t.Error("patterntest update not found in GetIntent response") +} + +// TestGetIntent_RunningIntent_CrossIntentRedaction verifies that fetching the +// running intent applies the full cross-intent union from the sensitivePathIndex. +// The running intent has no own markers; any intent's classification must apply +// to prevent device-echoed plaintext values from leaking. +func TestGetIntent_RunningIntent_CrossIntentRedaction(t *testing.T) { + ctx := context.Background() + + sc, schema, err := testhelper.InitSDCIOSchema() + if err != nil { + t.Fatal(err) + } + scb := schemaClient.NewSchemaClientBound(schema, sc) + + // marker-intent marks /patterntest sensitive — pre-loaded into index. + markerSensitivePaths := []*sdcpb.Path{{Elem: []*sdcpb.PathElem{{Name: "patterntest"}}, IsRootBased: true}} + idx := treetypes.NewSensitivePathIndex() + idx.Set("marker-intent", markerSensitivePaths) + + // Build syncTree with a running value for patterntest. + flagsExisting := treetypes.NewUpdateInsertFlags() + tc := tree.NewTreeContext(scb, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0))) + syncRoot, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + runningDevice := &sdcio_schema.Device{Patterntest: ygot.String("running-secret")} + _, err = testhelper.LoadYgotStructIntoTreeRoot(ctx, runningDevice, syncRoot.Entry, consts.RunningIntentName, consts.RunningValuesPrio, false, flagsExisting) + if err != nil { + t.Fatal(err) + } + if err := syncRoot.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + // GetIntent(running) does not call IntentGetAll — it reads from the index. + ccb := mockcacheclient.NewMockCacheClientBound(ctrl) + + ds := &Datastore{ + config: &config.DatastoreConfig{Name: "test-ds", Validation: config.NewValidationConfig()}, + syncTree: syncRoot, + syncTreeMutex: &sync.RWMutex{}, + taskPool: pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)), + cacheClient: ccb, + schemaClient: scb, + sensitivePathIndex: idx, + } + + resp, err := ds.GetIntent(ctx, consts.RunningIntentName, false) + if err != nil { + t.Fatalf("GetIntent(running) error: %v", err) + } + updates, err := resp.ToProtoUpdates(ctx) + if err != nil { + t.Fatalf("ToProtoUpdates() error: %v", err) + } + + for _, u := range updates { + elems := u.GetPath().GetElem() + if len(elems) > 0 && elems[len(elems)-1].GetName() == "patterntest" { + got := u.GetValue().GetStringVal() + if got != "***" { + t.Errorf("running intent patterntest = %q, want %q (running value should be redacted)", got, "***") + } + return + } + } + t.Error("patterntest update not found in running intent response") +} diff --git a/pkg/datastore/sync.go b/pkg/datastore/sync.go index 42101ec5..b48f1035 100644 --- a/pkg/datastore/sync.go +++ b/pkg/datastore/sync.go @@ -155,7 +155,7 @@ func (d *Datastore) performRevert(ctx context.Context, t *tree.RootEntry) error // if no deletes, check if we have updates if !performApply { - updList, err := ops.ToProtoUpdates(ctx, t.Entry, true) + updList, err := ops.ToProtoUpdates(ctx, t.Entry, ops.RenderOpts{OnlyNewOrUpdated: true, IncludeSensitive: true}) if err != nil { return err } diff --git a/pkg/datastore/transaction_rpc.go b/pkg/datastore/transaction_rpc.go index d89d55b1..d072f959 100644 --- a/pkg/datastore/transaction_rpc.go +++ b/pkg/datastore/transaction_rpc.go @@ -8,6 +8,7 @@ import ( "strings" "time" + "github.com/sdcio/data-server/pkg/cache" "github.com/sdcio/data-server/pkg/datastore/types" "github.com/sdcio/data-server/pkg/tree" "github.com/sdcio/data-server/pkg/tree/api" @@ -63,6 +64,9 @@ func (d *Datastore) SdcpbTransactionIntentToInternalTI(ctx context.Context, req ti.AddRevertPaths(req.GetRevertPaths()...) } } + + ti.SetSensitivePaths(req.GetSensitivePaths()) + // convert the sdcpb.updates to tree.UpdateSlice Updates, err := treetypes.ExpandAndConvertIntent(ctx, d.schemaClient, req.GetIntent(), req.GetPriority(), req.GetUpdate(), time.Now().Unix()) if err != nil { @@ -159,47 +163,58 @@ func (d *Datastore) replaceIntent(ctx context.Context, transaction *types.Transa return warnings, nil } -func (d *Datastore) LoadAllButRunningIntents(ctx context.Context, root *tree.RootEntry) ([]string, error) { - log := logger.FromContext(ctx) - - intentNames := []string{} - IntentChan := make(chan *tree_persist.Intent) - ErrChan := make(chan error, 1) - - go d.cacheClient.IntentGetAll(ctx, []string{consts.RunningIntentName}, IntentChan, ErrChan) - - for { - selectLoop: +// forEachIntent streams every intent from cc (excluding any names in exclude) +// and calls fn for each one. It returns the first error from fn or from the +// cache stream. Both channels are drained to completion before returning. +func forEachIntent( + ctx context.Context, + cc cache.CacheClientBound, + exclude []string, + fn func(*tree_persist.Intent) error, +) error { + intentChan := make(chan *tree_persist.Intent) + errChan := make(chan error, 1) + go cc.IntentGetAll(ctx, exclude, intentChan, errChan) + for errChan != nil || intentChan != nil { select { - case err, ok := <-ErrChan: + case err, ok := <-errChan: if !ok { - // ErrChan already closed which is fine, continue - ErrChan = nil - break selectLoop + errChan = nil + continue } - return nil, err + return err case <-ctx.Done(): - return nil, fmt.Errorf("context closed while retrieving all intents") - case intent, ok := <-IntentChan: + return fmt.Errorf("context closed while iterating intents") + case intent, ok := <-intentChan: if !ok { - // IntentChan closed due to finish - IntentChan = nil - break selectLoop + intentChan = nil + continue } - log.V(logger.VDebug).Info("adding intent to tree", "intent", intent.GetIntentName()) - log.V(logger.VTrace).Info("adding intent to tree", "intent", intent.GetIntentName(), "content", utils.FormatProtoJSON(intent)) - - intentNames = append(intentNames, intent.GetIntentName()) - protoLoader := treeproto.NewProtoTreeImporter(intent) - _, err := root.ImportConfig(ctx, nil, protoLoader, treetypes.NewUpdateInsertFlags(), d.taskPool) - if err != nil { - return nil, err + if err := fn(intent); err != nil { + return err } } - if ErrChan == nil && IntentChan == nil { - return intentNames, nil - } } + return nil +} + +// LoadAllButRunningIntents streams all non-running intents from cache and imports +// each into root. Returns the list of loaded intent names. +func (d *Datastore) LoadAllButRunningIntents(ctx context.Context, root *tree.RootEntry) ([]string, error) { + log := logger.FromContext(ctx) + var intentNames []string + err := forEachIntent(ctx, d.cacheClient, []string{consts.RunningIntentName}, func(intent *tree_persist.Intent) error { + log.V(logger.VDebug).Info("adding intent to tree", "intent", intent.GetIntentName()) + log.V(logger.VTrace).Info("adding intent to tree", "intent", intent.GetIntentName(), "content", utils.FormatProtoJSON(intent)) + intentNames = append(intentNames, intent.GetIntentName()) + protoLoader := treeproto.NewProtoTreeImporter(intent) + _, err := root.ImportConfig(ctx, nil, protoLoader, treetypes.NewUpdateInsertFlags(), d.taskPool) + return err + }) + if err != nil { + return nil, err + } + return intentNames, nil } // lowlevelTransactionSet @@ -381,15 +396,18 @@ func (d *Datastore) lowlevelTransactionSet(ctx context.Context, transaction *typ if err != nil { log.Error(err, "failed deleting intent from store") } + d.sensitivePathIndex.Delete(intent.GetName()) log.V(logger.VDebug).Info("delete intent from cache") continue case err != nil: return nil, err } + protoIntent.SensitivePaths = intent.GetSensitivePaths() err = d.cacheClient.IntentModify(ctx, protoIntent) if err != nil { return nil, fmt.Errorf("failed updating the intended store for %s: %w", d.Name(), err) } + d.sensitivePathIndex.Set(intent.GetName(), intent.GetSensitivePaths()) } // OPTIMISTIC WRITEBACK TO RUNNING / syncTree diff --git a/pkg/datastore/transaction_rpc_test.go b/pkg/datastore/transaction_rpc_test.go index 26eea6a4..5e35bf07 100644 --- a/pkg/datastore/transaction_rpc_test.go +++ b/pkg/datastore/transaction_rpc_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" "github.com/openconfig/ygot/ygot" "github.com/sdcio/data-server/mocks/mockcacheclient" "github.com/sdcio/data-server/mocks/mocktarget" @@ -169,13 +170,14 @@ func TestTransactionSet_PreviouslyApplied(t *testing.T) { Validation: config.NewValidationConfig(), Name: "test-ds", }, - syncTreeMutex: &sync.RWMutex{}, - syncTree: syncTreeRoot, // Pre-populated syncTree - taskPool: vpf, - cacheClient: ccb, - sbi: sbi, - dmutex: &sync.Mutex{}, - schemaClient: scb, + syncTreeMutex: &sync.RWMutex{}, + syncTree: syncTreeRoot, // Pre-populated syncTree + taskPool: vpf, + cacheClient: ccb, + sbi: sbi, + dmutex: &sync.Mutex{}, + schemaClient: scb, + sensitivePathIndex: treetypes.NewSensitivePathIndex(), } ds.transactionManager = types.NewTransactionManager(NewDatastoreRollbackAdapter(ds)) @@ -225,3 +227,162 @@ func TestTransactionSet_PreviouslyApplied(t *testing.T) { }) } } + +// TestTransactionSet_SensitivePathsPersisted verifies that sensitive_paths set +// on a TransactionIntent are written to the tree_persist.Intent passed to +// IntentModify (the cache write). This is the tracer bullet for Issue 03. +func TestTransactionSet_SensitivePathsPersisted(t *testing.T) { + ctx := context.Background() + + sc, schema, err := testhelper.InitSDCIOSchema() + if err != nil { + t.Fatal(err) + } + scb := schemaClient.NewSchemaClientBound(schema, sc) + + wantPaths := []*sdcpb.Path{ + {Elem: []*sdcpb.PathElem{{Name: "interface"}, {Name: "description"}}, IsRootBased: true}, + {Elem: []*sdcpb.PathElem{{Name: "bgp"}, {Name: "neighbors"}, {Name: "auth-password"}}, IsRootBased: true}, + } + + // Build a minimal intent payload with one leaf so TreeExport produces + // a non-empty tree_persist.Intent and IntentModify is called (not IntentDelete). + intentDevice := &sdcio_schema.Device{ + Interface: map[string]*sdcio_schema.SdcioModel_Interface{ + "ethernet-1/1": { + Name: ygot.String("ethernet-1/1"), + Description: ygot.String("sensitive-test"), + }, + }, + } + intentJSON, err := ygot.EmitJSON(intentDevice, &ygot.EmitJSONConfig{ + Format: ygot.RFC7951, + SkipValidation: false, + }) + if err != nil { + t.Fatalf("failed to marshal intent: %v", err) + } + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + var capturedIntent *tree_persist.Intent + ccb := mockcacheclient.NewMockCacheClientBound(ctrl) + ccb.EXPECT(). + IntentGetAll(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, excludeIntentNames []string, intentChan chan<- *tree_persist.Intent, errChan chan<- error) { + close(intentChan) + close(errChan) + }).AnyTimes() + ccb.EXPECT(). + IntentModify(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, intent *tree_persist.Intent) error { + if intent.GetIntentName() == "intent-sensitive" { + capturedIntent = intent + } + return nil + }).AnyTimes() + + sbi := mocktarget.NewMockTarget(ctrl) + sbi.EXPECT().Set(gomock.Any(), gomock.Any()).Return(&sdcpb.SetDataResponse{}, nil).AnyTimes() + + vpf := pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)) + tc := tree.NewTreeContext(scb, vpf) + syncTreeRoot, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + err = syncTreeRoot.FinishInsertionPhase(ctx) + if err != nil { + t.Fatal(err) + } + + ds := &Datastore{ + config: &config.DatastoreConfig{ + Validation: config.NewValidationConfig(), + Name: "test-ds", + }, + syncTreeMutex: &sync.RWMutex{}, + syncTree: syncTreeRoot, + taskPool: vpf, + cacheClient: ccb, + sbi: sbi, + dmutex: &sync.Mutex{}, + schemaClient: scb, + sensitivePathIndex: treetypes.NewSensitivePathIndex(), + } + ds.transactionManager = types.NewTransactionManager(NewDatastoreRollbackAdapter(ds)) + + ti := types.NewTransactionIntent("intent-sensitive", 10) + ti.SetSensitivePaths(wantPaths) + + updates, err := treetypes.ExpandAndConvertIntent(ctx, scb, "intent-sensitive", 10, []*sdcpb.Update{{ + Path: &sdcpb.Path{}, + Value: &sdcpb.TypedValue{ + Value: &sdcpb.TypedValue_JsonVal{JsonVal: []byte(intentJSON)}, + }, + }}, time.Now().Unix()) + if err != nil { + t.Fatalf("failed to expand intent: %v", err) + } + ti.AddUpdates(updates) + + _, err = ds.TransactionSet(ctx, "txn-sensitive", []*types.TransactionIntent{ti}, nil, 10*time.Second, false) + if err != nil { + t.Fatalf("TransactionSet failed: %v", err) + } + + if capturedIntent == nil { + t.Fatal("IntentModify was not called for intent-sensitive") + } + gotXPaths := make([]string, 0, len(capturedIntent.GetSensitivePaths())) + for _, p := range capturedIntent.GetSensitivePaths() { + gotXPaths = append(gotXPaths, p.ToXPath(true)) + } + wantXPaths := make([]string, 0, len(wantPaths)) + for _, p := range wantPaths { + wantXPaths = append(wantXPaths, p.ToXPath(true)) + } + if diff := cmp.Diff(wantXPaths, gotXPaths); diff != "" { + t.Errorf("SensitivePaths mismatch (-want +got):\n%s", diff) + } +} + +// TestSdcpbTransactionIntentToInternalTI_SensitivePaths verifies that +// sensitive_paths schema.Path values are passed through to the internal +// TransactionIntent unchanged. +func TestSdcpbTransactionIntentToInternalTI_SensitivePaths(t *testing.T) { + ctx := context.Background() + + sc, schema, err := testhelper.InitSDCIOSchema() + if err != nil { + t.Fatal(err) + } + scb := schemaClient.NewSchemaClientBound(schema, sc) + ds := &Datastore{schemaClient: scb} + + paths := []*sdcpb.Path{ + {Elem: []*sdcpb.PathElem{{Name: "bgp"}, {Name: "neighbors"}, {Name: "auth-password"}}, IsRootBased: true}, + {Elem: []*sdcpb.PathElem{{Name: "interface"}, {Name: "description"}}, IsRootBased: true}, + } + req := &sdcpb.TransactionIntent{ + Intent: "test-intent", + Priority: 10, + Delete: true, + SensitivePaths: paths, + } + + ti, err := ds.SdcpbTransactionIntentToInternalTI(ctx, req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + got := ti.GetSensitivePaths() + if len(got) != len(paths) { + t.Fatalf("got %d sensitive paths, want %d", len(got), len(paths)) + } + for i, p := range paths { + if got[i].ToXPath(true) != p.ToXPath(true) { + t.Errorf("sensitive_paths[%d]: got %q, want %q", i, got[i].ToXPath(true), p.ToXPath(true)) + } + } +} diff --git a/pkg/datastore/types/transaction_intent.go b/pkg/datastore/types/transaction_intent.go index 78137dec..9f582a9f 100644 --- a/pkg/datastore/types/transaction_intent.go +++ b/pkg/datastore/types/transaction_intent.go @@ -19,6 +19,10 @@ type TransactionIntent struct { previouslyApplied bool // revertPaths paths that are meant to be reverted. This is used for non-revertive intents. revertPaths []*sdcpb.Path + // sensitivePaths schema paths (key-free) identifying leaves whose values + // must be redacted in northbound responses. Absence is treated as an empty + // slice (non-sensitive). + sensitivePaths []*sdcpb.Path } func NewTransactionIntent(name string, priority int32) *TransactionIntent { @@ -116,3 +120,11 @@ func (ti *TransactionIntent) GetRevertPaths() []*sdcpb.Path { func (ti *TransactionIntent) AddRevertPaths(p ...*sdcpb.Path) { ti.revertPaths = append(ti.revertPaths, p...) } + +func (ti *TransactionIntent) SetSensitivePaths(paths []*sdcpb.Path) { + ti.sensitivePaths = paths +} + +func (ti *TransactionIntent) GetSensitivePaths() []*sdcpb.Path { + return ti.sensitivePaths +} diff --git a/pkg/server/datastore.go b/pkg/server/datastore.go index 10746556..03222e7b 100644 --- a/pkg/server/datastore.go +++ b/pkg/server/datastore.go @@ -342,7 +342,7 @@ func (s *Server) BlameConfig(ctx context.Context, req *sdcpb.BlameConfigRequest) return nil, err } - tree, err := ds.BlameConfig(ctx, req.GetIncludeDefaults()) + tree, err := ds.BlameConfig(ctx, req.GetIncludeDefaults(), req.GetIncludeSensitive()) if err != nil { return nil, err } diff --git a/pkg/server/intent.go b/pkg/server/intent.go index 45f7c56c..8a8c4ad1 100644 --- a/pkg/server/intent.go +++ b/pkg/server/intent.go @@ -67,7 +67,7 @@ func (s *Server) GetIntent(ctx context.Context, req *sdcpb.GetIntentRequest) (*s return nil, status.Error(codes.NotFound, err.Error()) } - rsp, err := ds.GetIntent(ctx, req.GetIntent()) + rsp, err := ds.GetIntent(ctx, req.GetIntent(), req.GetIncludeSensitive()) if err != nil { return nil, err } diff --git a/pkg/tree/api/adapter/entryoutputadapter.go b/pkg/tree/api/adapter/entryoutputadapter.go index 5222d807..3ed7cb58 100644 --- a/pkg/tree/api/adapter/entryoutputadapter.go +++ b/pkg/tree/api/adapter/entryoutputadapter.go @@ -20,19 +20,28 @@ func NewEntryOutputAdapter(e api.Entry) *EntryOutputAdapter { } func (t *EntryOutputAdapter) ToJson(ctx context.Context, onlyNewOrUpdated bool) (any, error) { - return ops.ToJson(ctx, t.entry, onlyNewOrUpdated) + // Southbound: always send real values to the device, never redact. + return ops.ToJson(ctx, t.entry, ops.RenderOpts{OnlyNewOrUpdated: onlyNewOrUpdated, IncludeSensitive: true}) } func (t *EntryOutputAdapter) ToJsonIETF(ctx context.Context, onlyNewOrUpdated bool) (any, error) { - return ops.ToJsonIETF(ctx, t.entry, onlyNewOrUpdated) + // Southbound: always send real values to the device, never redact. + return ops.ToJsonIETF(ctx, t.entry, ops.RenderOpts{OnlyNewOrUpdated: onlyNewOrUpdated, IncludeSensitive: true}) } func (t *EntryOutputAdapter) ToXML(ctx context.Context, onlyNewOrUpdated bool, honorNamespace bool, operationWithNamespace bool, useOperationRemove bool) (*etree.Document, error) { - return ops.ToXML(ctx, t.entry, onlyNewOrUpdated, honorNamespace, operationWithNamespace, useOperationRemove) + // Southbound: always send real values to the device, never redact. + return ops.ToXML(ctx, t.entry, ops.XMLRenderOpts{ + RenderOpts: ops.RenderOpts{OnlyNewOrUpdated: onlyNewOrUpdated, IncludeSensitive: true}, + HonorNamespace: honorNamespace, + OperationWithNamespace: operationWithNamespace, + UseOperationRemove: useOperationRemove, + }) } func (t *EntryOutputAdapter) ToProtoUpdates(ctx context.Context, onlyNewOrUpdated bool) ([]*sdcpb.Update, error) { - return ops.ToProtoUpdates(ctx, t.entry, onlyNewOrUpdated) + // Southbound: always send real values to the device, never redact. + return ops.ToProtoUpdates(ctx, t.entry, ops.RenderOpts{OnlyNewOrUpdated: onlyNewOrUpdated, IncludeSensitive: true}) } func (t *EntryOutputAdapter) ToProtoDeletes(ctx context.Context) ([]*sdcpb.Path, error) { diff --git a/pkg/tree/api/adapter/intentresponse.go b/pkg/tree/api/adapter/intentresponse.go index e221c752..fc8f765c 100644 --- a/pkg/tree/api/adapter/intentresponse.go +++ b/pkg/tree/api/adapter/intentresponse.go @@ -16,6 +16,7 @@ type IntentResponseAdapter struct { Orphan bool NonRevertive bool ExplicitDeletes []*sdcpb.Path + RenderOpts ops.RenderOpts } func (t *IntentResponseAdapter) GetIntentName() string { @@ -39,19 +40,22 @@ func (t *IntentResponseAdapter) GetExplicitDeletes() []*sdcpb.Path { } func (t *IntentResponseAdapter) ToJson(ctx context.Context) (any, error) { - return ops.ToJson(ctx, t.Entry, false) + return ops.ToJson(ctx, t.Entry, t.RenderOpts) } func (t *IntentResponseAdapter) ToJsonIETF(ctx context.Context) (any, error) { - return ops.ToJsonIETF(ctx, t.Entry, false) + return ops.ToJsonIETF(ctx, t.Entry, t.RenderOpts) } func (t *IntentResponseAdapter) ToXML(ctx context.Context) (*etree.Document, error) { - return ops.ToXML(ctx, t.Entry, false, true, false, false) + return ops.ToXML(ctx, t.Entry, ops.XMLRenderOpts{ + RenderOpts: t.RenderOpts, + HonorNamespace: true, + }) } func (t *IntentResponseAdapter) ToProtoUpdates(ctx context.Context) ([]*sdcpb.Update, error) { - return ops.ToProtoUpdates(ctx, t.Entry, false) + return ops.ToProtoUpdates(ctx, t.Entry, t.RenderOpts) } func (t *IntentResponseAdapter) ToProtoDeletes(ctx context.Context) ([]*sdcpb.Path, error) { @@ -59,5 +63,5 @@ func (t *IntentResponseAdapter) ToProtoDeletes(ctx context.Context) ([]*sdcpb.Pa } func (t *IntentResponseAdapter) ToXPath(ctx context.Context) (*sdcpb.PathValues, error) { - return ops.ToXPath(ctx, t.Entry, false, false) + return ops.ToXPath(ctx, t.Entry, ops.XPathRenderOpts{RenderOpts: t.RenderOpts}) } diff --git a/pkg/tree/api/leaf_variants.go b/pkg/tree/api/leaf_variants.go index 8558e30b..958b81f2 100644 --- a/pkg/tree/api/leaf_variants.go +++ b/pkg/tree/api/leaf_variants.go @@ -461,7 +461,8 @@ func (lv *LeafVariants) DeleteByOwner(owner string) *LeafEntry { return nil } -func (lv *LeafVariants) GetDeviations(ctx context.Context, ch chan<- *types.DeviationEntry, isActiveCase bool) { + +func (lv *LeafVariants) GetDeviations(ctx context.Context, ch chan<- *types.DeviationEntry, isActiveCase bool, includeSensitive bool, sps types.SensitivePathChecker) { lv.lesMutex.RLock() defer lv.lesMutex.RUnlock() @@ -473,10 +474,19 @@ func (lv *LeafVariants) GetDeviations(ctx context.Context, ch chan<- *types.Devi // is valid for all entries sdcpbPath := lv.parentEntry.SdcpbPath() + sensitive := types.ShouldRedact(includeSensitive, lv.parentEntry.GetSchema(), sdcpbPath, sps) + + redact := func(v *sdcpb.TypedValue) *sdcpb.TypedValue { + if sensitive { + return types.RedactedTypedValue + } + return v + } + // we are part of an inactive case of a choice if !isActiveCase { for _, le := range lv.les { - ch <- types.NewDeviationEntry(le.Owner(), types.DeviationReasonOverruled, sdcpbPath).SetExpectedValue(le.Value()) + ch <- types.NewDeviationEntry(le.Owner(), types.DeviationReasonOverruled, sdcpbPath).SetExpectedValue(redact(le.Value())) } return } @@ -520,7 +530,7 @@ func (lv *LeafVariants) GetDeviations(ctx context.Context, ch chan<- *types.Devi // skip if higher prio equals the overruled continue } - ch <- de.SetCurrentValue(highest.Value()) + ch <- de.SetExpectedValue(redact(de.ExpectedValue())).SetCurrentValue(redact(highest.Value())) } // if there is no running and no highest (probably a default), skip @@ -530,16 +540,21 @@ func (lv *LeafVariants) GetDeviations(ctx context.Context, ch chan<- *types.Devi // unhandled -> running but no intent data if running != nil && highest == nil { - ch <- types.NewDeviationEntry(running.Owner(), types.DeviationReasonUnhandled, sdcpbPath).SetCurrentValue(running.Value()) + ch <- types.NewDeviationEntry(running.Owner(), types.DeviationReasonUnhandled, sdcpbPath).SetCurrentValue(redact(running.Value())) return } // if highest exists but not running OR running != highest if (running == nil && highest != nil) || !running.Value().Equal(highest.Value()) { - de := types.NewDeviationEntry(highest.Owner(), types.DeviationReasonNotApplied, sdcpbPath).SetExpectedValue(highest.Value()) + var currentVal *sdcpb.TypedValue if running != nil { - de.SetCurrentValue(running.Value()) + currentVal = running.Value() } + // When sensitive and running is absent, still emit a redacted CurrentValue + // so that the absence of a device value is not exposed. + de := types.NewDeviationEntry(highest.Owner(), types.DeviationReasonNotApplied, sdcpbPath). + SetExpectedValue(redact(highest.Value())). + SetCurrentValue(redact(currentVal)) ch <- de } diff --git a/pkg/tree/entry_test.go b/pkg/tree/entry_test.go index 2bef7ab3..56249f0e 100644 --- a/pkg/tree/entry_test.go +++ b/pkg/tree/entry_test.go @@ -2254,7 +2254,7 @@ func Test_RevertNonRevertive(t *testing.T) { } t.Logf("Deletes: %v", deletes) - j, err := ops.ToJson(ctx, root.Entry, true) + j, err := ops.ToJson(ctx, root.Entry, ops.RenderOpts{OnlyNewOrUpdated: true}) if err != nil { t.Fatalf("failed to convert to JSON: %v", err) } diff --git a/pkg/tree/importer/xml/xml_tree_importer_test.go b/pkg/tree/importer/xml/xml_tree_importer_test.go index 5b396913..bb7ecb0c 100644 --- a/pkg/tree/importer/xml/xml_tree_importer_test.go +++ b/pkg/tree/importer/xml/xml_tree_importer_test.go @@ -118,7 +118,7 @@ func TestXmlTreeImporter(t *testing.T) { t.Error(err) } - result, err := ops.ToXML(ctx, root.Entry, false, false, false, false) + result, err := ops.ToXML(ctx, root.Entry, ops.XMLRenderOpts{}) if err != nil { t.Fatal(err) } @@ -200,7 +200,7 @@ func TestXmlTreeImporterElement_IdentityRef(t *testing.T) { t.Log(root.String()) - result, err := ops.ToXML(ctx, root.Entry, false, false, false, false) + result, err := ops.ToXML(ctx, root.Entry, ops.XMLRenderOpts{}) if err != nil { t.Fatal(err) } diff --git a/pkg/tree/ops/getdeviations.go b/pkg/tree/ops/getdeviations.go index d10fee06..2ce2d0aa 100644 --- a/pkg/tree/ops/getdeviations.go +++ b/pkg/tree/ops/getdeviations.go @@ -9,8 +9,10 @@ import ( "github.com/sdcio/data-server/pkg/tree/types" ) + type GetDeviationParams struct { Ch chan<- *types.DeviationEntry + RenderOpts } func GetDeviations(ctx context.Context, e api.Entry, config *GetDeviationParams, poolFactory pool.VirtualPoolFactory) error { @@ -58,7 +60,7 @@ func (dt *deviationTask) Run(ctx context.Context, submit func(pool.Task) error) if evalLeafvariants { // calculate Deviation on the LeafVariants - dt.entry.GetLeafVariants().GetDeviations(ctx, dt.config.Ch, dt.isActiveCase) + dt.entry.GetLeafVariants().GetDeviations(ctx, dt.config.Ch, dt.isActiveCase, dt.config.IncludeSensitive, dt.config.SensitivePathSet) } // iterate through all childs diff --git a/pkg/tree/ops/getdeviations_test.go b/pkg/tree/ops/getdeviations_test.go new file mode 100644 index 00000000..494bbaee --- /dev/null +++ b/pkg/tree/ops/getdeviations_test.go @@ -0,0 +1,233 @@ +package ops_test + +import ( + "context" + "runtime" + "testing" + + mockTreeEntry "github.com/sdcio/data-server/mocks/mocktreeentry" + "github.com/sdcio/data-server/pkg/pool" + "github.com/sdcio/data-server/pkg/tree/api" + schemaClient "github.com/sdcio/data-server/pkg/datastore/clients/schema" + . "github.com/sdcio/data-server/pkg/tree/consts" + "github.com/sdcio/data-server/pkg/tree/ops" + "github.com/sdcio/data-server/pkg/tree/types" + sdcpb "github.com/sdcio/sdc-protos/sdcpb" + "go.uber.org/mock/gomock" +) + +// fakeTreeContext is a minimal api.TreeContext for GetDeviations tests. +type fakeTreeContext struct{} + +func (f *fakeTreeContext) PoolFactory() pool.VirtualPoolFactory { return nil } +func (f *fakeTreeContext) SchemaClient() schemaClient.SchemaClientBound { return nil } +func (f *fakeTreeContext) DeepCopy() api.TreeContext { return f } +func (f *fakeTreeContext) ExplicitDeletes() *api.DeletePathSet { return nil } +func (f *fakeTreeContext) NonRevertiveInfo() api.NonRevertiveInfos { return nil } + +// leafDeviationEntry builds a LeafVariants with one intent entry (value expectedVal) +// and one running entry (value currentVal) attached to parentEntry. +func leafDeviationEntry(tc api.TreeContext, parentEntry api.Entry, expectedVal, currentVal string) *api.LeafVariants { + lv := api.NewLeafVariants(tc, parentEntry) + + intentVal := &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: expectedVal}} + lv.Add(api.NewLeafEntry( + types.NewUpdate(parentEntry, intentVal, 10, "test-intent", 0), + types.NewUpdateInsertFlags(), + parentEntry, + )) + + runningVal := &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: currentVal}} + lv.Add(api.NewLeafEntry( + types.NewUpdate(parentEntry, runningVal, RunningValuesPrio, RunningIntentName, 0), + types.NewUpdateInsertFlags(), + parentEntry, + )) + + return lv +} + +// collectDeviations runs ops.GetDeviations and returns all emitted DeviationEntries. +func collectDeviations(t *testing.T, entry api.Entry, params *ops.GetDeviationParams) []*types.DeviationEntry { + t.Helper() + taskPool := pool.NewSharedTaskPool(context.Background(), runtime.GOMAXPROCS(0)) + ch := make(chan *types.DeviationEntry, 32) + params.Ch = ch + + err := ops.GetDeviations(context.Background(), entry, params, taskPool) + close(ch) + if err != nil { + t.Fatalf("GetDeviations error: %v", err) + } + + var result []*types.DeviationEntry + for de := range ch { + result = append(result, de) + } + return result +} + +// setupLeafMock wires a MockEntry to behave as a single leaf node (no children) +// with the given schema and leaf variants. +func setupLeafMock(ctrl *gomock.Controller, schema *sdcpb.SchemaElem, tc api.TreeContext, lv *api.LeafVariants) *mockTreeEntry.MockEntry { + e := mockTreeEntry.NewMockEntry(ctrl) + path := &sdcpb.Path{Elem: []*sdcpb.PathElem{{Name: "test-leaf"}}, IsRootBased: true} + e.EXPECT().SdcpbPath().Return(path).AnyTimes() + e.EXPECT().GetSchema().Return(schema).AnyTimes() + e.EXPECT().GetChilds(types.DescendMethodActiveChilds).Return(api.EntryMap{}).AnyTimes() + e.EXPECT().GetChildMap().Return(api.NewChildMap()).AnyTimes() + e.EXPECT().GetLeafVariants().Return(lv).AnyTimes() + return e +} + +// TestGetDeviations_SchemaSensitiveRedacted is the tracer bullet. +// IncludeSensitive=false must replace both expected and current values with "***" +// when the leaf's schema marks it sensitive. +func TestGetDeviations_SchemaSensitiveRedacted(t *testing.T) { + ctrl := gomock.NewController(t) + + sensitiveSchema := &sdcpb.SchemaElem{ + Schema: &sdcpb.SchemaElem_Field{Field: &sdcpb.LeafSchema{Sensitive: true}}, + } + tc := &fakeTreeContext{} + + e := mockTreeEntry.NewMockEntry(ctrl) + path := &sdcpb.Path{Elem: []*sdcpb.PathElem{{Name: "secret-leaf"}}, IsRootBased: true} + e.EXPECT().SdcpbPath().Return(path).AnyTimes() + e.EXPECT().GetSchema().Return(sensitiveSchema).AnyTimes() + e.EXPECT().GetChilds(types.DescendMethodActiveChilds).Return(api.EntryMap{}).AnyTimes() + e.EXPECT().GetChildMap().Return(api.NewChildMap()).AnyTimes() + + lv := leafDeviationEntry(tc, e, "secret-expected", "secret-current") + e.EXPECT().GetLeafVariants().Return(lv).AnyTimes() + + deviations := collectDeviations(t, e, &ops.GetDeviationParams{RenderOpts: ops.RenderOpts{IncludeSensitive: false}}) + + if len(deviations) == 0 { + t.Fatal("expected at least one deviation, got none") + } + for _, de := range deviations { + if ev := de.ExpectedValue(); ev != nil { + if got := ev.GetStringVal(); got != "***" { + t.Errorf("ExpectedValue = %q, want %q", got, "***") + } + } + if cv := de.CurrentValue(); cv != nil { + if got := cv.GetStringVal(); got != "***" { + t.Errorf("CurrentValue = %q, want %q", got, "***") + } + } + } +} + +// TestGetDeviations_ExposeSensitivePassthrough verifies that IncludeSensitive=true +// leaves schema-sensitive values untouched. +func TestGetDeviations_ExposeSensitivePassthrough(t *testing.T) { + ctrl := gomock.NewController(t) + + sensitiveSchema := &sdcpb.SchemaElem{ + Schema: &sdcpb.SchemaElem_Field{Field: &sdcpb.LeafSchema{Sensitive: true}}, + } + tc := &fakeTreeContext{} + + e := mockTreeEntry.NewMockEntry(ctrl) + path := &sdcpb.Path{Elem: []*sdcpb.PathElem{{Name: "secret-leaf"}}, IsRootBased: true} + e.EXPECT().SdcpbPath().Return(path).AnyTimes() + e.EXPECT().GetSchema().Return(sensitiveSchema).AnyTimes() + e.EXPECT().GetChilds(types.DescendMethodActiveChilds).Return(api.EntryMap{}).AnyTimes() + e.EXPECT().GetChildMap().Return(api.NewChildMap()).AnyTimes() + + lv := leafDeviationEntry(tc, e, "raw-expected", "raw-current") + e.EXPECT().GetLeafVariants().Return(lv).AnyTimes() + + deviations := collectDeviations(t, e, &ops.GetDeviationParams{RenderOpts: ops.RenderOpts{IncludeSensitive: true}}) + + if len(deviations) == 0 { + t.Fatal("expected at least one deviation, got none") + } + de := deviations[0] + if got := de.ExpectedValue().GetStringVal(); got != "raw-expected" { + t.Errorf("ExpectedValue = %q, want %q (must not be redacted)", got, "raw-expected") + } + if got := de.CurrentValue().GetStringVal(); got != "raw-current" { + t.Errorf("CurrentValue = %q, want %q (must not be redacted)", got, "raw-current") + } +} + +// TestGetDeviations_IntentMarkerSensitiveRedacted verifies that IncludeSensitive=false +// redacts values when the path appears in GetDeviationParams.SensitivePathSet +// (intent-marker check), even when the schema does not mark it sensitive. +func TestGetDeviations_IntentMarkerSensitiveRedacted(t *testing.T) { + ctrl := gomock.NewController(t) + + nonSensitiveSchema := &sdcpb.SchemaElem{ + Schema: &sdcpb.SchemaElem_Field{Field: &sdcpb.LeafSchema{Sensitive: false}}, + } + leafPath := &sdcpb.Path{Elem: []*sdcpb.PathElem{{Name: "marker-leaf"}}, IsRootBased: true} + + sps := types.NewSensitivePathIndex() + sps.Add(leafPath) + tc := &fakeTreeContext{} + + e := mockTreeEntry.NewMockEntry(ctrl) + e.EXPECT().SdcpbPath().Return(leafPath).AnyTimes() + e.EXPECT().GetSchema().Return(nonSensitiveSchema).AnyTimes() + e.EXPECT().GetChilds(types.DescendMethodActiveChilds).Return(api.EntryMap{}).AnyTimes() + e.EXPECT().GetChildMap().Return(api.NewChildMap()).AnyTimes() + + lv := leafDeviationEntry(tc, e, "marker-expected", "marker-current") + e.EXPECT().GetLeafVariants().Return(lv).AnyTimes() + + deviations := collectDeviations(t, e, &ops.GetDeviationParams{RenderOpts: ops.RenderOpts{IncludeSensitive: false, SensitivePathSet: sps}}) + + if len(deviations) == 0 { + t.Fatal("expected at least one deviation, got none") + } + for _, de := range deviations { + if ev := de.ExpectedValue(); ev != nil { + if got := ev.GetStringVal(); got != "***" { + t.Errorf("ExpectedValue = %q, want %q (path-marker sensitive)", got, "***") + } + } + if cv := de.CurrentValue(); cv != nil { + if got := cv.GetStringVal(); got != "***" { + t.Errorf("CurrentValue = %q, want %q (path-marker sensitive)", got, "***") + } + } + } +} + +// TestGetDeviations_NonSensitiveNotRedacted verifies that IncludeSensitive=false +// does NOT redact values for paths that are neither schema-sensitive nor in +// GetDeviationParams.SensitivePathSet. +func TestGetDeviations_NonSensitiveNotRedacted(t *testing.T) { + ctrl := gomock.NewController(t) + + nonSensitiveSchema := &sdcpb.SchemaElem{ + Schema: &sdcpb.SchemaElem_Field{Field: &sdcpb.LeafSchema{Sensitive: false}}, + } + tc := &fakeTreeContext{} + + e := mockTreeEntry.NewMockEntry(ctrl) + path := &sdcpb.Path{Elem: []*sdcpb.PathElem{{Name: "plain-leaf"}}, IsRootBased: true} + e.EXPECT().SdcpbPath().Return(path).AnyTimes() + e.EXPECT().GetSchema().Return(nonSensitiveSchema).AnyTimes() + e.EXPECT().GetChilds(types.DescendMethodActiveChilds).Return(api.EntryMap{}).AnyTimes() + e.EXPECT().GetChildMap().Return(api.NewChildMap()).AnyTimes() + + lv := leafDeviationEntry(tc, e, "plain-expected", "plain-current") + e.EXPECT().GetLeafVariants().Return(lv).AnyTimes() + + deviations := collectDeviations(t, e, &ops.GetDeviationParams{RenderOpts: ops.RenderOpts{IncludeSensitive: false}}) + + if len(deviations) == 0 { + t.Fatal("expected at least one deviation, got none") + } + de := deviations[0] + if got := de.ExpectedValue().GetStringVal(); got != "plain-expected" { + t.Errorf("ExpectedValue = %q, want %q (must not be redacted)", got, "plain-expected") + } + if got := de.CurrentValue().GetStringVal(); got != "plain-current" { + t.Errorf("CurrentValue = %q, want %q (must not be redacted)", got, "plain-current") + } +} diff --git a/pkg/tree/ops/json.go b/pkg/tree/ops/json.go index 14ba8357..38198257 100644 --- a/pkg/tree/ops/json.go +++ b/pkg/tree/ops/json.go @@ -11,27 +11,25 @@ import ( sdcpb "github.com/sdcio/sdc-protos/sdcpb" ) -func ToJson(ctx context.Context, e api.Entry, onlyNewOrUpdated bool) (any, error) { - result, err := toJsonInternal(ctx, e, onlyNewOrUpdated, false) +func ToJson(ctx context.Context, e api.Entry, opts RenderOpts) (any, error) { + result, err := toJsonInternal(ctx, e, opts, false) if err != nil { return nil, err } return result, err } -func ToJsonIETF(ctx context.Context, e api.Entry, onlyNewOrUpdated bool) (any, error) { - result, err := toJsonInternal(ctx, e, onlyNewOrUpdated, true) +func ToJsonIETF(ctx context.Context, e api.Entry, opts RenderOpts) (any, error) { + result, err := toJsonInternal(ctx, e, opts, true) if err != nil { return nil, err } return result, err } -// ToJson returns the Branch of the tree as a struct that can be marshalled as JSON +// toJsonInternal returns the Branch of the tree as a struct that can be marshalled as JSON. // If the ietf parameter is set to true, JSON_IETF encoding is used. -// The actualPrefix is used only for the JSON_IETF encoding and can be ignored for JSON -// In the initial / users call with ietf == true, actualPrefix should be set to "" -func toJsonInternal(ctx context.Context, e api.Entry, onlyNewOrUpdated bool, ietf bool) (any, error) { +func toJsonInternal(ctx context.Context, e api.Entry, opts RenderOpts, ietf bool) (any, error) { switch e.GetSchema().GetSchema().(type) { case nil: // we're operating on a key level, no schema attached, but the @@ -42,7 +40,7 @@ func toJsonInternal(ctx context.Context, e api.Entry, onlyNewOrUpdated bool, iet ancest, _ := GetFirstAncestorWithSchema(e) prefixedKey := jsonGetIetfPrefixConditional(key, c, ancest, ietf) // recurse the call - js, err := toJsonInternal(ctx, c, onlyNewOrUpdated, ietf) + js, err := toJsonInternal(ctx, c, opts, ietf) if err != nil { return nil, err } @@ -70,7 +68,7 @@ func toJsonInternal(ctx context.Context, e api.Entry, onlyNewOrUpdated bool, iet result := make([]any, 0, len(childs)) for _, c := range childs { - j, err := toJsonInternal(ctx, c, onlyNewOrUpdated, ietf) + j, err := toJsonInternal(ctx, c, opts, ietf) if err != nil { return nil, err } @@ -84,13 +82,13 @@ func toJsonInternal(ctx context.Context, e api.Entry, onlyNewOrUpdated bool, iet return result, nil case e.GetSchema().GetContainer().IsPresence && ContainsOnlyDefaults(e): // Presence container without any childs - if onlyNewOrUpdated { + if opts.OnlyNewOrUpdated { // presence containers have leafvariantes with typedValue_Empty, so check that if e.GetLeafVariants().ShouldDelete() { return nil, nil } le := e.GetLeafVariants().GetHighestPrecedence(false, false, false) - if le == nil || onlyNewOrUpdated && !(le.IsNew || le.IsUpdated) { + if le == nil || opts.OnlyNewOrUpdated && !(le.IsNew || le.IsUpdated) { return nil, nil } } @@ -100,7 +98,7 @@ func toJsonInternal(ctx context.Context, e api.Entry, onlyNewOrUpdated bool, iet result := map[string]any{} for key, c := range e.GetChilds(types.DescendMethodActiveChilds) { prefixedKey := jsonGetIetfPrefixConditional(key, c, e, ietf) - js, err := toJsonInternal(ctx, c, onlyNewOrUpdated, ietf) + js, err := toJsonInternal(ctx, c, opts, ietf) if err != nil { return nil, err } @@ -118,10 +116,13 @@ func toJsonInternal(ctx context.Context, e api.Entry, onlyNewOrUpdated bool, iet if e.GetLeafVariants().CanDelete() { return nil, nil } - le := e.GetLeafVariants().GetHighestPrecedence(onlyNewOrUpdated, false, false) + le := e.GetLeafVariants().GetHighestPrecedence(opts.OnlyNewOrUpdated, false, false) if le == nil { return nil, nil } + if ShouldRedact(e, opts.IncludeSensitive, opts.SensitivePathSet) { + return types.RedactedStringValue, nil + } return utils.GetJsonValue(le.Value(), ietf) } return nil, fmt.Errorf("unable to convert to json (%s)", e.SdcpbPath().ToXPath(false)) diff --git a/pkg/tree/ops/json_test.go b/pkg/tree/ops/json_test.go index 84d677d4..8a5526df 100644 --- a/pkg/tree/ops/json_test.go +++ b/pkg/tree/ops/json_test.go @@ -9,7 +9,9 @@ import ( "github.com/google/go-cmp/cmp" "github.com/openconfig/ygot/ygot" + "github.com/sdcio/data-server/mocks/mockschemaclientbound" "github.com/sdcio/data-server/pkg/pool" + "github.com/sdcio/data-server/pkg/tree" "github.com/sdcio/data-server/pkg/tree/consts" "github.com/sdcio/data-server/pkg/tree/ops" @@ -21,6 +23,18 @@ import ( "go.uber.org/mock/gomock" ) +// newSchemaClientMarkingSensitive returns a mock schema client that behaves like +// the standard test schema client but marks the leaf named sensitiveLeafName as +// Sensitive=true in its returned schema. +func newSchemaClientMarkingSensitive(t *testing.T, ctrl *gomock.Controller, sensitiveLeafName string) *mockschemaclientbound.MockSchemaClientBound { + t.Helper() + scb, err := testhelper.GetSchemaClientBoundMarkingLeafSensitive(t, ctrl, sensitiveLeafName) + if err != nil { + t.Fatal(err) + } + return scb +} + func TestToJsonTable(t *testing.T) { var tests = []struct { @@ -431,13 +445,14 @@ func TestToJsonTable(t *testing.T) { var jsonStruct any + renderOpts := ops.RenderOpts{OnlyNewOrUpdated: tt.onlyNewOrUpdated} if tt.ietf { - jsonStruct, err = ops.ToJsonIETF(ctx, root.Entry, tt.onlyNewOrUpdated) + jsonStruct, err = ops.ToJsonIETF(ctx, root.Entry, renderOpts) if err != nil { t.Fatal(err) } } else { - jsonStruct, err = ops.ToJson(ctx, root.Entry, tt.onlyNewOrUpdated) + jsonStruct, err = ops.ToJson(ctx, root.Entry, renderOpts) if err != nil { t.Fatal(err) } @@ -462,3 +477,75 @@ func TestToJsonTable(t *testing.T) { }) } } + +func TestToJsonSensitiveRedaction(t *testing.T) { + flagsOld := types.NewUpdateInsertFlags() + + tests := []struct { + name string + exposeSensitive bool + wantPatterntest string + }{ + { + name: "redacts sensitive leaf when IncludeSensitive=false", + exposeSensitive: false, + wantPatterntest: "***", + }, + { + name: "reveals sensitive leaf when IncludeSensitive=true", + exposeSensitive: true, + wantPatterntest: "hallo 00", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + scb := newSchemaClientMarkingSensitive(t, ctrl, "patterntest") + + ctx := context.Background() + tc := tree.NewTreeContext(scb, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0))) + root, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + converter := utils.NewConverter(scb) + + upds, err := testhelper.ExpandUpdateFromConfig(ctx, testhelper.Config1(), converter) + if err != nil { + t.Fatal(err) + } + if err := testhelper.AddToRoot(ctx, root.Entry, upds, flagsOld, "owner1", 5); err != nil { + t.Fatal(err) + } + if err := root.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + + jsonStruct, err := ops.ToJson(ctx, root.Entry, ops.RenderOpts{IncludeSensitive: tt.exposeSensitive}) + if err != nil { + t.Fatal(err) + } + + jsonBytes, err := json.Marshal(jsonStruct) + if err != nil { + t.Fatal(err) + } + + var result map[string]any + if err := json.Unmarshal(jsonBytes, &result); err != nil { + t.Fatal(err) + } + + got, ok := result["patterntest"].(string) + if !ok { + t.Fatalf("patterntest not found or not a string in JSON output: %s", string(jsonBytes)) + } + if diff := cmp.Diff(tt.wantPatterntest, got); diff != "" { + t.Errorf("patterntest mismatch (-want +got):\n%s", diff) + } + + ctrl.Finish() + }) + } +} diff --git a/pkg/tree/ops/proto.go b/pkg/tree/ops/proto.go index e840cff7..fe18789c 100644 --- a/pkg/tree/ops/proto.go +++ b/pkg/tree/ops/proto.go @@ -4,13 +4,21 @@ import ( "context" "github.com/sdcio/data-server/pkg/tree/api" + "github.com/sdcio/data-server/pkg/tree/types" sdcpb "github.com/sdcio/sdc-protos/sdcpb" ) -func ToProtoUpdates(ctx context.Context, e api.Entry, onlyNewOrUpdated bool) ([]*sdcpb.Update, error) { - result := api.LeafVariantSlice{} - result = GetHighestPrecedence(e, onlyNewOrUpdated, false, true) - return result.ToSdcpbUpdateSlice(), nil +func ToProtoUpdates(ctx context.Context, e api.Entry, opts RenderOpts) ([]*sdcpb.Update, error) { + lvs := GetHighestPrecedence(e, opts.OnlyNewOrUpdated, false, true) + result := make([]*sdcpb.Update, 0, len(lvs)) + for _, lv := range lvs { + if !opts.IncludeSensitive && ShouldRedact(lv.GetEntry(), false, opts.SensitivePathSet) { + result = append(result, &sdcpb.Update{Path: lv.GetEntry().SdcpbPath(), Value: types.RedactedTypedValue}) + } else { + result = append(result, lv.ToSdcpbUpdate()) + } + } + return result, nil } func ToProtoDeletes(ctx context.Context, e api.Entry) ([]*sdcpb.Path, error) { diff --git a/pkg/tree/ops/render_opts.go b/pkg/tree/ops/render_opts.go new file mode 100644 index 00000000..df05c3fc --- /dev/null +++ b/pkg/tree/ops/render_opts.go @@ -0,0 +1,38 @@ +package ops + +import ( + "github.com/sdcio/data-server/pkg/tree/api" + "github.com/sdcio/data-server/pkg/tree/types" +) + +// ShouldRedact reports whether the value at e must be replaced with the +// redaction sentinel. Delegates to types.ShouldRedact with the entry's +// schema-sensitivity flag and path pre-computed. +func ShouldRedact(e api.Entry, includeSensitive bool, sps types.SensitivePathChecker) bool { + return types.ShouldRedact(includeSensitive, e.GetSchema(), e.SdcpbPath(), sps) +} + +// RenderOpts holds common options shared across all northbound render operations. +type RenderOpts struct { + OnlyNewOrUpdated bool + IncludeSensitive bool + // SensitivePathSet is the union of sensitive_paths from all in-scope intents. + // A nil value means no path-marker-based redaction is applied. + // Accepts any types.SensitivePathChecker — e.g. a *types.SensitivePathIndex + // in snapshot mode (Add) or the live datastore index (Set/Delete). + SensitivePathSet types.SensitivePathChecker +} + +// XMLRenderOpts extends RenderOpts with XML-specific flags. +type XMLRenderOpts struct { + RenderOpts + HonorNamespace bool + OperationWithNamespace bool + UseOperationRemove bool +} + +// XPathRenderOpts extends RenderOpts with XPath-specific flags. +type XPathRenderOpts struct { + RenderOpts + IncludeDefaults bool +} diff --git a/pkg/tree/ops/sensitive_test.go b/pkg/tree/ops/sensitive_test.go new file mode 100644 index 00000000..1434aa79 --- /dev/null +++ b/pkg/tree/ops/sensitive_test.go @@ -0,0 +1,98 @@ +package ops_test + +import ( + "testing" + + mockTreeEntry "github.com/sdcio/data-server/mocks/mocktreeentry" + "github.com/sdcio/data-server/pkg/tree/ops" + "github.com/sdcio/data-server/pkg/tree/types" + sdcpb "github.com/sdcio/sdc-protos/sdcpb" + "go.uber.org/mock/gomock" +) + +// alwaysContains is a SensitivePathChecker that treats every path as sensitive. +type alwaysContains struct{} + +func (alwaysContains) Contains(*sdcpb.Path) bool { return true } + +func TestShouldRedact(t *testing.T) { + somePath := &sdcpb.Path{Elem: []*sdcpb.PathElem{{Name: "x"}}} + + sensitiveLeaf := &sdcpb.SchemaElem{ + Schema: &sdcpb.SchemaElem_Field{Field: &sdcpb.LeafSchema{Sensitive: true}}, + } + plainLeaf := &sdcpb.SchemaElem{ + Schema: &sdcpb.SchemaElem_Field{Field: &sdcpb.LeafSchema{Sensitive: false}}, + } + + tests := []struct { + name string + schema *sdcpb.SchemaElem + includeSensitive bool + sps types.SensitivePathChecker + want bool + }{ + { + name: "schema-sensitive leaf, IncludeSensitive=false → redact", + schema: sensitiveLeaf, + want: true, + }, + { + name: "schema-sensitive leaf, IncludeSensitive=true → reveal", + schema: sensitiveLeaf, + includeSensitive: true, + want: false, + }, + { + name: "non-sensitive leaf → pass through", + schema: plainLeaf, + want: false, + }, + { + name: "sensitive leaf-list → redact", + schema: &sdcpb.SchemaElem{ + Schema: &sdcpb.SchemaElem_Leaflist{Leaflist: &sdcpb.LeafListSchema{Sensitive: true}}, + }, + want: true, + }, + { + name: "container → pass through", + schema: &sdcpb.SchemaElem{ + Schema: &sdcpb.SchemaElem_Container{Container: &sdcpb.ContainerSchema{}}, + }, + want: false, + }, + { + name: "nil schema → pass through", + schema: nil, + want: false, + }, + { + name: "path in SensitivePathSet → redact", + schema: plainLeaf, + sps: alwaysContains{}, + want: true, + }, + { + name: "path in SensitivePathSet, IncludeSensitive=true → reveal", + schema: plainLeaf, + sps: alwaysContains{}, + includeSensitive: true, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + e := mockTreeEntry.NewMockEntry(ctrl) + e.EXPECT().GetSchema().Return(tt.schema) + e.EXPECT().SdcpbPath().Return(somePath) + + got := ops.ShouldRedact(e, tt.includeSensitive, tt.sps) + if got != tt.want { + t.Errorf("ShouldRedact() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/tree/ops/xml.go b/pkg/tree/ops/xml.go index d1d2c35e..066d7b9b 100644 --- a/pkg/tree/ops/xml.go +++ b/pkg/tree/ops/xml.go @@ -14,20 +14,20 @@ import ( sdcpb "github.com/sdcio/sdc-protos/sdcpb" ) -// ToXML yields the xml representation of the tree. Either updates only (onlyNewOrUpdated flag) or the actual view on the whole tree. -// If honorNamespace is set, the xml elements will carry their respective namespace attributes. -// If operationWithNamespace is set, the operation attributes added to the to be deleted alements will also carry the Netconf Base namespace. -// If useOperationRemove is set, the remove operation will be used for deletes, instead of the delete operation. -func ToXML(ctx context.Context, e api.Entry, onlyNewOrUpdated, honorNamespace, operationWithNamespace, useOperationRemove bool) (*etree.Document, error) { +// ToXML yields the xml representation of the tree. Either updates only (OnlyNewOrUpdated flag) or the actual view on the whole tree. +// If HonorNamespace is set, the xml elements will carry their respective namespace attributes. +// If OperationWithNamespace is set, the operation attributes added to the to be deleted elements will also carry the Netconf Base namespace. +// If UseOperationRemove is set, the remove operation will be used for deletes, instead of the delete operation. +func ToXML(ctx context.Context, e api.Entry, opts XMLRenderOpts) (*etree.Document, error) { doc := etree.NewDocument() - _, err := toXmlInternal(ctx, e, &doc.Element, onlyNewOrUpdated, honorNamespace, operationWithNamespace, useOperationRemove) + _, err := toXmlInternal(ctx, e, &doc.Element, opts) if err != nil { return nil, err } return doc, nil } -func toXmlInternal(ctx context.Context, e api.Entry, parent *etree.Element, onlyNewOrUpdated bool, honorNamespace bool, operationWithNamespace bool, useOperationRemove bool) (doAdd bool, err error) { +func toXmlInternal(ctx context.Context, e api.Entry, parent *etree.Element, opts XMLRenderOpts) (doAdd bool, err error) { switch e.GetSchema().GetSchema().(type) { case nil: @@ -35,7 +35,7 @@ func toXmlInternal(ctx context.Context, e api.Entry, parent *etree.Element, only if e.ShouldDelete() { // If the element is to be deleted // add the delete operation to the parent element - utils.AddXMLOperation(parent, utils.XMLOperationDelete, operationWithNamespace, useOperationRemove) + utils.AddXMLOperation(parent, utils.XMLOperationDelete, opts.OperationWithNamespace, opts.UseOperationRemove) // retrieve the parent schema, we need to extract the key names // values are the tree level names xmlAddKeyElements(e, parent) @@ -78,7 +78,7 @@ func toXmlInternal(ctx context.Context, e api.Entry, parent *etree.Element, only for _, k := range keys { // recurse the call // no additional element is created, since we're on a key level, so add to parent element - doAdd, err := toXmlInternal(ctx, childs[k], parent, onlyNewOrUpdated, honorNamespace, operationWithNamespace, useOperationRemove) + doAdd, err := toXmlInternal(ctx, childs[k], parent, opts) if err != nil { return false, err } @@ -107,9 +107,9 @@ func toXmlInternal(ctx context.Context, e api.Entry, parent *etree.Element, only // create the element for the child, that in the recursed call will appear as parent newElem := etree.NewElement(e.PathName()) // process the honorNamespace instruction - xmlAddNamespaceConditional(e, e.GetParent(), newElem, honorNamespace) + xmlAddNamespaceConditional(e, e.GetParent(), newElem, opts.HonorNamespace) // recurse the call - doAdd, err := toXmlInternal(ctx, child, newElem, onlyNewOrUpdated, honorNamespace, operationWithNamespace, useOperationRemove) + doAdd, err := toXmlInternal(ctx, child, newElem, opts) if err != nil { return false, err } @@ -120,7 +120,7 @@ func toXmlInternal(ctx context.Context, e api.Entry, parent *etree.Element, only replaceAttr := newElem.SelectAttr("nc:operation") if replaceAttr != nil && replaceAttr.Value == string(utils.XMLOperationReplace) { // If we are replacing, we need to have all child values added to the xml tree else they will be removed from the device - err := xmlAddAllChildValues(ctx, child, newElem, honorNamespace, operationWithNamespace, useOperationRemove) + err := xmlAddAllChildValues(ctx, child, newElem, opts) if err != nil { return false, err } @@ -137,25 +137,25 @@ func toXmlInternal(ctx context.Context, e api.Entry, parent *etree.Element, only // if delete, create the element as child of parent newElem := parent.CreateElement(e.PathName()) // add namespace if we create doc with namespace and the actual namespace differs from the parent namespace - xmlAddNamespaceConditional(e, e.GetParent(), newElem, honorNamespace) + xmlAddNamespaceConditional(e, e.GetParent(), newElem, opts.HonorNamespace) // add the delete / remove operation - utils.AddXMLOperation(newElem, utils.XMLOperationDelete, operationWithNamespace, useOperationRemove) + utils.AddXMLOperation(newElem, utils.XMLOperationDelete, opts.OperationWithNamespace, opts.UseOperationRemove) return true, nil case e.GetSchema().GetContainer().IsPresence && ContainsOnlyDefaults(e): // process presence containers with no childs - if onlyNewOrUpdated { + if opts.OnlyNewOrUpdated { // presence containers have leafvariantes with typedValue_Empty, so check that if e.GetLeafVariants().ShouldDelete() { return false, nil } le := e.GetLeafVariants().GetHighestPrecedence(false, false, false) - if le == nil || onlyNewOrUpdated && !(le.IsNew || le.IsUpdated) { + if le == nil || opts.OnlyNewOrUpdated && !(le.IsNew || le.IsUpdated) { return false, nil } } newElem := parent.CreateElement(e.PathName()) // process the honorNamespace instruction - xmlAddNamespaceConditional(e, e.GetParent(), newElem, honorNamespace) + xmlAddNamespaceConditional(e, e.GetParent(), newElem, opts.HonorNamespace) return true, nil default: @@ -181,7 +181,7 @@ func toXmlInternal(ctx context.Context, e api.Entry, parent *etree.Element, only if e.GetParent() != nil { // only if not the root level, we can check if parent namespace != actual elements namespace // so if we need to add namespaces, check if they are equal, if not add the namespace attribute - xmlAddNamespaceConditional(e, e.GetParent(), newElem, honorNamespace) + xmlAddNamespaceConditional(e, e.GetParent(), newElem, opts.HonorNamespace) } else { // if this is the root node, we take the given element from the parent parameter as p // avoiding wrongly adding an additional level in the xml doc. @@ -193,7 +193,7 @@ func toXmlInternal(ctx context.Context, e api.Entry, parent *etree.Element, only return false, fmt.Errorf("child %s does not exist for %s", k, e.SdcpbPath().ToXPath(false)) } // TODO: Do we also need to xmlAddAllChildValues here too? - doAdd, err := toXmlInternal(ctx, child, newElem, onlyNewOrUpdated, honorNamespace, operationWithNamespace, useOperationRemove) + doAdd, err := toXmlInternal(ctx, child, newElem, opts) if err != nil { return false, err } @@ -217,13 +217,13 @@ func toXmlInternal(ctx context.Context, e api.Entry, parent *etree.Element, only ancestorWithSchema, _ := GetFirstAncestorWithSchema(e.GetParent()) if ancestorWithSchema != nil && slices.Contains(GetSchemaKeys(ancestorWithSchema), e.PathName()) { // Key leaf deletions should delete the list entry identified by key value. - utils.AddXMLOperation(parent, utils.XMLOperationDelete, operationWithNamespace, useOperationRemove) + utils.AddXMLOperation(parent, utils.XMLOperationDelete, opts.OperationWithNamespace, opts.UseOperationRemove) xmlAddKeyElements(e.GetParent(), parent) return true, nil } } // if not, add the remove / delete op - utils.AddXMLOperation(parent.CreateElement(e.PathName()), utils.XMLOperationDelete, operationWithNamespace, useOperationRemove) + utils.AddXMLOperation(parent.CreateElement(e.PathName()), utils.XMLOperationDelete, opts.OperationWithNamespace, opts.UseOperationRemove) // see case nil for an explanation of this, it is basically the same if e.GetParent().GetSchema() == nil { xmlAddKeyElements(e.GetParent(), parent) @@ -231,18 +231,22 @@ func toXmlInternal(ctx context.Context, e api.Entry, parent *etree.Element, only return true, nil } // if the Field or Leaflist remains to exist - // get highes Precedence value - le := e.GetLeafVariants().GetHighestPrecedence(onlyNewOrUpdated, false, false) + // get highest precedence value + le := e.GetLeafVariants().GetHighestPrecedence(opts.OnlyNewOrUpdated, false, false) if le == nil { return false, nil } ns := "" // process the namespace attribute - if e.GetParent() == nil || (honorNamespace && !namespaceIsEqual(e, e.GetParent())) { + if e.GetParent() == nil || (opts.HonorNamespace && !namespaceIsEqual(e, e.GetParent())) { ns = utils.GetNamespaceFromGetSchema(e.GetSchema()) } + value := le.Value() + if ShouldRedact(e, opts.IncludeSensitive, opts.SensitivePathSet) { + value = types.RedactedTypedValue + } // convert value to XML and add to parent - utils.TypedValueToXML(parent, le.Value(), e.PathName(), ns, onlyNewOrUpdated, operationWithNamespace, useOperationRemove) + utils.TypedValueToXML(parent, value, e.PathName(), ns, opts.OnlyNewOrUpdated, opts.OperationWithNamespace, opts.UseOperationRemove) return true, nil } return false, fmt.Errorf("unable to convert to xml (%s)", e.SdcpbPath().ToXPath(false)) @@ -306,9 +310,16 @@ func xmlAddKeyElements(s api.Entry, parent *etree.Element) { } } -func xmlAddAllChildValues(ctx context.Context, s api.Entry, parent *etree.Element, honorNamespace bool, operationWithNamespace bool, useOperationRemove bool) error { +func xmlAddAllChildValues(ctx context.Context, s api.Entry, parent *etree.Element, opts XMLRenderOpts) error { parent.Child = make([]etree.Token, 0) - _, err := toXmlInternal(ctx, s, parent, false, honorNamespace, operationWithNamespace, useOperationRemove) + // NETCONF replace semantics: the device replaces the entire list entry with exactly what is + // sent. If only changed children were included (OnlyNewOrUpdated=true), the device would + // treat the absent children as deletions. Override to false so every child is emitted, + // regardless of whether it changed. All other opts (namespace, sensitive-path filtering, …) + // are preserved from the caller. + allOpts := opts + allOpts.OnlyNewOrUpdated = false + _, err := toXmlInternal(ctx, s, parent, allOpts) if err != nil { return err } diff --git a/pkg/tree/ops/xml_test.go b/pkg/tree/ops/xml_test.go index d76bcfb0..150ef3e9 100644 --- a/pkg/tree/ops/xml_test.go +++ b/pkg/tree/ops/xml_test.go @@ -616,7 +616,12 @@ func TestToXMLTable(t *testing.T) { t.Error(err) } - xmlDoc, err := ops.ToXML(ctx, root.Entry, tt.onlyNewOrUpdated, tt.honorNamespace, tt.operationWithNamespace, tt.useOperationRemove) + xmlDoc, err := ops.ToXML(ctx, root.Entry, ops.XMLRenderOpts{ + RenderOpts: ops.RenderOpts{OnlyNewOrUpdated: tt.onlyNewOrUpdated}, + HonorNamespace: tt.honorNamespace, + OperationWithNamespace: tt.operationWithNamespace, + UseOperationRemove: tt.useOperationRemove, + }) if err != nil { t.Fatal(err) } @@ -697,7 +702,7 @@ func TestToXML_KeyLeafDeleteUpgradesToListEntryDelete(t *testing.T) { t.Fatal(err) } - xmlDoc, err := ops.ToXML(ctx, root.Entry, true, false, false, false) + xmlDoc, err := ops.ToXML(ctx, root.Entry, ops.XMLRenderOpts{RenderOpts: ops.RenderOpts{OnlyNewOrUpdated: true}}) if err != nil { t.Fatal(err) } @@ -773,7 +778,7 @@ func TestToXML_DoubleKeyLeafDeleteUpgradesToListEntryDelete(t *testing.T) { t.Fatal(err) } - xmlDoc, err := ops.ToXML(ctx, root.Entry, true, false, false, false) + xmlDoc, err := ops.ToXML(ctx, root.Entry, ops.XMLRenderOpts{RenderOpts: ops.RenderOpts{OnlyNewOrUpdated: true}}) if err != nil { t.Fatal(err) } @@ -865,7 +870,7 @@ func TestToXML_DoubleKeyLeafDeleteOnlyDeletesTargetEntry(t *testing.T) { t.Fatal(err) } - xmlDoc, err := ops.ToXML(ctx, root.Entry, true, false, false, false) + xmlDoc, err := ops.ToXML(ctx, root.Entry, ops.XMLRenderOpts{RenderOpts: ops.RenderOpts{OnlyNewOrUpdated: true}}) if err != nil { t.Fatal(err) } @@ -949,7 +954,7 @@ func TestToXML_DoubleKeyLeafDeleteViaKey1(t *testing.T) { t.Fatal(err) } - xmlDoc, err := ops.ToXML(ctx, root.Entry, true, false, false, false) + xmlDoc, err := ops.ToXML(ctx, root.Entry, ops.XMLRenderOpts{RenderOpts: ops.RenderOpts{OnlyNewOrUpdated: true}}) if err != nil { t.Fatal(err) } @@ -1028,7 +1033,7 @@ func TestToXML_DoubleKeyNonKeyLeafDeleteDoesNotPromote(t *testing.T) { t.Fatal(err) } - xmlDoc, err := ops.ToXML(ctx, root.Entry, true, false, false, false) + xmlDoc, err := ops.ToXML(ctx, root.Entry, ops.XMLRenderOpts{RenderOpts: ops.RenderOpts{OnlyNewOrUpdated: true}}) if err != nil { t.Fatal(err) } @@ -1052,6 +1057,77 @@ func TestToXML_DoubleKeyNonKeyLeafDeleteDoesNotPromote(t *testing.T) { } } +// TestToXML_SensitiveRedaction verifies that ToXML replaces sensitive-leaf values +// with the redaction sentinel when IncludeSensitive=false, and returns the real +// value when IncludeSensitive=true. +func TestToXML_SensitiveRedaction(t *testing.T) { + flagsOld := types.NewUpdateInsertFlags() + + tests := []struct { + name string + exposeSensitive bool + wantPatterntest string + }{ + { + name: "redacts sensitive leaf when IncludeSensitive=false", + exposeSensitive: false, + wantPatterntest: "***", + }, + { + name: "reveals sensitive leaf when IncludeSensitive=true", + exposeSensitive: true, + wantPatterntest: "hallo 00", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + scb := newSchemaClientMarkingSensitive(t, ctrl, "patterntest") + + ctx := context.Background() + tc := tree.NewTreeContext(scb, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0))) + root, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + converter := utils.NewConverter(scb) + + upds, err := testhelper.ExpandUpdateFromConfig(ctx, testhelper.Config1(), converter) + if err != nil { + t.Fatal(err) + } + if err := testhelper.AddToRoot(ctx, root.Entry, upds, flagsOld, "owner1", 5); err != nil { + t.Fatal(err) + } + if err := root.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + + xmlDoc, err := ops.ToXML(ctx, root.Entry, ops.XMLRenderOpts{ + RenderOpts: ops.RenderOpts{IncludeSensitive: tt.exposeSensitive}, + }) + if err != nil { + t.Fatal(err) + } + utils.XmlRecursiveSortElementsByTagName(&xmlDoc.Element) + xmlDoc.Indent(2) + xmlDocStr, err := xmlDoc.WriteToString() + if err != nil { + t.Fatal(err) + } + + pattElem := xmlDoc.FindElement("//patterntest") + if pattElem == nil { + t.Fatalf("patterntest element not found in XML output:\n%s", xmlDocStr) + } + if diff := cmp.Diff(tt.wantPatterntest, pattElem.Text()); diff != "" { + t.Errorf("patterntest value mismatch (-want +got):\n%s", diff) + } + }) + } +} + // TestToXML_DoubleKeyTwoSimultaneousListEntryDeletes verifies that deleting a key // leaf on two different list entries at the same time produces two independent // operation="delete" entries in the output. @@ -1125,7 +1201,7 @@ func TestToXML_DoubleKeyTwoSimultaneousListEntryDeletes(t *testing.T) { t.Fatal(err) } - xmlDoc, err := ops.ToXML(ctx, root.Entry, true, false, false, false) + xmlDoc, err := ops.ToXML(ctx, root.Entry, ops.XMLRenderOpts{RenderOpts: ops.RenderOpts{OnlyNewOrUpdated: true}}) if err != nil { t.Fatal(err) } diff --git a/pkg/tree/ops/xpath.go b/pkg/tree/ops/xpath.go index 344e61d4..14f5267c 100644 --- a/pkg/tree/ops/xpath.go +++ b/pkg/tree/ops/xpath.go @@ -4,19 +4,24 @@ import ( "context" "github.com/sdcio/data-server/pkg/tree/api" + "github.com/sdcio/data-server/pkg/tree/types" "github.com/sdcio/sdc-protos/sdcpb" ) // ToXPath converts the branch under e to a list of XPath entries. It returns only the highest precedence value for each path. -func ToXPath(_ context.Context, e api.Entry, onlyNewOrUpdated, includeDefaults bool) (*sdcpb.PathValues, error) { - lvs := GetHighestPrecedence(e, onlyNewOrUpdated, includeDefaults, false) +func ToXPath(_ context.Context, e api.Entry, opts XPathRenderOpts) (*sdcpb.PathValues, error) { + lvs := GetHighestPrecedence(e, opts.OnlyNewOrUpdated, opts.IncludeDefaults, false) xpaths := make([]*sdcpb.PathValue, 0, len(lvs)) for _, lv := range lvs { + value := lv.Value() + if ShouldRedact(lv.GetEntry(), opts.IncludeSensitive, opts.SensitivePathSet) { + value = types.RedactedTypedValue + } xpaths = append(xpaths, &sdcpb.PathValue{ Path: lv.SdcpbPath(), - Value: lv.Value(), + Value: value, }) } return &sdcpb.PathValues{PathValues: xpaths}, nil diff --git a/pkg/tree/ops/xpath_test.go b/pkg/tree/ops/xpath_test.go index 7599f1e4..d8ba888f 100644 --- a/pkg/tree/ops/xpath_test.go +++ b/pkg/tree/ops/xpath_test.go @@ -39,7 +39,7 @@ func TestToXPath_FromConfig1(t *testing.T) { t.Fatal(err) } - pvs, err := ops.ToXPath(ctx, root.Entry, false, false) + pvs, err := ops.ToXPath(ctx, root.Entry, ops.XPathRenderOpts{}) if err != nil { t.Fatal(err) } @@ -113,12 +113,12 @@ func TestToXPath_OnlyNewOrUpdated_WithSameNewAndExistingConfig1(t *testing.T) { t.Fatal(err) } - onlyNewOrUpdated, err := ops.ToXPath(ctx, root.Entry, true, false) + onlyNewOrUpdated, err := ops.ToXPath(ctx, root.Entry, ops.XPathRenderOpts{RenderOpts: ops.RenderOpts{OnlyNewOrUpdated: true}}) if err != nil { t.Fatal(err) } - allValues, err := ops.ToXPath(ctx, root.Entry, false, false) + allValues, err := ops.ToXPath(ctx, root.Entry, ops.XPathRenderOpts{}) if err != nil { t.Fatal(err) } diff --git a/pkg/tree/processors/processor_blame_config.go b/pkg/tree/processors/processor_blame_config.go index 3a935832..04cafde5 100644 --- a/pkg/tree/processors/processor_blame_config.go +++ b/pkg/tree/processors/processor_blame_config.go @@ -25,6 +25,9 @@ func NewBlameConfigProcessor(params *BlameConfigProcessorParams) *BlameConfigPro type BlameConfigProcessorParams struct { IncludeDefaults bool + // RenderOpts carries sensitive-redaction flags (IncludeSensitive, + // SensitivePathSet) shared with all other northbound render operations. + ops.RenderOpts } // Run processes the entry tree starting from e, building a blame tree showing which owner @@ -89,18 +92,27 @@ func (t *BlameConfigTask) Run(ctx context.Context, submit func(pool.Task) error) highestLe := t.selfEntry.GetLeafVariants().GetHighestPrecedence(false, true, true) if highestLe != nil { if highestLe.Update.Owner() != consts.DefaultsIntentName || t.context.IncludeDefaults { - t.self.SetValue(highestLe.Update.Value()).SetOwner(highestLe.Update.Owner()) - - // check if running equals the expected - runningLe := t.selfEntry.GetLeafVariants().GetRunning() - - switch { - case runningLe != nil: - // if running value is different from the highest precedence value, then we have a deviation, - // so we set the deviation value to the running value - if !proto.Equal(runningLe.Update.Value(), highestLe.Update.Value()) { - t.self.SetDeviationValue(runningLe.Value()) + shouldRedact := ops.ShouldRedact(t.selfEntry, t.context.IncludeSensitive, t.context.SensitivePathSet) + value := highestLe.Update.Value() + if shouldRedact { + value = types.RedactedTypedValue + } + t.self.SetValue(value).SetOwner(highestLe.Update.Owner()) + + // check if running equals the expected + runningLe := t.selfEntry.GetLeafVariants().GetRunning() + + switch { + case runningLe != nil: + // if running value is different from the highest precedence value, then we have a deviation, + // so we set the deviation value to the running value + if !proto.Equal(runningLe.Update.Value(), highestLe.Update.Value()) { + deviationValue := runningLe.Value() + if shouldRedact { + deviationValue = types.RedactedTypedValue } + t.self.SetDeviationValue(deviationValue) + } case runningLe == nil && highestLe.GetUpdate().Owner() != consts.DefaultsIntentName: // if running is nil and highest is not from default, then the deviation is from a non-existing running value, // so we set it to empty diff --git a/pkg/tree/processors/processor_blame_config_test.go b/pkg/tree/processors/processor_blame_config_test.go index 9e1bcfcd..0be60730 100644 --- a/pkg/tree/processors/processor_blame_config_test.go +++ b/pkg/tree/processors/processor_blame_config_test.go @@ -11,6 +11,7 @@ import ( "github.com/sdcio/data-server/pkg/pool" "github.com/sdcio/data-server/pkg/tree" "github.com/sdcio/data-server/pkg/tree/consts" + "github.com/sdcio/data-server/pkg/tree/ops" "github.com/sdcio/data-server/pkg/tree/processors" "github.com/sdcio/data-server/pkg/utils/testhelper" sdcio_schema "github.com/sdcio/data-server/tests/sdcioygot" @@ -185,3 +186,95 @@ func Test_sharedEntryAttributes_BlameConfig(t *testing.T) { }) } } + +func TestBlameConfigSensitiveRedaction(t *testing.T) { + ctx := context.TODO() + owner1 := "owner1" + + tests := []struct { + name string + exposeSensitive bool + wantPatterntestValue string + wantDeviationValue string + }{ + { + name: "redacts value and deviationValue when IncludeSensitive=false", + exposeSensitive: false, + wantPatterntestValue: "***", + wantDeviationValue: "***", + }, + { + name: "reveals value and deviationValue when IncludeSensitive=true", + exposeSensitive: true, + wantPatterntestValue: "hallo 00", + wantDeviationValue: "hallo 0", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + scb, err := testhelper.GetSchemaClientBoundMarkingLeafSensitive(t, mockCtrl, "patterntest") + if err != nil { + t.Fatal(err) + } + + tc := tree.NewTreeContext(scb, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0))) + root, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + + conf1 := testhelper.Config1() + _, err = testhelper.LoadYgotStructIntoTreeRoot(ctx, conf1, root.Entry, owner1, 5, false, flagsNew) + if err != nil { + t.Fatal(err) + } + + // running has a different patterntest so we get a deviationValue + running := testhelper.Config1() + running.Patterntest = ygot.String("hallo 0") + _, err = testhelper.LoadYgotStructIntoTreeRoot(ctx, running, root.Entry, consts.RunningIntentName, consts.RunningValuesPrio, false, flagsExisting) + if err != nil { + t.Fatal(err) + } + + if err := root.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + + sharedPool := pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)) + bp := processors.NewBlameConfigProcessor(&processors.BlameConfigProcessorParams{ + RenderOpts: ops.RenderOpts{IncludeSensitive: tt.exposeSensitive}, + }) + got, err := bp.Run(ctx, root.Entry, sharedPool) + if err != nil { + t.Fatalf("BlameConfig() error: %s", err) + } + + // Find patterntest in the blame tree + var patterntestElem *sdcpb.BlameTreeElement + for _, child := range got.GetChilds() { + if child.GetName() == "patterntest" { + patterntestElem = child + break + } + } + if patterntestElem == nil { + t.Fatal("patterntest element not found in blame tree") + } + + gotValue := patterntestElem.GetValue().GetStringVal() + if diff := cmp.Diff(tt.wantPatterntestValue, gotValue); diff != "" { + t.Errorf("patterntest Value mismatch (-want +got):\n%s", diff) + } + + gotDeviation := patterntestElem.GetDeviationValue().GetStringVal() + if diff := cmp.Diff(tt.wantDeviationValue, gotDeviation); diff != "" { + t.Errorf("patterntest DeviationValue mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/pkg/tree/types/sensitive_path_index.go b/pkg/tree/types/sensitive_path_index.go new file mode 100644 index 00000000..3a852808 --- /dev/null +++ b/pkg/tree/types/sensitive_path_index.go @@ -0,0 +1,135 @@ +package types + +import ( + "sync" + + sdcpb "github.com/sdcio/sdc-protos/sdcpb" +) + +// RedactedStringValue is the sentinel substituted in place of sensitive leaf +// values in deviation, render, and blame outputs. +const RedactedStringValue = "***" + +// RedactedTypedValue is the canonical TypedValue emitted in place of a +// sensitive leaf. Callers must not mutate the returned pointer. +var RedactedTypedValue = &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: RedactedStringValue}} + +// SensitivePathChecker is the read-only interface consumed by tree-layer +// render and deviation operations. Both *SensitivePathIndex (snapshot mode via +// Add) and *SensitivePathIndex (live mode via Set/Delete) satisfy it. +type SensitivePathChecker interface { + Contains(path *sdcpb.Path) bool +} + +// ShouldRedact reports whether a leaf value must be replaced with the +// redaction sentinel. It is the single authoritative implementation of the +// two-source sensitivity check; both pkg/tree/ops (entry-aware wrapper) and +// pkg/tree/api (leaf_variants.go) delegate here. +// +// Parameters: +// - includeSensitive: caller has opted in to receiving real values — skip redaction. +// - schema: the entry's schema element (pass entry.GetSchema()); nil is safe. +// - path: the entry's sdcpb path, used for intent path-marker lookup. +// - sps: cross-intent path-marker set; nil means no path-marker redaction. +func ShouldRedact(includeSensitive bool, schema *sdcpb.SchemaElem, path *sdcpb.Path, sps SensitivePathChecker) bool { + if includeSensitive { + return false + } + return (schema != nil && schema.IsSensitive()) || (sps != nil && sps.Contains(path)) +} + +var _ SensitivePathChecker = (*SensitivePathIndex)(nil) + +// SensitivePathIndex is a concurrent-safe index of sensitive schema paths. +// +// It supports two usage modes that share one struct: +// +// - Live index (datastore-wide): use Set/Delete for incremental per-intent +// updates. The map value tracks which intents own each path so that Delete +// can surgically remove only that intent's contribution. +// +// - Snapshot (per-intent): use Add to mark presence without intent tracking. +// The map value is nil — key existence is the only signal Contains needs. +// +// The two modes must not be mixed on the same instance. +type SensitivePathIndex struct { + mu sync.RWMutex + index map[string][]string // key-pruned XPath → intent names (nil = presence-only) +} + +// NewSensitivePathIndex returns a ready-to-use, empty SensitivePathIndex. +func NewSensitivePathIndex() *SensitivePathIndex { + return &SensitivePathIndex{index: make(map[string][]string)} +} + +// Add marks each path as sensitive without associating it with an intent name +// (presence-only mode). Safe to call concurrently. +func (s *SensitivePathIndex) Add(paths ...*sdcpb.Path) { + s.mu.Lock() + defer s.mu.Unlock() + for _, p := range paths { + if p != nil { + s.index[p.ToXPath(true)] = nil + } + } +} + +// Set replaces the sensitive paths contributed by intentName with paths. +// Existing entries for intentName are removed before the new ones are added. +// Safe to call concurrently. +func (s *SensitivePathIndex) Set(intentName string, paths []*sdcpb.Path) { + s.mu.Lock() + defer s.mu.Unlock() + s.removeLocked(intentName) + for _, p := range paths { + if p == nil { + continue + } + key := p.ToXPath(true) + s.index[key] = append(s.index[key], intentName) + } +} + +// Delete removes all sensitive-path contributions from intentName. +// Safe to call concurrently. +func (s *SensitivePathIndex) Delete(intentName string) { + s.mu.Lock() + defer s.mu.Unlock() + s.removeLocked(intentName) +} + +// removeLocked removes intentName from every entry in the index. +// Entries whose slice becomes empty are deleted from the map. +// Caller must hold the write lock. +func (s *SensitivePathIndex) removeLocked(intentName string) { + for key, names := range s.index { + if names == nil { + continue // presence-only entry (Add mode) — not managed by intent tracking + } + j := 0 + for _, n := range names { + if n != intentName { + names[j] = n + j++ + } + } + if j == 0 { + delete(s.index, key) + } else { + s.index[key] = names[:j] + } + } +} + +// Contains reports whether path is in the index. The lookup uses the same +// key-pruned XPath form as the stored keys, so /interface[name=eth0]/secret +// matches a stored /interface/secret. Safe to call on a nil receiver. +func (s *SensitivePathIndex) Contains(path *sdcpb.Path) bool { + if s == nil || path == nil { + return false + } + s.mu.RLock() + defer s.mu.RUnlock() + _, ok := s.index[path.ToXPath(true)] + return ok +} diff --git a/pkg/tree/types/sensitive_path_index_test.go b/pkg/tree/types/sensitive_path_index_test.go new file mode 100644 index 00000000..19300d15 --- /dev/null +++ b/pkg/tree/types/sensitive_path_index_test.go @@ -0,0 +1,101 @@ +package types_test + +import ( + "testing" + + "github.com/sdcio/data-server/pkg/tree/types" + sdcpb "github.com/sdcio/sdc-protos/sdcpb" +) + +func mustParsePath(t *testing.T, s string) *sdcpb.Path { + t.Helper() + p, err := sdcpb.ParsePath(s) + if err != nil { + t.Fatalf("mustParsePath(%q): %v", s, err) + } + return p +} + +// Cycle 1 — Add marks a path as sensitive; Contains returns true. +func TestSensitivePathIndex_AddContains(t *testing.T) { + idx := types.NewSensitivePathIndex() + idx.Add(mustParsePath(t, "/bgp/neighbors/auth-password")) + + if !idx.Contains(mustParsePath(t, "/bgp/neighbors/auth-password")) { + t.Error("Contains() = false after Add, want true") + } +} + +// Cycle 2 — Contains returns false for a path that was never added. +func TestSensitivePathIndex_ContainsUnknownPath(t *testing.T) { + idx := types.NewSensitivePathIndex() + idx.Add(mustParsePath(t, "/bgp/neighbors/auth-password")) + + if idx.Contains(mustParsePath(t, "/bgp/neighbors/other")) { + t.Error("Contains() = true for unknown path, want false") + } +} + +// Cycle 3 — key-pruned matching: keyed lookup matches keyless stored path and vice-versa. +func TestSensitivePathIndex_KeyPrunedMatch(t *testing.T) { + t.Run("keyless stored, keyed lookup", func(t *testing.T) { + idx := types.NewSensitivePathIndex() + idx.Add(mustParsePath(t, "/interface/secret")) + + if !idx.Contains(mustParsePath(t, "/interface[name=eth0]/secret")) { + t.Error("Contains() = false for keyed lookup against keyless stored path, want true") + } + }) + + t.Run("keyed stored, keyless lookup", func(t *testing.T) { + idx := types.NewSensitivePathIndex() + idx.Add(mustParsePath(t, "/interface[name=eth0]/secret")) + + if !idx.Contains(mustParsePath(t, "/interface/secret")) { + t.Error("Contains() = false for keyless lookup against keyed stored path, want true") + } + }) +} + +// Cycle 4 — Set(intentName, paths) makes paths discoverable via Contains. +func TestSensitivePathIndex_SetContains(t *testing.T) { + idx := types.NewSensitivePathIndex() + idx.Set("my-intent", []*sdcpb.Path{mustParsePath(t, "/ospf/auth-key")}) + + if !idx.Contains(mustParsePath(t, "/ospf/auth-key")) { + t.Error("Contains() = false after Set, want true") + } +} + +// Cycle 5 — Delete removes all paths contributed by that intent. +func TestSensitivePathIndex_DeleteRemovesIntentPaths(t *testing.T) { + idx := types.NewSensitivePathIndex() + idx.Set("my-intent", []*sdcpb.Path{mustParsePath(t, "/ospf/auth-key")}) + idx.Delete("my-intent") + + if idx.Contains(mustParsePath(t, "/ospf/auth-key")) { + t.Error("Contains() = true after Delete, want false") + } +} + +// Cycle 6 — Delete does not remove a path still contributed by another intent. +func TestSensitivePathIndex_DeleteKeepsSharedPath(t *testing.T) { + sharedPath := mustParsePath(t, "/bgp/neighbors/auth-password") + idx := types.NewSensitivePathIndex() + idx.Set("intent-a", []*sdcpb.Path{sharedPath}) + idx.Set("intent-b", []*sdcpb.Path{sharedPath}) + + idx.Delete("intent-a") + + if !idx.Contains(sharedPath) { + t.Error("Contains() = false after deleting one intent, want true (path still owned by intent-b)") + } +} + +// Cycle 7 — nil receiver returns false without panicking. +func TestSensitivePathIndex_NilReceiverContains(t *testing.T) { + var idx *types.SensitivePathIndex + if idx.Contains(mustParsePath(t, "/bgp/neighbors/auth-password")) { + t.Error("Contains() on nil receiver = true, want false") + } +} diff --git a/pkg/utils/testhelper/utils.go b/pkg/utils/testhelper/utils.go index 8d170367..2dcb2ecc 100644 --- a/pkg/utils/testhelper/utils.go +++ b/pkg/utils/testhelper/utils.go @@ -133,6 +133,44 @@ func GetSchemaClientBound(t *testing.T, mockCtrl *gomock.Controller) (*mockschem return mockscb, nil } +// GetSchemaClientBoundMarkingLeafSensitive creates a SchemaClientBound mock that behaves +// like GetSchemaClientBound but marks the leaf with the given name as Sensitive=true in +// its returned schema. Use in tests that need to exercise sensitive-data redaction paths. +func GetSchemaClientBoundMarkingLeafSensitive(t *testing.T, mockCtrl *gomock.Controller, sensitiveLeafName string) (*mockschemaclientbound.MockSchemaClientBound, error) { + t.Helper() + x, schema, err := InitSDCIOSchema() + if err != nil { + return nil, err + } + + sdcpbSchema := &sdcpb.Schema{ + Name: schema.Name, + Vendor: schema.Vendor, + Version: schema.Version, + } + + mockscb := mockschemaclientbound.NewMockSchemaClientBound(mockCtrl) + mockscb.EXPECT().GetSchemaSdcpbPath(gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn( + func(ctx context.Context, path *sdcpb.Path) (*sdcpb.GetSchemaResponse, error) { + resp, err := x.GetSchema(ctx, &sdcpb.GetSchemaRequest{ + Path: path, + Schema: sdcpbSchema, + }) + if err != nil || resp == nil { + return resp, err + } + elems := path.GetElem() + if len(elems) > 0 && elems[len(elems)-1].GetName() == sensitiveLeafName { + if field := resp.GetSchema().GetField(); field != nil { + field.Sensitive = true + } + } + return resp, nil + }, + ) + return mockscb, nil +} + type ImportStatsInterface interface { Changed() bool GetNewCount() int64 From 408ec52ea94b2c27ca0ac45d020ab63d01f4011a Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 10 Jun 2026 11:01:47 +0200 Subject: [PATCH 2/4] chore(deps): pin goyang and sdc-protos to pre-merge commit pseudo-versions Replaces the local-path replace directives with remote pseudo-version references so CI and collaborators can build without local checkouts. Pinned commits: - sdcio/goyang v1.6.2-2.0.20260608121857-4668a077cf72 (PR#4) - sdcio/sdc-protos v0.0.55-0.20260610090020-aeb8edf494c4 (PR#123) Replace with tagged releases once PRs are merged. Co-authored-by: Cursor --- go.mod | 8 ++++++++ go.sum | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 82891a09..3b4db526 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,14 @@ go 1.25.0 replace github.com/openconfig/goyang v1.6.0 => github.com/sdcio/goyang v1.6.2-2 +// Pinned to open PRs; replace with tagged releases once merged: +// - sdcio/goyang PR#4: fix(ApplyDeviate): replace Exts on deviate replace, not append +// - sdcio/sdc-protos PR#123: feat(sensitive): schema.Path + LeafSchema.sensitive flag +replace ( + github.com/sdcio/goyang v1.6.2-2 => github.com/sdcio/goyang v1.6.2-2.0.20260608121857-4668a077cf72 + github.com/sdcio/sdc-protos v0.0.54 => github.com/sdcio/sdc-protos v0.0.55-0.20260610090020-aeb8edf494c4 +) + require ( github.com/AlekSi/pointer v1.2.0 github.com/beevik/etree v1.6.0 diff --git a/go.sum b/go.sum index 4f8192a6..ad2c13a0 100644 --- a/go.sum +++ b/go.sum @@ -191,8 +191,8 @@ github.com/sdcio/logger v0.0.3 h1:IFUbObObGry+S8lHGwOQKKRxJSuOphgRU/hxVhOdMOM= github.com/sdcio/logger v0.0.3/go.mod h1:yWaOxK/G6vszjg8tKZiMqiEjlZouHsjFME4zSk+SAEA= github.com/sdcio/schema-server v0.0.34 h1:NNDOkvtUMONtBA7cVvN96F+FWGD/Do6HNqfchy9B8eI= github.com/sdcio/schema-server v0.0.34/go.mod h1:6t8HLXpqUqEJmE5yNZh29u/KZw0jlOICdNWns7zE4GE= -github.com/sdcio/sdc-protos v0.0.54 h1:1EbtU9ZbbJHFPOFGi5aW8Th79cuY9i+AJaP0ABVx8hw= -github.com/sdcio/sdc-protos v0.0.54/go.mod h1:YMLHbey0/aL1qtLW8csSYVPafsgnnn7aY54HkV5dbyQ= +github.com/sdcio/sdc-protos v0.0.55-0.20260610090020-aeb8edf494c4 h1:M1C1wzt2ni0fssvPEaMNPdHNKqTzBtT8VTT3/EzbrCE= +github.com/sdcio/sdc-protos v0.0.55-0.20260610090020-aeb8edf494c4/go.mod h1:NsvzvHnTonLcwQ/WNzxzBCauQmqxpuviaW0wh7Lkrts= github.com/sdcio/yang-parser v0.0.12 h1:RSSeqfAOIsJx5Lno5u4/ezyOmQYUduQ22rBfU/mtpJ4= github.com/sdcio/yang-parser v0.0.12/go.mod h1:CBqn3Miq85qmFVGHxHXHLluXkaIOsTzV06IM4DW6+D4= github.com/sirikothe/gotextfsm v1.0.1-0.20200816110946-6aa2cfd355e4 h1:FHUL2HofYJuslFOQdy/JjjP36zxqIpd/dcoiwLMIs7k= From 70a7374a454987a7878abb8c030a49da5cec0c74 Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 10 Jun 2026 11:14:59 +0200 Subject: [PATCH 3/4] docs(prd): add sensitive-data PRD and issue index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Makes the PR description link to docs/prd/sensitive-data/PRD.md valid on the branch; tracks implementation issues 01–05 for reviewers. Co-authored-by: Cursor --- docs/prd/sensitive-data/PRD.md | 186 ++++++++++++++++++ .../sensitive-data/issues/01-external-deps.md | 44 +++++ .../issues/02-schema-level-redaction.md | 122 ++++++++++++ .../03-intent-path-marker-persistence.md | 53 +++++ .../issues/04-path-marker-union-at-render.md | 45 +++++ .../issues/05-watchdeviations-masking.md | 47 +++++ docs/prd/sensitive-data/issues/index.md | 107 ++++++++++ 7 files changed, 604 insertions(+) create mode 100644 docs/prd/sensitive-data/PRD.md create mode 100644 docs/prd/sensitive-data/issues/01-external-deps.md create mode 100644 docs/prd/sensitive-data/issues/02-schema-level-redaction.md create mode 100644 docs/prd/sensitive-data/issues/03-intent-path-marker-persistence.md create mode 100644 docs/prd/sensitive-data/issues/04-path-marker-union-at-render.md create mode 100644 docs/prd/sensitive-data/issues/05-watchdeviations-masking.md create mode 100644 docs/prd/sensitive-data/issues/index.md diff --git a/docs/prd/sensitive-data/PRD.md b/docs/prd/sensitive-data/PRD.md new file mode 100644 index 00000000..c0d6d56d --- /dev/null +++ b/docs/prd/sensitive-data/PRD.md @@ -0,0 +1,186 @@ +# PRD: Sensitive Data Support + +## Problem Statement + +Device configuration managed by data-server may contain sensitive values — passwords, authentication keys, community strings, bearer tokens — that must reach the southbound target unchanged but must never be returned in plaintext over the northbound API. Today every northbound response (`GetIntent`, `BlameConfig`) returns leaf values verbatim, with no mechanism to classify or redact sensitive fields. Operators cannot safely expose northbound reads to dashboards, audit tools, or less-privileged users without leaking secrets. + +## Solution + +Introduce a **sensitive-path marker** mechanism that: + +1. Lets the caller of `TransactionSet` declare which key-pruned schema paths in an intent contain sensitive values. +2. Stores those markers alongside the intent blob in `sdcio/cache`. +3. At northbound render time, unions all sensitive path sets across all intents and replaces matching leaf values with the fixed sentinel `***`. +4. Leaves the southbound path (tree merge, YANG validation, Target apply) entirely unchanged — plain values flow to the device as before. + +Sensitivity is a **property of the schema path**, not a list instance. List keys are stripped from markers (e.g. `/bgp/neighbors/auth-password`, not `/bgp/neighbors[peer=10.0.0.1]/auth-password`). This eliminates staleness when list keys change and treats all instances of a leaf type uniformly. + +The feature also defines an **admin bypass** — a per-request flag that privileged callers may set to receive plaintext values. The bypass is a single hook in the render pipeline, designed to be layered with stronger auth mechanisms (mTLS cert role) later without changing the redaction logic. + +## User Stories + +1. As a network operator, I want to mark specific leaf paths as sensitive when submitting an intent, so that northbound consumers never see plaintext passwords or secrets. +2. As a network operator, I want sensitivity to be a property of the schema path rather than a specific list instance, so that I do not have to re-declare sensitivity when list keys change. +3. As a network operator, I want `BlameConfig` to return `***` as the value of a sensitive leaf (with the owner intent still visible), so that I can see who controls the field without seeing the value. +4. As a network operator, I want `GetIntent` to return `***` for sensitive leaf values, so that exported intent data is safe to store in logs or hand to less-privileged users. +5. As a network operator, I want a sensitive marker from any intent to protect a leaf even when the highest-precedence value comes from a different intent that did not mark it sensitive, so that priority re-ordering cannot accidentally expose a secret. +6. As a network operator, I want deviation values for sensitive paths to also be masked in `WatchDeviations`, so that drift notifications do not leak plaintext secrets. +7. As a platform operator, I want an admin bypass flag on `GetIntentRequest` and `BlameConfigRequest` to retrieve plaintext values for debugging, so that authorised operators can inspect actual device configuration when needed. +8. As a platform operator, I want existing intents without sensitivity metadata to be treated as non-sensitive by default, so that the feature can be deployed without any migration. + +## Implementation Decisions + +### Sensitivity model + +Sensitivity markers are **key-pruned schema paths** stored as a `repeated string sensitive_paths` alongside the intent blob in `sdcio/cache`. The format strips list key predicates so `/bgp/neighbors[peer=10.0.0.1]/auth-password` is stored as `/bgp/neighbors/auth-password`. + +At northbound render time the system applies the sensitive-path union scoped to +the in-scope intents for that operation: + +- **`GetIntent(regular)`** — only the fetched intent's own `sensitive_paths` markers apply. +- **`GetIntent(running)`**, **`BlameConfig`**, **`WatchDeviations`** — the union of all + non-running intents' markers applies, because these operations present a merged or + device-reflected view. + +The union is computed in O(1) from an in-memory reverse index maintained on the +Datastore (see ADR 0004). The index is populated eagerly at Datastore startup and +updated incrementally on every intent write or delete. + +For the in-scope set the system: + +1. Looks up the sensitive-path union from the index (or the intent's own markers for a single-intent GET). +2. Strips list keys from each leaf's path during the tree walk and checks for membership in the set. +3. Returns `***` as the string value for any matching leaf, regardless of its actual type. + +The southbound path (merge, validate, apply) reads the intent blob directly and is unaware of the sensitive-path slice. + +### Cross-intent rule + +The cross-intent rule applies to **merged views** (`BlameConfig`, `WatchDeviations`, +`GetIntent(running)`): if any intent marks a path sensitive, the path is treated as +sensitive in those views even if the highest-precedence value comes from an intent +that did not mark it sensitive. + +For **`GetIntent(regular)`** the scope is narrowed to the fetched intent's own +markers. An intent is responsible for its own sensitivity declarations; another +intent's marker does not retroactively affect how a single-intent GET presents its +data. See ADR 0004 for the rationale and trade-offs. + +### "once sensitive → always sensitive" and the replace intent + +A `replace` intent wipes all prior intents. The replace intent must explicitly carry the sensitive paths it considers sensitive. Sensitive-path slices from deleted intents are removed along with their intents. The responsibility for re-declaring sensitive paths lies with the entity performing the replace. + +> **Open:** whether the `replace` intent should automatically inherit prior sensitive paths (to guard against accidental exposure during a replace operation) is left as a follow-up decision. + +### Running intent + +The running intent (device-reported state) is subject to the same obfuscation rule. If any intent marks a path sensitive, the running value for that path is also returned as `***` in northbound responses. This prevents `BlameConfig` from leaking plaintext via the device echo (relevant for NETCONF `GetConfig` which may return passwords verbatim). + +### Cache API extension + +The `sdcio/cache` library requires a new field per intent record to carry the sensitive-path slice. Two options: + +- **First-class field** on the cache intent record — cleaner, requires a library PR to `sdcio/cache` before data-server work can begin. +- **Opaque metadata blob** stored as a parallel key in the cache — keeps the change data-server-internal but is less ergonomic. + +> **Decision required** before implementation starts. + +### Canonical path format + +Key-pruned schema paths. Path encoding (gNMI with module prefixes vs internal tree path) to be decided before implementation — affects cache layout and all lookup code. + +### Northbound API changes + +Add `include_sensitive: bool` to `GetIntentRequest` and `BlameConfigRequest`. When `true` and the caller is authorised, plaintext values are returned. This is the admin bypass (see below). + +Sensitive leaf values in responses are returned as the string `***` regardless of the leaf's YANG type. The field is present in the response (not omitted) so consumers know the field exists and is redacted. + +### Admin bypass + +The initial bypass mechanism is `include_sensitive: bool` in the request proto. Trust model: whoever can reach the gRPC port is trusted (same as today — the gRPC channel is currently unauthenticated). The flag is explicit in the API contract and auditable in request logs. + +The bypass check is a single point in the render pipeline, before any redaction is applied, so that stronger auth (mTLS client cert role) can be layered on later without touching the redaction logic. + +**Future: mTLS client cert role.** When internal gRPC mTLS is implemented (currently a TODO in config-server and data-server), bypass authorisation can be tied to a client certificate `OU=sdcio-admin` issued from a dedicated sdcio internal CA. The existing `TLS*` fields in the client config structs are already present but unimplemented. The sdcio internal CA must be separate from the kube-api CA (different trust domains). + +### Module overview + +| Component | Change | Notes | +|---|---|---| +| `sdc-protos` | `bool sensitive = 22` on `LeafSchema`; `bool sensitive = 24` on `LeafListSchema` | Schema-server sets flag when `sdcio-ext:sensitive` extension is present on a leaf | +| `sdc-protos` | `bool include_sensitive = 4` on `GetIntentRequest`; `bool include_sensitive = 2` on `BlameConfigRequest` | Admin bypass flag | +| `sdc-protos` | `repeated string sensitive_paths = 8` on `tree_persist.Intent` | Key-pruned paths stored alongside intent blob; absence = non-sensitive (no migration) | +| `sdcio/goyang` | One-line fix in `ApplyDeviate` — copy `Exts` for `DeviationAdd`/`DeviationReplace` | Enables `sdcio-ext:sensitive` in per-device-profile overlay YANG files | +| `pkg/tree/ops/render_opts.go` | New `RenderOpts` / `XMLRenderOpts` / `XPathRenderOpts` structs | Replaces scattered `bool` params on render ops; holds `IncludeSensitive` + `SensitivePathSet` | +| `pkg/tree/ops/sensitive.go` | `IsSensitive(e)` — reads `LeafSchema.sensitive`; `KeyPrunedPath(e)` — `SdcpbPath().ToXPath(true)` | Core helpers used by all render ops and blame | +| `pkg/tree/ops/json.go`, `proto.go`, `xml.go`, `xpath.go` | Emit `"***"` / `TypedValue{StringVal:"***"}` at leaf emit point when redaction condition is met | `!opts.IncludeSensitive && (IsSensitive(e) \|\| SensitivePathSet[KeyPrunedPath(e)])` | +| `pkg/tree/api/adapter/intentresponse.go` | `IntentResponseAdapter` forwards `RenderOpts` to ops | Carries `IncludeSensitive` and `SensitivePathSet` from `GetIntent` request | +| `pkg/tree/processors/processor_blame_config.go` | `BlameConfigProcessorParams` gains `IncludeSensitive bool`; redact `Value` and `DeviationValue` | Same condition as render ops | +| `pkg/datastore/intent_rpc.go` | Union `sensitive_paths` across all loaded intents → `SensitivePathSet`; build `RenderOpts`; pass `IncludeSensitive` from request | For `GetIntent` render | +| `pkg/datastore/datastore_rpc.go` | Same union + `IncludeSensitive` forwarding for `BlameConfig` | | +| `pkg/datastore/transaction_rpc.go` | Persist `sensitive_paths` atomically with intent blob during `TransactionSet` | Must be atomic — no window where blob exists without slice | +| `pkg/server/intent.go` | Read `req.GetIncludeSensitive()`, thread into datastore call | | +| `pkg/server/datastore.go` | Same for `BlameConfig` | | +| `WatchDeviations` emission path | Mask deviation values for sensitive paths | Both intent value and running value — Issue 05 | + +### Transaction lifecycle + +The sensitive-path slice is written during `TransactionSet`, atomically with the intent blob. It is deleted when the intent is deleted. If the call to write the slice fails, the intent blob write must also be rolled back. + +### Migration + +Existing intents in cache have no sensitive-path slice. Absence of a slice is treated as non-sensitive. No migration needed. This behaviour must be explicitly documented to avoid future misinterpretation. + +## Alternative Approaches Considered + +### YANG extension annotation (Alt A) + +Define an `sdcio-sensitive` YANG extension module and annotate vendor YANG leaves using `deviate add { sdcio-sens:mark-sensitive; }` in per-device-profile overlay files. Schema-server exposes a `sensitive: bool` flag on `LeafSchema`. data-server checks the flag at NB render time. + +**goyang finding:** `deviate add` for extension statements parses without error in `sdcio/goyang v1.6.2-2` (the `Deviate` struct carries `Extensions []*Statement`) but `ApplyDeviate` in `entry.go` silently ignores extensions — they are never copied onto the target node. A one-line fix in the sdcio/goyang fork would enable this. + +This approach is complementary rather than competing. It sets a **static baseline** (leaf types always sensitive by schema definition) while the path-slice approach handles **dynamic operator control** at intent-submission time. Both resolve to the same check: "is this key-pruned path in the sensitive set?" + +**Options for sourcing Alt A sensitivity without modifying vendor YANG:** + +| Option | Mechanism | Scope | +|---|---|---| +| Patch sdcio/goyang | Fix `ApplyDeviate` to copy extensions; use `deviate add` in overlay YANG files | Device-profile | +| Sidecar profile file | YAML file per device-profile; schema-server reads at load time, exposes `GetSensitivePaths(profile)` RPC | Device-profile | +| Leaf-name heuristic | Schema-server auto-marks leaves named `password`, `secret`, `auth-key`, etc. | Global | +| DatastoreConfig YAML | `sensitive-paths: []string` in existing datastore config; data-server reads directly | Per-datastore | +| Drop Alt A | Path-slice only; static baseline via heuristic if needed | Per-intent | + +### Sensitive flag on LeafVariant (Alt B) + +Add `sensitive: bool` to the `LeafVariant` proto so the flag travels with the value through the tree. Zero path-staleness risk; natural aggregation as the flag reaches the winning leaf. Rejected for v1: requires proto changes in `sdc-protos` and updates to every importer, exporter, and blame code path — large blast radius. + +### Encrypt at rest (Alt C) + +Encrypt sensitive leaf values before storing in the intent blob; southbound decrypts on apply. Rejected: requires KMS integration, makes southbound stateful, and significantly increases scope. + +### Sentinel + external store (Alt D) + +Replace sensitive values with a sentinel in the blob; store real values in an external secrets store; southbound resolves on apply. Rejected: southbound breaks if the external store is unavailable; sentinels break YANG typed-value comparisons in tree merge. + +## Open Questions + +| Question | Status | +|---|---| +| sdcio/cache API extension: first-class field vs opaque sidecar? | **Resolved** — first-class `repeated string sensitive_paths = 8` on `tree_persist.Intent` (in sdc-protos); cache library stores opaque bytes unchanged | +| Canonical path format: gNMI with module prefixes or internal tree path? | **Resolved** — `SdcpbPath().ToXPath(true)` (noKeys=true); format `/bgp/neighbors/auth-password`; consistent with `must`/`when` deviation path syntax | +| Replace-intent sensitive marker inheritance: auto-inherit prior slices or require explicit re-declaration? | Open — deferred follow-up | +| Should Alt A (YANG extension / sidecar profile) be implemented alongside path-slice as a static baseline layer? | **Resolved for v1** — Alt A (goyang fix + `sdcio-ext:sensitive` + schema `sensitive` flag) is implemented as the static baseline layer (Issue 02); dynamic path-slice is the per-intent layer (Issues 03–04) | +| Admin bypass: when to layer mTLS cert role on top of the proto flag? | Deferred — depends on internal gRPC mTLS rollout | + +## Out of Scope + +**Encryption at rest.** Intent blobs are stored as plaintext in BadgerDB. Protecting secrets at rest requires KMS integration and is a separate infrastructure concern. + +**Per-instance sensitivity.** List keys are stripped from markers — all instances of a leaf type are treated uniformly. Marking only a specific list entry's leaf as sensitive is not supported. + +**Fine-grained access control.** The initial admin bypass trusts any caller that can reach the gRPC port. Role-based access control (mTLS cert role, token-based auth) is a follow-up. + +**Schema-level static baseline.** Alt A (YANG extension or sidecar profile) provides a schema-level complement to the path-slice approach. Implementing it is not part of this PRD but is designed to compose cleanly with it. + +**Audit log.** Recording which calls exercised the admin bypass is not part of this PRD. diff --git a/docs/prd/sensitive-data/issues/01-external-deps.md b/docs/prd/sensitive-data/issues/01-external-deps.md new file mode 100644 index 00000000..60383248 --- /dev/null +++ b/docs/prd/sensitive-data/issues/01-external-deps.md @@ -0,0 +1,44 @@ +# External dependencies: goyang fix + sdc-protos fields + +## Parent + +[PRD: Sensitive Data Support](../PRD.md) + +## What to build + +Three external PRs that unblock all in-repo implementation work. These can be raised and merged in parallel before any data-server changes land. + +**1. `sdcio/goyang` — fix `ApplyDeviate` extension propagation** + +`ApplyDeviate` silently drops extensions on `deviate add` / `deviate replace` nodes. A one-line fix copies the deviated node's extensions onto the target: + +```go +deviatedNode.Exts = append(deviatedNode.Exts, devSpec.Exts()...) +``` + +This makes `sdcio-ext:sensitive;` annotations in per-device-profile overlay YANG files survive parsing and appear on the parsed `yang.Entry`. + +**2. `sdc-protos` — `LeafSchema` and `LeafListSchema` sensitive flag** + +Add `bool sensitive = 22` to `LeafSchema` and `bool sensitive = 24` to `LeafListSchema`. Schema-server sets this flag when it detects the `sdcio-ext:sensitive` extension on a parsed leaf. data-server reads it at northbound render time via `ops.IsSensitive(e)`. + +**3. `sdc-protos` — admin bypass on northbound request messages** + +Add `bool include_sensitive = ` to `GetIntentRequest` and `BlameConfigRequest`. When `true` the render pipeline skips redaction and returns plaintext values. Trust model for v1: any caller that can reach the gRPC port (unchanged from today). + +**4. `sdcio/cache` — first-class `sensitive_paths` field on intent record** + +Add `repeated string sensitive_paths` to the `tree_persist.Intent` proto (or equivalent intent record type in the cache library). data-server will write this field atomically alongside the intent blob during `TransactionSet` and read it during northbound render. Field absence (existing intents) is treated as non-sensitive — no migration required. + +## Acceptance criteria + +- [ ] `sdcio/goyang`: `deviate add` with `sdcio-ext:sensitive;` survives `ApplyDeviate` and appears on the parsed `yang.Entry.Exts` +- [ ] `sdc-protos`: `LeafSchema.sensitive` and `LeafListSchema.sensitive` fields present and generated in Go bindings +- [ ] `sdc-protos`: `GetIntentRequest.include_sensitive` and `BlameConfigRequest.include_sensitive` fields present and generated in Go bindings +- [ ] `sdcio/cache`: intent record exposes `sensitive_paths []string`; absent field treated as empty slice by existing readers +- [ ] All four repos have passing CI on their respective PRs +- [ ] data-server `go.mod` updated to reference the new versions of all changed modules + +## Blocked by + +None — can start immediately. diff --git a/docs/prd/sensitive-data/issues/02-schema-level-redaction.md b/docs/prd/sensitive-data/issues/02-schema-level-redaction.md new file mode 100644 index 00000000..01b0fb83 --- /dev/null +++ b/docs/prd/sensitive-data/issues/02-schema-level-redaction.md @@ -0,0 +1,122 @@ +# Schema-level render redaction + admin bypass (Phase 1) + +## Parent + +[PRD: Sensitive Data Support](../PRD.md) + +## What to build + +Implement the schema-level (static baseline) redaction layer end-to-end: from schema flag detection through all northbound render paths, covering `GetIntent`, `BlameConfig`, and the admin bypass. + +--- + +### `ops.IsSensitive(e api.Entry) bool` + +New helper in `pkg/tree/ops`. Reads `LeafSchema.sensitive` / `LeafListSchema.sensitive` from the entry's schema via a type-switch on `e.GetSchema().GetSchema().(type)`. Returns `false` when schema is absent (non-leaf nodes, existing intents without schema flags). + +--- + +### `ops.KeyPrunedPath(e api.Entry) string` + +Returns `e.SdcpbPath().ToXPath(true)` — the existing `noKeys=true` argument already strips list key predicates. Format: `/bgp/neighbors/auth-password`. This is the canonical sensitive-path lookup key used both at render time and in stored `sensitive_paths` markers (Issue 03). + +--- + +### `RenderOpts` — struct hierarchy in `pkg/tree/ops/render_opts.go` + +Replace the current per-function boolean parameters with structured option types. The existing `onlyNewOrUpdated` bool is absorbed into `RenderOpts.OnlyNewOrUpdated`. + +```go +// ops/render_opts.go + +// RenderOpts are the common render options shared across all northbound +// render operations. +type RenderOpts struct { + OnlyNewOrUpdated bool + IncludeSensitive bool + // SensitivePathSet is the union of sensitive_paths slices read from all + // in-scope intents (populated in Issue 04). An empty/nil map means no + // path-marker-based redaction is applied (correct for schema-only Phase 1). + SensitivePathSet map[string]bool +} + +type XMLRenderOpts struct { + RenderOpts + HonorNamespace bool + OperationWithNamespace bool + UseOperationRemove bool +} + +type XPathRenderOpts struct { + RenderOpts + IncludeDefaults bool +} +``` + +New signatures: + +```go +ToJson(ctx, entry, RenderOpts) (any, error) +ToJsonIETF(ctx, entry, RenderOpts) (any, error) +ToProtoUpdates(ctx, entry, RenderOpts) ([]*sdcpb.Update, error) +ToXML(ctx, entry, XMLRenderOpts) (*etree.Document, error) +ToXPath(ctx, entry, XPathRenderOpts) (*sdcpb.PathValues, error) +``` + +Call sites to update (all currently in the same package or one hop away): +- `pkg/tree/api/adapter/intentresponse.go` — `IntentResponseAdapter.ToJson`, `.ToJsonIETF`, `.ToXML`, `.ToProtoUpdates`, `.ToXPath` (all currently hardcode `false` for `onlyNewOrUpdated`) +- `pkg/tree/ops/xml_test.go`, `json_test.go`, `xpath_test.go` — test structs use named bool params, migrate to `RenderOpts{...}` / `XMLRenderOpts{...}` / `XPathRenderOpts{...}` +- `pkg/datastore/intent_rpc.go` — calls ops indirectly via the adapter; will thread `IncludeSensitive` here in step 7 + +--- + +### Redaction condition + +At the leaf/leaf-list emit point in each render op: + +```go +!opts.IncludeSensitive && (ops.IsSensitive(e) || opts.SensitivePathSet[ops.KeyPrunedPath(e)]) +``` + +Redacted values are emitted as: +- JSON/XML: string `"***"` +- Proto (`TypedValue`): `&sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: "***"}}` +- XPath (`PathValues`): same `TypedValue` sentinel + +The field is **always present** in the response — never omitted. + +--- + +### `processor_blame_config.go` + +Add `IncludeSensitive bool` to `BlameConfigProcessorParams`. Apply the same redaction condition at both `t.self.SetValue(...)` and `t.self.SetDeviationValue(...)` assignments in `BlameConfigTask.Run`. + +--- + +### Threading `include_sensitive` from request to render + +| Layer | File | Change | +|---|---|---| +| gRPC handler | `pkg/server/intent.go` | Read `req.GetIncludeSensitive()`, pass to `ds.GetIntent(ctx, name, exposeSensitive)` | +| gRPC handler | `pkg/server/datastore.go` | Read `req.GetIncludeSensitive()`, pass to `ds.BlameConfig(ctx, req)` | +| Datastore | `pkg/datastore/intent_rpc.go` | Accept `exposeSensitive bool`, build `RenderOpts{IncludeSensitive: exposeSensitive}`, pass to adapter | +| Datastore | `pkg/datastore/datastore_rpc.go` | Same — pass into `BlameConfigProcessorParams` | + +--- + +## Acceptance criteria + +- [ ] `ops.IsSensitive(e)` returns `true` for a leaf whose schema has `sensitive = true`; `false` for all other entries +- [ ] `ops.KeyPrunedPath(e)` returns a keyless XPath string (no `[key=value]` predicates); verified by unit test +- [ ] All of `ToJson`, `ToJsonIETF`, `ToProtoUpdates`, `ToXML`, `ToXPath` emit `***` for a sensitive leaf when `IncludeSensitive = false` +- [ ] Same render ops return the real value when `IncludeSensitive = true` +- [ ] `BlameConfig` response: `Value` and `DeviationValue` for a sensitive leaf are `***` when `include_sensitive` not set +- [ ] `GetIntent` response: sensitive leaf values are `***` when `include_sensitive` not set +- [ ] `GetIntent` and `BlameConfig` with `include_sensitive = true` return plaintext values +- [ ] Southbound path (tree merge, YANG validation, target apply) is unaffected — no schema flag checks added there +- [ ] Existing intents without `sensitive` schema flags behave identically to today +- [ ] Integration test: submit an intent with a schema-marked-sensitive leaf; verify `GetIntent` returns `***`; verify with `include_sensitive = true` returns plaintext + +## Blocked by + +Issue 01 (external deps) — needs `LeafSchema.sensitive`, `LeafListSchema.sensitive`, and `include_sensitive` proto fields. ✅ Done. diff --git a/docs/prd/sensitive-data/issues/03-intent-path-marker-persistence.md b/docs/prd/sensitive-data/issues/03-intent-path-marker-persistence.md new file mode 100644 index 00000000..3278b7ad --- /dev/null +++ b/docs/prd/sensitive-data/issues/03-intent-path-marker-persistence.md @@ -0,0 +1,53 @@ +# Intent path-marker persistence (Phase 2) + +## Parent + +[PRD: Sensitive Data Support](../PRD.md) + +## What to build + +Extend `TransactionSet` to accept, validate, and atomically persist a `sensitive_paths` slice alongside the intent blob in `sdcio/cache`. This is the write side of Phase 2 — the read side (union at render time) is a separate issue. + +**API change** + +`TransactionSetRequest.Intent` gains `repeated string sensitive_paths`. Callers submit keyless XPath strings (e.g. `/bgp/neighbors/auth-password`). Paths with key predicates (e.g. `/bgp/neighbors[peer=10.0.0.1]/auth-password`) are rejected with a descriptive `InvalidArgument` gRPC error — callers must strip keys before submitting. + +**Validation in `pkg/datastore/transaction_rpc.go`** + +Before any write: +- Parse each path in `sensitive_paths` +- Reject (error) if any path element contains key predicates +- Reject if a path is empty or malformed + +**Atomic persistence** + +`cacheClient.IntentModify` (or equivalent) must write the `sensitive_paths` slice to the cache intent record's `sensitive_paths` field atomically with the intent blob. If the cache write fails, the intent blob write is also rolled back — there must be no window where the blob exists without its slice, or vice versa. + +**Intent deletion** + +When an intent is deleted (`IntentDelete`), the `sensitive_paths` stored with it are removed along with the blob. No tombstone or residual marker is left. + +**Replace intent semantics** + +A `replace` intent wipes all prior intents for the datastore. The replacing intent must explicitly supply the `sensitive_paths` it considers sensitive. Sensitive-path slices from deleted intents are removed along with those intents. The responsibility for re-declaring sensitive paths lies with the entity performing the replace. This behaviour must be documented in code and in operator-facing docs. + +**Missing slice = non-sensitive** + +Existing intents in cache have no `sensitive_paths` slice (field absent). Absence is treated as an empty slice — non-sensitive. No migration is required. This must be explicitly documented to avoid future misinterpretation. + +## Acceptance criteria + +- [ ] `TransactionSetRequest.Intent.sensitive_paths` is wired from proto through `types.TransactionIntent` to the cache write +- [ ] Paths containing key predicates return `InvalidArgument` with a message identifying the offending path +- [ ] Malformed or empty paths return `InvalidArgument` +- [ ] Valid `sensitive_paths` are stored in the cache intent record and round-trip correctly through `IntentGet` +- [ ] If the cache write fails mid-transaction, the intent blob is rolled back (no partial state) +- [ ] Intent deletion removes the stored `sensitive_paths` slice +- [ ] A replace intent clears prior intents' slices; only the replacing intent's slice remains +- [ ] Existing intents without a slice are readable and treated as non-sensitive (no migration, no crash) +- [ ] Unit test: submit intent with valid sensitive paths, read back via cache, assert round-trip +- [ ] Unit test: submit intent with key-predicate path, assert `InvalidArgument` + +## Blocked by + +Issue 01 (external deps) — needs `sdcio/cache` first-class `sensitive_paths` field and `sdc-protos` `TransactionSetRequest.Intent.sensitive_paths`. diff --git a/docs/prd/sensitive-data/issues/04-path-marker-union-at-render.md b/docs/prd/sensitive-data/issues/04-path-marker-union-at-render.md new file mode 100644 index 00000000..93444ed9 --- /dev/null +++ b/docs/prd/sensitive-data/issues/04-path-marker-union-at-render.md @@ -0,0 +1,45 @@ +# Path-marker union at northbound render time (Phase 2) + +## Parent + +[PRD: Sensitive Data Support](../PRD.md) + +## What to build + +Wire the per-intent `sensitive_paths` slices stored in `sdcio/cache` into the northbound render pipeline. At render time, all intents' slices are loaded and unioned into `RenderOpts.SensitivePathSet`, which the render ops (added in issue 02) already check. + +**Union logic** + +A helper (e.g. `ops.LoadSensitivePathSet(intents []CacheIntent) map[string]bool`) iterates all loaded intent records, reads each `sensitive_paths` slice, and populates a `map[string]bool`. Key format is keyless XPath (`/bgp/neighbors/auth-password`), consistent with what was stored in issue 03 and what `ops.KeyPrunedPath(e)` produces at render time. + +**`GetIntent` path (`pkg/datastore/intent_rpc.go`)** + +After loading the intent(s) from cache, call the union helper over all intents for the datastore, populate `RenderOpts.SensitivePathSet`, and pass the opts into `IntentResponseAdapter` / the ops render call. The existing `IncludeSensitive` field on `RenderOpts` is also set from the request flag (wired in issue 02). + +**`BlameConfig` path (`pkg/datastore/datastore_rpc.go`)** + +The blame processor already loads all intents to compute the merged view. Before invoking `processor_blame_config`, build the union set from the loaded intents and pass it into the processor's render context. The blame processor already checks `SensitivePathSet` (wired in issue 02). + +**Cross-intent rule** + +If *any* intent marks a path sensitive, all northbound views redact that path — even if the highest-precedence value comes from an intent that did not mark it sensitive. This is enforced naturally by the union: any intent contributing `true` for a path key keeps it in the set. + +**Running intent** + +The running intent (device-reported state, loaded from the `running` cache entry) is subject to the same obfuscation rule. Include the running entry when building the union if it carries a `sensitive_paths` slice; in practice the running entry will not declare sensitive paths, but the render-time check against the unioned set already covers it. + +## Acceptance criteria + +- [ ] `GetIntent`: if any intent for the datastore has `/foo/bar` in its `sensitive_paths`, `/foo/bar` is redacted in the `GetIntent` response regardless of which intent holds the winning value +- [ ] `BlameConfig`: same cross-intent rule applies; `Value` and `DeviationValue` for a path in any intent's slice are `***` +- [ ] `GetIntent` with `include_sensitive = true` returns plaintext even when path is in a stored slice +- [ ] `BlameConfig` with `include_sensitive = true` same +- [ ] Intent with no `sensitive_paths` slice (existing intents) contributes nothing to the union — behaviour unchanged +- [ ] Running intent value for a path marked sensitive by any other intent is also redacted +- [ ] Unit test: two intents, only one marks a path sensitive; verify the path is redacted in both `GetIntent` and `BlameConfig` +- [ ] Integration test: replace intent that omits a previously-sensitive path; verify the path is no longer redacted after replace + +## Blocked by + +Issue 02 (schema-level redaction) — `RenderOpts.SensitivePathSet` and `ops.KeyPrunedPath` must exist. +Issue 03 (path-marker persistence) — `sensitive_paths` must be stored and retrievable from cache. diff --git a/docs/prd/sensitive-data/issues/05-watchdeviations-masking.md b/docs/prd/sensitive-data/issues/05-watchdeviations-masking.md new file mode 100644 index 00000000..c78e1acd --- /dev/null +++ b/docs/prd/sensitive-data/issues/05-watchdeviations-masking.md @@ -0,0 +1,47 @@ +# WatchDeviations sensitive-path masking + +## Parent + +[PRD: Sensitive Data Support](../PRD.md) + +## What to build + +Mask sensitive leaf values in the `WatchDeviations` stream. Both `ExpectedValue` and `CurrentValue` in each streamed `WatchDeviationResponse` must be replaced with `***` when the leaf's path is in the sensitive set — whether that sensitivity comes from the schema flag (`IsSensitive`) or from any intent's stored `sensitive_paths` slice. + +**Where the change lives** + +The deviation manager loop (`pkg/datastore/deviations.go`, `DeviationMgr`) deep-copies the sync tree, loads all intents, calls `ops.GetDeviations`, then streams results via `SendDeviations`. The sensitive-path union must be built once per deviation cycle (not per streamed message) and passed into both the deviation detection walk and the emit step. + +**Union building** + +Reuse the same `ops.LoadSensitivePathSet` helper introduced in issue 04. Load it from the same intent set that `DeviationMgr` already loads. Schema-level sensitivity (`IsSensitive`) is checked inline at the leaf level, same as in the render ops. + +**Masking at emit** + +In `SendDeviations`, before writing `ExpectedValue` and `CurrentValue` into the response message, apply: + +``` +if !exposeSensitive && (ops.IsSensitive(leaf) || sensitivePathSet[ops.KeyPrunedPath(leaf)]) { + expectedValue = "***" + currentValue = "***" +} +``` + +`WatchDeviationsRequest` does not currently have an `include_sensitive` flag. Whether to add one is a decision for this issue — the PRD defers it but the bypass hook is already designed to be a single point in the pipeline. + +**No `include_sensitive` on `WatchDeviationsRequest` in v1** + +Streaming bypass is a distinct auth problem (the stream runs continuously; the caller's privilege level needs to be checked per-message or at connection time). For v1, always redact and leave the bypass as a follow-up when mTLS cert roles are implemented. + +## Acceptance criteria + +- [ ] `WatchDeviationsResponse`: `ExpectedValue` and `CurrentValue` are `***` for any leaf whose path is sensitive (schema flag or any intent's stored slice) +- [ ] Non-sensitive leaves are unaffected +- [ ] The union is built once per deviation cycle, not per leaf or per message +- [ ] Schema-flag sensitivity (`IsSensitive`) and path-slice sensitivity (`SensitivePathSet`) are both checked +- [ ] No `include_sensitive` bypass added to `WatchDeviationsRequest` in this issue (documented as follow-up) +- [ ] Integration test: intent marks `/foo/bar` sensitive; running value differs from expected; verify both deviation values are `***` in the stream + +## Blocked by + +Issue 04 (path-marker union at render time) — `ops.LoadSensitivePathSet` and `ops.KeyPrunedPath` must exist; `IsSensitive` already exists from issue 02. diff --git a/docs/prd/sensitive-data/issues/index.md b/docs/prd/sensitive-data/issues/index.md new file mode 100644 index 00000000..650b7fa4 --- /dev/null +++ b/docs/prd/sensitive-data/issues/index.md @@ -0,0 +1,107 @@ +# Sensitive Data Support — Issue Index + +## Issues + +| # | Title | Status | Blocked by | +|---|---|---|---| +| [01](./01-external-deps.md) | External deps: goyang fix + sdc-protos + cache field | done | — | +| [02](./02-schema-level-redaction.md) | Schema-level render redaction + admin bypass | done | 01 | +| [03](./03-intent-path-marker-persistence.md) | Intent path-marker persistence | done | 01 | +| [04](./04-path-marker-union-at-render.md) | Path-marker union at northbound render time | done | 02, 03 | +| [05](./05-watchdeviations-masking.md) | WatchDeviations sensitive-path masking | done | 04 | + +--- + +## Issue 01 — External deps + +**Status:** done + +**PRs:** +- [sdcio/goyang#4](https://github.com/sdcio/goyang/pull/4) — `fix-apply-deviate-exts-replace` +- [sdcio/sdc-protos#123](https://github.com/sdcio/sdc-protos/pull/123) — `feat-sensitive-paths-schema-path` + +**Files touched:** +- `goyang/pkg/yang/entry.go` — fix `ApplyDeviate`: `DeviationAdd` appends `Exts`, `DeviationReplace` replaces them (was incorrectly appending for both); mirrors existing `Default` branching +- `goyang/pkg/yang/entry_test.go` — new `TestDeviation` case `"deviate replace replaces existing extensions, does not accumulate"` in addition to the two propagation cases +- `sdc-protos/schema.proto` — `LeafListSchema.sensitive = 24`, `LeafSchema.sensitive = 22` +- `sdc-protos/data.proto` — `GetIntentRequest.include_sensitive = 4`, `BlameConfigRequest.include_sensitive = 2`; `TransactionIntent.sensitive_paths: repeated schema.Path = 11` +- `sdc-protos/tree_persist.proto` — `Intent.sensitive_paths: repeated schema.Path = 8` (was `repeated string`) +- `sdc-protos/sdcpb/schema.pb.go`, `sdc-protos/sdcpb/data.pb.go`, `sdc-protos/tree_persist/tree_persist.pb.go` — regenerated +- `sdc-protos/tree_persist/tree_persist_sensitive_test.go` — round-trip + backward-compat tests +- `data-server/go.mod` — local replace directives (update to tagged PRs after merge) + +--- + +## Issue 02 — Schema-level render redaction + admin bypass + +**Status:** done + +**Design decisions recorded in issue doc:** +- `RenderOpts` / `XMLRenderOpts` / `XPathRenderOpts` struct hierarchy in `pkg/tree/ops/render_opts.go` +- `SensitivePathSet map[string]bool` in `RenderOpts` — populated from per-intent `sensitive_paths` union (Issue 04); nil/empty = schema-flag-only redaction (Phase 1) +- `KeyPrunedPath(e)` = `e.SdcpbPath().ToXPath(true)` — reuses existing `noKeys` parameter +- Callers to update: `IntentResponseAdapter`, all ops test files, datastore intent/blame RPC layers, server handlers + +**Files touched:** +- `pkg/tree/ops/sensitive.go` — new: `IsSensitive`, `KeyPrunedPath` +- `pkg/tree/ops/sensitive_test.go` — new: unit tests for above +- `pkg/tree/ops/render_opts.go` — new: `RenderOpts`, `XMLRenderOpts`, `XPathRenderOpts` +- `pkg/tree/ops/json.go` — `ToJson`/`ToJsonIETF` migrated to `RenderOpts`; redaction added at leaf emit +- `pkg/tree/ops/xml.go` — `ToXML` migrated to `XMLRenderOpts`; redaction added at leaf emit +- `pkg/tree/ops/proto.go` — `ToProtoUpdates` migrated to `RenderOpts`; redaction added per leaf-entry +- `pkg/tree/ops/xpath.go` — `ToXPath` migrated to `XPathRenderOpts`; redaction added per leaf-entry +- `pkg/tree/ops/json_test.go` — migrated to `RenderOpts`; `TestToJsonSensitiveRedaction` added +- `pkg/tree/ops/xml_test.go` — migrated to `XMLRenderOpts` +- `pkg/tree/ops/xpath_test.go` — migrated to `XPathRenderOpts` +- `pkg/tree/api/adapter/intentresponse.go` — added `RenderOpts` field; all `To*` methods forward it; `ToXML`/`ToXPath` use typed opt structs +- `pkg/tree/api/adapter/entryoutputadapter.go` — southbound adapter wraps bools into structs; uses `IncludeSensitive: true` everywhere +- `pkg/tree/processors/processor_blame_config.go` — `BlameConfigProcessorParams` gains `IncludeSensitive`, `SensitivePathSet`; `BlameConfigTask.Run` redacts `Value`/`DeviationValue` +- `pkg/tree/processors/processor_blame_config_test.go` — `TestBlameConfigSensitiveRedaction` added +- `pkg/datastore/intent_rpc.go` — `GetIntent` accepts `exposeSensitive bool`; threads into `IntentResponseAdapter.RenderOpts` +- `pkg/datastore/datastore_rpc.go` — `BlameConfig` accepts `exposeSensitive bool`; threads into `BlameConfigProcessorParams` +- `pkg/datastore/sync.go` — southbound `ToProtoUpdates` call updated to `IncludeSensitive: true` +- `pkg/server/intent.go` — passes `req.GetIncludeSensitive()` to `ds.GetIntent` +- `pkg/server/datastore.go` — passes `req.GetIncludeSensitive()` to `ds.BlameConfig` +- `pkg/utils/testhelper/utils.go` — `GetSchemaClientBoundMarkingLeafSensitive` helper added +- `mocks/mocktreeentry/entry.go` — regenerated (stale `AddChild` → `AddOrGetChild`) + +--- + +## Issue 03 — Intent path-marker persistence + +**Status:** done + +**Files touched:** +- `sdc-protos/data.proto` — `TransactionIntent.sensitive_paths = 11` +- `sdc-protos/sdcpb/data.pb.go` — regenerated +- `pkg/datastore/types/transaction_intent.go` — `sensitivePaths []string` field; `SetSensitivePaths` / `GetSensitivePaths` accessors +- `pkg/datastore/transaction_rpc.go` — `validateSensitivePaths` (rejects key predicates, empty, slash-less paths); wired into `SdcpbTransactionIntentToInternalTI`; `protoIntent.SensitivePaths` set in `lowlevelTransactionSet` before `IntentModify` +- `pkg/datastore/transaction_rpc_test.go` — `TestTransactionSet_SensitivePathsPersisted` (round-trip tracer bullet); `TestSdcpbTransactionIntentToInternalTI_SensitivePaths` (key-predicate, empty, no-slash, valid-pass-through) + +--- + +## Issue 04 — Path-marker union at northbound render time + +**Status:** done + +**Files touched:** +- `pkg/tree/ops/sensitive.go` — `UnionSensitivePaths(perIntentPaths [][]string) map[string]bool` added +- `pkg/tree/ops/sensitive_test.go` — `TestUnionSensitivePaths` added (nil, empty, single, union, dedup cases) +- `pkg/datastore/transaction_rpc.go` — `LoadAllButRunningIntents` signature changed to `([]string, map[string]bool, error)`; collects `intent.GetSensitivePaths()` per intent and returns `ops.UnionSensitivePaths(...)` as second value +- `pkg/datastore/sync.go` — call site updated (`_, _, err`) +- `pkg/datastore/deviations.go` — call site updated (`addedIntentNames, _, err`) +- `pkg/datastore/datastore_rpc.go` — `BlameConfig` captures the union from `LoadAllButRunningIntents` and passes it as `BlameConfigProcessorParams.SensitivePathSet` +- `pkg/datastore/intent_rpc.go` — `loadSensitivePathSet(ctx)` helper added (streams all intents via `IntentGetAll`, collects sensitive paths, returns `ops.UnionSensitivePaths`); `GetIntent` calls it before rendering for both the running-intent and single-intent branches, populates `RenderOpts.SensitivePathSet` +- `pkg/datastore/sensitive_path_union_test.go` — new: `TestBlameConfig_CrossIntentSensitivePathRedaction`, `TestGetIntent_CrossIntentSensitivePathRedaction`, `TestGetIntent_NoSensitivePaths_NoRedaction`, `TestGetIntent_RunningIntent_CrossIntentRedaction` + +--- + +## Issue 05 — WatchDeviations masking + +**Status:** done + +**Files touched:** +- `pkg/tree/ops/sensitive.go` — `IsSensitiveSchemaElem(*sdcpb.SchemaElem) bool` added; `IsSensitive` refactored to call it +- `pkg/tree/ops/sensitive_test.go` — `TestIsSensitiveSchemaElem` added +- `pkg/datastore/deviations.go` — `calculateDeviations` signature changed to `(<-chan, map[string]bool, error)`; `DeviationMgr` captures `sensitivePathSet` and passes it to `SendDeviations`; `SendDeviations` gains `sensitivePathSet map[string]bool` parameter and masks `ExpectedValue`/`CurrentValue` with `***` when sensitive; `isSensitiveDeviation` helper added (checks path-marker set + schema flag) +- `pkg/datastore/deviations_test.go` — new: `fakeDeviationStream` test helper; `TestSendDeviations_PathMarkerSensitiveMasking`, `TestSendDeviations_SchemaFlagSensitiveMasking`, `TestSendDeviations_NonSensitiveNotMasked`, `TestWatchDeviations_SensitivePathMasking` (integration acceptance test) From ce872c005b13db497c6bf134decf3170a58b8aa7 Mon Sep 17 00:00:00 2001 From: steiler Date: Wed, 10 Jun 2026 11:19:28 +0200 Subject: [PATCH 4/4] Revert "docs(prd): add sensitive-data PRD and issue index" This reverts commit 70a7374a454987a7878abb8c030a49da5cec0c74. --- docs/prd/sensitive-data/PRD.md | 186 ------------------ .../sensitive-data/issues/01-external-deps.md | 44 ----- .../issues/02-schema-level-redaction.md | 122 ------------ .../03-intent-path-marker-persistence.md | 53 ----- .../issues/04-path-marker-union-at-render.md | 45 ----- .../issues/05-watchdeviations-masking.md | 47 ----- docs/prd/sensitive-data/issues/index.md | 107 ---------- 7 files changed, 604 deletions(-) delete mode 100644 docs/prd/sensitive-data/PRD.md delete mode 100644 docs/prd/sensitive-data/issues/01-external-deps.md delete mode 100644 docs/prd/sensitive-data/issues/02-schema-level-redaction.md delete mode 100644 docs/prd/sensitive-data/issues/03-intent-path-marker-persistence.md delete mode 100644 docs/prd/sensitive-data/issues/04-path-marker-union-at-render.md delete mode 100644 docs/prd/sensitive-data/issues/05-watchdeviations-masking.md delete mode 100644 docs/prd/sensitive-data/issues/index.md diff --git a/docs/prd/sensitive-data/PRD.md b/docs/prd/sensitive-data/PRD.md deleted file mode 100644 index c0d6d56d..00000000 --- a/docs/prd/sensitive-data/PRD.md +++ /dev/null @@ -1,186 +0,0 @@ -# PRD: Sensitive Data Support - -## Problem Statement - -Device configuration managed by data-server may contain sensitive values — passwords, authentication keys, community strings, bearer tokens — that must reach the southbound target unchanged but must never be returned in plaintext over the northbound API. Today every northbound response (`GetIntent`, `BlameConfig`) returns leaf values verbatim, with no mechanism to classify or redact sensitive fields. Operators cannot safely expose northbound reads to dashboards, audit tools, or less-privileged users without leaking secrets. - -## Solution - -Introduce a **sensitive-path marker** mechanism that: - -1. Lets the caller of `TransactionSet` declare which key-pruned schema paths in an intent contain sensitive values. -2. Stores those markers alongside the intent blob in `sdcio/cache`. -3. At northbound render time, unions all sensitive path sets across all intents and replaces matching leaf values with the fixed sentinel `***`. -4. Leaves the southbound path (tree merge, YANG validation, Target apply) entirely unchanged — plain values flow to the device as before. - -Sensitivity is a **property of the schema path**, not a list instance. List keys are stripped from markers (e.g. `/bgp/neighbors/auth-password`, not `/bgp/neighbors[peer=10.0.0.1]/auth-password`). This eliminates staleness when list keys change and treats all instances of a leaf type uniformly. - -The feature also defines an **admin bypass** — a per-request flag that privileged callers may set to receive plaintext values. The bypass is a single hook in the render pipeline, designed to be layered with stronger auth mechanisms (mTLS cert role) later without changing the redaction logic. - -## User Stories - -1. As a network operator, I want to mark specific leaf paths as sensitive when submitting an intent, so that northbound consumers never see plaintext passwords or secrets. -2. As a network operator, I want sensitivity to be a property of the schema path rather than a specific list instance, so that I do not have to re-declare sensitivity when list keys change. -3. As a network operator, I want `BlameConfig` to return `***` as the value of a sensitive leaf (with the owner intent still visible), so that I can see who controls the field without seeing the value. -4. As a network operator, I want `GetIntent` to return `***` for sensitive leaf values, so that exported intent data is safe to store in logs or hand to less-privileged users. -5. As a network operator, I want a sensitive marker from any intent to protect a leaf even when the highest-precedence value comes from a different intent that did not mark it sensitive, so that priority re-ordering cannot accidentally expose a secret. -6. As a network operator, I want deviation values for sensitive paths to also be masked in `WatchDeviations`, so that drift notifications do not leak plaintext secrets. -7. As a platform operator, I want an admin bypass flag on `GetIntentRequest` and `BlameConfigRequest` to retrieve plaintext values for debugging, so that authorised operators can inspect actual device configuration when needed. -8. As a platform operator, I want existing intents without sensitivity metadata to be treated as non-sensitive by default, so that the feature can be deployed without any migration. - -## Implementation Decisions - -### Sensitivity model - -Sensitivity markers are **key-pruned schema paths** stored as a `repeated string sensitive_paths` alongside the intent blob in `sdcio/cache`. The format strips list key predicates so `/bgp/neighbors[peer=10.0.0.1]/auth-password` is stored as `/bgp/neighbors/auth-password`. - -At northbound render time the system applies the sensitive-path union scoped to -the in-scope intents for that operation: - -- **`GetIntent(regular)`** — only the fetched intent's own `sensitive_paths` markers apply. -- **`GetIntent(running)`**, **`BlameConfig`**, **`WatchDeviations`** — the union of all - non-running intents' markers applies, because these operations present a merged or - device-reflected view. - -The union is computed in O(1) from an in-memory reverse index maintained on the -Datastore (see ADR 0004). The index is populated eagerly at Datastore startup and -updated incrementally on every intent write or delete. - -For the in-scope set the system: - -1. Looks up the sensitive-path union from the index (or the intent's own markers for a single-intent GET). -2. Strips list keys from each leaf's path during the tree walk and checks for membership in the set. -3. Returns `***` as the string value for any matching leaf, regardless of its actual type. - -The southbound path (merge, validate, apply) reads the intent blob directly and is unaware of the sensitive-path slice. - -### Cross-intent rule - -The cross-intent rule applies to **merged views** (`BlameConfig`, `WatchDeviations`, -`GetIntent(running)`): if any intent marks a path sensitive, the path is treated as -sensitive in those views even if the highest-precedence value comes from an intent -that did not mark it sensitive. - -For **`GetIntent(regular)`** the scope is narrowed to the fetched intent's own -markers. An intent is responsible for its own sensitivity declarations; another -intent's marker does not retroactively affect how a single-intent GET presents its -data. See ADR 0004 for the rationale and trade-offs. - -### "once sensitive → always sensitive" and the replace intent - -A `replace` intent wipes all prior intents. The replace intent must explicitly carry the sensitive paths it considers sensitive. Sensitive-path slices from deleted intents are removed along with their intents. The responsibility for re-declaring sensitive paths lies with the entity performing the replace. - -> **Open:** whether the `replace` intent should automatically inherit prior sensitive paths (to guard against accidental exposure during a replace operation) is left as a follow-up decision. - -### Running intent - -The running intent (device-reported state) is subject to the same obfuscation rule. If any intent marks a path sensitive, the running value for that path is also returned as `***` in northbound responses. This prevents `BlameConfig` from leaking plaintext via the device echo (relevant for NETCONF `GetConfig` which may return passwords verbatim). - -### Cache API extension - -The `sdcio/cache` library requires a new field per intent record to carry the sensitive-path slice. Two options: - -- **First-class field** on the cache intent record — cleaner, requires a library PR to `sdcio/cache` before data-server work can begin. -- **Opaque metadata blob** stored as a parallel key in the cache — keeps the change data-server-internal but is less ergonomic. - -> **Decision required** before implementation starts. - -### Canonical path format - -Key-pruned schema paths. Path encoding (gNMI with module prefixes vs internal tree path) to be decided before implementation — affects cache layout and all lookup code. - -### Northbound API changes - -Add `include_sensitive: bool` to `GetIntentRequest` and `BlameConfigRequest`. When `true` and the caller is authorised, plaintext values are returned. This is the admin bypass (see below). - -Sensitive leaf values in responses are returned as the string `***` regardless of the leaf's YANG type. The field is present in the response (not omitted) so consumers know the field exists and is redacted. - -### Admin bypass - -The initial bypass mechanism is `include_sensitive: bool` in the request proto. Trust model: whoever can reach the gRPC port is trusted (same as today — the gRPC channel is currently unauthenticated). The flag is explicit in the API contract and auditable in request logs. - -The bypass check is a single point in the render pipeline, before any redaction is applied, so that stronger auth (mTLS client cert role) can be layered on later without touching the redaction logic. - -**Future: mTLS client cert role.** When internal gRPC mTLS is implemented (currently a TODO in config-server and data-server), bypass authorisation can be tied to a client certificate `OU=sdcio-admin` issued from a dedicated sdcio internal CA. The existing `TLS*` fields in the client config structs are already present but unimplemented. The sdcio internal CA must be separate from the kube-api CA (different trust domains). - -### Module overview - -| Component | Change | Notes | -|---|---|---| -| `sdc-protos` | `bool sensitive = 22` on `LeafSchema`; `bool sensitive = 24` on `LeafListSchema` | Schema-server sets flag when `sdcio-ext:sensitive` extension is present on a leaf | -| `sdc-protos` | `bool include_sensitive = 4` on `GetIntentRequest`; `bool include_sensitive = 2` on `BlameConfigRequest` | Admin bypass flag | -| `sdc-protos` | `repeated string sensitive_paths = 8` on `tree_persist.Intent` | Key-pruned paths stored alongside intent blob; absence = non-sensitive (no migration) | -| `sdcio/goyang` | One-line fix in `ApplyDeviate` — copy `Exts` for `DeviationAdd`/`DeviationReplace` | Enables `sdcio-ext:sensitive` in per-device-profile overlay YANG files | -| `pkg/tree/ops/render_opts.go` | New `RenderOpts` / `XMLRenderOpts` / `XPathRenderOpts` structs | Replaces scattered `bool` params on render ops; holds `IncludeSensitive` + `SensitivePathSet` | -| `pkg/tree/ops/sensitive.go` | `IsSensitive(e)` — reads `LeafSchema.sensitive`; `KeyPrunedPath(e)` — `SdcpbPath().ToXPath(true)` | Core helpers used by all render ops and blame | -| `pkg/tree/ops/json.go`, `proto.go`, `xml.go`, `xpath.go` | Emit `"***"` / `TypedValue{StringVal:"***"}` at leaf emit point when redaction condition is met | `!opts.IncludeSensitive && (IsSensitive(e) \|\| SensitivePathSet[KeyPrunedPath(e)])` | -| `pkg/tree/api/adapter/intentresponse.go` | `IntentResponseAdapter` forwards `RenderOpts` to ops | Carries `IncludeSensitive` and `SensitivePathSet` from `GetIntent` request | -| `pkg/tree/processors/processor_blame_config.go` | `BlameConfigProcessorParams` gains `IncludeSensitive bool`; redact `Value` and `DeviationValue` | Same condition as render ops | -| `pkg/datastore/intent_rpc.go` | Union `sensitive_paths` across all loaded intents → `SensitivePathSet`; build `RenderOpts`; pass `IncludeSensitive` from request | For `GetIntent` render | -| `pkg/datastore/datastore_rpc.go` | Same union + `IncludeSensitive` forwarding for `BlameConfig` | | -| `pkg/datastore/transaction_rpc.go` | Persist `sensitive_paths` atomically with intent blob during `TransactionSet` | Must be atomic — no window where blob exists without slice | -| `pkg/server/intent.go` | Read `req.GetIncludeSensitive()`, thread into datastore call | | -| `pkg/server/datastore.go` | Same for `BlameConfig` | | -| `WatchDeviations` emission path | Mask deviation values for sensitive paths | Both intent value and running value — Issue 05 | - -### Transaction lifecycle - -The sensitive-path slice is written during `TransactionSet`, atomically with the intent blob. It is deleted when the intent is deleted. If the call to write the slice fails, the intent blob write must also be rolled back. - -### Migration - -Existing intents in cache have no sensitive-path slice. Absence of a slice is treated as non-sensitive. No migration needed. This behaviour must be explicitly documented to avoid future misinterpretation. - -## Alternative Approaches Considered - -### YANG extension annotation (Alt A) - -Define an `sdcio-sensitive` YANG extension module and annotate vendor YANG leaves using `deviate add { sdcio-sens:mark-sensitive; }` in per-device-profile overlay files. Schema-server exposes a `sensitive: bool` flag on `LeafSchema`. data-server checks the flag at NB render time. - -**goyang finding:** `deviate add` for extension statements parses without error in `sdcio/goyang v1.6.2-2` (the `Deviate` struct carries `Extensions []*Statement`) but `ApplyDeviate` in `entry.go` silently ignores extensions — they are never copied onto the target node. A one-line fix in the sdcio/goyang fork would enable this. - -This approach is complementary rather than competing. It sets a **static baseline** (leaf types always sensitive by schema definition) while the path-slice approach handles **dynamic operator control** at intent-submission time. Both resolve to the same check: "is this key-pruned path in the sensitive set?" - -**Options for sourcing Alt A sensitivity without modifying vendor YANG:** - -| Option | Mechanism | Scope | -|---|---|---| -| Patch sdcio/goyang | Fix `ApplyDeviate` to copy extensions; use `deviate add` in overlay YANG files | Device-profile | -| Sidecar profile file | YAML file per device-profile; schema-server reads at load time, exposes `GetSensitivePaths(profile)` RPC | Device-profile | -| Leaf-name heuristic | Schema-server auto-marks leaves named `password`, `secret`, `auth-key`, etc. | Global | -| DatastoreConfig YAML | `sensitive-paths: []string` in existing datastore config; data-server reads directly | Per-datastore | -| Drop Alt A | Path-slice only; static baseline via heuristic if needed | Per-intent | - -### Sensitive flag on LeafVariant (Alt B) - -Add `sensitive: bool` to the `LeafVariant` proto so the flag travels with the value through the tree. Zero path-staleness risk; natural aggregation as the flag reaches the winning leaf. Rejected for v1: requires proto changes in `sdc-protos` and updates to every importer, exporter, and blame code path — large blast radius. - -### Encrypt at rest (Alt C) - -Encrypt sensitive leaf values before storing in the intent blob; southbound decrypts on apply. Rejected: requires KMS integration, makes southbound stateful, and significantly increases scope. - -### Sentinel + external store (Alt D) - -Replace sensitive values with a sentinel in the blob; store real values in an external secrets store; southbound resolves on apply. Rejected: southbound breaks if the external store is unavailable; sentinels break YANG typed-value comparisons in tree merge. - -## Open Questions - -| Question | Status | -|---|---| -| sdcio/cache API extension: first-class field vs opaque sidecar? | **Resolved** — first-class `repeated string sensitive_paths = 8` on `tree_persist.Intent` (in sdc-protos); cache library stores opaque bytes unchanged | -| Canonical path format: gNMI with module prefixes or internal tree path? | **Resolved** — `SdcpbPath().ToXPath(true)` (noKeys=true); format `/bgp/neighbors/auth-password`; consistent with `must`/`when` deviation path syntax | -| Replace-intent sensitive marker inheritance: auto-inherit prior slices or require explicit re-declaration? | Open — deferred follow-up | -| Should Alt A (YANG extension / sidecar profile) be implemented alongside path-slice as a static baseline layer? | **Resolved for v1** — Alt A (goyang fix + `sdcio-ext:sensitive` + schema `sensitive` flag) is implemented as the static baseline layer (Issue 02); dynamic path-slice is the per-intent layer (Issues 03–04) | -| Admin bypass: when to layer mTLS cert role on top of the proto flag? | Deferred — depends on internal gRPC mTLS rollout | - -## Out of Scope - -**Encryption at rest.** Intent blobs are stored as plaintext in BadgerDB. Protecting secrets at rest requires KMS integration and is a separate infrastructure concern. - -**Per-instance sensitivity.** List keys are stripped from markers — all instances of a leaf type are treated uniformly. Marking only a specific list entry's leaf as sensitive is not supported. - -**Fine-grained access control.** The initial admin bypass trusts any caller that can reach the gRPC port. Role-based access control (mTLS cert role, token-based auth) is a follow-up. - -**Schema-level static baseline.** Alt A (YANG extension or sidecar profile) provides a schema-level complement to the path-slice approach. Implementing it is not part of this PRD but is designed to compose cleanly with it. - -**Audit log.** Recording which calls exercised the admin bypass is not part of this PRD. diff --git a/docs/prd/sensitive-data/issues/01-external-deps.md b/docs/prd/sensitive-data/issues/01-external-deps.md deleted file mode 100644 index 60383248..00000000 --- a/docs/prd/sensitive-data/issues/01-external-deps.md +++ /dev/null @@ -1,44 +0,0 @@ -# External dependencies: goyang fix + sdc-protos fields - -## Parent - -[PRD: Sensitive Data Support](../PRD.md) - -## What to build - -Three external PRs that unblock all in-repo implementation work. These can be raised and merged in parallel before any data-server changes land. - -**1. `sdcio/goyang` — fix `ApplyDeviate` extension propagation** - -`ApplyDeviate` silently drops extensions on `deviate add` / `deviate replace` nodes. A one-line fix copies the deviated node's extensions onto the target: - -```go -deviatedNode.Exts = append(deviatedNode.Exts, devSpec.Exts()...) -``` - -This makes `sdcio-ext:sensitive;` annotations in per-device-profile overlay YANG files survive parsing and appear on the parsed `yang.Entry`. - -**2. `sdc-protos` — `LeafSchema` and `LeafListSchema` sensitive flag** - -Add `bool sensitive = 22` to `LeafSchema` and `bool sensitive = 24` to `LeafListSchema`. Schema-server sets this flag when it detects the `sdcio-ext:sensitive` extension on a parsed leaf. data-server reads it at northbound render time via `ops.IsSensitive(e)`. - -**3. `sdc-protos` — admin bypass on northbound request messages** - -Add `bool include_sensitive = ` to `GetIntentRequest` and `BlameConfigRequest`. When `true` the render pipeline skips redaction and returns plaintext values. Trust model for v1: any caller that can reach the gRPC port (unchanged from today). - -**4. `sdcio/cache` — first-class `sensitive_paths` field on intent record** - -Add `repeated string sensitive_paths` to the `tree_persist.Intent` proto (or equivalent intent record type in the cache library). data-server will write this field atomically alongside the intent blob during `TransactionSet` and read it during northbound render. Field absence (existing intents) is treated as non-sensitive — no migration required. - -## Acceptance criteria - -- [ ] `sdcio/goyang`: `deviate add` with `sdcio-ext:sensitive;` survives `ApplyDeviate` and appears on the parsed `yang.Entry.Exts` -- [ ] `sdc-protos`: `LeafSchema.sensitive` and `LeafListSchema.sensitive` fields present and generated in Go bindings -- [ ] `sdc-protos`: `GetIntentRequest.include_sensitive` and `BlameConfigRequest.include_sensitive` fields present and generated in Go bindings -- [ ] `sdcio/cache`: intent record exposes `sensitive_paths []string`; absent field treated as empty slice by existing readers -- [ ] All four repos have passing CI on their respective PRs -- [ ] data-server `go.mod` updated to reference the new versions of all changed modules - -## Blocked by - -None — can start immediately. diff --git a/docs/prd/sensitive-data/issues/02-schema-level-redaction.md b/docs/prd/sensitive-data/issues/02-schema-level-redaction.md deleted file mode 100644 index 01b0fb83..00000000 --- a/docs/prd/sensitive-data/issues/02-schema-level-redaction.md +++ /dev/null @@ -1,122 +0,0 @@ -# Schema-level render redaction + admin bypass (Phase 1) - -## Parent - -[PRD: Sensitive Data Support](../PRD.md) - -## What to build - -Implement the schema-level (static baseline) redaction layer end-to-end: from schema flag detection through all northbound render paths, covering `GetIntent`, `BlameConfig`, and the admin bypass. - ---- - -### `ops.IsSensitive(e api.Entry) bool` - -New helper in `pkg/tree/ops`. Reads `LeafSchema.sensitive` / `LeafListSchema.sensitive` from the entry's schema via a type-switch on `e.GetSchema().GetSchema().(type)`. Returns `false` when schema is absent (non-leaf nodes, existing intents without schema flags). - ---- - -### `ops.KeyPrunedPath(e api.Entry) string` - -Returns `e.SdcpbPath().ToXPath(true)` — the existing `noKeys=true` argument already strips list key predicates. Format: `/bgp/neighbors/auth-password`. This is the canonical sensitive-path lookup key used both at render time and in stored `sensitive_paths` markers (Issue 03). - ---- - -### `RenderOpts` — struct hierarchy in `pkg/tree/ops/render_opts.go` - -Replace the current per-function boolean parameters with structured option types. The existing `onlyNewOrUpdated` bool is absorbed into `RenderOpts.OnlyNewOrUpdated`. - -```go -// ops/render_opts.go - -// RenderOpts are the common render options shared across all northbound -// render operations. -type RenderOpts struct { - OnlyNewOrUpdated bool - IncludeSensitive bool - // SensitivePathSet is the union of sensitive_paths slices read from all - // in-scope intents (populated in Issue 04). An empty/nil map means no - // path-marker-based redaction is applied (correct for schema-only Phase 1). - SensitivePathSet map[string]bool -} - -type XMLRenderOpts struct { - RenderOpts - HonorNamespace bool - OperationWithNamespace bool - UseOperationRemove bool -} - -type XPathRenderOpts struct { - RenderOpts - IncludeDefaults bool -} -``` - -New signatures: - -```go -ToJson(ctx, entry, RenderOpts) (any, error) -ToJsonIETF(ctx, entry, RenderOpts) (any, error) -ToProtoUpdates(ctx, entry, RenderOpts) ([]*sdcpb.Update, error) -ToXML(ctx, entry, XMLRenderOpts) (*etree.Document, error) -ToXPath(ctx, entry, XPathRenderOpts) (*sdcpb.PathValues, error) -``` - -Call sites to update (all currently in the same package or one hop away): -- `pkg/tree/api/adapter/intentresponse.go` — `IntentResponseAdapter.ToJson`, `.ToJsonIETF`, `.ToXML`, `.ToProtoUpdates`, `.ToXPath` (all currently hardcode `false` for `onlyNewOrUpdated`) -- `pkg/tree/ops/xml_test.go`, `json_test.go`, `xpath_test.go` — test structs use named bool params, migrate to `RenderOpts{...}` / `XMLRenderOpts{...}` / `XPathRenderOpts{...}` -- `pkg/datastore/intent_rpc.go` — calls ops indirectly via the adapter; will thread `IncludeSensitive` here in step 7 - ---- - -### Redaction condition - -At the leaf/leaf-list emit point in each render op: - -```go -!opts.IncludeSensitive && (ops.IsSensitive(e) || opts.SensitivePathSet[ops.KeyPrunedPath(e)]) -``` - -Redacted values are emitted as: -- JSON/XML: string `"***"` -- Proto (`TypedValue`): `&sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: "***"}}` -- XPath (`PathValues`): same `TypedValue` sentinel - -The field is **always present** in the response — never omitted. - ---- - -### `processor_blame_config.go` - -Add `IncludeSensitive bool` to `BlameConfigProcessorParams`. Apply the same redaction condition at both `t.self.SetValue(...)` and `t.self.SetDeviationValue(...)` assignments in `BlameConfigTask.Run`. - ---- - -### Threading `include_sensitive` from request to render - -| Layer | File | Change | -|---|---|---| -| gRPC handler | `pkg/server/intent.go` | Read `req.GetIncludeSensitive()`, pass to `ds.GetIntent(ctx, name, exposeSensitive)` | -| gRPC handler | `pkg/server/datastore.go` | Read `req.GetIncludeSensitive()`, pass to `ds.BlameConfig(ctx, req)` | -| Datastore | `pkg/datastore/intent_rpc.go` | Accept `exposeSensitive bool`, build `RenderOpts{IncludeSensitive: exposeSensitive}`, pass to adapter | -| Datastore | `pkg/datastore/datastore_rpc.go` | Same — pass into `BlameConfigProcessorParams` | - ---- - -## Acceptance criteria - -- [ ] `ops.IsSensitive(e)` returns `true` for a leaf whose schema has `sensitive = true`; `false` for all other entries -- [ ] `ops.KeyPrunedPath(e)` returns a keyless XPath string (no `[key=value]` predicates); verified by unit test -- [ ] All of `ToJson`, `ToJsonIETF`, `ToProtoUpdates`, `ToXML`, `ToXPath` emit `***` for a sensitive leaf when `IncludeSensitive = false` -- [ ] Same render ops return the real value when `IncludeSensitive = true` -- [ ] `BlameConfig` response: `Value` and `DeviationValue` for a sensitive leaf are `***` when `include_sensitive` not set -- [ ] `GetIntent` response: sensitive leaf values are `***` when `include_sensitive` not set -- [ ] `GetIntent` and `BlameConfig` with `include_sensitive = true` return plaintext values -- [ ] Southbound path (tree merge, YANG validation, target apply) is unaffected — no schema flag checks added there -- [ ] Existing intents without `sensitive` schema flags behave identically to today -- [ ] Integration test: submit an intent with a schema-marked-sensitive leaf; verify `GetIntent` returns `***`; verify with `include_sensitive = true` returns plaintext - -## Blocked by - -Issue 01 (external deps) — needs `LeafSchema.sensitive`, `LeafListSchema.sensitive`, and `include_sensitive` proto fields. ✅ Done. diff --git a/docs/prd/sensitive-data/issues/03-intent-path-marker-persistence.md b/docs/prd/sensitive-data/issues/03-intent-path-marker-persistence.md deleted file mode 100644 index 3278b7ad..00000000 --- a/docs/prd/sensitive-data/issues/03-intent-path-marker-persistence.md +++ /dev/null @@ -1,53 +0,0 @@ -# Intent path-marker persistence (Phase 2) - -## Parent - -[PRD: Sensitive Data Support](../PRD.md) - -## What to build - -Extend `TransactionSet` to accept, validate, and atomically persist a `sensitive_paths` slice alongside the intent blob in `sdcio/cache`. This is the write side of Phase 2 — the read side (union at render time) is a separate issue. - -**API change** - -`TransactionSetRequest.Intent` gains `repeated string sensitive_paths`. Callers submit keyless XPath strings (e.g. `/bgp/neighbors/auth-password`). Paths with key predicates (e.g. `/bgp/neighbors[peer=10.0.0.1]/auth-password`) are rejected with a descriptive `InvalidArgument` gRPC error — callers must strip keys before submitting. - -**Validation in `pkg/datastore/transaction_rpc.go`** - -Before any write: -- Parse each path in `sensitive_paths` -- Reject (error) if any path element contains key predicates -- Reject if a path is empty or malformed - -**Atomic persistence** - -`cacheClient.IntentModify` (or equivalent) must write the `sensitive_paths` slice to the cache intent record's `sensitive_paths` field atomically with the intent blob. If the cache write fails, the intent blob write is also rolled back — there must be no window where the blob exists without its slice, or vice versa. - -**Intent deletion** - -When an intent is deleted (`IntentDelete`), the `sensitive_paths` stored with it are removed along with the blob. No tombstone or residual marker is left. - -**Replace intent semantics** - -A `replace` intent wipes all prior intents for the datastore. The replacing intent must explicitly supply the `sensitive_paths` it considers sensitive. Sensitive-path slices from deleted intents are removed along with those intents. The responsibility for re-declaring sensitive paths lies with the entity performing the replace. This behaviour must be documented in code and in operator-facing docs. - -**Missing slice = non-sensitive** - -Existing intents in cache have no `sensitive_paths` slice (field absent). Absence is treated as an empty slice — non-sensitive. No migration is required. This must be explicitly documented to avoid future misinterpretation. - -## Acceptance criteria - -- [ ] `TransactionSetRequest.Intent.sensitive_paths` is wired from proto through `types.TransactionIntent` to the cache write -- [ ] Paths containing key predicates return `InvalidArgument` with a message identifying the offending path -- [ ] Malformed or empty paths return `InvalidArgument` -- [ ] Valid `sensitive_paths` are stored in the cache intent record and round-trip correctly through `IntentGet` -- [ ] If the cache write fails mid-transaction, the intent blob is rolled back (no partial state) -- [ ] Intent deletion removes the stored `sensitive_paths` slice -- [ ] A replace intent clears prior intents' slices; only the replacing intent's slice remains -- [ ] Existing intents without a slice are readable and treated as non-sensitive (no migration, no crash) -- [ ] Unit test: submit intent with valid sensitive paths, read back via cache, assert round-trip -- [ ] Unit test: submit intent with key-predicate path, assert `InvalidArgument` - -## Blocked by - -Issue 01 (external deps) — needs `sdcio/cache` first-class `sensitive_paths` field and `sdc-protos` `TransactionSetRequest.Intent.sensitive_paths`. diff --git a/docs/prd/sensitive-data/issues/04-path-marker-union-at-render.md b/docs/prd/sensitive-data/issues/04-path-marker-union-at-render.md deleted file mode 100644 index 93444ed9..00000000 --- a/docs/prd/sensitive-data/issues/04-path-marker-union-at-render.md +++ /dev/null @@ -1,45 +0,0 @@ -# Path-marker union at northbound render time (Phase 2) - -## Parent - -[PRD: Sensitive Data Support](../PRD.md) - -## What to build - -Wire the per-intent `sensitive_paths` slices stored in `sdcio/cache` into the northbound render pipeline. At render time, all intents' slices are loaded and unioned into `RenderOpts.SensitivePathSet`, which the render ops (added in issue 02) already check. - -**Union logic** - -A helper (e.g. `ops.LoadSensitivePathSet(intents []CacheIntent) map[string]bool`) iterates all loaded intent records, reads each `sensitive_paths` slice, and populates a `map[string]bool`. Key format is keyless XPath (`/bgp/neighbors/auth-password`), consistent with what was stored in issue 03 and what `ops.KeyPrunedPath(e)` produces at render time. - -**`GetIntent` path (`pkg/datastore/intent_rpc.go`)** - -After loading the intent(s) from cache, call the union helper over all intents for the datastore, populate `RenderOpts.SensitivePathSet`, and pass the opts into `IntentResponseAdapter` / the ops render call. The existing `IncludeSensitive` field on `RenderOpts` is also set from the request flag (wired in issue 02). - -**`BlameConfig` path (`pkg/datastore/datastore_rpc.go`)** - -The blame processor already loads all intents to compute the merged view. Before invoking `processor_blame_config`, build the union set from the loaded intents and pass it into the processor's render context. The blame processor already checks `SensitivePathSet` (wired in issue 02). - -**Cross-intent rule** - -If *any* intent marks a path sensitive, all northbound views redact that path — even if the highest-precedence value comes from an intent that did not mark it sensitive. This is enforced naturally by the union: any intent contributing `true` for a path key keeps it in the set. - -**Running intent** - -The running intent (device-reported state, loaded from the `running` cache entry) is subject to the same obfuscation rule. Include the running entry when building the union if it carries a `sensitive_paths` slice; in practice the running entry will not declare sensitive paths, but the render-time check against the unioned set already covers it. - -## Acceptance criteria - -- [ ] `GetIntent`: if any intent for the datastore has `/foo/bar` in its `sensitive_paths`, `/foo/bar` is redacted in the `GetIntent` response regardless of which intent holds the winning value -- [ ] `BlameConfig`: same cross-intent rule applies; `Value` and `DeviationValue` for a path in any intent's slice are `***` -- [ ] `GetIntent` with `include_sensitive = true` returns plaintext even when path is in a stored slice -- [ ] `BlameConfig` with `include_sensitive = true` same -- [ ] Intent with no `sensitive_paths` slice (existing intents) contributes nothing to the union — behaviour unchanged -- [ ] Running intent value for a path marked sensitive by any other intent is also redacted -- [ ] Unit test: two intents, only one marks a path sensitive; verify the path is redacted in both `GetIntent` and `BlameConfig` -- [ ] Integration test: replace intent that omits a previously-sensitive path; verify the path is no longer redacted after replace - -## Blocked by - -Issue 02 (schema-level redaction) — `RenderOpts.SensitivePathSet` and `ops.KeyPrunedPath` must exist. -Issue 03 (path-marker persistence) — `sensitive_paths` must be stored and retrievable from cache. diff --git a/docs/prd/sensitive-data/issues/05-watchdeviations-masking.md b/docs/prd/sensitive-data/issues/05-watchdeviations-masking.md deleted file mode 100644 index c78e1acd..00000000 --- a/docs/prd/sensitive-data/issues/05-watchdeviations-masking.md +++ /dev/null @@ -1,47 +0,0 @@ -# WatchDeviations sensitive-path masking - -## Parent - -[PRD: Sensitive Data Support](../PRD.md) - -## What to build - -Mask sensitive leaf values in the `WatchDeviations` stream. Both `ExpectedValue` and `CurrentValue` in each streamed `WatchDeviationResponse` must be replaced with `***` when the leaf's path is in the sensitive set — whether that sensitivity comes from the schema flag (`IsSensitive`) or from any intent's stored `sensitive_paths` slice. - -**Where the change lives** - -The deviation manager loop (`pkg/datastore/deviations.go`, `DeviationMgr`) deep-copies the sync tree, loads all intents, calls `ops.GetDeviations`, then streams results via `SendDeviations`. The sensitive-path union must be built once per deviation cycle (not per streamed message) and passed into both the deviation detection walk and the emit step. - -**Union building** - -Reuse the same `ops.LoadSensitivePathSet` helper introduced in issue 04. Load it from the same intent set that `DeviationMgr` already loads. Schema-level sensitivity (`IsSensitive`) is checked inline at the leaf level, same as in the render ops. - -**Masking at emit** - -In `SendDeviations`, before writing `ExpectedValue` and `CurrentValue` into the response message, apply: - -``` -if !exposeSensitive && (ops.IsSensitive(leaf) || sensitivePathSet[ops.KeyPrunedPath(leaf)]) { - expectedValue = "***" - currentValue = "***" -} -``` - -`WatchDeviationsRequest` does not currently have an `include_sensitive` flag. Whether to add one is a decision for this issue — the PRD defers it but the bypass hook is already designed to be a single point in the pipeline. - -**No `include_sensitive` on `WatchDeviationsRequest` in v1** - -Streaming bypass is a distinct auth problem (the stream runs continuously; the caller's privilege level needs to be checked per-message or at connection time). For v1, always redact and leave the bypass as a follow-up when mTLS cert roles are implemented. - -## Acceptance criteria - -- [ ] `WatchDeviationsResponse`: `ExpectedValue` and `CurrentValue` are `***` for any leaf whose path is sensitive (schema flag or any intent's stored slice) -- [ ] Non-sensitive leaves are unaffected -- [ ] The union is built once per deviation cycle, not per leaf or per message -- [ ] Schema-flag sensitivity (`IsSensitive`) and path-slice sensitivity (`SensitivePathSet`) are both checked -- [ ] No `include_sensitive` bypass added to `WatchDeviationsRequest` in this issue (documented as follow-up) -- [ ] Integration test: intent marks `/foo/bar` sensitive; running value differs from expected; verify both deviation values are `***` in the stream - -## Blocked by - -Issue 04 (path-marker union at render time) — `ops.LoadSensitivePathSet` and `ops.KeyPrunedPath` must exist; `IsSensitive` already exists from issue 02. diff --git a/docs/prd/sensitive-data/issues/index.md b/docs/prd/sensitive-data/issues/index.md deleted file mode 100644 index 650b7fa4..00000000 --- a/docs/prd/sensitive-data/issues/index.md +++ /dev/null @@ -1,107 +0,0 @@ -# Sensitive Data Support — Issue Index - -## Issues - -| # | Title | Status | Blocked by | -|---|---|---|---| -| [01](./01-external-deps.md) | External deps: goyang fix + sdc-protos + cache field | done | — | -| [02](./02-schema-level-redaction.md) | Schema-level render redaction + admin bypass | done | 01 | -| [03](./03-intent-path-marker-persistence.md) | Intent path-marker persistence | done | 01 | -| [04](./04-path-marker-union-at-render.md) | Path-marker union at northbound render time | done | 02, 03 | -| [05](./05-watchdeviations-masking.md) | WatchDeviations sensitive-path masking | done | 04 | - ---- - -## Issue 01 — External deps - -**Status:** done - -**PRs:** -- [sdcio/goyang#4](https://github.com/sdcio/goyang/pull/4) — `fix-apply-deviate-exts-replace` -- [sdcio/sdc-protos#123](https://github.com/sdcio/sdc-protos/pull/123) — `feat-sensitive-paths-schema-path` - -**Files touched:** -- `goyang/pkg/yang/entry.go` — fix `ApplyDeviate`: `DeviationAdd` appends `Exts`, `DeviationReplace` replaces them (was incorrectly appending for both); mirrors existing `Default` branching -- `goyang/pkg/yang/entry_test.go` — new `TestDeviation` case `"deviate replace replaces existing extensions, does not accumulate"` in addition to the two propagation cases -- `sdc-protos/schema.proto` — `LeafListSchema.sensitive = 24`, `LeafSchema.sensitive = 22` -- `sdc-protos/data.proto` — `GetIntentRequest.include_sensitive = 4`, `BlameConfigRequest.include_sensitive = 2`; `TransactionIntent.sensitive_paths: repeated schema.Path = 11` -- `sdc-protos/tree_persist.proto` — `Intent.sensitive_paths: repeated schema.Path = 8` (was `repeated string`) -- `sdc-protos/sdcpb/schema.pb.go`, `sdc-protos/sdcpb/data.pb.go`, `sdc-protos/tree_persist/tree_persist.pb.go` — regenerated -- `sdc-protos/tree_persist/tree_persist_sensitive_test.go` — round-trip + backward-compat tests -- `data-server/go.mod` — local replace directives (update to tagged PRs after merge) - ---- - -## Issue 02 — Schema-level render redaction + admin bypass - -**Status:** done - -**Design decisions recorded in issue doc:** -- `RenderOpts` / `XMLRenderOpts` / `XPathRenderOpts` struct hierarchy in `pkg/tree/ops/render_opts.go` -- `SensitivePathSet map[string]bool` in `RenderOpts` — populated from per-intent `sensitive_paths` union (Issue 04); nil/empty = schema-flag-only redaction (Phase 1) -- `KeyPrunedPath(e)` = `e.SdcpbPath().ToXPath(true)` — reuses existing `noKeys` parameter -- Callers to update: `IntentResponseAdapter`, all ops test files, datastore intent/blame RPC layers, server handlers - -**Files touched:** -- `pkg/tree/ops/sensitive.go` — new: `IsSensitive`, `KeyPrunedPath` -- `pkg/tree/ops/sensitive_test.go` — new: unit tests for above -- `pkg/tree/ops/render_opts.go` — new: `RenderOpts`, `XMLRenderOpts`, `XPathRenderOpts` -- `pkg/tree/ops/json.go` — `ToJson`/`ToJsonIETF` migrated to `RenderOpts`; redaction added at leaf emit -- `pkg/tree/ops/xml.go` — `ToXML` migrated to `XMLRenderOpts`; redaction added at leaf emit -- `pkg/tree/ops/proto.go` — `ToProtoUpdates` migrated to `RenderOpts`; redaction added per leaf-entry -- `pkg/tree/ops/xpath.go` — `ToXPath` migrated to `XPathRenderOpts`; redaction added per leaf-entry -- `pkg/tree/ops/json_test.go` — migrated to `RenderOpts`; `TestToJsonSensitiveRedaction` added -- `pkg/tree/ops/xml_test.go` — migrated to `XMLRenderOpts` -- `pkg/tree/ops/xpath_test.go` — migrated to `XPathRenderOpts` -- `pkg/tree/api/adapter/intentresponse.go` — added `RenderOpts` field; all `To*` methods forward it; `ToXML`/`ToXPath` use typed opt structs -- `pkg/tree/api/adapter/entryoutputadapter.go` — southbound adapter wraps bools into structs; uses `IncludeSensitive: true` everywhere -- `pkg/tree/processors/processor_blame_config.go` — `BlameConfigProcessorParams` gains `IncludeSensitive`, `SensitivePathSet`; `BlameConfigTask.Run` redacts `Value`/`DeviationValue` -- `pkg/tree/processors/processor_blame_config_test.go` — `TestBlameConfigSensitiveRedaction` added -- `pkg/datastore/intent_rpc.go` — `GetIntent` accepts `exposeSensitive bool`; threads into `IntentResponseAdapter.RenderOpts` -- `pkg/datastore/datastore_rpc.go` — `BlameConfig` accepts `exposeSensitive bool`; threads into `BlameConfigProcessorParams` -- `pkg/datastore/sync.go` — southbound `ToProtoUpdates` call updated to `IncludeSensitive: true` -- `pkg/server/intent.go` — passes `req.GetIncludeSensitive()` to `ds.GetIntent` -- `pkg/server/datastore.go` — passes `req.GetIncludeSensitive()` to `ds.BlameConfig` -- `pkg/utils/testhelper/utils.go` — `GetSchemaClientBoundMarkingLeafSensitive` helper added -- `mocks/mocktreeentry/entry.go` — regenerated (stale `AddChild` → `AddOrGetChild`) - ---- - -## Issue 03 — Intent path-marker persistence - -**Status:** done - -**Files touched:** -- `sdc-protos/data.proto` — `TransactionIntent.sensitive_paths = 11` -- `sdc-protos/sdcpb/data.pb.go` — regenerated -- `pkg/datastore/types/transaction_intent.go` — `sensitivePaths []string` field; `SetSensitivePaths` / `GetSensitivePaths` accessors -- `pkg/datastore/transaction_rpc.go` — `validateSensitivePaths` (rejects key predicates, empty, slash-less paths); wired into `SdcpbTransactionIntentToInternalTI`; `protoIntent.SensitivePaths` set in `lowlevelTransactionSet` before `IntentModify` -- `pkg/datastore/transaction_rpc_test.go` — `TestTransactionSet_SensitivePathsPersisted` (round-trip tracer bullet); `TestSdcpbTransactionIntentToInternalTI_SensitivePaths` (key-predicate, empty, no-slash, valid-pass-through) - ---- - -## Issue 04 — Path-marker union at northbound render time - -**Status:** done - -**Files touched:** -- `pkg/tree/ops/sensitive.go` — `UnionSensitivePaths(perIntentPaths [][]string) map[string]bool` added -- `pkg/tree/ops/sensitive_test.go` — `TestUnionSensitivePaths` added (nil, empty, single, union, dedup cases) -- `pkg/datastore/transaction_rpc.go` — `LoadAllButRunningIntents` signature changed to `([]string, map[string]bool, error)`; collects `intent.GetSensitivePaths()` per intent and returns `ops.UnionSensitivePaths(...)` as second value -- `pkg/datastore/sync.go` — call site updated (`_, _, err`) -- `pkg/datastore/deviations.go` — call site updated (`addedIntentNames, _, err`) -- `pkg/datastore/datastore_rpc.go` — `BlameConfig` captures the union from `LoadAllButRunningIntents` and passes it as `BlameConfigProcessorParams.SensitivePathSet` -- `pkg/datastore/intent_rpc.go` — `loadSensitivePathSet(ctx)` helper added (streams all intents via `IntentGetAll`, collects sensitive paths, returns `ops.UnionSensitivePaths`); `GetIntent` calls it before rendering for both the running-intent and single-intent branches, populates `RenderOpts.SensitivePathSet` -- `pkg/datastore/sensitive_path_union_test.go` — new: `TestBlameConfig_CrossIntentSensitivePathRedaction`, `TestGetIntent_CrossIntentSensitivePathRedaction`, `TestGetIntent_NoSensitivePaths_NoRedaction`, `TestGetIntent_RunningIntent_CrossIntentRedaction` - ---- - -## Issue 05 — WatchDeviations masking - -**Status:** done - -**Files touched:** -- `pkg/tree/ops/sensitive.go` — `IsSensitiveSchemaElem(*sdcpb.SchemaElem) bool` added; `IsSensitive` refactored to call it -- `pkg/tree/ops/sensitive_test.go` — `TestIsSensitiveSchemaElem` added -- `pkg/datastore/deviations.go` — `calculateDeviations` signature changed to `(<-chan, map[string]bool, error)`; `DeviationMgr` captures `sensitivePathSet` and passes it to `SendDeviations`; `SendDeviations` gains `sensitivePathSet map[string]bool` parameter and masks `ExpectedValue`/`CurrentValue` with `***` when sensitive; `isSensitiveDeviation` helper added (checks path-marker set + schema flag) -- `pkg/datastore/deviations_test.go` — new: `fakeDeviationStream` test helper; `TestSendDeviations_PathMarkerSensitiveMasking`, `TestSendDeviations_SchemaFlagSensitiveMasking`, `TestSendDeviations_NonSensitiveNotMasked`, `TestWatchDeviations_SensitivePathMasking` (integration acceptance test)