From b4aaed4b7c06b9ca68b168252701d309d689e5f4 Mon Sep 17 00:00:00 2001 From: Schalli Date: Mon, 21 Sep 2026 14:01:36 +0200 Subject: [PATCH] fix(quick-260921-iwr): drei bag.cert-Zusicherungen in cert-manager.service.ts zu echten Waechtern gemacht MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit node-forge setzt bag.cert bei einem wohlgeformten, aber nicht als X.509 lesbaren Zertifikats-Bag auf null (lib/pkcs12.js Zeile 703-709). Die drei Zusicherungen in parseCert, mergeCerts und convertCert behaupteten also etwas Falsches, obwohl die Folge bereits behandelt war: alle vier Pfade antworteten schon vorher mit 400, nie mit 500 (mit einer selbst gebauten 83-Byte-PFX nachgemessen). Ersetzt die drei Zusicherungen durch ausdrueckliche Pruefungen mit praeziser BadRequestException. In mergeCerts wird kein Zertifikat mehr verschluckt: alle Bags werden auf Vollstaendigkeit geprueft, bevor die Liste ueber ein Typpraedikat zurueckgegeben wird. RED-Tests zuerst geschrieben und mit den heutigen Meldungen ("Failed to extract certificate details" / "Failed to create merged certificate output" / "Failed to convert certificate to pem: serialization error") rot bestaetigt, dann die Waechter ergaenzt: GREEN. Verhaltensaenderung ausdruecklich beabsichtigt (D-02): nur der Text der Fehlermeldung fuer diese eine Eingabeklasse aendert sich, der Statuscode bleibt in allen vier Pfaden 400. NONNULL 11 -> 8, davon 3 in cert-manager.service.ts (Zeilen 133/516/661 bleiben unveraendert — durch Hash-Laenge bzw. vorgelagerte Passwort- Pruefung garantiert). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TPPB4ApQxzSU1rwV2Ffj9J --- .../cert-manager/cert-manager.service.spec.ts | 250 ++++++++++++++++++ .../src/cert-manager/cert-manager.service.ts | 37 ++- 2 files changed, 284 insertions(+), 3 deletions(-) diff --git a/apps/api/src/cert-manager/cert-manager.service.spec.ts b/apps/api/src/cert-manager/cert-manager.service.spec.ts index 484a585..c087a4c 100644 --- a/apps/api/src/cert-manager/cert-manager.service.spec.ts +++ b/apps/api/src/cert-manager/cert-manager.service.spec.ts @@ -526,3 +526,253 @@ describe('convertCert', () => { ).rejects.toThrow(BadRequestException); }); }); + +// --------------------------------------------------------------------------- +// PFX mit unlesbarem Zertifikats-Bag — bag.cert = null (quick-260921-iwr, D-01/D-04) +// +// node-forge setzt bag.cert bei einem wohlgeformten, aber nicht als X.509 +// lesbaren Zertifikats-Bag auf null (lib/pkcs12.js Zeile 703-709: der Fehler +// aus certificateFromAsn1 wird abgefangen und durch bag.cert = null ersetzt). +// Die drei betroffenen Zusicherungen (parseCert Zeile 207, mergeCerts Zeile +// 469, convertCert Zeile 602) behaupten also etwas, das node-forge selbst +// widerlegt. Alle vier Pfade antworteten schon vorher mit 400 statt 500 — die +// hier gepruefte Aenderung ist die Meldung, nicht der Statuscode (D-02). +// --------------------------------------------------------------------------- + +describe('PFX mit unlesbarem Zertifikats-Bag (bag.cert = null)', () => { + let malformedPfxBuffer: Buffer; + + beforeAll(() => { + // Handgebaute ~83-Byte-PFX-Datei mit forge.asn1: wohlgeformte DER-Struktur + // von aussen nach innen (PFX -> ContentInfo -> AuthenticatedSafe -> + // ContentInfo -> SafeContents -> SafeBag -> CertBag), deren + // Zertifikats-Bag-Inhalt aber nur "SEQUENCE { INTEGER 1 }" ist — kein + // X.509-Zertifikat. Passwort ist die leere Zeichenkette. + const fakeCertAsn1 = forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.SEQUENCE, + true, + [forge.asn1.create(forge.asn1.Class.UNIVERSAL, forge.asn1.Type.INTEGER, false, String.fromCharCode(1))], + ); + const fakeCertDer = forge.asn1.toDer(fakeCertAsn1).getBytes(); + + const certBagAsn1 = forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.SEQUENCE, + true, + [ + forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.OID, + false, + forge.asn1.oidToDer(forge.pki.oids.x509Certificate).getBytes(), + ), + forge.asn1.create(forge.asn1.Class.CONTEXT_SPECIFIC, 0, true, [ + forge.asn1.create(forge.asn1.Class.UNIVERSAL, forge.asn1.Type.OCTETSTRING, false, fakeCertDer), + ]), + ], + ); + + const safeBagAsn1 = forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.SEQUENCE, + true, + [ + forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.OID, + false, + forge.asn1.oidToDer(forge.pki.oids.certBag).getBytes(), + ), + forge.asn1.create(forge.asn1.Class.CONTEXT_SPECIFIC, 0, true, [certBagAsn1]), + ], + ); + + const safeContentsAsn1 = forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.SEQUENCE, + true, + [safeBagAsn1], + ); + const safeContentsDer = forge.asn1.toDer(safeContentsAsn1).getBytes(); + + const innerContentInfoAsn1 = forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.SEQUENCE, + true, + [ + forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.OID, + false, + forge.asn1.oidToDer(forge.pki.oids.data).getBytes(), + ), + forge.asn1.create(forge.asn1.Class.CONTEXT_SPECIFIC, 0, true, [ + forge.asn1.create(forge.asn1.Class.UNIVERSAL, forge.asn1.Type.OCTETSTRING, false, safeContentsDer), + ]), + ], + ); + + const authSafeAsn1 = forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.SEQUENCE, + true, + [innerContentInfoAsn1], + ); + const authSafeDer = forge.asn1.toDer(authSafeAsn1).getBytes(); + + const outerContentInfoAsn1 = forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.SEQUENCE, + true, + [ + forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.OID, + false, + forge.asn1.oidToDer(forge.pki.oids.data).getBytes(), + ), + forge.asn1.create(forge.asn1.Class.CONTEXT_SPECIFIC, 0, true, [ + forge.asn1.create(forge.asn1.Class.UNIVERSAL, forge.asn1.Type.OCTETSTRING, false, authSafeDer), + ]), + ], + ); + + const pfxAsn1 = forge.asn1.create( + forge.asn1.Class.UNIVERSAL, + forge.asn1.Type.SEQUENCE, + true, + [ + forge.asn1.create(forge.asn1.Class.UNIVERSAL, forge.asn1.Type.INTEGER, false, String.fromCharCode(3)), + outerContentInfoAsn1, + ], + ); + + const pfxDer = forge.asn1.toDer(pfxAsn1).getBytes(); + malformedPfxBuffer = Buffer.from(pfxDer, 'binary'); + }); + + it('Vorbedingung: forge liefert genau einen Bag mit cert=null fuer diese Datei (belegt, dass der Waechter den gemeinten Fall trifft)', () => { + expect(malformedPfxBuffer.length).toBe(83); + + const p12Asn1 = forge.asn1.fromDer( + forge.util.createBuffer(malformedPfxBuffer.toString('binary')), + ); + const p12 = forge.pkcs12.pkcs12FromAsn1(p12Asn1, ''); + const certBags = p12.getBags({ bagType: forge.pki.oids.certBag }); + const bags = certBags[forge.pki.oids.certBag] ?? []; + + expect(bags).toHaveLength(1); + expect(bags[0].cert).toBeNull(); + }); + + it('parseCert: wirft 400 mit einer Meldung, die den Zertifikats-Bag benennt, statt der irrefuehrenden "Failed to extract certificate details"', async () => { + const service = new CertManagerService(); + + await expect( + service.parseCert({ + file: { originalname: 'bad.pfx', buffer: malformedPfxBuffer }, + password: '', + }), + ).rejects.toThrow(BadRequestException); + + await expect( + service.parseCert({ + file: { originalname: 'bad.pfx', buffer: malformedPfxBuffer }, + password: '', + }), + ).rejects.toThrow(/certificate bag/i); + }); + + it('mergeCerts (outputFormat pem): wirft 400 mit derselben praezisen Aussage statt "Failed to create merged certificate output"', async () => { + const service = new CertManagerService(); + + await expect( + service.mergeCerts({ + files: [{ originalname: 'bad.pfx', buffer: malformedPfxBuffer }], + outputFormat: 'pem', + password: '', + }), + ).rejects.toThrow(BadRequestException); + + await expect( + service.mergeCerts({ + files: [{ originalname: 'bad.pfx', buffer: malformedPfxBuffer }], + outputFormat: 'pem', + password: '', + }), + ).rejects.toThrow(/certificate bag/i); + }); + + it('mergeCerts (outputFormat pfx): wirft 400 mit derselben praezisen Aussage statt "Failed to create merged certificate output"', async () => { + const service = new CertManagerService(); + + await expect( + service.mergeCerts({ + files: [{ originalname: 'bad.pfx', buffer: malformedPfxBuffer }], + outputFormat: 'pfx', + password: 'secret', + }), + ).rejects.toThrow(BadRequestException); + + await expect( + service.mergeCerts({ + files: [{ originalname: 'bad.pfx', buffer: malformedPfxBuffer }], + outputFormat: 'pfx', + password: 'secret', + }), + ).rejects.toThrow(/certificate bag/i); + }); + + it('convertCert: wirft 400 mit einer Meldung, die den Zertifikats-Bag benennt, statt der irrefuehrenden "Failed to convert certificate to pem: serialization error"', async () => { + const service = new CertManagerService(); + + await expect( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + service.convertCert({ + file: { originalname: 'bad.pfx', buffer: malformedPfxBuffer }, + password: '', + targetFormat: 'pem', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any), + ).rejects.toThrow(BadRequestException); + + await expect( + service.convertCert({ + file: { originalname: 'bad.pfx', buffer: malformedPfxBuffer }, + password: '', + targetFormat: 'pem', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any), + ).rejects.toThrow(/certificate bag/i); + }); + + it('eine gueltige PFX-Datei verhaelt sich unveraendert: parseCert liefert weiterhin die CertDetails', async () => { + const service = new CertManagerService(); + + const keys = forge.pki.rsa.generateKeyPair(1024); + const cert = forge.pki.createCertificate(); + cert.publicKey = keys.publicKey; + cert.serialNumber = '01'; + cert.validity.notBefore = new Date(); + cert.validity.notAfter = new Date(); + cert.validity.notAfter.setFullYear(cert.validity.notBefore.getFullYear() + 1); + const attrs = [{ name: 'commonName', value: 'valid-pfx.example.com' }]; + cert.setSubject(attrs); + cert.setIssuer(attrs); + cert.sign(keys.privateKey, forge.md.sha256.create()); + + const p12Asn1 = forge.pkcs12.toPkcs12Asn1(keys.privateKey, [cert], 'secret', { + algorithm: '3des', + }); + const validPfxBuffer = Buffer.from(forge.asn1.toDer(p12Asn1).getBytes(), 'binary'); + + const result = await (service.parseCert({ + file: { originalname: 'valid.pfx', buffer: validPfxBuffer }, + password: 'secret', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + }) as Promise); + + expect(result.subject.cn).toBe('valid-pfx.example.com'); + }, 15000); +}); diff --git a/apps/api/src/cert-manager/cert-manager.service.ts b/apps/api/src/cert-manager/cert-manager.service.ts index fc54101..0b033c4 100644 --- a/apps/api/src/cert-manager/cert-manager.service.ts +++ b/apps/api/src/cert-manager/cert-manager.service.ts @@ -204,7 +204,17 @@ export class CertManagerService { if (bags.length === 0) { throw new Error('No certificate bag found in PFX/PKCS12'); } - cert = bags[0].cert!; + // node-forge sets bag.cert to null when the bag's content parses as + // valid DER but is not a readable X.509 certificate (lib/pkcs12.js + // certBag decoder). An explicit guard here — not an assertion — so + // the 400 names the real cause (quick-260921-iwr, D-01/D-04). + const parsedCert = bags[0].cert; + if (!parsedCert) { + throw new BadRequestException( + 'Certificate bag in PFX/PKCS12 does not contain a readable X.509 certificate', + ); + } + cert = parsedCert; } else { // ── P7B/PKCS7 file — PEM-wrapped or binary DER (Pitfall 4) ──────── const isPemP7b = (file.buffer as Buffer) @@ -466,7 +476,18 @@ export class CertManagerService { const p12 = forge.pkcs12.pkcs12FromAsn1(p12Asn1, password ?? ''); const certBags = p12.getBags({ bagType: forge.pki.oids.certBag }); const bags = certBags[forge.pki.oids.certBag] ?? []; - return bags.map((bag) => bag.cert!); + // node-forge sets bag.cert to null when a bag's content parses as + // valid DER but is not a readable X.509 certificate (lib/pkcs12.js + // certBag decoder). No entry may be silently dropped (D-04) — check + // every bag and reject the whole file, naming it, before returning. + const bagCerts = bags.map((bag) => bag.cert); + const missingCertIndex = bagCerts.findIndex((c) => c === null); + if (missingCertIndex !== -1) { + throw new BadRequestException( + `Certificate bag in "${file.originalname as string}" does not contain a readable X.509 certificate`, + ); + } + return bagCerts.filter((c): c is forge.pki.Certificate => c !== null); } else { // P7B/PKCS7 — PEM-wrapped or binary DER (Pitfall 4) const isPemP7b = (file.buffer as Buffer) @@ -599,7 +620,17 @@ export class CertManagerService { if (bags.length === 0) { throw new Error('No certificate bag found in PFX/PKCS12'); } - cert = bags[0].cert!; + // node-forge sets bag.cert to null when the bag's content parses as + // valid DER but is not a readable X.509 certificate (lib/pkcs12.js + // certBag decoder). An explicit guard here — not an assertion — so + // the 400 names the real cause (quick-260921-iwr, D-01/D-04). + const parsedCert = bags[0].cert; + if (!parsedCert) { + throw new BadRequestException( + 'Certificate bag in PFX/PKCS12 does not contain a readable X.509 certificate', + ); + } + cert = parsedCert; } else { // P7B — extract first cert const isPemP7b = (file.buffer as Buffer)