Implement Azure fetchers and asset provider #1310

jeniawhite · 2023-09-03T14:25:43Z

Summary of your changes

Implementation of assets provider using ARG and Azure fetcher in order to fetch resources for evaluation.

Related Issues

Fixes: [CIS Azure] Asset inventory / fetchers #1250

Checklist

I have added tests that prove my fix is effective or that my feature works
I have added the necessary README/documentation (if appropriate)

mergify · 2023-09-03T14:26:18Z

This pull request does not have a backport label. Could you fix it @jeniawhite? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit
NOTE: backport-skip has been added to this pull request.

github-actions · 2023-09-03T14:29:34Z

📊 Allure Report - 💚 No failures were reported.

Result	Count
🟥 Failed	0
🟩 Passed	39
⬜ Skipped	1

mergify · 2023-09-11T08:14:20Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b evgb-AzureFetchers upstream/evgb-AzureFetchers
git merge upstream/main
git push upstream evgb-AzureFetchers

mergify · 2023-09-19T09:10:43Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b evgb-AzureFetchers upstream/evgb-AzureFetchers
git merge upstream/main
git push upstream evgb-AzureFetchers

orestisfl

I think we can merge this after some tests and resolving the TODOs

orestisfl · 2023-09-19T09:45:46Z

resources/providers/azurelib/inventory/provider.go

+			// TODO: Populate from config or query (not sensitive but still don't want to commit)
+			to.Ptr(os.Getenv("AZURE_SUBSCRIPTION_ID"))},


As discussed, this will be handled in a follow-up ticket

orestisfl · 2023-09-19T09:48:11Z

resources/fetching/fetcher.go

+	AzureNetworkInterface     = "azure-network-interface"
+	AzureApplicationService   = "azure-application-service"
+	AzureLoggingAndMonitoring = "azure-logging-and-monitoring"
+	AzureDatabaseService      = "azure-database-service"


Suggested change

AzureNetworkInterface = "azure-network-interface"

AzureApplicationService = "azure-application-service"

AzureLoggingAndMonitoring = "azure-logging-and-monitoring"

AzureDatabaseService = "azure-database-service"

Let's add these one by one as we implement the rules. We can merge this PR without them.

orestisfl · 2023-09-19T09:48:41Z

resources/fetching/fetchers/azure/assets_fetcher.go

+
+func (f *AzureAssetsFetcher) Fetch(ctx context.Context, cMetadata fetching.CycleMetadata) error {
+	f.log.Info("Starting AzureAssetsFetcher.Fetch")
+	// TODO: Maybe we should use a query per type instead of listing all assets in a single query


Suggested change

// TODO: Maybe we should use a query per type instead of listing all assets in a single query

Is there any benefit to that?

I assume that it might help us If we will need to collect multiple resource types to evaluate a single rule.

orestisfl · 2023-09-19T09:49:04Z

resources/fetching/fetchers/azure/assets_fetcher.go

+	TenantId       string         `json:"tenant_id,omitempty"`
+}
+
+// TODO: Implement other types


Suggested change

// TODO: Implement other types

Can we track this in a new issue?

This is basically the other piece of work regarding the comment above:
Let's add these one by one as we implement the rules. We can merge this PR without them

orestisfl

Let's try to merge this relatively clean in main unless if it's blocking more important work

I left comments on TODOs and we should definitely guard the casting or avoid casting alltogether

orestisfl · 2023-09-19T12:43:36Z

resources/fetching/fetchers/azure/assets_fetcher.go

+	Asset   AzureAssetInfo `json:"asset,omitempty"`
+}
+
+// TODO: Fill this struct with the required fields


Isn't this done?

Suggested change

// TODO: Fill this struct with the required fields

orestisfl · 2023-09-19T12:44:57Z

resources/fetching/fetchers/azure/assets_fetcher.go

+		case f.resourceCh <- fetching.ResourceInfo{
+			CycleMetadata: cMetadata,
+			// TODO: Safe guard this conversion
+			Resource: getAssetFromData(asset.(map[string]any)),


Shouldn't ListAllAssetTypesByName return the correct type / handle the casting instead of the caller?

orestisfl · 2023-09-19T12:47:22Z

resources/fetching/fetchers/azure/assets_fetcher.go

+	return value
+}
+
+// TODO: Handle this function


Let's resolve the TODOs, is this relevant?

orestisfl · 2023-09-19T12:47:40Z

resources/fetching/fetchers/azure/assets_fetcher.go

+
+func (r *AzureAsset) GetElasticCommonData() (map[string]any, error) { return nil, nil }
+
+// TODO: Implement this function


What's the significance of this? Is it a blocker?

It is not a blocker because we did not decide on the subTypes yet.
As of current we only utilize the resource type and do not look at subTypes in the policies.
Future implementation of additional rules might change that, this is why it is left blank as of current.

orestisfl · 2023-09-19T12:47:59Z

resources/providers/azurelib/inventory/provider.go

+	return resourceAssets, nil
+}
+
+// TODO: Handle this function


…oudbeat into evgb-AzureFetchers

orestisfl · 2023-09-21T07:55:31Z

resources/fetching/fetchers/azure/assets_fetcher_test.go

+		s.NoError(err)
+		s.NotNil(meta)
+		s.NoError(err)
+		s.NotEmpty(meta)


Suggested change

s.NoError(err)

s.NotNil(meta)

s.NoError(err)

s.NotEmpty(meta)

require.NoError(s.T(), err)

s.NotEmpty(meta)

orestisfl · 2023-09-21T07:56:37Z

resources/fetching/fetchers/azure/assets_fetcher_test.go

+		s.Equal(mockAssets[index].Id, meta.ID)
+		s.Equal(AzureResourceTypes[mockAssets[index].Type], meta.Type)
+		s.Equal("", meta.SubType)
+		s.Equal(mockAssets[index].Name, meta.Name)
+		s.Equal(mockAssets[index].Location, meta.Region)


Suggested change

s.Equal(mockAssets[index].Id, meta.ID)

s.Equal(AzureResourceTypes[mockAssets[index].Type], meta.Type)

s.Equal("", meta.SubType)

s.Equal(mockAssets[index].Name, meta.Name)

s.Equal(mockAssets[index].Location, meta.Region)

s.Equal(mockAssets[index].Id, meta.ID)

s.Equal(AzureResourceTypes[mockAssets[index].Type], meta.Type)

s.Equal("", meta.SubType)

s.Equal(mockAssets[index].Name, meta.Name)

s.Equal(mockAssets[index].Location, meta.Region)

I would construct an expected meta object and compare it using the Equal function, this produces a better diff and it is more feature-proof as we can catch cases where we forget to use new field if we combine it with exhauststruct.

orestisfl

LGTM but the coverails still fails

Apply suggestions from elastic#1310 - Reduce code - Protect from panics via `Require()` - Compare objects directly

Apply suggestions from #1310 - Reduce code - Protect from panics via `Require()` - Compare objects directly

jeniawhite added 2 commits September 3, 2023 17:22

Implement Azure fetchers and asset provider

bc33202

Fixing subscription to env

bed328c

jeniawhite requested a review from a team as a code owner September 3, 2023 14:25

mergify bot assigned jeniawhite Sep 3, 2023

mergify bot added the backport-skip label Sep 3, 2023

jeniawhite mentioned this pull request Sep 3, 2023

Implement Azure benchmark #1311

Merged

2 tasks

jeniawhite marked this pull request as draft September 5, 2023 12:01

Merge branch 'main' into evgb-AzureFetchers

229dc83

jeniawhite marked this pull request as ready for review September 5, 2023 16:59

orestisfl added 2 commits September 7, 2023 19:33

Merge branch 'main' into evgb-AzureFetchers

643aefa

Merge branch 'main' into evgb-AzureFetchers

c20fa48

Merge remote-tracking branch 'upstream/main' into evgb-AzureFetchers

46e80d3

orestisfl self-assigned this Sep 18, 2023

orestisfl added 2 commits September 18, 2023 07:44

Merge remote-tracking branch 'upstream/main' into evgb-AzureFetchers

b13c32f

up

4044768

Merge remote-tracking branch 'upstream/main' into evgb-AzureFetchers

4cdd79c

orestisfl requested changes Sep 19, 2023

View reviewed changes

orestisfl removed their assignment Sep 19, 2023

CR comments

8f08bc3

jeniawhite requested a review from orestisfl September 19, 2023 11:27

Merge branch 'main' into evgb-AzureFetchers

53cb17c

jeniawhite enabled auto-merge (squash) September 19, 2023 12:45

orestisfl reviewed Sep 19, 2023

View reviewed changes

jeniawhite added 3 commits September 20, 2023 13:36

CR changes

52eeb17

Merge branch 'evgb-AzureFetchers' of https://github.com/jeniawhite/cl…

5a58aa8

…oudbeat into evgb-AzureFetchers

Merge branch 'main' into evgb-AzureFetchers

a9b164c

jeniawhite requested a review from orestisfl September 20, 2023 10:36

jeniawhite added 6 commits September 20, 2023 20:12

Adding unit tests

c05df25

Merge branch 'evgb-AzureFetchers' of https://github.com/jeniawhite/cl…

79f6482

…oudbeat into evgb-AzureFetchers

Merge branch 'main' into evgb-AzureFetchers

c3d188a

This breaks linter

d1c4cee

Merge branch 'evgb-AzureFetchers' of https://github.com/jeniawhite/cl…

3731861

…oudbeat into evgb-AzureFetchers

Fixing mocks

5b216f1

orestisfl reviewed Sep 21, 2023

View reviewed changes

orestisfl approved these changes Sep 21, 2023

View reviewed changes

jeniawhite merged commit 83fa2d8 into elastic:main Sep 21, 2023

orestisfl added a commit to orestisfl/cloudbeat that referenced this pull request Sep 25, 2023

Azure fetchers: Improve tests

6c248a1

Apply suggestions from elastic#1310 - Reduce code - Protect from panics via `Require()` - Compare objects directly

orestisfl mentioned this pull request Sep 25, 2023

Azure fetchers: Improve tests #1365

Merged

2 tasks

orestisfl added a commit that referenced this pull request Sep 27, 2023

Azure fetchers: Improve tests (#1365)

2fd7f71

Apply suggestions from #1310 - Reduce code - Protect from panics via `Require()` - Compare objects directly

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Azure fetchers and asset provider #1310

Implement Azure fetchers and asset provider #1310

jeniawhite commented Sep 3, 2023 •

edited by orestisfl

Loading

mergify bot commented Sep 3, 2023

github-actions bot commented Sep 3, 2023 •

edited

Loading

mergify bot commented Sep 11, 2023

mergify bot commented Sep 19, 2023

orestisfl left a comment

orestisfl Sep 19, 2023

orestisfl Sep 19, 2023

orestisfl Sep 19, 2023

jeniawhite Sep 19, 2023

orestisfl Sep 19, 2023

jeniawhite Sep 19, 2023

orestisfl left a comment

orestisfl Sep 19, 2023

orestisfl Sep 19, 2023

orestisfl Sep 19, 2023

orestisfl Sep 19, 2023

jeniawhite Sep 19, 2023

orestisfl Sep 19, 2023

orestisfl Sep 21, 2023

orestisfl Sep 21, 2023

orestisfl left a comment

		// TODO: Populate from config or query (not sensitive but still don't want to commit)
		to.Ptr(os.Getenv("AZURE_SUBSCRIPTION_ID"))},


		func (r *AzureAsset) GetElasticCommonData() (map[string]any, error) { return nil, nil }

		// TODO: Implement this function

Implement Azure fetchers and asset provider #1310

Implement Azure fetchers and asset provider #1310

Conversation

jeniawhite commented Sep 3, 2023 • edited by orestisfl Loading

Summary of your changes

Related Issues

Checklist

mergify bot commented Sep 3, 2023

github-actions bot commented Sep 3, 2023 • edited Loading

📊 Allure Report - 💚 No failures were reported.

mergify bot commented Sep 11, 2023

mergify bot commented Sep 19, 2023

orestisfl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orestisfl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orestisfl left a comment

Choose a reason for hiding this comment

jeniawhite commented Sep 3, 2023 •

edited by orestisfl

Loading

github-actions bot commented Sep 3, 2023 •

edited

Loading