Plugins: Only set non-existing headers for core plugin requests

[aangelisc]

Fixes #78629

When a plugin request has headers that have been set by some logic in the plugin or some other middleware this change prevents the HTTPClient middleware from overwriting those headers. This is the same logic as that which already exists in the plugin-sdk.

[andresmgot]

LGTM, makes sense.

[wbrowne]

Please make sure to document this as a breaking change 🙏

[andresmgot]

Please make sure to document this as a breaking change 🙏

Can this be considered a breaking change? I would say it's more of a bug fix.

[wbrowne]

Please make sure to document this as a breaking change 🙏

Can this be considered a breaking change? I would say it's more of a bug fix.

Hmmm.. I guess depending on how you look at it, but not sure if someone could be tripped up by this as result and it'd be better to have notice like this to point to. I would say it's safer to add this, but I'd defer to @marefr as he has the most context here.

[marefr]

@aangelisc can you provide an example of headers causing problems for you? Might be security concerns to allow the client (end user) to override certain headers

[aangelisc]

@marefr the Azure auth middleware sets the Authorization header. When attempting to do user-based authentication the access token sent in the plugin request will be exchanged for another access token with the appropriate permissions. This middleware will then overwrite the new token with the original one which does not have the required permissions.

Afaict, the plugin-sdk middleware will not overwrite a header if it has a non-empty value which is what I'd expect to happen in core as well.

[marefr]

@marefr the Azure auth middleware sets the Authorization header. When attempting to do user-based authentication the access token sent in the plugin request will be exchanged for another access token with the appropriate permissions. This middleware will then overwrite the new token with the original one which does not have the required permissions.

Afaict, the plugin-sdk middleware will not overwrite a header if it has a non-empty value which is what I'd expect to happen in core as well.

@aangelisc thanks. Since only Authorization header I would perhaps suggest to only add this logic for this specific header. I'm a bit concerned with security and at the same time have a hard time to see everything that this potentially could affect. But a general example I think about is that the client/user could include HTTP headers that with your logic they'll be sent to the plugin rather than being overwritten by Grafana's internal middlewares.

Your main problem is that your Azure HTTP client middleware is executed after the ContextualMiddleware. Rather than appending your middleware here https://github.com/grafana/grafana-azure-sdk-go/blob/b20aac068928f54c7c89e32ef07b517d9cec8750/azhttpclient/options.go#L26 you might be able to use the clientOpts.ConfigureMiddleware function to insert the azure middleware before the contextual middleware. Something along these lines:

opts.ConfigureMiddleware = func(opts Options, existingMiddleware []Middleware) []Middleware {
	mws := []httpclient.Middleware{}
        inserted := false
	for _, mw := range existingMiddleware {
		if name, ok := mw.(httpclient.MiddlewareName); ok && name.MiddlewareName() ==httpclient.ContextualMiddlewareName {
			mws = append(mws, <azure middleware>
			inserted = true
		}
		mws = append(mws, mw)
	}

	if !inserted {
		mws = append(mws, <azure middleware>
	}
}

Update:
Sorry I thought about this the wrong way. Please disregard my comment above. I'll leave it for completeness. A bit confusing and easy to get tricked with the contextual middleware which is for HTTP headers in outgoing HTTP requests and not plugin request. In addition the HTTP requests are created from each plugin so the plugin author should be able to set HTTP headers without them being overwritten I think. Noticed we do have similar logic for user agent and basic authentication middlewares.

I'll go ahead and approve this.

[marefr]

LGTM and agree bugfix and not breaking and should only affect core plugins (I believe)

[aangelisc]

Thanks for the review comments all @andresmgot @wbrowne @marefr!