From cd30d9e0cedb0b095c5fdb8aba85d33b6291a1b1 Mon Sep 17 00:00:00 2001 From: Houssem Ben Mabrouk Date: Fri, 11 Oct 2024 17:31:52 +0200 Subject: [PATCH 01/10] works but not final pki solution Signed-off-by: Houssem Ben Mabrouk --- cmd/dex/serve.go | 1 + connector/cert/cert.go | 224 ++++++++++++++++++++++++++++++++++++ connector/cert/cert_test.go | 188 ++++++++++++++++++++++++++++++ server/handlers.go | 74 +++++++++++- server/handlers_test.go | 1 + server/oauth2.go | 1 + server/server.go | 3 + server/server_test.go | 9 +- 8 files changed, 496 insertions(+), 5 deletions(-) create mode 100644 connector/cert/cert.go create mode 100644 connector/cert/cert_test.go diff --git a/cmd/dex/serve.go b/cmd/dex/serve.go index 6fcca04da3..52bdf3ddcb 100644 --- a/cmd/dex/serve.go +++ b/cmd/dex/serve.go @@ -558,6 +558,7 @@ func applyConfigOverrides(options serveOptions, config *Config) { if len(config.OAuth2.GrantTypes) == 0 { config.OAuth2.GrantTypes = []string{ "authorization_code", + "certificate", "implicit", "password", "refresh_token", diff --git a/connector/cert/cert.go b/connector/cert/cert.go new file mode 100644 index 0000000000..db44468cb1 --- /dev/null +++ b/connector/cert/cert.go @@ -0,0 +1,224 @@ +package cert + +import ( + "context" + "crypto/x509" + "encoding/base64" + "encoding/pem" + "errors" + "fmt" + "log/slog" + "net/http" + "net/url" + "os" + + "github.com/dexidp/dex/connector" +) + +type Config struct { + // ClientCAPath is the path of the CA certificate used to validate client certificates + ClientCAPath string `json:"clientCAPath"` + // CertHeader is the name of the HTTP header containing the client certificate (if using a proxy) + CertHeader string `json:"certHeader"` + + UserIDKey string `json:"userIDKey"` + UserNameKey string `json:"userNameKey"` + PreferredUserNameKey string `json:"preferredUserNameKey"` + GroupKey string `json:"groupKey"` +} + +// CertConnector implements the CallbackConnector interface +type CertConnector struct { + clientCA *x509.CertPool + certHeader string + userIDKey string + userNameKey string + preferredUserNameKey string + groupKey string + logger *slog.Logger +} + +var ( + _ connector.CallbackConnector = (*CertConnector)(nil) +) + +// loadCACert loads the CA certificate from the file +func loadCACert(caCertFile string) (*x509.CertPool, error) { + clientCA := x509.NewCertPool() + caCertBytes, err := os.ReadFile(caCertFile) + if err != nil { + return nil, fmt.Errorf("failed to read CA cert file: %v", err) + } + + if !clientCA.AppendCertsFromPEM(caCertBytes) { + return nil, errors.New("failed to append CA certs from PEM file") + } + + return clientCA, nil +} + +// Open initializes the PKI Connector +func (c *Config) Open(id string, logger *slog.Logger) (connector.Connector, error) { + if c.ClientCAPath == "" { + return nil, errors.New("missing required config field 'clientCAPath'") + } + + // TODO: maybe support multiple CAs + clientCA, err := loadCACert(c.ClientCAPath) + if err != nil { + return nil, fmt.Errorf("failed to load CA certificate: %v", err) + } + + return &CertConnector { + clientCA: clientCA, + certHeader: c.CertHeader, + userIDKey: c.UserIDKey, + userNameKey: c.UserNameKey, + preferredUserNameKey: c.PreferredUserNameKey, + groupKey: c.GroupKey, + logger: logger, + }, nil +} + +// Close is a no-op for this connector +func (c *CertConnector) Close() error { + return nil +} + +// LoginURL implements connector.CallbackConnector +func (c *CertConnector) LoginURL(s connector.Scopes, callbackURL, state string) (string, error) { + u, err := url.Parse(callbackURL) + if err != nil { + return "", fmt.Errorf("failed to parse callback URL: %v", err) + } + + q := u.Query() + q.Set("state", state) + u.RawQuery = q.Encode() + + return u.String(), nil +} + +// HandleCallback implements connector.CallbackConnector +func (c *CertConnector) HandleCallback(s connector.Scopes, r *http.Request) (identity connector.Identity, err error) { + cert, err := c.ExtractCertificate(r) + if err != nil { + c.logger.Error("failed to extract certificate", "error", err) + return identity, err + } + + return c.ValidateCertificate(r.Context(), cert) +} + +// ExtractCertificate extract the client certificate from the request +func (c *CertConnector) ExtractCertificate(r *http.Request) (*x509.Certificate, error) { + // Check if the certificate is in the TLS connector + if r.TLS != nil && len(r.TLS.PeerCertificates) > 0 { + return r.TLS.PeerCertificates[0], nil + } + + // Check the header (for proxy cases) + c.logger.Debug("I have this cerHeader configured", "certHeader", c.certHeader) + if c.certHeader != "" { + certHeader := r.Header.Get(c.certHeader) + if certHeader != "" { + certData, err := base64.StdEncoding.DecodeString(certHeader) + if err != nil { + return nil, errors.New("failed decoding certificate PEM") + } + block, _ := pem.Decode([]byte(certData)) + if block == nil { + return nil, errors.New("failed to parse certificate PEM") + } + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return nil, fmt.Errorf("failed to parse certificate: %v", err) + } + return cert, nil + } + } + + return nil, errors.New("no client certificate found") +} + +// ValidateCertificate validates the certificate against the CA pool (implements CertificateConnector) +func (c *CertConnector) ValidateCertificate(ctx context.Context, cert *x509.Certificate) (identity connector.Identity, err error) { + // Verify the certificate + _, err = cert.Verify(x509.VerifyOptions{ + Roots: c.clientCA, + // TODO maybe more verification options? + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + }) + if err != nil { + c.logger.Error("certificate validation failed", "error", err) + return identity, fmt.Errorf("certificate validation failed: %v", err) + } + + // Extract value to be used as userID from certificate + var userID string + if c.userIDKey != "" { + userID = getValueFromCertificate(cert, c.userIDKey) + } else { + defaultUserIDKey := "0.9.2342.19200300.100.1.1" // OID for UID + userID = getValueFromCertificate(cert, defaultUserIDKey) + } + // safe guard + if userID == "" { + userID = cert.Subject.CommonName + } + + // Extract value to be used as username from certificate + var userName string + if c.userNameKey != "" { + userName = getValueFromCertificate(cert, c.userNameKey) + } else { + userName = cert.Subject.CommonName + } + + // Extract value to be used as preferredUsername from certificate + var preferredUserName string + if c.preferredUserNameKey != "" { + preferredUserName = getValueFromCertificate(cert, c.preferredUserNameKey) + } else { + preferredUserName = userName + } + + // Extract email from certificate + var email string + if cert.EmailAddresses != nil && len(cert.EmailAddresses) > 0 { + email = cert.EmailAddresses[0] + } + + // Extract organization from certificate (used as a group identifier) + var groups []string + if c.groupKey != "" { + groups = append(groups, getValueFromCertificate(cert, c.groupKey)) + } else { + defaultGroupKey := "2.5.4.10" // OID for Organization + groups = append(groups, getValueFromCertificate(cert, defaultGroupKey)) + } + + // Extract identity information from the certificate + identity = connector.Identity{ + UserID: userID, + Username: userName, + PreferredUsername: preferredUserName, + Email: email, + EmailVerified: false, + Groups: groups, + } + + c.logger.Info("certificate validation successful", "user", identity, "subject", cert.Subject) + return identity, nil +} + +func getValueFromCertificate(cert *x509.Certificate, key string) string { + var value string + for _, name := range cert.Subject.Names { + if name.Type.String() == key { + value = name.Value.(string) + break + } + } + return value +} diff --git a/connector/cert/cert_test.go b/connector/cert/cert_test.go new file mode 100644 index 0000000000..a852c67401 --- /dev/null +++ b/connector/cert/cert_test.go @@ -0,0 +1,188 @@ +package cert + +import ( + "crypto/rand" + "crypto/rsa" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/base64" + "encoding/pem" + "log/slog" + "math/big" + "net/http" + "net/http/httptest" + "os" + "testing" + "time" + + "github.com/dexidp/dex/connector" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestOpen(t *testing.T) { + // Create a temporary CA cert file for testing + caFile, err := os.CreateTemp("", "ca-*.pem") + assert.NoError(t, err, "Failed to create temp CA file") + defer os.Remove(caFile.Name()) + + // Generate a test CA certificate + caCert, _, err := generateCACertificate() + require.NoError(t, err, "Failed to generate CA certificate") + + // Write the CA certificate into to the temp file + err = pem.Encode(caFile, &pem.Block{Type: "CERTIFICATE", Bytes: caCert.Raw}) + require.NoError(t, err, "Failed to write CA cert to file") + caFile.Close() + + // Create a config with the test CA cert file + config := Config{ + ClientCAPath: caFile.Name(), + CertHeader: "X-Client-Cert", + } + + // Create a logger + logger := slog.New(slog.NewTextHandler(os.Stdout, nil)) + + // Open the certConnector + conn, err := config.Open("test-connector", logger) + assert.NoError(t, err, "Failed to open connector") + + // Check if it implements the connector interface + certConnector, ok := conn.(*CertConnector) + assert.True(t, ok, "Returned connector is not a certConnector") + + assert.Equal(t, config.CertHeader, certConnector.certHeader, "Mismatched certHeader") +} + +func TestLoginURL(t *testing.T) { + certConnector := &CertConnector{} + + loginURL, err := certConnector.LoginURL(connector.Scopes{}, "https://example.com/auth/callback", "test-state") + assert.NoError(t, err, "LoginURL failed") + + expected := "https://example.com/auth/callback?state=test-state" + assert.Equal(t, expected, loginURL, "Unexpected LoginURL") +} + +func TestHandleCallback(t *testing.T) { + // Generate a test CA certificate + caCert, caPrivKey, err := generateCACertificate() + require.NoError(t, err, "Failed to generate CA certificate") + + // Generate a test client certificate + clientCert, err := generateClientCertificate(caCert, caPrivKey) + require.NoError(t, err, "Failed to generate client certificate") + + caPool := x509.NewCertPool() + caPool.AddCert(caCert) + + certConnector := &CertConnector{ + clientCA: caPool, + certHeader: "X-Client-Cert", + logger: slog.New(slog.NewTextHandler(os.Stdout, nil)), + } + + // Test with valid certificate in TLS + t.Run("ValidCertificateTLS", func(t *testing.T) { + req := &http.Request{ + TLS: &tls.ConnectionState{ + PeerCertificates: []*x509.Certificate{clientCert}, + }, + } + + identity, err := certConnector.HandleCallback(connector.Scopes{}, req) + assert.NoError(t, err, "HandleCallback failed") + assert.Equal(t, "CUID2048", identity.UserID, "Unexpected UserID") + }) + // Test with valid certificate in header + t.Run("ValidCertificateInHeader", func(t *testing.T) { + certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: clientCert.Raw}) + req := httptest.NewRequest("GET", "/callback", nil) + req.Header.Set("X-Client-Cert", base64.StdEncoding.EncodeToString(certPEM)) + + identity, err := certConnector.HandleCallback(connector.Scopes{}, req) + assert.NoError(t, err, "HandleCallback failed") + assert.Equal(t, "CUID2048", identity.UserID, "Unexpected UserID") + }) + // Test with no certificate + t.Run("NoCertificate", func(t *testing.T) { + req := httptest.NewRequest("GET", "/callback", nil) + + _, err := certConnector.HandleCallback(connector.Scopes{}, req) + assert.Error(t, err, "Expected error for no certificate") + }) +} + +func generateCACertificate() (*x509.Certificate, *rsa.PrivateKey, error) { + caPrivKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + return nil, nil, err + } + + caTemplate := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{ + Country: []string{"FR"}, + Organization: []string{"Orange CA"}, + CommonName: "Test CA", + }, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Hour * 24), + KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + IsCA: true, + } + + caBytes, err := x509.CreateCertificate(rand.Reader, &caTemplate, &caTemplate, &caPrivKey.PublicKey, caPrivKey) + if err != nil { + return nil, nil, err + } + + caCert, err := x509.ParseCertificate(caBytes) + if err != nil { + return nil, nil, err + } + + return caCert, caPrivKey, nil +} + +func generateClientCertificate(caCert *x509.Certificate, caPrivKey *rsa.PrivateKey) (*x509.Certificate, error) { + clientPrivKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + return nil, err + } + + clientTemplate := x509.Certificate{ + SerialNumber: big.NewInt(2), + Subject: pkix.Name{ + Country: []string{"FR"}, + Organization: []string{"Orange"}, + CommonName: "Test Client", + ExtraNames: []pkix.AttributeTypeAndValue{ + { + Type: []int{0, 9, 2342, 19200300, 100, 1, 1}, // OID for UID + Value: "CUID2048", + }, + }, + }, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Hour * 24), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + } + + clientBytes, err := x509.CreateCertificate(rand.Reader, &clientTemplate, caCert, &clientPrivKey.PublicKey, caPrivKey) + if err != nil { + return nil, err + } + + clientCert, err := x509.ParseCertificate(clientBytes) + if err != nil { + return nil, err + } + + return clientCert, nil +} diff --git a/server/handlers.go b/server/handlers.go index 63cb612295..17390c9805 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -844,7 +844,7 @@ func (s *Server) handleToken(w http.ResponseWriter, r *http.Request) { grantType := r.PostFormValue("grant_type") if !contains(s.supportedGrantTypes, grantType) { - s.logger.ErrorContext(r.Context(), "unsupported grant type", "grant_type", grantType) + s.logger.ErrorContext(r.Context(), "unsupported grant type", "grant_type", grantType, "supportedGrantTypes", s.supportedGrantTypes) s.tokenErrHelper(w, errUnsupportedGrantType, "", http.StatusBadRequest) return } @@ -859,6 +859,8 @@ func (s *Server) handleToken(w http.ResponseWriter, r *http.Request) { s.withClientFromStorage(w, r, s.handlePasswordGrant) case grantTypeTokenExchange: s.withClientFromStorage(w, r, s.handleTokenExchange) + case grantTypeCertificate: + s.handleCertificateToken(w, r) default: s.tokenErrHelper(w, errUnsupportedGrantType, "", http.StatusBadRequest) } @@ -876,6 +878,76 @@ func (s *Server) calculateCodeChallenge(codeVerifier, codeChallengeMethod string } } +func (s *Server) handleCertificateToken(w http.ResponseWriter, r *http.Request) { + if err := r.ParseForm(); err != nil { + s.tokenErrHelper(w, errInvalidRequest, "", http.StatusBadRequest) + return + } + q := r.Form + + nonce := q.Get("nonce") + if nonce == "" { + s.tokenErrHelper(w, errInvalidRequest, "No nonce provided", http.StatusBadRequest) + return + } + // Some clients, like the old go-oidc, provide extra whitespace. Tolerate this. + scopes := strings.Fields(q.Get("scope")) + // TODO check for unrecognized/invalid scopes + + connID := q.Get("connector_id") + if connID == "" { + s.tokenErrHelper(w, errInvalidRequest, "No connector_id provided", http.StatusBadRequest) + return + } + + conn, err := s.getConnector(connID) + if err != nil { + s.tokenErrHelper(w, errInvalidRequest, "Invalid connector_id", http.StatusBadRequest) + return + } + + // TODO create own connector instead of the callback connector + certConnector, ok := conn.Connector.(connector.CallbackConnector) + if !ok { + s.tokenErrHelper(w, errInvalidRequest, "Connector doesnt not support certificate authentication", http.StatusBadRequest) + return + } + + identity, err := certConnector.HandleCallback(parseScopes(scopes), r) + if err != nil { + s.tokenErrHelper(w, errInvalidRequest, "Invalid certificate", http.StatusBadRequest) + return + } + + claims := storage.Claims{ + UserID: identity.UserID, + Username: identity.Username, + PreferredUsername: identity.PreferredUsername, + Email: identity.Email, + EmailVerified: identity.EmailVerified, + Groups: identity.Groups, + } + + accessToken, _, err := s.newAccessToken(r.Context(), q.Get("client_id"), claims, scopes, nonce, connID) + if err != nil { + s.logger.ErrorContext(r.Context(), "certificate grant failed to create new access token", "err", err) + s.tokenErrHelper(w, errServerError, "", http.StatusInternalServerError) + return + } + + idToken, expiry, err := s.newIDToken(r.Context(), q.Get("client_id"), claims, scopes, nonce, accessToken, "", connID) + if err != nil { + s.logger.ErrorContext(r.Context(), "certificate grant failed to create new ID token", "err", err) + s.tokenErrHelper(w, errServerError, "", http.StatusInternalServerError) + return + } + + // TODO: make refresh token? + + resp := s.toAccessTokenResponse(idToken, accessToken, "", expiry) + s.writeAccessToken(w, resp) +} + // handle an access token request https://tools.ietf.org/html/rfc6749#section-4.1.3 func (s *Server) handleAuthCode(w http.ResponseWriter, r *http.Request, client storage.Client) { ctx := r.Context() diff --git a/server/handlers_test.go b/server/handlers_test.go index d32101b1cf..2695ccb098 100644 --- a/server/handlers_test.go +++ b/server/handlers_test.go @@ -63,6 +63,7 @@ func TestHandleDiscovery(t *testing.T) { Introspect: fmt.Sprintf("%s/token/introspect", httpServer.URL), GrantTypes: []string{ "authorization_code", + "certificate", "refresh_token", "urn:ietf:params:oauth:grant-type:device_code", "urn:ietf:params:oauth:grant-type:token-exchange", diff --git a/server/oauth2.go b/server/oauth2.go index ec972beab1..911d660d12 100644 --- a/server/oauth2.go +++ b/server/oauth2.go @@ -133,6 +133,7 @@ const ( grantTypePassword = "password" grantTypeDeviceCode = "urn:ietf:params:oauth:grant-type:device_code" grantTypeTokenExchange = "urn:ietf:params:oauth:grant-type:token-exchange" + grantTypeCertificate = "certificate" ) const ( diff --git a/server/server.go b/server/server.go index 1cf71c5038..44bbe886f5 100644 --- a/server/server.go +++ b/server/server.go @@ -33,6 +33,7 @@ import ( "github.com/dexidp/dex/connector/atlassiancrowd" "github.com/dexidp/dex/connector/authproxy" "github.com/dexidp/dex/connector/bitbucketcloud" + "github.com/dexidp/dex/connector/cert" "github.com/dexidp/dex/connector/gitea" "github.com/dexidp/dex/connector/github" "github.com/dexidp/dex/connector/gitlab" @@ -236,6 +237,7 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy) grantTypeRefreshToken: true, grantTypeDeviceCode: true, grantTypeTokenExchange: true, + grantTypeCertificate: true, } supportedRes := make(map[string]bool) @@ -640,6 +642,7 @@ var ConnectorsConfig = map[string]func() ConnectorConfig{ "bitbucket-cloud": func() ConnectorConfig { return new(bitbucketcloud.Config) }, "openshift": func() ConnectorConfig { return new(openshift.Config) }, "atlassian-crowd": func() ConnectorConfig { return new(atlassiancrowd.Config) }, + "cert": func() ConnectorConfig { return new(cert.Config) }, // Keep around for backwards compatibility. "samlExperimental": func() ConnectorConfig { return new(saml.Config) }, } diff --git a/server/server_test.go b/server/server_test.go index 8936c90a07..8f0c3200cd 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -101,6 +101,7 @@ func newTestServer(ctx context.Context, t *testing.T, updateConfig func(c *Confi grantTypeTokenExchange, grantTypeImplicit, grantTypePassword, + grantTypeCertificate, }, } if updateConfig != nil { @@ -1760,7 +1761,7 @@ func TestServerSupportedGrants(t *testing.T) { { name: "Simple", config: func(c *Config) {}, - resGrants: []string{grantTypeAuthorizationCode, grantTypeRefreshToken, grantTypeDeviceCode, grantTypeTokenExchange}, + resGrants: []string{grantTypeAuthorizationCode, grantTypeCertificate, grantTypeRefreshToken, grantTypeDeviceCode, grantTypeTokenExchange}, }, { name: "Minimal", @@ -1770,12 +1771,12 @@ func TestServerSupportedGrants(t *testing.T) { { name: "With password connector", config: func(c *Config) { c.PasswordConnector = "local" }, - resGrants: []string{grantTypeAuthorizationCode, grantTypePassword, grantTypeRefreshToken, grantTypeDeviceCode, grantTypeTokenExchange}, + resGrants: []string{grantTypeAuthorizationCode, grantTypeCertificate, grantTypePassword, grantTypeRefreshToken, grantTypeDeviceCode, grantTypeTokenExchange}, }, { name: "With token response", config: func(c *Config) { c.SupportedResponseTypes = append(c.SupportedResponseTypes, responseTypeToken) }, - resGrants: []string{grantTypeAuthorizationCode, grantTypeImplicit, grantTypeRefreshToken, grantTypeDeviceCode, grantTypeTokenExchange}, + resGrants: []string{grantTypeAuthorizationCode, grantTypeCertificate, grantTypeImplicit, grantTypeRefreshToken, grantTypeDeviceCode, grantTypeTokenExchange}, }, { name: "All", @@ -1783,7 +1784,7 @@ func TestServerSupportedGrants(t *testing.T) { c.PasswordConnector = "local" c.SupportedResponseTypes = append(c.SupportedResponseTypes, responseTypeToken) }, - resGrants: []string{grantTypeAuthorizationCode, grantTypeImplicit, grantTypePassword, grantTypeRefreshToken, grantTypeDeviceCode, grantTypeTokenExchange}, + resGrants: []string{grantTypeAuthorizationCode, grantTypeCertificate, grantTypeImplicit, grantTypePassword, grantTypeRefreshToken, grantTypeDeviceCode, grantTypeTokenExchange}, }, } From 48f24141d43feba2c234f641bd9bc000a9e62467 Mon Sep 17 00:00:00 2001 From: Houssem Ben Mabrouk Date: Fri, 25 Oct 2024 13:42:04 +0200 Subject: [PATCH 02/10] dedicated certificate connector --- connector/cert/cert.go | 40 +++++++++---------------------------- connector/cert/cert_test.go | 27 ++++++++++--------------- connector/connector.go | 9 +++++++++ server/handlers.go | 13 ++++++++---- 4 files changed, 38 insertions(+), 51 deletions(-) diff --git a/connector/cert/cert.go b/connector/cert/cert.go index db44468cb1..1bd4a9e7f1 100644 --- a/connector/cert/cert.go +++ b/connector/cert/cert.go @@ -1,7 +1,6 @@ package cert import ( - "context" "crypto/x509" "encoding/base64" "encoding/pem" @@ -9,7 +8,6 @@ import ( "fmt" "log/slog" "net/http" - "net/url" "os" "github.com/dexidp/dex/connector" @@ -39,7 +37,7 @@ type CertConnector struct { } var ( - _ connector.CallbackConnector = (*CertConnector)(nil) + _ connector.CertificateConnector = (*CertConnector)(nil) ) // loadCACert loads the CA certificate from the file @@ -85,33 +83,8 @@ func (c *CertConnector) Close() error { return nil } -// LoginURL implements connector.CallbackConnector -func (c *CertConnector) LoginURL(s connector.Scopes, callbackURL, state string) (string, error) { - u, err := url.Parse(callbackURL) - if err != nil { - return "", fmt.Errorf("failed to parse callback URL: %v", err) - } - - q := u.Query() - q.Set("state", state) - u.RawQuery = q.Encode() - - return u.String(), nil -} - -// HandleCallback implements connector.CallbackConnector -func (c *CertConnector) HandleCallback(s connector.Scopes, r *http.Request) (identity connector.Identity, err error) { - cert, err := c.ExtractCertificate(r) - if err != nil { - c.logger.Error("failed to extract certificate", "error", err) - return identity, err - } - - return c.ValidateCertificate(r.Context(), cert) -} - // ExtractCertificate extract the client certificate from the request -func (c *CertConnector) ExtractCertificate(r *http.Request) (*x509.Certificate, error) { +func (c *CertConnector) ExtractCertificate(r *http.Request) (cert *x509.Certificate, err error) { // Check if the certificate is in the TLS connector if r.TLS != nil && len(r.TLS.PeerCertificates) > 0 { return r.TLS.PeerCertificates[0], nil @@ -141,8 +114,13 @@ func (c *CertConnector) ExtractCertificate(r *http.Request) (*x509.Certificate, return nil, errors.New("no client certificate found") } -// ValidateCertificate validates the certificate against the CA pool (implements CertificateConnector) -func (c *CertConnector) ValidateCertificate(ctx context.Context, cert *x509.Certificate) (identity connector.Identity, err error) { +// ValidateCertificate validates the certificate against the CA pool +func (c *CertConnector) ValidateCertificate(cert *x509.Certificate) (identity connector.Identity, err error) { + if cert == nil { + c.logger.Error("certificate validation failed", "error", "Certificate is nil") + return identity, fmt.Errorf("certificate validation failed: Certificate is nil") + } + // Verify the certificate _, err = cert.Verify(x509.VerifyOptions{ Roots: c.clientCA, diff --git a/connector/cert/cert_test.go b/connector/cert/cert_test.go index a852c67401..be0d97cef5 100644 --- a/connector/cert/cert_test.go +++ b/connector/cert/cert_test.go @@ -16,7 +16,6 @@ import ( "testing" "time" - "github.com/dexidp/dex/connector" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -56,17 +55,7 @@ func TestOpen(t *testing.T) { assert.Equal(t, config.CertHeader, certConnector.certHeader, "Mismatched certHeader") } -func TestLoginURL(t *testing.T) { - certConnector := &CertConnector{} - - loginURL, err := certConnector.LoginURL(connector.Scopes{}, "https://example.com/auth/callback", "test-state") - assert.NoError(t, err, "LoginURL failed") - - expected := "https://example.com/auth/callback?state=test-state" - assert.Equal(t, expected, loginURL, "Unexpected LoginURL") -} - -func TestHandleCallback(t *testing.T) { +func TestExtractValidateCertificate(t *testing.T) { // Generate a test CA certificate caCert, caPrivKey, err := generateCACertificate() require.NoError(t, err, "Failed to generate CA certificate") @@ -92,8 +81,11 @@ func TestHandleCallback(t *testing.T) { }, } - identity, err := certConnector.HandleCallback(connector.Scopes{}, req) - assert.NoError(t, err, "HandleCallback failed") + cert, err := certConnector.ExtractCertificate(req) + assert.NoError(t, err, "ExtractCertificate failed") + + identity, err := certConnector.ValidateCertificate(cert) + assert.NoError(t, err, "ValidateCertificate failed") assert.Equal(t, "CUID2048", identity.UserID, "Unexpected UserID") }) // Test with valid certificate in header @@ -102,7 +94,10 @@ func TestHandleCallback(t *testing.T) { req := httptest.NewRequest("GET", "/callback", nil) req.Header.Set("X-Client-Cert", base64.StdEncoding.EncodeToString(certPEM)) - identity, err := certConnector.HandleCallback(connector.Scopes{}, req) + cert, err := certConnector.ExtractCertificate(req) + assert.NoError(t, err, "ExtractCertificate failed") + + identity, err := certConnector.ValidateCertificate(cert) assert.NoError(t, err, "HandleCallback failed") assert.Equal(t, "CUID2048", identity.UserID, "Unexpected UserID") }) @@ -110,7 +105,7 @@ func TestHandleCallback(t *testing.T) { t.Run("NoCertificate", func(t *testing.T) { req := httptest.NewRequest("GET", "/callback", nil) - _, err := certConnector.HandleCallback(connector.Scopes{}, req) + _, err := certConnector.ExtractCertificate(req) assert.Error(t, err, "Expected error for no certificate") }) } diff --git a/connector/connector.go b/connector/connector.go index d812390f0c..3cf7ad2b30 100644 --- a/connector/connector.go +++ b/connector/connector.go @@ -3,6 +3,7 @@ package connector import ( "context" + "crypto/x509" "net/http" ) @@ -38,6 +39,14 @@ type Identity struct { ConnectorData []byte } +type CertificateConnector interface { + // ExtractCertificate retrueves the ckuebt certificate from the request + ExtractCertificate(r *http.Request) (cert *x509.Certificate, err error) + + // ValidateCertificate checks the provided certificate and returns an Identity + ValidateCertificate(cert *x509.Certificate) (identity Identity, err error) +} + // PasswordConnector is an interface implemented by connectors which take a // username and password. // Prompt() is used to inform the handler what to display in the password diff --git a/server/handlers.go b/server/handlers.go index 17390c9805..6d1bd9a375 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -906,19 +906,24 @@ func (s *Server) handleCertificateToken(w http.ResponseWriter, r *http.Request) return } - // TODO create own connector instead of the callback connector - certConnector, ok := conn.Connector.(connector.CallbackConnector) + certConnector, ok := conn.Connector.(connector.CertificateConnector) if !ok { - s.tokenErrHelper(w, errInvalidRequest, "Connector doesnt not support certificate authentication", http.StatusBadRequest) + s.tokenErrHelper(w, errInvalidRequest, "Connector does not support certificate authentication", http.StatusBadRequest) return } - identity, err := certConnector.HandleCallback(parseScopes(scopes), r) + cert, err := certConnector.ExtractCertificate(r) if err != nil { s.tokenErrHelper(w, errInvalidRequest, "Invalid certificate", http.StatusBadRequest) return } + identity, err := certConnector.ValidateCertificate(cert) + if err != nil { + s.tokenErrHelper(w, errInvalidRequest, "Unable to validate certificate", http.StatusBadRequest) + return + } + claims := storage.Claims{ UserID: identity.UserID, Username: identity.Username, From 591a3766df27de5556b7d2085aa94295342fa399 Mon Sep 17 00:00:00 2001 From: Houssem Ben Mabrouk Date: Mon, 28 Oct 2024 17:22:20 +0100 Subject: [PATCH 03/10] cert connector process the cert not the pem of the cert Signed-off-by: Houssem Ben Mabrouk --- connector/cert/cert.go | 9 ++------- connector/cert/cert_test.go | 3 +-- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/connector/cert/cert.go b/connector/cert/cert.go index 1bd4a9e7f1..c0b2ba0117 100644 --- a/connector/cert/cert.go +++ b/connector/cert/cert.go @@ -3,7 +3,6 @@ package cert import ( "crypto/x509" "encoding/base64" - "encoding/pem" "errors" "fmt" "log/slog" @@ -97,13 +96,9 @@ func (c *CertConnector) ExtractCertificate(r *http.Request) (cert *x509.Certific if certHeader != "" { certData, err := base64.StdEncoding.DecodeString(certHeader) if err != nil { - return nil, errors.New("failed decoding certificate PEM") + return nil, errors.New("failed decoding certificate") } - block, _ := pem.Decode([]byte(certData)) - if block == nil { - return nil, errors.New("failed to parse certificate PEM") - } - cert, err := x509.ParseCertificate(block.Bytes) + cert, err := x509.ParseCertificate(certData) if err != nil { return nil, fmt.Errorf("failed to parse certificate: %v", err) } diff --git a/connector/cert/cert_test.go b/connector/cert/cert_test.go index be0d97cef5..fcc24f6852 100644 --- a/connector/cert/cert_test.go +++ b/connector/cert/cert_test.go @@ -90,9 +90,8 @@ func TestExtractValidateCertificate(t *testing.T) { }) // Test with valid certificate in header t.Run("ValidCertificateInHeader", func(t *testing.T) { - certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: clientCert.Raw}) req := httptest.NewRequest("GET", "/callback", nil) - req.Header.Set("X-Client-Cert", base64.StdEncoding.EncodeToString(certPEM)) + req.Header.Set("X-Client-Cert", base64.StdEncoding.EncodeToString(clientCert.Raw)) cert, err := certConnector.ExtractCertificate(req) assert.NoError(t, err, "ExtractCertificate failed") From 85a6fd5a86cbaf9ec15c4f243a5e1b07afa1a80e Mon Sep 17 00:00:00 2001 From: Houssem Ben Mabrouk Date: Tue, 29 Oct 2024 17:19:00 +0100 Subject: [PATCH 04/10] fix lint + remove unecessary pr check step Signed-off-by: Houssem Ben Mabrouk --- .github/workflows/ci.yaml | 12 --------- connector/cert/cert.go | 18 +++++-------- connector/cert/cert_test.go | 28 ++++++++++---------- server/handlers.go | 52 +++++++++++++++++++++++++++++-------- 4 files changed, 61 insertions(+), 49 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 534edea15f..8943485f50 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -164,15 +164,3 @@ jobs: packages: write id-token: write security-events: write - - dependency-review: - name: Dependency review - runs-on: ubuntu-latest - if: github.event_name == 'pull_request' - - steps: - - name: Checkout repository - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - - - name: Dependency Review - uses: actions/dependency-review-action@5a2ce3f5b92ee19cbb1541a4984c76d921601d7c # v4.3.4 diff --git a/connector/cert/cert.go b/connector/cert/cert.go index c0b2ba0117..80b02b2ead 100644 --- a/connector/cert/cert.go +++ b/connector/cert/cert.go @@ -14,9 +14,9 @@ import ( type Config struct { // ClientCAPath is the path of the CA certificate used to validate client certificates - ClientCAPath string `json:"clientCAPath"` + ClientCAPath string `json:"clientCAPath"` // CertHeader is the name of the HTTP header containing the client certificate (if using a proxy) - CertHeader string `json:"certHeader"` + CertHeader string `json:"certHeader"` UserIDKey string `json:"userIDKey"` UserNameKey string `json:"userNameKey"` @@ -35,10 +35,6 @@ type CertConnector struct { logger *slog.Logger } -var ( - _ connector.CertificateConnector = (*CertConnector)(nil) -) - // loadCACert loads the CA certificate from the file func loadCACert(caCertFile string) (*x509.CertPool, error) { clientCA := x509.NewCertPool() @@ -66,7 +62,7 @@ func (c *Config) Open(id string, logger *slog.Logger) (connector.Connector, erro return nil, fmt.Errorf("failed to load CA certificate: %v", err) } - return &CertConnector { + return &CertConnector{ clientCA: clientCA, certHeader: c.CertHeader, userIDKey: c.UserIDKey, @@ -118,8 +114,7 @@ func (c *CertConnector) ValidateCertificate(cert *x509.Certificate) (identity co // Verify the certificate _, err = cert.Verify(x509.VerifyOptions{ - Roots: c.clientCA, - // TODO maybe more verification options? + Roots: c.clientCA, KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, }) if err != nil { @@ -132,7 +127,7 @@ func (c *CertConnector) ValidateCertificate(cert *x509.Certificate) (identity co if c.userIDKey != "" { userID = getValueFromCertificate(cert, c.userIDKey) } else { - defaultUserIDKey := "0.9.2342.19200300.100.1.1" // OID for UID + defaultUserIDKey := "0.9.2342.19200300.100.1.1" // OID for UID userID = getValueFromCertificate(cert, defaultUserIDKey) } // safe guard @@ -167,7 +162,7 @@ func (c *CertConnector) ValidateCertificate(cert *x509.Certificate) (identity co if c.groupKey != "" { groups = append(groups, getValueFromCertificate(cert, c.groupKey)) } else { - defaultGroupKey := "2.5.4.10" // OID for Organization + defaultGroupKey := "2.5.4.10" // OID for Organization groups = append(groups, getValueFromCertificate(cert, defaultGroupKey)) } @@ -181,7 +176,6 @@ func (c *CertConnector) ValidateCertificate(cert *x509.Certificate) (identity co Groups: groups, } - c.logger.Info("certificate validation successful", "user", identity, "subject", cert.Subject) return identity, nil } diff --git a/connector/cert/cert_test.go b/connector/cert/cert_test.go index fcc24f6852..5dbf125691 100644 --- a/connector/cert/cert_test.go +++ b/connector/cert/cert_test.go @@ -68,9 +68,9 @@ func TestExtractValidateCertificate(t *testing.T) { caPool.AddCert(caCert) certConnector := &CertConnector{ - clientCA: caPool, + clientCA: caPool, certHeader: "X-Client-Cert", - logger: slog.New(slog.NewTextHandler(os.Stdout, nil)), + logger: slog.New(slog.NewTextHandler(os.Stdout, nil)), } // Test with valid certificate in TLS @@ -118,16 +118,16 @@ func generateCACertificate() (*x509.Certificate, *rsa.PrivateKey, error) { caTemplate := x509.Certificate{ SerialNumber: big.NewInt(1), Subject: pkix.Name{ - Country: []string{"FR"}, + Country: []string{"FR"}, Organization: []string{"Orange CA"}, - CommonName: "Test CA", + CommonName: "Test CA", }, - NotBefore: time.Now(), - NotAfter: time.Now().Add(time.Hour * 24), - KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageDigitalSignature, - ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Hour * 24), + KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, BasicConstraintsValid: true, - IsCA: true, + IsCA: true, } caBytes, err := x509.CreateCertificate(rand.Reader, &caTemplate, &caTemplate, &caPrivKey.PublicKey, caPrivKey) @@ -152,9 +152,9 @@ func generateClientCertificate(caCert *x509.Certificate, caPrivKey *rsa.PrivateK clientTemplate := x509.Certificate{ SerialNumber: big.NewInt(2), Subject: pkix.Name{ - Country: []string{"FR"}, + Country: []string{"FR"}, Organization: []string{"Orange"}, - CommonName: "Test Client", + CommonName: "Test Client", ExtraNames: []pkix.AttributeTypeAndValue{ { Type: []int{0, 9, 2342, 19200300, 100, 1, 1}, // OID for UID @@ -162,9 +162,9 @@ func generateClientCertificate(caCert *x509.Certificate, caPrivKey *rsa.PrivateK }, }, }, - NotBefore: time.Now(), - NotAfter: time.Now().Add(time.Hour * 24), - KeyUsage: x509.KeyUsageDigitalSignature, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Hour * 24), + KeyUsage: x509.KeyUsageDigitalSignature, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, } diff --git a/server/handlers.go b/server/handlers.go index 6d1bd9a375..26f9246ce5 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -860,7 +860,7 @@ func (s *Server) handleToken(w http.ResponseWriter, r *http.Request) { case grantTypeTokenExchange: s.withClientFromStorage(w, r, s.handleTokenExchange) case grantTypeCertificate: - s.handleCertificateToken(w, r) + s.withClientFromStorage(w, r, s.handleCertificateToken) default: s.tokenErrHelper(w, errUnsupportedGrantType, "", http.StatusBadRequest) } @@ -878,7 +878,7 @@ func (s *Server) calculateCodeChallenge(codeVerifier, codeChallengeMethod string } } -func (s *Server) handleCertificateToken(w http.ResponseWriter, r *http.Request) { +func (s *Server) handleCertificateToken(w http.ResponseWriter, r *http.Request, client storage.Client) { if err := r.ParseForm(); err != nil { s.tokenErrHelper(w, errInvalidRequest, "", http.StatusBadRequest) return @@ -890,9 +890,41 @@ func (s *Server) handleCertificateToken(w http.ResponseWriter, r *http.Request) s.tokenErrHelper(w, errInvalidRequest, "No nonce provided", http.StatusBadRequest) return } - // Some clients, like the old go-oidc, provide extra whitespace. Tolerate this. + scopes := strings.Fields(q.Get("scope")) - // TODO check for unrecognized/invalid scopes + // Parse the scopes if they are passed + var ( + unrecognized []string + invalidScopes []string + ) + for _, scope := range scopes { + switch scope { + case scopeOfflineAccess, scopeEmail, scopeProfile, scopeGroups, scopeFederatedID: + default: + peerID, ok := parseCrossClientScope(scope) + if !ok { + unrecognized = append(unrecognized, scope) + continue + } + + isTrusted, err := s.validateCrossClientTrust(r.Context(), client.ID, peerID) + if err != nil { + s.tokenErrHelper(w, errInvalidClient, fmt.Sprintf("Error validating cross client trust %v.", err), http.StatusBadRequest) + return + } + if !isTrusted { + invalidScopes = append(invalidScopes, scope) + } + } + } + if len(unrecognized) > 0 { + s.tokenErrHelper(w, errInvalidRequest, fmt.Sprintf("Unrecognized scope(s) %q", unrecognized), http.StatusBadRequest) + return + } + if len(invalidScopes) > 0 { + s.tokenErrHelper(w, errInvalidRequest, fmt.Sprintf("Client can't request scope(s) %q", invalidScopes), http.StatusBadRequest) + return + } connID := q.Get("connector_id") if connID == "" { @@ -925,12 +957,12 @@ func (s *Server) handleCertificateToken(w http.ResponseWriter, r *http.Request) } claims := storage.Claims{ - UserID: identity.UserID, - Username: identity.Username, + UserID: identity.UserID, + Username: identity.Username, PreferredUsername: identity.PreferredUsername, - Email: identity.Email, - EmailVerified: identity.EmailVerified, - Groups: identity.Groups, + Email: identity.Email, + EmailVerified: identity.EmailVerified, + Groups: identity.Groups, } accessToken, _, err := s.newAccessToken(r.Context(), q.Get("client_id"), claims, scopes, nonce, connID) @@ -947,8 +979,6 @@ func (s *Server) handleCertificateToken(w http.ResponseWriter, r *http.Request) return } - // TODO: make refresh token? - resp := s.toAccessTokenResponse(idToken, accessToken, "", expiry) s.writeAccessToken(w, resp) } From 7a1a80a941a86432d4f97e6a9b3260bea4d0504f Mon Sep 17 00:00:00 2001 From: Houssem Ben Mabrouk Date: Wed, 30 Oct 2024 11:10:18 +0100 Subject: [PATCH 05/10] update trivy image to 0.28.0 Signed-off-by: Houssem Ben Mabrouk --- .github/workflows/artifacts.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/artifacts.yaml b/.github/workflows/artifacts.yaml index 81bc378654..74a4f9f282 100644 --- a/.github/workflows/artifacts.yaml +++ b/.github/workflows/artifacts.yaml @@ -193,7 +193,7 @@ jobs: if: inputs.publish - name: Run Trivy vulnerability scanner - uses: aquasecurity/trivy-action@6e7b7d1fd3e4fef0c5fa8cce1229c54b2c9bd0d8 # 0.24.0 + uses: aquasecurity/trivy-action@0.28.0 with: input: image format: sarif From d3cf6ddc04aa054310ddaa92fd1e600720b73614 Mon Sep 17 00:00:00 2001 From: Houssem Ben Mabrouk Date: Tue, 5 Nov 2024 14:20:03 +0100 Subject: [PATCH 06/10] remove extra code due to debug Signed-off-by: Houssem Ben Mabrouk --- server/handlers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/handlers.go b/server/handlers.go index 26f9246ce5..7b12aeec4a 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -844,7 +844,7 @@ func (s *Server) handleToken(w http.ResponseWriter, r *http.Request) { grantType := r.PostFormValue("grant_type") if !contains(s.supportedGrantTypes, grantType) { - s.logger.ErrorContext(r.Context(), "unsupported grant type", "grant_type", grantType, "supportedGrantTypes", s.supportedGrantTypes) + s.logger.ErrorContext(r.Context(), "unsupported grant type", "grant_type", grantType) s.tokenErrHelper(w, errUnsupportedGrantType, "", http.StatusBadRequest) return } From 92dedbf0e663f3bfe413bfddd9cbaa1be1f5dfed Mon Sep 17 00:00:00 2001 From: Houssem Ben Mabrouk Date: Wed, 6 Nov 2024 16:35:23 +0100 Subject: [PATCH 07/10] handle multiple root certificates Signed-off-by: Houssem Ben Mabrouk --- connector/cert/cert.go | 55 +++++++++++++++++++++++-------------- connector/cert/cert_test.go | 6 ++-- 2 files changed, 38 insertions(+), 23 deletions(-) diff --git a/connector/cert/cert.go b/connector/cert/cert.go index 80b02b2ead..fcc64b0699 100644 --- a/connector/cert/cert.go +++ b/connector/cert/cert.go @@ -13,8 +13,8 @@ import ( ) type Config struct { - // ClientCAPath is the path of the CA certificate used to validate client certificates - ClientCAPath string `json:"clientCAPath"` + // RootCAs are the paths of the certificates for client certificate validation + RootCAs []string `json:"rootCAs"` // CertHeader is the name of the HTTP header containing the client certificate (if using a proxy) CertHeader string `json:"certHeader"` @@ -26,7 +26,7 @@ type Config struct { // CertConnector implements the CallbackConnector interface type CertConnector struct { - clientCA *x509.CertPool + rootCAs []*x509.CertPool certHeader string userIDKey string userNameKey string @@ -44,7 +44,7 @@ func loadCACert(caCertFile string) (*x509.CertPool, error) { } if !clientCA.AppendCertsFromPEM(caCertBytes) { - return nil, errors.New("failed to append CA certs from PEM file") + return nil, fmt.Errorf("no certs found in root CA file %q", caCertFile) } return clientCA, nil @@ -52,18 +52,21 @@ func loadCACert(caCertFile string) (*x509.CertPool, error) { // Open initializes the PKI Connector func (c *Config) Open(id string, logger *slog.Logger) (connector.Connector, error) { - if c.ClientCAPath == "" { - return nil, errors.New("missing required config field 'clientCAPath'") + if len(c.RootCAs) == 0 { + return nil, errors.New("missing required config field 'rootCAs'") } - // TODO: maybe support multiple CAs - clientCA, err := loadCACert(c.ClientCAPath) - if err != nil { - return nil, fmt.Errorf("failed to load CA certificate: %v", err) + var rootCAs []*x509.CertPool + for _, rootCA := range c.RootCAs { + pool, err := loadCACert(rootCA) + if err != nil { + return nil, fmt.Errorf("failed to load CA certificate: %v", err) + } + rootCAs = append(rootCAs, pool) } return &CertConnector{ - clientCA: clientCA, + rootCAs: rootCAs, certHeader: c.CertHeader, userIDKey: c.UserIDKey, userNameKey: c.UserNameKey, @@ -86,7 +89,6 @@ func (c *CertConnector) ExtractCertificate(r *http.Request) (cert *x509.Certific } // Check the header (for proxy cases) - c.logger.Debug("I have this cerHeader configured", "certHeader", c.certHeader) if c.certHeader != "" { certHeader := r.Header.Get(c.certHeader) if certHeader != "" { @@ -112,14 +114,27 @@ func (c *CertConnector) ValidateCertificate(cert *x509.Certificate) (identity co return identity, fmt.Errorf("certificate validation failed: Certificate is nil") } - // Verify the certificate - _, err = cert.Verify(x509.VerifyOptions{ - Roots: c.clientCA, - KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, - }) - if err != nil { - c.logger.Error("certificate validation failed", "error", err) - return identity, fmt.Errorf("certificate validation failed: %v", err) + // Verify the certificate against all configured rootCAs + // Only one must successfully verifies the client certificate + validClientCertificate := true + verificationErrors := []error{} + for _, rootCA := range c.rootCAs { + validClientCertificate = true + _, err = cert.Verify(x509.VerifyOptions{ + Roots: rootCA, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + }) + if err != nil { + validClientCertificate = false + verificationErrors = append(verificationErrors, err) + } + if validClientCertificate { + break + } + } + if !validClientCertificate { + c.logger.Error("certificate validation failed", "errors", verificationErrors) + return identity, fmt.Errorf("certificate validation failed: %v", verificationErrors) } // Extract value to be used as userID from certificate diff --git a/connector/cert/cert_test.go b/connector/cert/cert_test.go index 5dbf125691..23c1c7ac43 100644 --- a/connector/cert/cert_test.go +++ b/connector/cert/cert_test.go @@ -37,8 +37,8 @@ func TestOpen(t *testing.T) { // Create a config with the test CA cert file config := Config{ - ClientCAPath: caFile.Name(), - CertHeader: "X-Client-Cert", + RootCAs: []string{caFile.Name()}, + CertHeader: "X-Client-Cert", } // Create a logger @@ -68,7 +68,7 @@ func TestExtractValidateCertificate(t *testing.T) { caPool.AddCert(caCert) certConnector := &CertConnector{ - clientCA: caPool, + rootCAs: []*x509.CertPool{caPool}, certHeader: "X-Client-Cert", logger: slog.New(slog.NewTextHandler(os.Stdout, nil)), } From a86e5da9c2ab6f7404404d1c11af6e8957be285d Mon Sep 17 00:00:00 2001 From: Houssem Ben Mabrouk Date: Thu, 7 Nov 2024 11:43:10 +0100 Subject: [PATCH 08/10] add back oidc scope to cert connector Signed-off-by: Houssem Ben Mabrouk --- server/handlers.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/server/handlers.go b/server/handlers.go index 7b12aeec4a..03d5cfc965 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -897,8 +897,11 @@ func (s *Server) handleCertificateToken(w http.ResponseWriter, r *http.Request, unrecognized []string invalidScopes []string ) + hasOpenIDScope := false for _, scope := range scopes { switch scope { + case scopeOpenID: + hasOpenIDScope = true case scopeOfflineAccess, scopeEmail, scopeProfile, scopeGroups, scopeFederatedID: default: peerID, ok := parseCrossClientScope(scope) @@ -917,6 +920,10 @@ func (s *Server) handleCertificateToken(w http.ResponseWriter, r *http.Request, } } } + if !hasOpenIDScope { + s.tokenErrHelper(w, errInvalidRequest, `Missing required scope(s) ["openid"].`, http.StatusBadRequest) + return + } if len(unrecognized) > 0 { s.tokenErrHelper(w, errInvalidRequest, fmt.Sprintf("Unrecognized scope(s) %q", unrecognized), http.StatusBadRequest) return From 12a393f7ae7ecee5fd221c780ca32b3a5b5961d6 Mon Sep 17 00:00:00 2001 From: Houssem Ben Mabrouk Date: Wed, 13 Nov 2024 13:53:40 +0100 Subject: [PATCH 09/10] fix lint Signed-off-by: Houssem Ben Mabrouk --- connector/cert/cert.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/connector/cert/cert.go b/connector/cert/cert.go index fcc64b0699..0a1fc6130f 100644 --- a/connector/cert/cert.go +++ b/connector/cert/cert.go @@ -56,7 +56,7 @@ func (c *Config) Open(id string, logger *slog.Logger) (connector.Connector, erro return nil, errors.New("missing required config field 'rootCAs'") } - var rootCAs []*x509.CertPool + rootCAs := []*x509.CertPool{} for _, rootCA := range c.RootCAs { pool, err := loadCACert(rootCA) if err != nil { From b83f699a5d49cbde131813612e6dd52cc38540da Mon Sep 17 00:00:00 2001 From: Houssem Ben Mabrouk Date: Mon, 30 Dec 2024 11:42:35 +0100 Subject: [PATCH 10/10] fix typo on comment Signed-off-by: Houssem Ben Mabrouk --- connector/cert/cert.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/connector/cert/cert.go b/connector/cert/cert.go index 0a1fc6130f..cd32baf5d9 100644 --- a/connector/cert/cert.go +++ b/connector/cert/cert.go @@ -115,7 +115,7 @@ func (c *CertConnector) ValidateCertificate(cert *x509.Certificate) (identity co } // Verify the certificate against all configured rootCAs - // Only one must successfully verifies the client certificate + // Only one must successfully verify the client certificate validClientCertificate := true verificationErrors := []error{} for _, rootCA := range c.rootCAs {