-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[filbeat][azure-blob-storage] - Adding support for Microsoft Entra ID RBAC authentication #40879
base: main
Are you sure you want to change the base?
Conversation
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
This pull request is now in conflicts. Could you fix it? 🙏
|
"time" | ||
|
||
"github.com/Azure/azure-sdk-for-go/sdk/azcore" | ||
"github.com/h2non/gock" |
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.
Do we need this? From what I can see, an implementation of http.RoundTripper
would satisfy the testing requirements that tests here add, arguably more cleanly.
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 had the same thought when I gave this a quick scan yesterday. The declarative setup is nice, but I'm not sure it's worth another dependency.
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.
If we go that route then we would lose the context to the serv.URL thats local to each test. serv.URL would be required to set & override the urls in the response JSON without which the sdk will always try to call
https://login.microsoftonline.com
for the token validation check, as this is hardwired into the sdk and cannot be overridden by conventional means, hence the only option is to intercept the call from within the test and override at that point, which is why I was relying on gock here. If we chose not to use gock then the option remains to set serv.URL globally which I'm not sure would be right as future test expansion could lead to test concurrency issues.
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.
If we go that route then we would lose the context to the serv.URL thats local to each test.
Why? There is nothing in the gock code that could not be done easily and more transparently with local code. AFAICS the mock code provides author convenience at the cost of transparency for the maintainer.
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.
@efd6, have removed gock and simplified the test.
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.
nits only
1. `client_id` : The client ID of the Azure Entra ID application. | ||
2. `client_secret` : The client secret of the Azure Entra ID application. | ||
3. `tenant_id` : The tenant ID of the Azure Entra ID application. |
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.
1. `client_id` : The client ID of the Azure Entra ID application. | |
2. `client_secret` : The client secret of the Azure Entra ID application. | |
3. `tenant_id` : The tenant ID of the Azure Entra ID application. | |
1. `client_id`: The client ID of the Azure Entra ID application. | |
2. `client_secret`: The client secret of the Azure Entra ID application. | |
3. `tenant_id`: The tenant ID of the Azure Entra ID application. |
---- | ||
How to setup the `auth.oauth2` credentials can be found in the Azure documentation https://docs.microsoft.com/en-us/azure/active-directory/develop/quickstart-register-app[here] | ||
|
||
NOTE: According to our internal testing it seems that we require at least an access level of **blobOwner** for the service principle to be able to read the blobs. If you are facing any issues with the access level, please ensure that the access level is set to **blobOwner**. |
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.
NOTE: According to our internal testing it seems that we require at least an access level of **blobOwner** for the service principle to be able to read the blobs. If you are facing any issues with the access level, please ensure that the access level is set to **blobOwner**. | |
NOTE: According to our internal testing it seems that we require at least an access level of **blobOwner** for the service principle to be able to read the blobs. If you are facing any issues with the access level, ensure that the access level is set to **blobOwner**. |
Type of change
Proposed commit message
Added support for Microsoft Entra ID RBAC authentication.
Added mock tests by injecting the gock transport layer in the azure client.
Added some config tests with the new config options.
Checklist
- [ ] I have made corresponding change to the default configuration filesCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Disruptive User Impact
No Impact
Live Testing
Live testing was performed using our internal Azure dev environment.
Process followed:
Author's Checklist
How to test this PR locally
Related issues
Testing
We performed live testing internally using an active azure account and the new authentication system is working properly after assigning the service principal app with the blobOwner permission level.
Use cases
Screenshots
Logs