Skip to content

Commit

Permalink
Better hygiene on asset group members endpoint (#137)
Browse files Browse the repository at this point in the history 
* better hygeine for type conversion

* order of operations: handle negative case first

* Use node properties helper to clean up code a bit

* fix: rework AGI membership logic to account for more edge cases
fix: skip AG membership for damaged nodes

* fix: existing agi tests patched

* chore: additional test conditions

---------

Co-authored-by: James Barnett <[email protected]>
Co-authored-by: Alyx Holms <[email protected]>
  • Loading branch information
@brandonshearin @juggernot325 @superlinkx
3 people authored Oct 18, 2023
1 parent 026ca83 commit 3179fa2
Show file tree
Hide file tree
Showing 3 changed files with 153 additions and 36 deletions.
52 changes: 44 additions & 8 deletions cmd/api/src/api/v2/agi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/specterops/bloodhound/graphschema/azure"
"github.com/specterops/bloodhound/graphschema/common"
"github.com/specterops/bloodhound/headers"
"github.com/specterops/bloodhound/log"
"github.com/specterops/bloodhound/slices"
"github.com/specterops/bloodhound/src/api"
"github.com/specterops/bloodhound/src/ctx"
Expand Down Expand Up @@ -434,27 +435,62 @@ func parseAGMembersFromNodes(nodes graph.NodeSet, selectors model.AssetGroupSele
isCustomMember := false
// a member is custom if at least one selector exists for that object ID
for _, agSelector := range selectors {
if agSelector.Selector == node.Properties.Map[common.ObjectID.String()].(string) {
if objectId, err := node.Properties.Get(common.ObjectID.String()).String(); err != nil {
log.Warnf("objectid is missing for node %d", node.ID)
} else if agSelector.Selector == objectId {
isCustomMember = true
}
}

var (
memberObjectId string
memberName string
)

if objectId, err := node.Properties.Get(common.ObjectID.String()).String(); err != nil {
log.Warnf("objectid is missing for node %d", node.ID)
memberObjectId = ""
} else {
memberObjectId = objectId
}

if name, err := node.Properties.Get(common.Name.String()).String(); err != nil {
log.Warnf("name is missing for node %d", node.ID)
memberName = ""
} else {
memberName = name
}

agMember := api.AssetGroupMember{
AssetGroupID: assetGroupID,
ObjectID: node.Properties.Map[common.ObjectID.String()].(string),
ObjectID: memberObjectId,
PrimaryKind: analysis.GetNodeKindDisplayLabel(node),
Kinds: node.Kinds.Strings(),
Name: node.Properties.Map[common.Name.String()].(string),
Name: memberName,
CustomMember: isCustomMember,
}

if tenantID := node.Properties.Map[azure.TenantID.String()]; tenantID != nil {
agMember.EnvironmentID = tenantID.(string)
agMember.EnvironmentKind = azure.Tenant.String()
if node.Kinds.ContainsOneOf(azure.Entity) {
if tenantID, err := node.Properties.Get(azure.TenantID.String()).String(); err != nil {
log.Warnf("%s is missing for node %d, skipping AG Membership...", azure.TenantID.String(), node.ID)
continue
} else {
agMember.EnvironmentKind = azure.Tenant.String()
agMember.EnvironmentID = tenantID
}
} else if node.Kinds.ContainsOneOf(ad.Entity) {
if domainSID, err := node.Properties.Get(ad.DomainSID.String()).String(); err != nil {
log.Warnf("%s is missing for node %d, skipping AG Membership...", ad.DomainSID.String(), node.ID)
continue
} else {
agMember.EnvironmentKind = ad.Domain.String()
agMember.EnvironmentID = domainSID
}
} else {
agMember.EnvironmentID = node.Properties.Map[ad.DomainSID.String()].(string)
agMember.EnvironmentKind = ad.Domain.String()
log.Warnf("Node %d is missing valid base entity, skipping AG Membership...", node.ID)
continue
}

agMembers = append(agMembers, agMember)
}
return agMembers
Expand Down
50 changes: 40 additions & 10 deletions cmd/api/src/api/v2/agi_internal_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
// Copyright 2023 Specter Ops, Inc.
//
//
// Licensed under the Apache License, Version 2.0
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//
// http://www.apache.org/licenses/LICENSE-2.0
//
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
//
// SPDX-License-Identifier: Apache-2.0

package v2
Expand All @@ -20,13 +20,13 @@ import (
"net/http"
"testing"

"github.com/specterops/bloodhound/src/api"
"github.com/specterops/bloodhound/src/model"
"github.com/stretchr/testify/require"
"github.com/specterops/bloodhound/dawgs/graph"
"github.com/specterops/bloodhound/graphschema/ad"
"github.com/specterops/bloodhound/graphschema/azure"
"github.com/specterops/bloodhound/graphschema/common"
"github.com/specterops/bloodhound/src/api"
"github.com/specterops/bloodhound/src/model"
"github.com/stretchr/testify/require"
)

func TestGetLatestQueryParameter_LatestMalformed(t *testing.T) {
Expand Down Expand Up @@ -55,21 +55,21 @@ func TestParseAGMembersFromNodes_(t *testing.T) {
nodes := graph.NodeSet{
1: &graph.Node{
ID: 1,
Kinds: graph.Kinds{ad.Domain},
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "a", common.Name.String(): "a", ad.DomainSID.String(): "a"},
},
},
2: &graph.Node{
ID: 2,
Kinds: graph.Kinds{azure.Tenant},
Kinds: graph.Kinds{azure.Entity, azure.Tenant},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "b", common.Name.String(): "b", azure.TenantID.String(): "b"},
},
},
3: &graph.Node{
ID: 3,
Kinds: graph.Kinds{ad.Domain},
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "c", common.Name.String(): "c", ad.DomainSID.String(): "c"},
},
Expand Down Expand Up @@ -97,3 +97,33 @@ func TestParseAGMembersFromNodes_(t *testing.T) {
require.Equal(t, customMembersExpected, customMembersFound)
require.Equal(t, azureNodesExpected, azureNodesFound)
}

func TestParseAGMembersFromNodes_MissingNodeProperties(t *testing.T) {
nodes := graph.NodeSet{
// the parse fn should handle nodes with missing name and missing properties with warnings and no output
1: &graph.Node{
ID: 1,
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{},
},
},
2: &graph.Node{
ID: 2,
Kinds: graph.Kinds{azure.Entity, azure.Tenant},
Properties: &graph.Properties{
Map: map[string]any{},
},
},
}

members := parseAGMembersFromNodes(nodes,
model.AssetGroupSelectors{model.AssetGroupSelector{
AssetGroupID: 1,
Name: "a",
Selector: "a",
SystemSelector: false,
}}, 1)

require.Equal(t, 0, len(members))
}
87 changes: 69 additions & 18 deletions cmd/api/src/api/v2/agi_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
// Copyright 2023 Specter Ops, Inc.
//
//
// Licensed under the Apache License, Version 2.0
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//
// http://www.apache.org/licenses/LICENSE-2.0
//
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
//
// SPDX-License-Identifier: Apache-2.0

package v2_test
Expand All @@ -24,6 +24,12 @@ import (
"net/url"
"testing"

"github.com/gorilla/mux"
"github.com/specterops/bloodhound/dawgs/graph"
"github.com/specterops/bloodhound/errors"
"github.com/specterops/bloodhound/graphschema/ad"
"github.com/specterops/bloodhound/graphschema/azure"
"github.com/specterops/bloodhound/graphschema/common"
"github.com/specterops/bloodhound/src/api"
v2 "github.com/specterops/bloodhound/src/api/v2"
"github.com/specterops/bloodhound/src/api/v2/apitest"
Expand All @@ -32,13 +38,8 @@ import (
"github.com/specterops/bloodhound/src/model"
queriesMocks "github.com/specterops/bloodhound/src/queries/mocks"
"github.com/specterops/bloodhound/src/utils/test"
"github.com/gorilla/mux"
"github.com/stretchr/testify/require"
"go.uber.org/mock/gomock"
"github.com/specterops/bloodhound/dawgs/graph"
"github.com/specterops/bloodhound/errors"
"github.com/specterops/bloodhound/graphschema/ad"
"github.com/specterops/bloodhound/graphschema/common"
)

func TestCreateAssetGroupRequest_AuditData(t *testing.T) {
Expand Down Expand Up @@ -884,7 +885,7 @@ func TestResources_ListAssetGroupMembers(t *testing.T) {
},
},
{
Name: "SuccessDataTest",
Name: "Node missing base entity kind",
Input: func(input *apitest.Input) {
apitest.SetURLVar(input, api.URIPathVariableAssetGroupID, "1")
},
Expand All @@ -897,14 +898,64 @@ func TestResources_ListAssetGroupMembers(t *testing.T) {
Return(graph.NodeSet{
1: &graph.Node{
ID: 1,
Kinds: graph.Kinds{ad.Domain},
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "a", common.Name.String(): "a", ad.DomainSID.String(): "a"},
},
},
2: &graph.Node{
ID: 2,
Kinds: graph.Kinds{azure.Entity, azure.Tenant},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "b", common.Name.String(): "b", azure.TenantID.String(): "b"},
},
},
3: &graph.Node{
ID: 3,
Kinds: graph.Kinds{ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "c", common.Name.String(): "c", ad.DomainSID.String(): "c"},
},
},
4: &graph.Node{
ID: 4,
Kinds: graph.Kinds{azure.Tenant},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "d", common.Name.String(): "d", azure.TenantID.String(): "d"},
},
},
}, nil)
},
Test: func(output apitest.Output) {
apitest.StatusCode(output, http.StatusOK)
result := api.ListAssetGroupMembersResponse{}
apitest.UnmarshalData(output, &result)
apitest.BodyContains(output, `"custom_member":true`)
apitest.Equal(output, 2, len(result.Members))
},
},
{
Name: "SuccessDataTest",
Input: func(input *apitest.Input) {
apitest.SetURLVar(input, api.URIPathVariableAssetGroupID, "1")
},
Setup: func() {
mockDB.EXPECT().
GetAssetGroup(gomock.Any()).
Return(assetGroup, nil)
mockGraph.EXPECT().
GetAssetGroupNodes(gomock.Any(), gomock.Any()).
Return(graph.NodeSet{
1: &graph.Node{
ID: 1,
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "a", common.Name.String(): "a", ad.DomainSID.String(): "a"},
},
},
2: &graph.Node{
ID: 2,
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "b", common.Name.String(): "b", ad.DomainSID.String(): "b"},
},
Expand Down Expand Up @@ -974,21 +1025,21 @@ func TestResources_ListAssetGroupMembers(t *testing.T) {
Return(graph.NodeSet{
1: &graph.Node{
ID: 1,
Kinds: graph.Kinds{ad.Domain},
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "a", common.Name.String(): "a", ad.DomainSID.String(): "a"},
},
},
2: &graph.Node{
ID: 2,
Kinds: graph.Kinds{ad.Domain},
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "b", common.Name.String(): "b", ad.DomainSID.String(): "b"},
},
},
3: &graph.Node{
ID: 3,
Kinds: graph.Kinds{ad.Domain},
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "c", common.Name.String(): "c", ad.DomainSID.String(): "c"},
},
Expand Down Expand Up @@ -1017,14 +1068,14 @@ func TestResources_ListAssetGroupMembers(t *testing.T) {
Return(graph.NodeSet{
1: &graph.Node{
ID: 1,
Kinds: graph.Kinds{ad.Domain},
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "a", common.Name.String(): "a", ad.DomainSID.String(): "a"},
},
},
2: &graph.Node{
ID: 2,
Kinds: graph.Kinds{ad.Domain},
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "b", common.Name.String(): "b", ad.DomainSID.String(): "b"},
},
Expand Down Expand Up @@ -1052,14 +1103,14 @@ func TestResources_ListAssetGroupMembers(t *testing.T) {
Return(graph.NodeSet{
1: &graph.Node{
ID: 1,
Kinds: graph.Kinds{ad.Domain},
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "a", common.Name.String(): "a", ad.DomainSID.String(): "a"},
},
},
2: &graph.Node{
ID: 2,
Kinds: graph.Kinds{ad.Domain},
Kinds: graph.Kinds{ad.Entity, ad.Domain},
Properties: &graph.Properties{
Map: map[string]any{common.ObjectID.String(): "b", common.Name.String(): "b", ad.DomainSID.String(): "b"},
},
Expand Down

0 comments on commit 3179fa2

Please sign in to comment.