UI: catch error when verifying certificates with unsupported signature algorithms #21926
Conversation
hellobontempo
added
ui
bug
ui/app/utils/parse-pki-cert.js
Outdated
| // ed25519 is an unsupported signature algorithm | ||
| // catch the error and instead check the AKID (authority key ID) includes the SKID (subject key ID) |
There was a problem hiding this comment.
should i move this comment to the catch block? 🤔
There was a problem hiding this comment.
I think moving it makes sense!
| try { | ||
| // ed25519 is an unsupported signature algorithm | ||
| // catch the error and instead check the AKID (authority key ID) includes the SKID (subject key ID) | ||
| return await child.verify(parent); |
There was a problem hiding this comment.
We don't need await here, we can just return child.verify directly and even remove async on the method since we're just returning the promise
There was a problem hiding this comment.
I didn't hit the error block without the await 🤔
|
Build Results: |
| const skidExtension = parent.extensions.find((ext) => ext.extnID === OTHER_OIDs.subject_key_identifier); | ||
| const akidExtension = parent.extensions.find((ext) => ext.extnID === OTHER_OIDs.authority_key_identifier); | ||
| const skid = new Uint8Array(skidExtension.parsedValue.valueBlock.valueHex); | ||
| const akid = new Uint8Array(akidExtension.extnValue.valueBlock.valueHex); |
There was a problem hiding this comment.
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.
yes! I was going to consult you about how to concisely explain :) thank you!
| // catch the error and instead check the AKID (authority key ID) includes the SKID (subject key ID) | ||
| return await child.verify(parent); | ||
| } catch (error) { | ||
| const skidExtension = parent.extensions.find((ext) => ext.extnID === OTHER_OIDs.subject_key_identifier); |
There was a problem hiding this comment.
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 false is fine here, IMO? At worse we have a false-negative, which should be less likely than the false-positive case if we return true.
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.
I think that's a good idea, will return false as a fallback
|
CI Results: All Go tests passed! ✅ |
hellobontempo
deleted the
ui/VAULT-18198/catch-verify-error-for-ed25519-key-types
branch
The pki.js library doesn't support the
ed25519key type source code so ourverify()function was failing when checking whether or not a certificate is a root.This wraps the verify function in a
try...catchso if the verify function fails, we instead check the authority key ID includes the signature key ID.