-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Asset Inventory][Azure] Add Acitive Directory fetcher to fetch Service Principals #2625
Conversation
This pull request does not have a backport label. Could you fix it @kubasobon? 🙏
|
This pull request is now in conflicts. Could you fix it? 🙏
|
This pull request is now in conflicts. Could you fix it? 🙏
|
This pull request is now in conflicts. Could you fix it? 🙏
|
return true // to continue the iteration | ||
}) | ||
if err != nil { | ||
p.log.Errorf("error iterating over Service Principals: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we return the error if principals are not returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a log, because we might still get some resources. The pagination failed, but items
slice has been populated. We try to get the best scenario for us - getting data, even if incomplete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It's just that we don't check if there was indeed any item or not. But also I don't have a good argument to bring the error up if the items is empty and error exist. In practice an error will be logged anyway
@romulets Please take another look |
Summary of your changes
This PR adds an Active Directory fetcher and fetchers Azure Service Principals. It utilizes microsoftgraph/msgraph-sdk-go, since this Active Directory API is not exposed in the SDK we've been using so far. This is the recommended approach.
Screenshot/Data
Related Issues
Closes https://github.com/elastic/security-team/issues/10161
Checklist