Skip to content

Commit 303b538

Browse files
committed
fix: return clear error for unsupported ECDSA/EC provisioning certificates
* Detect when node-forge cannot parse EC certificates from a PFX and return a descriptive error instead of a generic decrypt failure. * Add null guards for empty cert arrays and missing CN fields in domainSuffixChecker. * Route cert errors through a dedicated middleware to bypass express-validator field-level formatting. Addresses #2505
1 parent d063b88 commit 303b538

7 files changed

Lines changed: 161 additions & 14 deletions

File tree

src/certManager.test.ts

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* SPDX-License-Identifier: Apache-2.0
44
**********************************************************************/
55

6-
import { CertManager } from './certManager.js'
6+
import { CertManager, UnsupportedCertificateError } from './certManager.js'
77
import { NodeForge } from './NodeForge.js'
88
import { type AMTKeyUsage, type CertAttributes, type CertificateObject } from './models/index.js'
99
import Logger from './Logger.js'
@@ -63,6 +63,29 @@ describe('certManager tests', () => {
6363
expect(pfxobj.certs).toHaveLength(2)
6464
expect(pfxobj.keys).toHaveLength(1)
6565
})
66+
67+
test('throws UnsupportedCertificateError when cert bags contain no parseable certs (e.g. ECDSA)', () => {
68+
const nodeForge = new NodeForge()
69+
const certManager = new CertManager(new Logger('CertManager'), nodeForge)
70+
const certBagOid = nodeForge.pkiOidsCertBag
71+
const keyBagOid = nodeForge.pkcs8ShroudedKeyBag
72+
73+
spyOn(nodeForge, 'pkcs12FromAsn1').mockReturnValue({
74+
getBags: (opts: any) => {
75+
if (opts.bagType === certBagOid) {
76+
return { [certBagOid]: [{ cert: null }, { cert: null }] }
77+
}
78+
79+
return { [keyBagOid]: [] }
80+
}
81+
} as any)
82+
83+
expect(() => certManager.convertPfxToObject(TEST_PFXCERT, 'P@ssw0rd')).toThrow(UnsupportedCertificateError)
84+
85+
expect(() => certManager.convertPfxToObject(TEST_PFXCERT, 'P@ssw0rd')).toThrow(
86+
/No certificates could be parsed from the provisioning certificate/
87+
)
88+
})
6689
})
6790

6891
describe('dumpPfx tests', () => {

src/certManager.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,15 @@ interface Attribute {
2323
value: string
2424
}
2525

26+
export class UnsupportedCertificateError extends Error {
27+
public readonly code = 'UNSUPPORTED_CERTIFICATE'
28+
29+
constructor(message: string) {
30+
super(message)
31+
this.name = 'UnsupportedCertificateError'
32+
}
33+
}
34+
2635
export class CertManager {
2736
private readonly nodeForge: NodeForge
2837
private readonly logger: ILogger
@@ -191,6 +200,16 @@ export class CertManager {
191200
throw new Error('No certificate bags found')
192201
}
193202

203+
if (pfxOut.certs.length === 0) {
204+
throw new UnsupportedCertificateError(
205+
'No certificates could be parsed from the provisioning certificate. ' +
206+
'This may occur if the certificate uses an unsupported key algorithm (e.g. ECDSA/EC) ' +
207+
'or if the PKCS#12 structure is malformed. Please use a valid RSA certificate. ' +
208+
'Example RSA certificate generation: ' +
209+
'openssl genrsa -out key.pem 2048 && openssl req -new -x509 -key key.pem -out cert.pem -days 365'
210+
)
211+
}
212+
194213
// Process key bags
195214
const keyBags = pfx.getBags({ bagType: this.nodeForge.pkcs8ShroudedKeyBag })
196215
const keyBagArray = keyBags[this.nodeForge.pkcs8ShroudedKeyBag]

src/custom.d.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,14 @@
55

66
import { type IDB } from './interfaces/database/IDb.js'
77
import { type ISecretManagerService } from './interfaces/ISecretManagerService.js'
8+
import { type CertsAndKeys } from './models/index.js'
89

910
declare module 'express' {
1011
export interface Request {
1112
secretsManager: ISecretManagerService
1213
tenantId?: string
1314
db: IDB
15+
certError?: string
16+
pfxobj?: CertsAndKeys | null
1417
}
1518
}

src/routes/admin/domains/domain.test.ts

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,11 @@
33
* SPDX-License-Identifier: Apache-2.0
44
**********************************************************************/
55

6-
import { passwordChecker, domainSuffixChecker, expirationChecker } from './domain.js'
6+
import { passwordChecker, domainSuffixChecker, expirationChecker, certErrorMiddleware } from './domain.js'
77
import { NodeForge } from '../../../NodeForge.js'
88
import { CertManager } from '../../../certManager.js'
99
import Logger from '../../../Logger.js'
10+
import { fn } from 'jest-mock'
1011

1112
describe('Domain Profile Validation', () => {
1213
const nodeForge = new NodeForge()
@@ -184,6 +185,23 @@ describe('Domain Profile Validation', () => {
184185
}).toThrowError(new Error('FQDN not associated with provisioning certificate'))
185186
})
186187

188+
test('domainSuffixChecker - empty certs array', () => {
189+
const emptyCertsPfx = { certs: [], keys: [] }
190+
expect(() => {
191+
domainSuffixChecker(emptyCertsPfx as any, 'vprodemo.com')
192+
}).toThrowError(new Error('No certificates found in the provisioning certificate'))
193+
})
194+
195+
test('domainSuffixChecker - cert without CN', () => {
196+
const noCnPfx = {
197+
certs: [{ subject: { getField: () => null } }],
198+
keys: []
199+
}
200+
expect(() => {
201+
domainSuffixChecker(noCnPfx as any, 'vprodemo.com')
202+
}).toThrowError(new Error('Provisioning certificate does not contain a Common Name (CN) in the subject'))
203+
})
204+
187205
test('expirationChecker - success', () => {
188206
expect(() => {
189207
expirationChecker(pfxobj)
@@ -196,4 +214,39 @@ describe('Domain Profile Validation', () => {
196214
expirationChecker(pfxobj)
197215
}).toThrowError(new Error('Uploaded certificate has expired'))
198216
})
217+
218+
describe('certErrorMiddleware', () => {
219+
let mockReq: any
220+
let mockRes: any
221+
let mockNext: jest.Mock
222+
223+
beforeEach(() => {
224+
mockReq = { body: {} }
225+
mockRes = {
226+
status: fn().mockReturnThis(),
227+
json: fn().mockReturnThis(),
228+
end: fn().mockReturnThis()
229+
}
230+
mockNext = fn()
231+
})
232+
233+
test('returns 400 with cert error when no other validation errors', () => {
234+
mockReq.certError = 'No certificates could be parsed from the provisioning certificate.'
235+
236+
certErrorMiddleware(mockReq, mockRes, mockNext)
237+
238+
expect(mockRes.status).toHaveBeenCalledWith(400)
239+
expect(mockRes.json).toHaveBeenCalledWith({
240+
message: 'No certificates could be parsed from the provisioning certificate.'
241+
})
242+
expect(mockNext).not.toHaveBeenCalled()
243+
})
244+
245+
test('calls next when no cert error is set', () => {
246+
certErrorMiddleware(mockReq, mockRes, mockNext)
247+
248+
expect(mockNext).toHaveBeenCalled()
249+
expect(mockRes.status).not.toHaveBeenCalled()
250+
})
251+
})
199252
})

src/routes/admin/domains/domain.ts

Lines changed: 57 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,15 @@
33
* SPDX-License-Identifier: Apache-2.0
44
**********************************************************************/
55

6-
import { check, type CustomValidator } from 'express-validator'
6+
import { check, validationResult, type CustomValidator } from 'express-validator'
7+
import { type Request, type Response, type NextFunction } from 'express'
78
import { NodeForge } from '../../../NodeForge.js'
8-
import { CertManager } from '../../../certManager.js'
9+
import { CertManager, UnsupportedCertificateError } from '../../../certManager.js'
910
import Logger from '../../../Logger.js'
1011
import { type CertsAndKeys } from '../../../models/index.js'
1112

1213
const nodeForge = new NodeForge()
1314
const certManager = new CertManager(new Logger('CertManager'), nodeForge)
14-
let pfxobj
1515

1616
export const domainInsertValidator = (): any => [
1717
check('profileName')
@@ -60,13 +60,49 @@ export const domainUpdateValidator = (): any => [
6060

6161
function passwordValidator(): CustomValidator {
6262
return (value, { req }) => {
63-
pfxobj = passwordChecker(certManager, req)
63+
const provisioningCert = req?.body?.provisioningCert
64+
65+
if (value == null || value === '' || provisioningCert == null || provisioningCert === '') {
66+
return true
67+
}
68+
69+
try {
70+
req.pfxobj = passwordChecker(certManager, req)
71+
} catch (error) {
72+
req.pfxobj = null
73+
if (error instanceof UnsupportedCertificateError) {
74+
req.certError = error.message
75+
return true
76+
}
77+
throw error
78+
}
6479
return true
6580
}
6681
}
6782

83+
export function certErrorMiddleware(req: Request, res: Response, next: NextFunction): void {
84+
const errors = validationResult(req)
85+
86+
// If there are other validation errors, let the normal validation handling run.
87+
if (!errors.isEmpty()) {
88+
next()
89+
90+
return
91+
}
92+
93+
if (req.certError) {
94+
res.status(400).json({ message: req.certError }).end()
95+
96+
return
97+
}
98+
99+
next()
100+
}
101+
68102
function domainSuffixValidator(): CustomValidator {
69103
return (value, { req }) => {
104+
const pfxobj = req.pfxobj
105+
70106
if (pfxobj != null) {
71107
domainSuffixChecker(pfxobj, value)
72108
}
@@ -76,6 +112,8 @@ function domainSuffixValidator(): CustomValidator {
76112

77113
function expirationValidator(): CustomValidator {
78114
return (value, { req }) => {
115+
const pfxobj = req.pfxobj
116+
79117
if (pfxobj != null) {
80118
expirationChecker(pfxobj)
81119
}
@@ -88,6 +126,10 @@ export function passwordChecker(certManager: CertManager, req: any): CertsAndKey
88126
const pfxobj = certManager.convertPfxToObject(req.body.provisioningCert, req.body.provisioningCertPassword)
89127
return pfxobj
90128
} catch (error) {
129+
if (error instanceof UnsupportedCertificateError) {
130+
throw error
131+
}
132+
91133
throw new Error(
92134
'Unable to decrypt provisioning certificate. Please check that the password is correct, and that the certificate is a valid certificate.',
93135
{ cause: error }
@@ -96,7 +138,17 @@ export function passwordChecker(certManager: CertManager, req: any): CertsAndKey
96138
}
97139

98140
export function domainSuffixChecker(pfxobj: CertsAndKeys, value: any): void {
99-
const certCommonName = pfxobj.certs[0].subject.getField('CN').value
141+
if (!pfxobj.certs || pfxobj.certs.length === 0) {
142+
throw new Error('No certificates found in the provisioning certificate')
143+
}
144+
145+
const cnField = pfxobj.certs[0].subject.getField('CN')
146+
147+
if (!cnField) {
148+
throw new Error('Provisioning certificate does not contain a Common Name (CN) in the subject')
149+
}
150+
151+
const certCommonName = cnField.value
100152
const splittedCertCommonName: string[] = certCommonName.split('.')
101153
const parsedCertCommonName = (
102154
splittedCertCommonName[splittedCertCommonName.length - 2] +

src/routes/admin/domains/index.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,14 @@ import { getDomain } from './get.js'
99
import { DomainCreate } from './create.js'
1010
import { deleteDomain } from './delete.js'
1111
import { editDomain } from './edit.js'
12-
import { domainInsertValidator, domainUpdateValidator } from './domain.js'
12+
import { domainInsertValidator, domainUpdateValidator, certErrorMiddleware } from './domain.js'
1313
import { odataValidator } from '../odataValidator.js'
1414
import validateMiddleware from '../../../middleware/validate.js'
1515
const domainRouter: Router = Router()
1616
const dc = new DomainCreate()
1717
domainRouter.get('/', odataValidator(), validateMiddleware, getAllDomains)
1818
domainRouter.get('/:domainName', getDomain)
19-
domainRouter.post('/', domainInsertValidator(), validateMiddleware, dc.createDomain)
19+
domainRouter.post('/', domainInsertValidator(), certErrorMiddleware, validateMiddleware, dc.createDomain)
2020
domainRouter.patch('/', domainUpdateValidator(), validateMiddleware, editDomain)
2121
domainRouter.delete('/:domainName', deleteDomain)
2222

src/test/collections/rps.postman_collection.json

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -442,12 +442,9 @@
442442
"});\r",
443443
"pm.test(\"Creation should fail when certificate doesn't exist\", function () {\r",
444444
" var result = pm.response.json();\r",
445-
" pm.expect(result.errors[0].msg).to.eql(\"Unable to decrypt provisioning certificate. Please check that the password is correct, and that the certificate is a valid certificate.\")\r",
446-
" pm.expect(result.errors[0].path).to.eql(\"provisioningCertPassword\")\r",
445+
" pm.expect(result.errors[0].msg).to.eql(\"Provisioning certificate is required\")\r",
446+
" pm.expect(result.errors[0].path).to.eql(\"provisioningCert\")\r",
447447
" pm.expect(result.errors[0].location).to.eql(\"body\")\r",
448-
" pm.expect(result.errors[1].msg).to.eql(\"Provisioning certificate is required\")\r",
449-
" pm.expect(result.errors[1].path).to.eql(\"provisioningCert\")\r",
450-
" pm.expect(result.errors[1].location).to.eql(\"body\")\r",
451448
"});"
452449
],
453450
"type": "text/javascript"

0 commit comments

Comments
 (0)