From 34e3e8d9c159612c5fbb438dfcc37938e3ee5d3e Mon Sep 17 00:00:00 2001 From: Ed Schouten Date: Tue, 29 Aug 2023 21:41:19 +0200 Subject: [PATCH] Minor improvements to the HTTP authentication middleware change - In our case a shallow copy of the HTTP request using .Context() instead of .Clone() is sufficient. - Do a better job at explaining how the cookie encryption key is computed. - Leave a 'reserved' marker in place for the 'listen_address' field, to make it easier for people to figure out how to migrate. --- pkg/http/authenticating_handler.go | 2 +- pkg/http/authenticator.go | 6 +++++- pkg/http/oidc_authenticator.go | 11 ++++++++++- pkg/http/server.go | 6 +++--- pkg/proto/configuration/global/global.pb.go | 17 +++++++++-------- pkg/proto/configuration/global/global.proto | 6 +++++- pkg/proto/configuration/http/http.proto | 4 ++++ 7 files changed, 37 insertions(+), 15 deletions(-) diff --git a/pkg/http/authenticating_handler.go b/pkg/http/authenticating_handler.go index d4165189..8418f954 100644 --- a/pkg/http/authenticating_handler.go +++ b/pkg/http/authenticating_handler.go @@ -32,6 +32,6 @@ func (h *authenticatingHandler) ServeHTTP(w http.ResponseWriter, r *http.Request // not write a response to the client (e.g., emit a // redirect). Forward the request to the underlying // handler. - h.handler.ServeHTTP(w, r.Clone(auth.NewContextWithAuthenticationMetadata(r.Context(), metadata))) + h.handler.ServeHTTP(w, r.WithContext(auth.NewContextWithAuthenticationMetadata(r.Context(), metadata))) } } diff --git a/pkg/http/authenticator.go b/pkg/http/authenticator.go index e8ae42a0..d8f990b6 100644 --- a/pkg/http/authenticator.go +++ b/pkg/http/authenticator.go @@ -62,7 +62,7 @@ func NewAuthenticatorFromConfiguration(policy *configuration.AuthenticationPolic case *configuration.AuthenticationPolicy_Oidc: // Select a name and encryption key for the session // state cookie. Even though the configuration has a - // dedicted cookie seed field, we include the rest of + // dedicated cookie seed field, we include the rest of // the configuration message as well. This ensures that // any changes to the configuration automatically // invalidate existing sessions. @@ -74,6 +74,10 @@ func NewAuthenticatorFromConfiguration(policy *configuration.AuthenticationPolic return nil, status.Error(codes.InvalidArgument, "Failed to marshal configuration to compute OIDC cookie seed") } cookieSeedHash := sha256.Sum256(fullCookieSeed) + + // Let the first 128 bits of the seed hash be the name + // of the cookie, while the last 128 bits are used as + // the AES key for encrypting/signing the cookie value. cookieName := base64.RawURLEncoding.EncodeToString(cookieSeedHash[:sha256.Size/2]) cookieCipher, err := aes.NewCipher(cookieSeedHash[sha256.Size/2:]) if err != nil { diff --git a/pkg/http/oidc_authenticator.go b/pkg/http/oidc_authenticator.go index 7086be58..fd58eec6 100644 --- a/pkg/http/oidc_authenticator.go +++ b/pkg/http/oidc_authenticator.go @@ -42,7 +42,16 @@ type oidcAuthenticator struct { // requests are authorized by an OAuth2 server. Authentication metadata // is constructed by obtaining claims through the OpenID Connect user // info endpoint, and transforming it using a JMESPath expression. -func NewOIDCAuthenticator(oauth2Config *oauth2.Config, userInfoURL string, metadataExtractor *jmespath.JMESPath, httpClient *http.Client, randomNumberGenerator random.ThreadSafeGenerator, cookieName string, cookieAEAD cipher.AEAD, clock clock.Clock) (Authenticator, error) { +func NewOIDCAuthenticator( + oauth2Config *oauth2.Config, + userInfoURL string, + metadataExtractor *jmespath.JMESPath, + httpClient *http.Client, + randomNumberGenerator random.ThreadSafeGenerator, + cookieName string, + cookieAEAD cipher.AEAD, + clock clock.Clock, +) (Authenticator, error) { // Extract the path in the redirect URL of the OAuth2 // configuration, as we need to match it in incoming HTTP // requests. diff --git a/pkg/http/server.go b/pkg/http/server.go index a58f79e4..a6adabe3 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -10,9 +10,9 @@ import ( ) // NewServersFromConfigurationAndServe spawns HTTP servers as part of a -// program.Group, based on a configuration specification. The web -// servers are automatically terminated if the context associated with -// the group is canceled. +// program.Group, based on a configuration message. The web servers are +// automatically terminated if the context associated with the group is +// canceled. func NewServersFromConfigurationAndServe(configurations []*configuration.ServerConfiguration, handler http.Handler, group program.Group) error { for _, configuration := range configurations { authenticator, err := NewAuthenticatorFromConfiguration(configuration.AuthenticationPolicy) diff --git a/pkg/proto/configuration/global/global.pb.go b/pkg/proto/configuration/global/global.pb.go index ed304345..10b07800 100644 --- a/pkg/proto/configuration/global/global.pb.go +++ b/pkg/proto/configuration/global/global.pb.go @@ -322,7 +322,7 @@ type DiagnosticsHTTPServerConfiguration struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - HttpServers []*http.ServerConfiguration `protobuf:"bytes,1,rep,name=http_servers,json=httpServers,proto3" json:"http_servers,omitempty"` + HttpServers []*http.ServerConfiguration `protobuf:"bytes,5,rep,name=http_servers,json=httpServers,proto3" json:"http_servers,omitempty"` EnablePprof bool `protobuf:"varint,2,opt,name=enable_pprof,json=enablePprof,proto3" json:"enable_pprof,omitempty"` EnablePrometheus bool `protobuf:"varint,3,opt,name=enable_prometheus,json=enablePrometheus,proto3" json:"enable_prometheus,omitempty"` EnableActiveSpans bool `protobuf:"varint,4,opt,name=enable_active_spans,json=enableActiveSpans,proto3" json:"enable_active_spans,omitempty"` @@ -1157,10 +1157,10 @@ var file_pkg_proto_configuration_global_global_proto_rawDesc = []byte{ 0x72, 0x65, 0x75, 0x73, 0x65, 0x5f, 0x6d, 0x65, 0x74, 0x61, 0x64, 0x61, 0x74, 0x61, 0x18, 0x08, 0x20, 0x03, 0x28, 0x09, 0x52, 0x1b, 0x67, 0x72, 0x70, 0x63, 0x46, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x41, 0x6e, 0x64, 0x52, 0x65, 0x75, 0x73, 0x65, 0x4d, 0x65, 0x74, 0x61, 0x64, 0x61, 0x74, - 0x61, 0x4a, 0x04, 0x08, 0x04, 0x10, 0x05, 0x22, 0xfa, 0x01, 0x0a, 0x22, 0x44, 0x69, 0x61, 0x67, + 0x61, 0x4a, 0x04, 0x08, 0x04, 0x10, 0x05, 0x22, 0x80, 0x02, 0x0a, 0x22, 0x44, 0x69, 0x61, 0x67, 0x6e, 0x6f, 0x73, 0x74, 0x69, 0x63, 0x73, 0x48, 0x54, 0x54, 0x50, 0x53, 0x65, 0x72, 0x76, 0x65, 0x72, 0x43, 0x6f, 0x6e, 0x66, 0x69, 0x67, 0x75, 0x72, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x12, 0x54, - 0x0a, 0x0c, 0x68, 0x74, 0x74, 0x70, 0x5f, 0x73, 0x65, 0x72, 0x76, 0x65, 0x72, 0x73, 0x18, 0x01, + 0x0a, 0x0c, 0x68, 0x74, 0x74, 0x70, 0x5f, 0x73, 0x65, 0x72, 0x76, 0x65, 0x72, 0x73, 0x18, 0x05, 0x20, 0x03, 0x28, 0x0b, 0x32, 0x31, 0x2e, 0x62, 0x75, 0x69, 0x6c, 0x64, 0x62, 0x61, 0x72, 0x6e, 0x2e, 0x63, 0x6f, 0x6e, 0x66, 0x69, 0x67, 0x75, 0x72, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x2e, 0x68, 0x74, 0x74, 0x70, 0x2e, 0x53, 0x65, 0x72, 0x76, 0x65, 0x72, 0x43, 0x6f, 0x6e, 0x66, 0x69, 0x67, @@ -1173,11 +1173,12 @@ var file_pkg_proto_configuration_global_global_proto_rawDesc = []byte{ 0x68, 0x65, 0x75, 0x73, 0x12, 0x2e, 0x0a, 0x13, 0x65, 0x6e, 0x61, 0x62, 0x6c, 0x65, 0x5f, 0x61, 0x63, 0x74, 0x69, 0x76, 0x65, 0x5f, 0x73, 0x70, 0x61, 0x6e, 0x73, 0x18, 0x04, 0x20, 0x01, 0x28, 0x08, 0x52, 0x11, 0x65, 0x6e, 0x61, 0x62, 0x6c, 0x65, 0x41, 0x63, 0x74, 0x69, 0x76, 0x65, 0x53, - 0x70, 0x61, 0x6e, 0x73, 0x42, 0x40, 0x5a, 0x3e, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, - 0x6f, 0x6d, 0x2f, 0x62, 0x75, 0x69, 0x6c, 0x64, 0x62, 0x61, 0x72, 0x6e, 0x2f, 0x62, 0x62, 0x2d, - 0x73, 0x74, 0x6f, 0x72, 0x61, 0x67, 0x65, 0x2f, 0x70, 0x6b, 0x67, 0x2f, 0x70, 0x72, 0x6f, 0x74, - 0x6f, 0x2f, 0x63, 0x6f, 0x6e, 0x66, 0x69, 0x67, 0x75, 0x72, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x2f, - 0x67, 0x6c, 0x6f, 0x62, 0x61, 0x6c, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x70, 0x61, 0x6e, 0x73, 0x4a, 0x04, 0x08, 0x01, 0x10, 0x02, 0x42, 0x40, 0x5a, 0x3e, 0x67, 0x69, + 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x62, 0x75, 0x69, 0x6c, 0x64, 0x62, 0x61, + 0x72, 0x6e, 0x2f, 0x62, 0x62, 0x2d, 0x73, 0x74, 0x6f, 0x72, 0x61, 0x67, 0x65, 0x2f, 0x70, 0x6b, + 0x67, 0x2f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2f, 0x63, 0x6f, 0x6e, 0x66, 0x69, 0x67, 0x75, 0x72, + 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x2f, 0x67, 0x6c, 0x6f, 0x62, 0x61, 0x6c, 0x62, 0x06, 0x70, 0x72, + 0x6f, 0x74, 0x6f, 0x33, } var ( diff --git a/pkg/proto/configuration/global/global.proto b/pkg/proto/configuration/global/global.proto index 000ff7ad..2c3c1909 100644 --- a/pkg/proto/configuration/global/global.proto +++ b/pkg/proto/configuration/global/global.proto @@ -241,10 +241,14 @@ message Configuration { } message DiagnosticsHTTPServerConfiguration { + // Was 'listen_address'. The listen address of the diagnostics HTTP + // server can now be configured through the 'http_servers' option. + reserved 1; + // Default endpoints: // - /-/healthy: Returns HTTP 200 OK if the application managed to // start successfully. - repeated buildbarn.configuration.http.ServerConfiguration http_servers = 1; + repeated buildbarn.configuration.http.ServerConfiguration http_servers = 5; // Enables endpoints: // - /debug/pprof/*: Endpoints for Go's pprof debug tool. diff --git a/pkg/proto/configuration/http/http.proto b/pkg/proto/configuration/http/http.proto index b6966995..8e1ed43f 100644 --- a/pkg/proto/configuration/http/http.proto +++ b/pkg/proto/configuration/http/http.proto @@ -121,6 +121,10 @@ message OIDCAuthenticationPolicy { // service returns refresh tokens. This permits the HTTP server to // continue sessions with fewer redirects to the authorization // service. + // + // A list of commonly used scopes can be found in the OpenID Connect + // specification: + // https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims repeated string scopes = 8; // A random seed of how cookies containing session state are named,