-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UI: catch error when verifying certificates with unsupported signature algorithms #21926
Changes from all commits
91fd5be
f871983
2ffa89f
879a5d1
9865025
f5aee69
2446101
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:bug | ||
ui: Fixes problem displaying certificates issued with unsupported signature algorithms (i.e. ed25519) | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ import { Certificate } from 'pkijs'; | |
import { differenceInHours, getUnixTime } from 'date-fns'; | ||
import { | ||
EXTENSION_OIDs, | ||
IGNORED_OIDs, | ||
OTHER_OIDs, | ||
KEY_USAGE_BITS, | ||
SAN_TYPES, | ||
SIGNATURE_ALGORITHM_OIDs, | ||
|
@@ -131,15 +131,34 @@ export async function verifyCertificates(certA, certB, leaf) { | |
const parsedCertB = jsonToCertObject(certB); | ||
if (leaf) { | ||
const parsedLeaf = jsonToCertObject(leaf); | ||
const chainA = await parsedLeaf.verify(parsedCertA); | ||
const chainB = await parsedLeaf.verify(parsedCertB); | ||
const chainA = await verifySignature(parsedCertA, parsedLeaf); | ||
const chainB = await verifySignature(parsedCertB, parsedLeaf); | ||
// the leaf's issuer should be equal to the subject data of the intermediate certs | ||
const isEqualA = parsedLeaf.issuer.isEqual(parsedCertA.subject); | ||
const isEqualB = parsedLeaf.issuer.isEqual(parsedCertB.subject); | ||
return chainA && chainB && isEqualA && isEqualB; | ||
} | ||
// can be used to validate if a certificate is self-signed (i.e. a root cert), by passing it as both certA and B | ||
return (await parsedCertA.verify(parsedCertB)) && parsedCertA.issuer.isEqual(parsedCertB.subject); | ||
return (await verifySignature(parsedCertA, parsedCertB)) && parsedCertA.issuer.isEqual(parsedCertB.subject); | ||
} | ||
|
||
export async function verifySignature(parent, child) { | ||
try { | ||
return await child.verify(parent); | ||
} catch (error) { | ||
// ed25519 is an unsupported signature algorithm and so verify() errors | ||
// SKID (subject key ID) is the byte array of the key identifier | ||
// AKID (authority key ID) is a SEQUENCE-type extension that includes the key identifier and potentially other information. | ||
const skidExtension = parent.extensions.find((ext) => ext.extnID === OTHER_OIDs.subject_key_identifier); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do think there's some (imported) certificates this could fail on due to them not conforming with RFC 5280; do we want to safe-guard this if there's a missing SKID or AKID extension? I think failing and returning Or, do the plumbing to make this return an unknown status, which seems like more work than its worth, tbh (since that's what this exception was doing, technically, and would mean handling that). Just some thoughts :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's a good idea, will return |
||
const akidExtension = parent.extensions.find((ext) => ext.extnID === OTHER_OIDs.authority_key_identifier); | ||
// return false if either extension is missing | ||
// this could mean a false-negative but that's okay for our use-case | ||
if (!skidExtension || !akidExtension) return false; | ||
const skid = new Uint8Array(skidExtension.parsedValue.valueBlock.valueHex); | ||
const akid = new Uint8Array(akidExtension.extnValue.valueBlock.valueHex); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wanna add the note about why we did this? :-) SKID is just the byte array of the key identifier, but AKID is a SEQUENCE-type extension which does include the key identifier but also potentially includes other information. This saves us a parse of AKID and is unlikely to return false-positives. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes! I was going to consult you about how to concisely explain :) thank you! |
||
// Check that AKID includes the SKID, which saves us from parsing the AKID and is unlikely to return false-positives. | ||
return akid.toString().includes(skid.toString()); | ||
} | ||
} | ||
|
||
//* PARSING HELPERS | ||
|
@@ -182,7 +201,7 @@ export function parseExtensions(extensions) { | |
if (!extensions) return null; | ||
const values = {}; | ||
const errors = []; | ||
const allowedOids = Object.values({ ...EXTENSION_OIDs, ...IGNORED_OIDs }); | ||
const allowedOids = Object.values({ ...EXTENSION_OIDs, ...OTHER_OIDs }); | ||
const isUnknownExtension = (ext) => !allowedOids.includes(ext.extnID); | ||
if (extensions.any(isUnknownExtension)) { | ||
const unknown = extensions.filter(isUnknownExtension).map((ext) => ext.extnID); | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need
await
here, we can just returnchild.verify
directly and even removeasync
on the method since we're just returning the promiseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't hit the error block without the await 🤔