Skip to content

Commit

Permalink
backport of commit 96bb634 (#21931)
Browse files Browse the repository at this point in the history
Co-authored-by: claire bontempo <[email protected]>
  • Loading branch information
1 parent 72d035f commit 2bd5d4e
Show file tree
Hide file tree
Showing 8 changed files with 96 additions and 9 deletions.
3 changes: 3 additions & 0 deletions changelog/21926.txt
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)
```
2 changes: 1 addition & 1 deletion ui/app/utils/parse-pki-cert-oids.js
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
29 changes: 24 additions & 5 deletions ui/app/utils/parse-pki-cert.js
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,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);
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);
// 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
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion ui/lib/pki/addon/components/page/pki-issuer-list.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<LinkedBlock class="list-item-row" @params={{array "issuers.issuer.details" pkiIssuer.id}} @linkPrefix={{@mountPoint}}>
<div class="level is-mobile">
<div class="level-left">
<div>
<div data-test-issuer-list={{pkiIssuer.id}}>
<Icon @name="certificate" class="has-text-grey-light" />
<span class="has-text-weight-semibold is-underline">
{{pkiIssuer.issuerRef}}
Expand Down
27 changes: 26 additions & 1 deletion ui/tests/acceptance/pki/pki-configuration-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
import { setupMirage } from 'ember-cli-mirage/test-support';
import { click, currentURL, fillIn, visit, isSettled, waitUntil } from '@ember/test-helpers';
import { click, currentURL, fillIn, visit, isSettled, waitUntil, find } from '@ember/test-helpers';
import { v4 as uuidv4 } from 'uuid';

import authPage from 'vault/tests/pages/auth';
Expand Down Expand Up @@ -193,5 +193,30 @@ module('Acceptance | pki configuration test', function (hooks) {
"This PKI mount hasn't yet been configured with a certificate issuer. There are existing certificates. Use the CLI to perform any operations with them until an issuer is configured."
);
});

// test coverage for ed25519 certs not displaying because the verify() function errors
test('it generates and displays a root issuer of key type = ed25519', async function (assert) {
assert.expect(4);
await authPage.login(this.pkiAdminToken);
await visit(`/vault/secrets/${this.mountPath}/pki/overview`);
await click(SELECTORS.issuersTab);
await click(SELECTORS.generateIssuerDropdown);
await click(SELECTORS.generateIssuerRoot);
await fillIn(SELECTORS.configuration.inputByName('type'), 'internal');
await fillIn(SELECTORS.configuration.inputByName('commonName'), 'my-certificate');
await click(SELECTORS.configuration.keyParamsGroupToggle);
await fillIn(SELECTORS.configuration.inputByName('keyType'), 'ed25519');
await click(SELECTORS.configuration.generateRootSave);

const issuerId = find(SELECTORS.configuration.saved.issuerLink).innerHTML;
await visit(`/vault/secrets/${this.mountPath}/pki/issuers`);
assert.dom(SELECTORS.issuerListItem(issuerId)).exists();
assert
.dom('[data-test-common-name="0"]')
.hasText('my-certificate', 'parses certificate metadata in the list view');
await click(SELECTORS.issuerListItem(issuerId));
assert.strictEqual(currentURL(), `/vault/secrets/${this.mountPath}/pki/issuers/${issuerId}/details`);
assert.dom(SELECTORS.configuration.saved.commonName).exists('renders issuer details');
});
});
});
3 changes: 3 additions & 0 deletions ui/tests/helpers/pki/values.js

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

3 changes: 3 additions & 0 deletions ui/tests/helpers/pki/workflow.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { SELECTORS as CONFIGURATION } from './pki-configure-create';
import { SELECTORS as DELETE } from './pki-delete-all-issuers';
import { SELECTORS as TIDY } from './page/pki-tidy-form';
import { SELECTORS as CONFIGEDIT } from './page/pki-configuration-edit';
import { SELECTORS as GENROOT } from './pki-generate-root';

export const SELECTORS = {
breadcrumbContainer: '[data-test-breadcrumbs]',
Expand Down Expand Up @@ -51,6 +52,7 @@ export const SELECTORS = {
...KEYPAGES,
},
// ISSUERS
issuerListItem: (id) => `[data-test-issuer-list="${id}"]`,
importIssuerLink: '[data-test-generate-issuer="import"]',
generateIssuerDropdown: '[data-test-issuer-generate-dropdown]',
generateIssuerRoot: '[data-test-generate-issuer="root"]',
Expand All @@ -70,6 +72,7 @@ export const SELECTORS = {
...CONFIGURATION,
...DELETE,
...TIDY,
...GENROOT,
},
// EDIT CONFIGURATION
configEdit: {
Expand Down
36 changes: 35 additions & 1 deletion ui/tests/integration/utils/parse-pki-cert-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,19 @@ import { fromBase64, stringToArrayBuffer } from 'pvutils';
import { Certificate } from 'pkijs';
import { addHours, fromUnixTime, isSameDay } from 'date-fns';
import errorMessage from 'vault/utils/error-message';
import { SAN_TYPES } from 'vault/utils/parse-pki-cert-oids';
import { OTHER_OIDs, SAN_TYPES } from 'vault/utils/parse-pki-cert-oids';
import {
certWithoutCN,
loadedCert,
pssTrueCert,
skeletonCert,
unsupportedOids,
unsupportedSignatureRoot,
unsupportedSignatureInt,
} from 'vault/tests/helpers/pki/values';
import { verifyCertificates } from 'vault/utils/parse-pki-cert';
import { jsonToCertObject } from 'vault/utils/parse-pki-cert';
import { verifySignature } from 'vault/utils/parse-pki-cert';

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

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

const filterExtensions = (list, oid) => list.filter((ext) => ext.extnID !== oid);
const { subject_key_identifier, authority_key_identifier } = OTHER_OIDs;
const testCert = jsonToCertObject(unsupportedSignatureRoot);
const certWithoutSKID = testCert;
certWithoutSKID.extensions = filterExtensions(testCert.extensions, subject_key_identifier);
assert.false(
await verifySignature(certWithoutSKID, certWithoutSKID),
'returns false if no subject key ID'
);

const certWithoutAKID = testCert;
certWithoutAKID.extensions = filterExtensions(testCert.extensions, authority_key_identifier);
assert.false(await verifySignature(certWithoutAKID, certWithoutAKID), 'returns false if no AKID');

const certWithoutKeyID = testCert;
certWithoutAKID.extensions = [];
assert.false(
await verifySignature(certWithoutKeyID, certWithoutKeyID),
'returns false if neither SKID or AKID'
);
});

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

0 comments on commit 2bd5d4e

Please sign in to comment.