Skip to content
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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jul 19, 2023
Merged

UI: catch error when verifying certificates with unsupported signature algorithms #21926

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
91fd5be
wrap verify in a try..catch
hellobontempo Jul 18, 2023
f871983
add tests
hellobontempo Jul 18, 2023
2ffa89f
add comments and one more test
hellobontempo Jul 18, 2023
879a5d1
add test coverage
hellobontempo Jul 18, 2023
9865025
add changelog
hellobontempo Jul 18, 2023
f5aee69
remove link
hellobontempo Jul 18, 2023
2446101
Merge branch 'main' into ui/VAULT-18198/catch-verify-error-for-ed2551…
hellobontempo Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ui/app/utils/parse-pki-cert-oids.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export const EXTENSION_OIDs = {
};

// these are allowed ext oids, but not parsed and passed to cross-signed certs
export const IGNORED_OIDs = {
export const OTHER_OIDs = {
// These two extensions are controlled by the parent authority.
authority_key_identifier: '2.5.29.35',
authority_access_info: '1.3.6.1.5.5.7.1.1',
Expand Down
24 changes: 19 additions & 5 deletions ui/app/utils/parse-pki-cert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -131,15 +131,29 @@ 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);
}

async function verifySignature(parent, child) {
try {
// ed25519 is an unsupported signature algorithm
// catch the error and instead check the AKID (authority key ID) includes the SKID (subject key ID)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should i move this comment to the catch block? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think moving it makes sense!

return await child.verify(parent);
Copy link
Contributor

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 return child.verify directly and even remove async on the method since we're just returning the promise

Copy link
Contributor Author

@hellobontempo hellobontempo Jul 18, 2023 •
edited
Loading

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 🤔

} catch (error) {
const skidExtension = parent.extensions.find((ext) => ext.extnID === OTHER_OIDs.subject_key_identifier);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 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 :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good idea, will return false as a fallback

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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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!

return akid.toString().includes(skid.toString());
}
}

//* PARSING HELPERS
Expand Down Expand Up @@ -182,7 +196,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);
Expand Down
3 changes: 3 additions & 0 deletions ui/tests/helpers/pki/values.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions ui/tests/integration/utils/parse-pki-cert-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ import {
pssTrueCert,
skeletonCert,
unsupportedOids,
unsupportedSignatureRoot,
unsupportedSignatureInt,
} from 'vault/tests/helpers/pki/values';
import { verifyCertificates } from 'vault/utils/parse-pki-cert';

module('Integration | Util | parse pki certificate', function (hooks) {
setupTest(hooks);
Expand Down Expand Up @@ -227,6 +230,14 @@ module('Integration | Util | parse pki certificate', function (hooks) {
);
});

test('the helper verifyCertificates catches errors', async function (assert) {
assert.expect(2);
const verifiedRoot = await verifyCertificates(unsupportedSignatureRoot, unsupportedSignatureRoot);
const verifiedInt = await verifyCertificates(unsupportedSignatureInt, unsupportedSignatureInt);
assert.true(verifiedRoot, 'returns true for root certificate');
assert.false(verifiedInt, 'returns false for intermediate cert');
});

test('it fails silently when passed null', async function (assert) {
assert.expect(3);
const parsedCert = parseCertificate(certWithoutCN);
Expand Down