From 79908c6dbe41b174d660f96f4438f54f24cfdd83 Mon Sep 17 00:00:00 2001 From: noctchillin Date: Mon, 24 Mar 2025 11:18:08 -0500 Subject: [PATCH 1/4] confighttp: Ensure that errors use provided error handler and error message from auth extensions is included in the response --- config/confighttp/confighttp.go | 11 +++++--- config/confighttp/confighttp_test.go | 38 +++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/config/confighttp/confighttp.go b/config/confighttp/confighttp.go index 06dd9daa560..e0d1c4c1042 100644 --- a/config/confighttp/confighttp.go +++ b/config/confighttp/confighttp.go @@ -441,7 +441,7 @@ func (hss *ServerConfig) ToServer(_ context.Context, host component.Host, settin return nil, err } - handler = authInterceptor(handler, server, hss.Auth.RequestParameters) + handler = authInterceptor(handler, server, hss.Auth.RequestParameters, serverOpts) } if hss.CORS != nil && len(hss.CORS.AllowedOrigins) > 0 { @@ -537,7 +537,7 @@ func NewDefaultCORSConfig() *CORSConfig { return &CORSConfig{} } -func authInterceptor(next http.Handler, server extensionauth.Server, requestParams []string) http.Handler { +func authInterceptor(next http.Handler, server extensionauth.Server, requestParams []string, serverOpts *internal.ToServerOptions) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { sources := r.Header query := r.URL.Query() @@ -548,7 +548,12 @@ func authInterceptor(next http.Handler, server extensionauth.Server, requestPara } ctx, err := server.Authenticate(r.Context(), sources) if err != nil { - http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) + if serverOpts.ErrHandler != nil { + serverOpts.ErrHandler(w, r, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) + } else { + http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) + } + return } diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index 121ae7eeb7a..41a09f06cc9 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -1212,7 +1212,7 @@ func TestFailedServerAuth(t *testing.T) { host := &mockHost{ ext: map[component.ID]component.Component{ mockID: newMockAuthServer(func(ctx context.Context, _ map[string][]string) (context.Context, error) { - return ctx, errors.New("Settings failed") + return ctx, errors.New("invalid authorization") }), }, } @@ -1224,6 +1224,42 @@ func TestFailedServerAuth(t *testing.T) { response := &httptest.ResponseRecorder{} srv.Handler.ServeHTTP(response, httptest.NewRequest(http.MethodGet, "/", nil)) + // verify + assert.Equal(t, http.StatusUnauthorized, response.Result().StatusCode) + assert.Equal(t, "invalid authorization", response.Result().Status) +} + +func TestFailedServerAuthWithErrorHandler(t *testing.T) { + // prepare + hss := ServerConfig{ + Endpoint: "localhost:0", + Auth: &AuthConfig{ + Authentication: configauth.Authentication{ + AuthenticatorID: mockID, + }, + }, + } + host := &mockHost{ + ext: map[component.ID]component.Component{ + mockID: newMockAuthServer(func(ctx context.Context, _ map[string][]string) (context.Context, error) { + return ctx, errors.New("Unauthorized due to error") + }), + }, + } + + eh := func(w http.ResponseWriter, _ *http.Request, _ string, statusCode int) { + assert.Equal(t, http.StatusUnauthorized, statusCode) + // custom error handler changes returned error message + http.Error(w, fmt.Sprintf("%v %s", http.StatusUnauthorized, http.StatusText(http.StatusUnauthorized)), statusCode) + } + + srv, err := hss.ToServer(context.Background(), host, componenttest.NewNopTelemetrySettings(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), WithErrorHandler(eh)) + require.NoError(t, err) + + // tt + response := &httptest.ResponseRecorder{} + srv.Handler.ServeHTTP(response, httptest.NewRequest(http.MethodGet, "/", nil)) + // verify assert.Equal(t, http.StatusUnauthorized, response.Result().StatusCode) assert.Equal(t, fmt.Sprintf("%v %s", http.StatusUnauthorized, http.StatusText(http.StatusUnauthorized)), response.Result().Status) From 383ee59251918d08048781e2611e7e23e95f4d24 Mon Sep 17 00:00:00 2001 From: noctchillin Date: Mon, 24 Mar 2025 11:34:50 -0500 Subject: [PATCH 2/4] Add changelog for fix --- .chloggen/fix-confighttp-auth-error.yaml | 25 ++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .chloggen/fix-confighttp-auth-error.yaml diff --git a/.chloggen/fix-confighttp-auth-error.yaml b/.chloggen/fix-confighttp-auth-error.yaml new file mode 100644 index 00000000000..59268bf205f --- /dev/null +++ b/.chloggen/fix-confighttp-auth-error.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: confighttp + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: "Ensure http server authentication failures are handled by the provided error handler" + +# One or more tracking issues or pull requests related to the change +issues: [12666] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [user] From a0b10cf419dd607d51460aca0407d26a2e69dc72 Mon Sep 17 00:00:00 2001 From: noctchillin Date: Mon, 24 Mar 2025 11:56:51 -0500 Subject: [PATCH 3/4] confighttp: expose auth error to handler, fix test logic --- config/confighttp/confighttp.go | 4 ++-- config/confighttp/confighttp_test.go | 16 +++++++++------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/config/confighttp/confighttp.go b/config/confighttp/confighttp.go index e0d1c4c1042..43193c957ee 100644 --- a/config/confighttp/confighttp.go +++ b/config/confighttp/confighttp.go @@ -549,9 +549,9 @@ func authInterceptor(next http.Handler, server extensionauth.Server, requestPara ctx, err := server.Authenticate(r.Context(), sources) if err != nil { if serverOpts.ErrHandler != nil { - serverOpts.ErrHandler(w, r, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) + serverOpts.ErrHandler(w, r, err.Error(), http.StatusUnauthorized) } else { - http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) + http.Error(w, err.Error(), http.StatusUnauthorized) } return diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index 41a09f06cc9..df49dae95e3 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -1226,7 +1226,7 @@ func TestFailedServerAuth(t *testing.T) { // verify assert.Equal(t, http.StatusUnauthorized, response.Result().StatusCode) - assert.Equal(t, "invalid authorization", response.Result().Status) + assert.Equal(t, fmt.Sprintf("%v %s", http.StatusUnauthorized, http.StatusText(http.StatusUnauthorized)), response.Result().Status) } func TestFailedServerAuthWithErrorHandler(t *testing.T) { @@ -1242,15 +1242,17 @@ func TestFailedServerAuthWithErrorHandler(t *testing.T) { host := &mockHost{ ext: map[component.ID]component.Component{ mockID: newMockAuthServer(func(ctx context.Context, _ map[string][]string) (context.Context, error) { - return ctx, errors.New("Unauthorized due to error") + return ctx, errors.New("invalid authorization") }), }, } - eh := func(w http.ResponseWriter, _ *http.Request, _ string, statusCode int) { + eh := func(w http.ResponseWriter, _ *http.Request, err string, statusCode int) { assert.Equal(t, http.StatusUnauthorized, statusCode) - // custom error handler changes returned error message - http.Error(w, fmt.Sprintf("%v %s", http.StatusUnauthorized, http.StatusText(http.StatusUnauthorized)), statusCode) + // custom error handler uses real error string + assert.Equal(t, "invalid authorization", err) + // custom error handler changes returned status code + http.Error(w, err, http.StatusInternalServerError) } srv, err := hss.ToServer(context.Background(), host, componenttest.NewNopTelemetrySettings(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), WithErrorHandler(eh)) @@ -1261,8 +1263,8 @@ func TestFailedServerAuthWithErrorHandler(t *testing.T) { srv.Handler.ServeHTTP(response, httptest.NewRequest(http.MethodGet, "/", nil)) // verify - assert.Equal(t, http.StatusUnauthorized, response.Result().StatusCode) - assert.Equal(t, fmt.Sprintf("%v %s", http.StatusUnauthorized, http.StatusText(http.StatusUnauthorized)), response.Result().Status) + assert.Equal(t, http.StatusInternalServerError, response.Result().StatusCode) + assert.Equal(t, fmt.Sprintf("%v %s", http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError)), response.Result().Status) } func TestServerWithErrorHandler(t *testing.T) { From 6191a4e44cb46bf4cd10af8dde788755b6fb9b1e Mon Sep 17 00:00:00 2001 From: noctchillin Date: Mon, 24 Mar 2025 12:02:58 -0500 Subject: [PATCH 4/4] Fix changelog for clarity --- .chloggen/fix-confighttp-auth-error.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chloggen/fix-confighttp-auth-error.yaml b/.chloggen/fix-confighttp-auth-error.yaml index 59268bf205f..98247f7ea3b 100644 --- a/.chloggen/fix-confighttp-auth-error.yaml +++ b/.chloggen/fix-confighttp-auth-error.yaml @@ -7,7 +7,7 @@ change_type: bug_fix component: confighttp # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: "Ensure http server authentication failures are handled by the provided error handler" +note: "Ensure http authentication server failures are handled by the provided error handler" # One or more tracking issues or pull requests related to the change issues: [12666]