-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[extensions/azureauth] Retrieve scope from Host/URL.Host instead of Header for client authentication #40033
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
base: main
Are you sure you want to change the base?
Conversation
|
||
host := req.Header.Get("Host") | ||
host := req.Host |
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 not correct. This host
is meant to be a Host
header. If you look at the README file, the request is meant to have headers, of which one of them is Host
(see azure documentation).
This host will then be used to define the scope of the authentication request.
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.
For example, set the Host
of your request to management.azure.com
. Will your use case work now? Which azure service are you using? The host might change.
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 not correct. This
host
is meant to be aHost
header. If you look at the README file, the request is meant to have headers, of which one of them isHost
(see azure documentation).This host will then be used to define the scope of the authentication request.
No, it won't be added. I can give you a minimum example to reproduce the issue if you don't have a chance to verify this.
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.
For example, set the
Host
of your request tomanagement.azure.com
. Will your use case work now? Which azure service are you using? The host might change.
Not all exporters give user the opportunity to override the HTTP header.
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.
can give you a minimum example to reproduce the issue if you don't have a chance to verify this.
I would that, if you could.
I'm very busy this week, so I am trying not to respond slow to this issue.
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.
No worries, take your time since I've already unblocked myself.
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.
@ms-hujia Can you share with me the config.yaml you are using for the collector? So I can have a reference and test it Sorry, I see you have put the comment in the issue
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.
Thank you for your example @ms-hujia. I think I understand the issue now.
I am not sure how to resolve this in an idiomatic way. I believe requests for authentication on azure should be in sync with what the user expects - so based on azure documentation, the host should be on the header.
But it is true that if you use azureauth
, then you cannot set the headers of the request - there is headers_setter
extension, but since you can only use 1 authenticator, you could not use it at the same time as azureauth
.
I believe the best way would be to use a new component that could set the header Host
, and then use azureauth
right after. So, what headers_transform
extension could possible do (this is not an approved component, it is just something we use internally at the company I work at, Elastic, to handle this exact same issue).
If we set the host the way this PR shows, then we are expecting the request to be hosted on azure, and I am not sure if that is always right. What do you think?
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.
For Golang's HTTP client implementation, the Host
entry will be added to the HTTP header server received, even if it's not set in the Header
field of http.Request
. This is done in the base http.RoundTripper
. Generally, user don't need to pass those basic HTTP headers explicitly, e.g., another example is Content-Length
.
Going back to the issue itself, the issue happens due to the Host
entry is checked before base http.RoundTripper
add it to request.
@ms-hujia Could you add a changelog please? So we can start working on getting this merged :) |
…eader for client authentication
I've added the changelog and rebased my branch. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Description
For client request, there won't be a
Host
entry inHeader
. The scope should be retrieved fromHost
orURL.Host
.Link to tracking issue
Fixes #40032.
Testing
Updated the existing test case.
Documentation
N/A.