fix(apigw): harden SAML/OIDC integration security and error handling

- Session leak: add defer-based cleanup on error paths in both OIDC and
  SAML callback handlers so sessions are always deleted if document
  storage or credential creation fails
- SAML assertion time validation: check NotBefore/NotOnOrAfter per
  SAML 2.0 §2.5.1; reject expired or future-dated assertions
- SAML nil guards: validate IdP metadata has SSO descriptors, and
  assertion has Subject/NameID/Conditions before accessing them
- Dynamic registration: limit response body to 1MB via io.LimitReader
  to prevent OOM from malicious registration endpoints
- OIDC IdP errors: check for error/error_description query params in
  callback (RFC 6749 §4.1.2.1) instead of returning generic error
- Cookie cleanup: delete auth_method and redirect URL cookies after
  successful OIDC/SAML callbacks and in user cancel endpoint
- State leak prevention: log OIDC state at Debug (not Info) level and
  remove state values from error messages returned to callers
- SAML standalone: return explicit error when JWK is nil instead of
  passing nil to credential creation
- Multi-valued attrs: log warning when SAML attributes have multiple
  values (only first is used)
- Doc retrieval: replace non-deterministic map iteration in UserLookup
  with firstDocument() helper that validates document is non-nil
- Scope validation: validate scope maps to a credential constructor
  before initiating SAML/OIDC auth from consent endpoint

Author: leifj

internal/apigw/apiv1/handlers_oidcrp.go

@@ -84,13 +84,21 @@ func (c *Client) OIDCRPCallback(ctx context.Context, req *OIDCRPCallbackRequest,
 		return nil, err
 	}
 
-	// Retrieve session to get credential type
+	// Retrieve session to get credential type.
+	// ProcessCallback already validated the session, but we need it for credential type etc.
 	session, err := service.GetSession(ctx, req.State)
 	if err != nil {
 		span.SetStatus(codes.Error, "session retrieval failed")
 		return nil, fmt.Errorf("failed to retrieve session: %w", err)
 	}
 
+	// Ensure session is cleaned up if any subsequent step fails
+	defer func() {
+		if err != nil {
+			service.DeleteSession(ctx, req.State)
+		}
+	}()
+
 	// Build transformer from config
 	transformer, err := service.BuildTransformer()
 	if err != nil {
@@ -141,7 +149,8 @@ func (c *Client) OIDCRPCallback(ctx context.Context, req *OIDCRPCallbackRequest,
 			return nil, fmt.Errorf("failed to store VCI documents: %w", err)
 		}
 
-		// Clean up OIDC session
+		// Clean up OIDC session (clear err so defer doesn't double-delete)
+		err = nil
 		service.DeleteSession(ctx, req.State)
 
 		return &OIDCRPCallbackResponse{
@@ -174,7 +183,8 @@ func (c *Client) OIDCRPCallback(ctx context.Context, req *OIDCRPCallbackRequest,
 		return nil, fmt.Errorf("failed to generate credential offer: %w", err)
 	}
 
-	// Clean up session
+	// Clean up session (clear err so defer doesn't double-delete)
+	err = nil
 	service.DeleteSession(ctx, req.State)
 
 	c.log.Info("Credential issued successfully via OIDC RP",

internal/apigw/apiv1/handlers_users.go

@@ -166,27 +166,12 @@ func (c *Client) UserLookup(ctx context.Context, req *vcclient.UserLookupRequest
 
 		c.log.Debug("userLookup - retrieved docs from cache", "session_id", authorizationContext.SessionID, "num_docs", len(docs))
 
-		// TODO(masv): fix this monstrosity
-		authenticSource := ""
-		for key, doc := range docs {
-			c.log.Debug("userLookup - examining doc", "key", key, "authentic_source", doc.Meta.AuthenticSource, "doc_nil", doc == nil)
-			authenticSource = doc.Meta.AuthenticSource
-			break
-		}
-		c.log.Debug("userLookup", "authenticSource", authenticSource, "docs", docs)
-
-		doc, ok := docs[authenticSource]
-		if !ok {
-			c.log.Error(nil, "no document found for authentic source", "authenticSource", authenticSource, "available_keys", func() []string {
-				keys := make([]string, 0, len(docs))
-				for k := range docs {
-					keys = append(keys, k)
-				}
-				return keys
-			}())
-			return nil, fmt.Errorf("no document found for authentic source %s", authenticSource)
+		doc, err := firstDocument(docs)
+		if err != nil {
+			c.log.Error(err, "no usable document in cache")
+			return nil, err
 		}
-		c.log.Debug("userLookup", "doc", doc)
+		c.log.Debug("userLookup", "authenticSource", doc.Meta.AuthenticSource, "docs", docs)
 
 		jsonPaths, err := req.VCTM.ClaimJSONPath()
 		if err != nil {
@@ -240,18 +225,10 @@ func (c *Client) UserLookup(ctx context.Context, req *vcclient.UserLookupRequest
 		c.log.Debug("userLookup - retrieved SAML/OIDC docs from cache",
 			"session_id", authorizationContext.SessionID, "num_docs", len(docs))
 
-		// Extract the first document (same approach as pid_auth)
-		authenticSource := ""
-		for key, doc := range docs {
-			c.log.Debug("userLookup - examining doc", "key", key,
-				"authentic_source", doc.Meta.AuthenticSource, "doc_nil", doc == nil)
-			authenticSource = doc.Meta.AuthenticSource
-			break
-		}
-
-		doc, ok := docs[authenticSource]
-		if !ok {
-			return nil, fmt.Errorf("no document found for authentic source %s", authenticSource)
+		doc, err := firstDocument(docs)
+		if err != nil {
+			c.log.Error(err, "no usable document in cache for SAML/OIDC session")
+			return nil, err
 		}
 
 		jsonPaths, err := req.VCTM.ClaimJSONPath()
@@ -315,6 +292,18 @@ func (c *Client) UserLookup(ctx context.Context, req *vcclient.UserLookupRequest
 	return reply, nil
 }
 
+// firstDocument returns the single document from the cache map.
+// Returns an error if the map is empty or any entry is nil.
+func firstDocument(docs map[string]*model.CompleteDocument) (*model.CompleteDocument, error) {
+	for key, doc := range docs {
+		if doc == nil || doc.DocumentData == nil {
+			return nil, fmt.Errorf("cached document for key %q is nil or has no data", key)
+		}
+		return doc, nil
+	}
+	return nil, fmt.Errorf("no documents in cache")
+}
+
 // findValueByName searches the document data for a claim value matching
 // the VCTM claim path. It first tries an exact JSONPath-style lookup,
 // then falls back to searching recursively by the leaf key name.

internal/apigw/httpserver/endpoints_oauth.go

@@ -3,6 +3,7 @@ package httpserver
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net/http"
 	"vc/internal/apigw/apiv1"
 	"vc/pkg/model"
@@ -210,6 +211,11 @@ func (s *Service) endpointOAuthAuthorizationConsent(ctx context.Context, c *gin.
 		}
 
 		scope, _ := session.Get("scope").(string)
+		if s.cfg.GetCredentialConstructor(scope) == nil {
+			err := fmt.Errorf("scope %q not configured for credential issuance", scope)
+			span.SetStatus(codes.Error, err.Error())
+			return nil, err
+		}
 
 		// Determine the IdP entity ID: use static IdP or default from config
 		idpEntityID := s.samlSPService.GetStaticIDPEntityID()
@@ -231,6 +237,11 @@ func (s *Service) endpointOAuthAuthorizationConsent(ctx context.Context, c *gin.
 		}
 
 		scope, _ := session.Get("scope").(string)
+		if s.cfg.GetCredentialConstructor(scope) == nil {
+			err := fmt.Errorf("scope %q not configured for credential issuance", scope)
+			span.SetStatus(codes.Error, err.Error())
+			return nil, err
+		}
 
 		authReq, err := s.oidcrpService.InitiateAuthForVCI(ctx, scope, sessionID)
 		if err != nil {

internal/apigw/httpserver/endpoints_oidcrp.go

@@ -71,6 +71,17 @@ func (s *Service) endpointOIDCRPCallback(ctx context.Context, c *gin.Context) (a
 		State: c.Query("state"),
 	}
 
+	// Check for IdP error responses (RFC 6749 §4.1.2.1)
+	if idpError := c.Query("error"); idpError != "" {
+		idpDesc := c.Query("error_description")
+		s.log.Error(nil, "OIDC IdP returned error",
+			"error", idpError,
+			"error_description", idpDesc,
+			"state", req.State)
+		span.SetStatus(codes.Error, "IdP error: "+idpError)
+		return nil, fmt.Errorf("identity provider error: %s - %s", idpError, idpDesc)
+	}
+
 	if req.Code == "" || req.State == "" {
 		span.SetStatus(codes.Error, "missing code or state parameter")
 		return nil, fmt.Errorf("missing required parameters: code and state")
@@ -85,6 +96,9 @@ func (s *Service) endpointOIDCRPCallback(ctx context.Context, c *gin.Context) (a
 
 	// VCI mode: redirect browser back to consent page
 	if reply != nil && reply.VCIRedirectURL != "" {
+		// Clean up auth cookies before redirect
+		c.SetCookie("auth_method", "", -1, "/authorization/consent", "", false, false)
+		c.SetCookie("oidc_redirect_url", "", -1, "/authorization/consent", "", false, false)
 		c.Redirect(http.StatusFound, reply.VCIRedirectURL)
 		return nil, nil
 	}

internal/apigw/httpserver/endpoints_saml.go

@@ -144,12 +144,18 @@ func (s *Service) endpointSAMLACS(ctx context.Context, c *gin.Context) (any, err
 		return nil, err
 	}
 
-	// Retrieve session to get credential type
+	// Retrieve session to get credential type.
+	// Ensure session is cleaned up if any subsequent step fails.
 	session, err := s.samlSPService.GetSession(ctx, relayState)
 	if err != nil {
 		span.SetStatus(codes.Error, "session retrieval failed")
 		return nil, fmt.Errorf("failed to retrieve session: %w", err)
 	}
+	defer func() {
+		if err != nil {
+			s.samlSPService.DeleteSession(ctx, relayState)
+		}
+	}()
 
 	// Build transformer from config
 	transformer, err := s.samlSPService.BuildTransformer()
@@ -169,6 +175,10 @@ func (s *Service) endpointSAMLACS(ctx context.Context, c *gin.Context) (any, err
 	// Take the first value from each attribute array
 	samlAttrs := make(map[string]any)
 	for key, values := range assertion.Attributes {
+		if len(values) > 1 {
+			s.log.Warn("SAML attribute has multiple values, using first",
+				"attribute", key, "count", len(values))
+		}
 		if len(values) > 0 {
 			samlAttrs[key] = values[0] // Use first value
 		}
@@ -209,9 +219,14 @@ func (s *Service) endpointSAMLACS(ctx context.Context, c *gin.Context) (any, err
 			return nil, fmt.Errorf("failed to store VCI documents: %w", err)
 		}
 
-		// Clean up SAML session
+		// Clean up SAML session (clear err so defer doesn't double-delete)
+		err = nil
 		s.samlSPService.DeleteSession(ctx, relayState)
 
+		// Clean up auth cookies before redirect
+		c.SetCookie("auth_method", "", -1, "/authorization/consent", "", false, false)
+		c.SetCookie("saml_redirect_url", "", -1, "/authorization/consent", "", false, false)
+
 		// Redirect browser back to the consent page to continue the VCI flow
 		c.Redirect(http.StatusFound, "/authorization/consent/#/credentials")
 		return nil, nil
@@ -229,10 +244,8 @@ func (s *Service) endpointSAMLACS(ctx context.Context, c *gin.Context) (any, err
 	// In a production scenario, the wallet would provide the JWK
 	jwk := session.JWK
 	if jwk == nil {
-		// For SAML flow without wallet involvement, we generate a JWK
-		// This is primarily for testing and backward compatibility
-		s.log.Info("No JWK provided in session, generating ephemeral key")
-		// TODO: Consider if we should require JWK from wallet instead
+		span.SetStatus(codes.Error, "JWK required for standalone SAML credential binding")
+		return nil, fmt.Errorf("wallet must provide a JWK for credential binding in standalone SAML mode")
 	}
 
 	// Create credential using the issuer gRPC API
@@ -249,7 +262,8 @@ func (s *Service) endpointSAMLACS(ctx context.Context, c *gin.Context) (any, err
 		return nil, fmt.Errorf("failed to generate credential offer: %w", err)
 	}
 
-	// Clean up SAML session
+	// Clean up SAML session (clear err so defer doesn't double-delete)
+	err = nil
 	s.samlSPService.DeleteSession(ctx, relayState)
 
 	s.log.Info("Credential issued successfully",

internal/apigw/httpserver/endpoints_users.go

@@ -210,6 +210,8 @@ func (s *Service) endpointUserCancel(ctx context.Context, c *gin.Context) (any,
 	// Delete all cookies, not just session.
 	c.SetCookie("auth_method", "", -1, "/authorization/consent", "", false, false)
 	c.SetCookie("pid_auth_redirect_url", "", -1, "/authorization/consent", "", false, false)
+	c.SetCookie("oidc_redirect_url", "", -1, "/authorization/consent", "", false, false)
+	c.SetCookie("saml_redirect_url", "", -1, "/authorization/consent", "", false, false)
 
 	client, ok := s.cfg.APIGW.OauthServer.Clients[clientId]
 	if !ok {

internal/apigw/oidcrp/dynamic_registration.go

@@ -117,8 +117,9 @@ func (s *Service) dynamicClientRegistration(ctx context.Context, registrationEnd
 	}
 	defer resp.Body.Close()
 
-	// Read response body
-	respBody, err := io.ReadAll(resp.Body)
+	// Read response body with a size limit to prevent OOM from malicious endpoints
+	const maxResponseSize = 1 << 20 // 1 MB
+	respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize))
 	if err != nil {
 		return nil, fmt.Errorf("failed to read registration response: %w", err)
 	}

internal/apigw/oidcrp/service.go

@@ -168,7 +168,7 @@ func (s *Service) InitiateAuth(ctx context.Context, credentialType string) (*Aut
 		oauth2.SetAuthURLParam("code_challenge_method", "S256"),
 	)
 
-	s.log.Info("OIDC authorization URL generated",
+	s.log.Debug("OIDC authorization URL generated",
 		"credential_type", credentialType,
 		"state", session.State)
 
@@ -337,7 +337,7 @@ func (s *Service) createSession(ctx context.Context, credentialType string) (*Se
 func (s *Service) getSession(ctx context.Context, state string) (*Session, error) {
 	session, ok := s.sessionCache.Get(ctx, state)
 	if !ok || session == nil {
-		return nil, fmt.Errorf("session not found or expired for state: %s", state)
+		return nil, fmt.Errorf("session not found or expired")
 	}
 
 	return session, nil

internal/apigw/samlsp/service.go

@@ -161,6 +161,14 @@ func (s *Service) InitiateAuth(ctx context.Context, idpEntityID, credentialType
 		return nil, fmt.Errorf("failed to get IdP metadata: %w", err)
 	}
 
+	// Validate IdP metadata structure before accessing
+	if len(idpMetadata.IDPSSODescriptors) == 0 {
+		return nil, fmt.Errorf("no IdP SSO descriptors in metadata for %s", idpEntityID)
+	}
+	if len(idpMetadata.IDPSSODescriptors[0].SingleSignOnServices) == 0 {
+		return nil, fmt.Errorf("no SSO services in IdP metadata for %s", idpEntityID)
+	}
+
 	// Create authentication request
 	req, err := s.sp.MakeAuthenticationRequest(
 		idpMetadata.IDPSSODescriptors[0].SingleSignOnServices[0].Location,
@@ -272,6 +280,23 @@ func (s *Service) ProcessAssertion(ctx context.Context, samlResponseEncoded stri
 		}
 	}
 
+	// Validate assertion subject and conditions are present
+	if samlResp.Subject == nil || samlResp.Subject.NameID == nil {
+		return nil, fmt.Errorf("SAML assertion missing Subject or NameID")
+	}
+	if samlResp.Conditions == nil {
+		return nil, fmt.Errorf("SAML assertion missing Conditions")
+	}
+
+	// Validate assertion time conditions (SAML 2.0 §2.5.1)
+	now := time.Now()
+	if !samlResp.Conditions.NotBefore.IsZero() && now.Before(samlResp.Conditions.NotBefore) {
+		return nil, fmt.Errorf("SAML assertion not yet valid: now=%s, NotBefore=%s", now, samlResp.Conditions.NotBefore)
+	}
+	if !samlResp.Conditions.NotOnOrAfter.IsZero() && now.After(samlResp.Conditions.NotOnOrAfter) {
+		return nil, fmt.Errorf("SAML assertion expired: now=%s, NotOnOrAfter=%s", now, samlResp.Conditions.NotOnOrAfter)
+	}
+
 	return &Assertion{
 		NameID:     samlResp.Subject.NameID.Value,
 		Attributes: attributes,
@@ -295,7 +320,7 @@ func (s *Service) DeleteSession(ctx context.Context, sessionID string) {
 func (s *Service) getSession(ctx context.Context, id string) (*Session, error) {
 	session, ok := s.sessionCache.Get(ctx, id)
 	if !ok || session == nil {
-		return nil, fmt.Errorf("session not found or expired: %s", id)
+		return nil, fmt.Errorf("session not found or expired")
 	}
 
 	return session, nil