fix: address PR review comments for OtherName SAN generation

Resolves review findings from PR #74:
- ERR-4: lowercase error strings in ResolveOtherNameOID and csr.go
- ERR-2: add ErrUnknownOtherNameType sentinel error, use errors.Is in tests
- ERR-5: add slog.Debug before continue in parseOtherNameEntriesFromSANBytes
- Fix marshalOtherNameGN encoding UTF8String via explicit RawValue tag
- Add IP address length and nil URI validation in MarshalSANExtension
- T-9: remove stdlib-testing assertions from TestMarshalSANExtension_mTLSUserCert
- Refactor OtherName SAN parsing to share walkOtherNameSANs callback
- CL-3/CL-4: add [#74] refs to all Unreleased changelog entries

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: danielewood

CHANGELOG.md

@@ -9,25 +9,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
-- Add `MarshalSANExtension` for building complete SAN extensions with OtherName support (UPN, XMPP, SRV, SmtpUTF8Mailbox, arbitrary OIDs)
-- Add `ResolveOtherNameOID` for resolving OtherName labels or dotted-decimal OID strings
-- Add `OtherNameSAN` and `MarshalSANExtensionInput` types for OtherName SAN generation
-- Add `other_names` field to `CSRTemplate` for mTLS user identity certificate CSRs
-- Add OtherName SAN preservation in `GenerateCSRFromCSR` — OtherName entries survive CSR-to-CSR key rotation
+- Add `MarshalSANExtension` for building complete SAN extensions with OtherName support (UPN, XMPP, SRV, SmtpUTF8Mailbox, arbitrary OIDs) ([#74])
+- Add `ResolveOtherNameOID` for resolving OtherName labels or dotted-decimal OID strings ([#74])
+- Add `OtherNameSAN` and `MarshalSANExtensionInput` types for OtherName SAN generation ([#74])
+- Add `other_names` field to `CSRTemplate` for mTLS user identity certificate CSRs ([#74])
+- Add OtherName SAN preservation in `GenerateCSRFromCSR` — OtherName entries survive CSR-to-CSR key rotation ([#74])
+- Add `ErrUnknownOtherNameType` sentinel error for invalid OtherName type strings ([#74])
 
 ### Fixed
 
 - Fix AIA proxy rejecting `cacerts.geotrust.com` and `cacerts.thawte.com` — consolidate all per-host CA entries into suffix matches for broader coverage of CA subdomains
+- Fix `marshalOtherNameGN` encoding non-SRV OtherName values as PrintableString instead of UTF8String ([#74])
+- Fix `MarshalSANExtension` accepting nil URI entries and invalid IP addresses without validation ([#74])
+- Fix `parseOtherNameEntriesFromSANBytes` silently discarding parse errors without logging (ERR-5) ([#74])
+- Fix error strings in `ResolveOtherNameOID` using capitalized "OtherName" instead of lowercase (ERR-4) ([#74])
+- Refactor OtherName SAN parsing to share common walker between `parseOtherNamesFromSANBytes` and `parseOtherNameEntriesFromSANBytes` ([#74])
 
 ### Tests
 
-- Add `TestMarshalSANExtension` table-driven tests covering UPN, SRV (IA5String), DNS+UPN mixed, all types combined, multiple OtherNames, arbitrary OIDs, IPv4+IPv6
-- Add `TestMarshalSANExtension_CertificateRoundTrip` — full encode→decode round-trip through `x509.CreateCertificate`
-- Add `TestMarshalSANExtension_mTLSUserCert` — CA-signed leaf with UPN + rfc822Name + ClientAuth EKU, chain verification
-- Add `TestResolveOtherNameOID` table-driven tests for known labels, dotted OIDs, and error cases
-- Add `TestParseCSRTemplate_WithOtherNames` — JSON parsing with and without `other_names` field
-- Add `TestGenerateCSRFromTemplate_WithOtherNames` table-driven tests including RFC 5280 duplicate SAN check
-- Add `TestGenerateCSRFromCSR_PreservesOtherNames` — OtherName survival through CSR regeneration
+- Add `TestMarshalSANExtension` table-driven tests covering UPN, SRV (IA5String), DNS+UPN mixed, all types combined, multiple OtherNames, arbitrary OIDs, IPv4+IPv6 ([#74])
+- Add `TestMarshalSANExtension_CertificateRoundTrip` — full encode→decode round-trip through `x509.CreateCertificate` ([#74])
+- Add `TestMarshalSANExtension_mTLSUserCert` — CA-signed leaf with UPN + rfc822Name + ClientAuth EKU ([#74])
+- Add `TestResolveOtherNameOID` table-driven tests for known labels, dotted OIDs, and error cases ([#74])
+- Add `TestParseCSRTemplate_WithOtherNames` — JSON parsing with and without `other_names` field ([#74])
+- Add `TestGenerateCSRFromTemplate_WithOtherNames` table-driven tests including RFC 5280 duplicate SAN check ([#74])
+- Add `TestGenerateCSRFromCSR_PreservesOtherNames` — OtherName survival through CSR regeneration ([#74])
+- Remove stdlib-testing assertions from `TestMarshalSANExtension_mTLSUserCert` (EKU check, chain verification) per T-9 ([#74])
 
 ## [0.8.1] - 2026-02-25
 
@@ -690,6 +697,7 @@ Initial release.
 [`55b5c1e`]: https://github.com/sensiblebit/certkit/commit/55b5c1e
 [`8cf81d9`]: https://github.com/sensiblebit/certkit/commit/8cf81d9
 [`3569926`]: https://github.com/sensiblebit/certkit/commit/3569926
+[#74]: https://github.com/sensiblebit/certkit/pull/74
 [#64]: https://github.com/sensiblebit/certkit/pull/64
 [#63]: https://github.com/sensiblebit/certkit/pull/63
 [#58]: https://github.com/sensiblebit/certkit/pull/58

csr.go

@@ -159,7 +159,7 @@ func GenerateCSRFromTemplate(tmpl *CSRTemplate, signer crypto.Signer) (string, e
 		for _, on := range tmpl.OtherNames {
 			oid, err := ResolveOtherNameOID(on.Type)
 			if err != nil {
-				return "", fmt.Errorf("resolving OtherName type: %w", err)
+				return "", fmt.Errorf("resolving othername type: %w", err)
 			}
 			otherNames = append(otherNames, OtherNameSAN{OID: oid, Value: on.Value})
 		}

csr_test.go

@@ -8,6 +8,7 @@ import (
 	"crypto/x509/pkix"
 	"encoding/asn1"
 	"encoding/pem"
+	"errors"
 	"slices"
 	"strings"
 	"testing"
@@ -865,8 +866,8 @@ func TestGenerateCSRFromTemplate_InvalidOtherNameType(t *testing.T) {
 	if err == nil {
 		t.Error("expected error for invalid OtherName type")
 	}
-	if !strings.Contains(err.Error(), "resolving OtherName type") {
-		t.Errorf("error should mention resolving OtherName type, got: %v", err)
+	if !errors.Is(err, ErrUnknownOtherNameType) {
+		t.Errorf("error should wrap ErrUnknownOtherNameType, got: %v", err)
 	}
 }
 

dn.go

@@ -7,12 +7,17 @@ import (
 	"encoding/hex"
 	"errors"
 	"fmt"
+	"log/slog"
 	"net"
 	"net/url"
 	"strconv"
 	"strings"
 )
 
+// ErrUnknownOtherNameType is returned when an OtherName type string is not a
+// recognized label or a valid dotted-decimal OID.
+var ErrUnknownOtherNameType = errors.New("unknown othername type")
+
 var extKeyUsageNames = map[x509.ExtKeyUsage]string{
 	x509.ExtKeyUsageAny:                        "Any",
 	x509.ExtKeyUsageServerAuth:                 "Server Authentication",
@@ -210,6 +215,9 @@ func MarshalSANExtension(input MarshalSANExtensionInput) (pkix.Extension, error)
 		if ipBytes == nil {
 			ipBytes = ip.To16()
 		}
+		if len(ipBytes) != 4 && len(ipBytes) != 16 {
+			return pkix.Extension{}, fmt.Errorf("invalid IP address %v: must be 4 or 16 bytes", ip)
+		}
 		b, err := asn1.Marshal(asn1.RawValue{
 			Class: asn1.ClassContextSpecific,
 			Tag:   7,
@@ -222,6 +230,9 @@ func MarshalSANExtension(input MarshalSANExtensionInput) (pkix.Extension, error)
 	}
 
 	for _, uri := range input.URIs {
+		if uri == nil {
+			return pkix.Extension{}, errors.New("nil URI in SAN input")
+		}
 		b, err := asn1.Marshal(asn1.RawValue{
 			Class: asn1.ClassContextSpecific,
 			Tag:   6,
@@ -255,20 +266,20 @@ func MarshalSANExtension(input MarshalSANExtensionInput) (pkix.Extension, error)
 // asn1.ObjectIdentifier.
 func ResolveOtherNameOID(s string) (asn1.ObjectIdentifier, error) {
 	if s == "" {
-		return nil, errors.New("empty OtherName type")
+		return nil, errors.New("empty othername type")
 	}
 	if oid, ok := otherNameOIDs[s]; ok {
 		return oid, nil
 	}
 	parts := strings.Split(s, ".")
 	if len(parts) < 2 {
-		return nil, fmt.Errorf("invalid OtherName type %q: not a known label or valid OID", s)
+		return nil, fmt.Errorf("%w %q: not a known label or valid OID", ErrUnknownOtherNameType, s)
 	}
 	oid := make(asn1.ObjectIdentifier, 0, len(parts))
 	for _, part := range parts {
 		n, err := strconv.Atoi(part)
 		if err != nil || n < 0 {
-			return nil, fmt.Errorf("invalid OtherName type %q: not a known label or valid OID", s)
+			return nil, fmt.Errorf("%w %q: not a known label or valid OID", ErrUnknownOtherNameType, s)
 		}
 		oid = append(oid, n)
 	}
@@ -306,7 +317,11 @@ func marshalOtherNameGN(on OtherNameSAN) ([]byte, error) {
 			Bytes: []byte(on.Value),
 		})
 	} else {
-		valueBytes, err = asn1.Marshal(on.Value)
+		valueBytes, err = asn1.Marshal(asn1.RawValue{
+			Tag:   tag,
+			Class: asn1.ClassUniversal,
+			Bytes: []byte(on.Value),
+		})
 	}
 	if err != nil {
 		return nil, fmt.Errorf("marshaling OtherName value: %w", err)
@@ -346,13 +361,31 @@ func parseOtherNameSANEntries(extensions []pkix.Extension) []OtherNameSAN {
 }
 
 func parseOtherNameEntriesFromSANBytes(raw []byte) []OtherNameSAN {
+	var entries []OtherNameSAN
+	walkOtherNameSANs(raw, func(oid asn1.ObjectIdentifier, valueBytes []byte) {
+		var strVal string
+		if _, strErr := asn1.Unmarshal(valueBytes, &strVal); strErr != nil {
+			slog.Debug("skipping OtherName entry: string value parse failed", "oid", oid, "error", strErr)
+			return
+		}
+		entries = append(entries, OtherNameSAN{OID: oid, Value: strVal})
+	})
+	return entries
+}
+
+// walkOtherNameSANs iterates over the OtherName GeneralName entries in raw
+// SAN extension bytes and calls fn for each successfully parsed entry. The
+// valueBytes passed to fn are the inner bytes of the [0] EXPLICIT wrapper
+// (i.e. the TLV-encoded string value). This shared walker is used by both
+// parseOtherNameEntriesFromSANBytes (structured extraction) and
+// parseOtherNamesFromSANBytes (display formatting).
+func walkOtherNameSANs(raw []byte, fn func(oid asn1.ObjectIdentifier, valueBytes []byte)) {
 	var seq asn1.RawValue
 	rest, err := asn1.Unmarshal(raw, &seq)
 	if err != nil || len(rest) > 0 {
-		return nil
+		return
 	}
 
-	var entries []OtherNameSAN
 	inner := seq.Bytes
 	for len(inner) > 0 {
 		var gn asn1.RawValue
@@ -374,20 +407,18 @@ func parseOtherNameEntriesFromSANBytes(raw []byte) []OtherNameSAN {
 		var oid asn1.ObjectIdentifier
 		oidRest, oidErr := asn1.Unmarshal(content, &oid)
 		if oidErr != nil {
+			slog.Debug("skipping OtherName entry: OID parse failed", "error", oidErr)
 			continue
 		}
 
 		var explicit asn1.RawValue
 		if _, explErr := asn1.Unmarshal(oidRest, &explicit); explErr != nil {
+			slog.Debug("skipping OtherName entry: explicit tag parse failed", "oid", oid, "error", explErr)
 			continue
 		}
 
-		var strVal string
-		if _, strErr := asn1.Unmarshal(explicit.Bytes, &strVal); strErr == nil {
-			entries = append(entries, OtherNameSAN{OID: oid, Value: strVal})
-		}
+		fn(oid, explicit.Bytes)
 	}
-	return entries
 }
 
 // ParseOtherNameSANs extracts SAN entries that Go's x509 package silently
@@ -413,7 +444,23 @@ func parseOtherNamesFromSANBytes(raw []byte) []string {
 		return nil
 	}
 
+	// Collect OtherName entries via shared walker.
 	var sans []string
+	walkOtherNameSANs(raw, func(oid asn1.ObjectIdentifier, valueBytes []byte) {
+		label := oid.String()
+		if name, ok := otherNameLabels[label]; ok {
+			label = name
+		}
+		var strVal string
+		if _, strErr := asn1.Unmarshal(valueBytes, &strVal); strErr == nil {
+			sans = append(sans, label+":"+strVal)
+		} else {
+			sans = append(sans, label+":"+hex.EncodeToString(valueBytes))
+		}
+	})
+
+	// Handle DirectoryName (tag 4) and RegisteredID (tag 8) which the
+	// OtherName walker does not cover.
 	inner := seq.Bytes
 	for len(inner) > 0 {
 		var gn asn1.RawValue
@@ -425,11 +472,6 @@ func parseOtherNamesFromSANBytes(raw []byte) []string {
 			continue
 		}
 		switch gn.Tag {
-		case 0: // otherName [0] IMPLICIT SEQUENCE { OID, [0] EXPLICIT ANY }
-			s := formatOtherName(gn.Bytes)
-			if s != "" {
-				sans = append(sans, s)
-			}
 		case 4: // directoryName [4]
 			var name pkix.RDNSequence
 			if _, err := asn1.Unmarshal(gn.Bytes, &name); err == nil {
@@ -444,46 +486,11 @@ func parseOtherNamesFromSANBytes(raw []byte) []string {
 			}
 		}
 	}
-	return sans
-}
-
-func formatOtherName(data []byte) string {
-	// OtherName ::= SEQUENCE { type-id OID, value [0] EXPLICIT ANY }
-	//
-	// With IMPLICIT tagging (RFC 5280), [0] replaces the SEQUENCE tag so
-	// data starts with the OID directly. Some encoders use EXPLICIT tagging
-	// or wrap in a SEQUENCE, so data may start with a SEQUENCE tag (0x30).
-	content := data
-	var probe asn1.RawValue
-	if _, err := asn1.Unmarshal(data, &probe); err == nil && probe.Tag == asn1.TagSequence && probe.Class == asn1.ClassUniversal {
-		content = probe.Bytes
-	}
 
-	var oid asn1.ObjectIdentifier
-	rest, err := asn1.Unmarshal(content, &oid)
-	if err != nil {
-		return ""
-	}
-
-	label := oid.String()
-	if name, ok := otherNameLabels[label]; ok {
-		label = name
-	}
-
-	// Unwrap the [0] EXPLICIT wrapper to get the inner value.
-	var explicit asn1.RawValue
-	if _, err := asn1.Unmarshal(rest, &explicit); err != nil {
-		return label + ":<unparseable>"
-	}
-
-	// Try common string types (UTF8String, IA5String, PrintableString).
-	var strVal string
-	if _, err := asn1.Unmarshal(explicit.Bytes, &strVal); err == nil {
-		return label + ":" + strVal
+	if len(sans) == 0 {
+		return nil
 	}
-
-	// Fallback: hex-encode the raw value.
-	return label + ":" + hex.EncodeToString(explicit.Bytes)
+	return sans
 }
 
 // extraOIDLabels maps OIDs not handled by Go's pkix.Name.String() to their

dn_test.go

@@ -1217,21 +1217,6 @@ func TestMarshalSANExtension_mTLSUserCert(t *testing.T) {
 		t.Errorf("OtherNames = %v, want [UPN:alice@corp.example.com]", otherNames)
 	}
 
-	// Verify ClientAuth EKU
-	if len(leafCert.ExtKeyUsage) != 1 || leafCert.ExtKeyUsage[0] != x509.ExtKeyUsageClientAuth {
-		t.Errorf("ExtKeyUsage = %v, want [ClientAuth]", leafCert.ExtKeyUsage)
-	}
-
-	// Verify the certificate chains to the CA
-	roots := x509.NewCertPool()
-	roots.AddCert(caCert)
-	_, err = leafCert.Verify(x509.VerifyOptions{
-		Roots:     roots,
-		KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
-	})
-	if err != nil {
-		t.Errorf("certificate chain verification failed: %v", err)
-	}
 }
 
 // mustParseURL parses a URL string or fails the test.