Skip to content

Commit

Permalink
refactor(auth): make Buffer use more idiomatic (#4041)
Browse files Browse the repository at this point in the history
We don't need the `numericArray` helper or spreading into array literals passed to `Buffer.from` calls. Instead, we should use the more idiomatic and efficient `Buffer.alloc` and `Buffer.concat` calls. This also changes `Buffer.from([…])` calls to `Buffer.of(…)` as the intermediate array is just extra noise (they probably amount to about the same in terms of perf for small N, at any rate).
  • Loading branch information
eventualbuddha authored Oct 9, 2023
1 parent 93b214c commit 9b5ccf5
Show file tree
Hide file tree
Showing 10 changed files with 136 additions and 224 deletions.
18 changes: 9 additions & 9 deletions libs/auth/scripts/configure_java_card.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,26 +116,26 @@ function configureKeySlotCommandApdu(
data: constructTlv(
PUT_DATA_ADMIN.CREATE_KEY_TAG,
Buffer.concat([
constructTlv(PUT_DATA_ADMIN.ID_TAG, Buffer.from([keyId])),
constructTlv(PUT_DATA_ADMIN.ID_TAG, Buffer.of(keyId)),
constructTlv(
PUT_DATA_ADMIN.ACCESS_MODE_OVER_CONTACT_INTERFACE_TAG,
Buffer.from([accessMode])
Buffer.of(accessMode)
),
constructTlv(
PUT_DATA_ADMIN.ACCESS_MODE_OVER_CONTACTLESS_INTERFACE_TAG,
Buffer.from([PUT_DATA_ADMIN.ACCESS_MODE_NEVER])
Buffer.of(PUT_DATA_ADMIN.ACCESS_MODE_NEVER)
),
constructTlv(
PUT_DATA_ADMIN.KEY_MECHANISM_TAG,
Buffer.from([CRYPTOGRAPHIC_ALGORITHM_IDENTIFIER.ECC256])
Buffer.of(CRYPTOGRAPHIC_ALGORITHM_IDENTIFIER.ECC256)
),
constructTlv(
PUT_DATA_ADMIN.KEY_ROLE_TAG,
Buffer.from([PUT_DATA_ADMIN.KEY_ROLE_SIGN])
Buffer.of(PUT_DATA_ADMIN.KEY_ROLE_SIGN)
),
constructTlv(
PUT_DATA_ADMIN.KEY_ATTRIBUTE_TAG,
Buffer.from([PUT_DATA_ADMIN.KEY_ATTRIBUTE_NONE])
Buffer.of(PUT_DATA_ADMIN.KEY_ATTRIBUTE_NONE)
),
])
),
Expand All @@ -154,11 +154,11 @@ function configureDataObjectSlotCommandApdu(objectId: Buffer): CommandApdu {
constructTlv(PUT_DATA_ADMIN.ID_TAG, objectId),
constructTlv(
PUT_DATA_ADMIN.ACCESS_MODE_OVER_CONTACT_INTERFACE_TAG,
Buffer.from([PUT_DATA_ADMIN.ACCESS_MODE_ALWAYS])
Buffer.of(PUT_DATA_ADMIN.ACCESS_MODE_ALWAYS)
),
constructTlv(
PUT_DATA_ADMIN.ACCESS_MODE_OVER_CONTACTLESS_INTERFACE_TAG,
Buffer.from([PUT_DATA_ADMIN.ACCESS_MODE_NEVER])
Buffer.of(PUT_DATA_ADMIN.ACCESS_MODE_NEVER)
),
])
),
Expand Down Expand Up @@ -199,7 +199,7 @@ async function runAppletConfigurationCommands(): Promise<void> {
PUT_DATA_ADMIN.PIN_POLICY_TAG,
constructTlv(
PUT_DATA_ADMIN.MAX_INCORRECT_PIN_ATTEMPTS_OVER_CONTACT_INTERFACE_TAG,
Buffer.from([MAX_NUM_INCORRECT_PIN_ATTEMPTS])
Buffer.of(MAX_NUM_INCORRECT_PIN_ATTEMPTS)
)
)
),
Expand Down
132 changes: 50 additions & 82 deletions libs/auth/src/apdu.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { Buffer } from 'buffer';
import { Byte } from '@votingworks/types';

import { numericArray } from '../test/utils';
import {
CardCommand,
CommandApdu,
Expand All @@ -28,7 +27,7 @@ test.each<{
data: Buffer.of(),
});
expect(apdu.asBuffer()).toEqual(
Buffer.from([expectedFirstByte, 0x01, 0x02, 0x03, 0x00])
Buffer.of(expectedFirstByte, 0x01, 0x02, 0x03, 0x00)
);
});

Expand All @@ -37,10 +36,10 @@ test('CommandApdu with data', () => {
ins: 0x01,
p1: 0x02,
p2: 0x03,
data: Buffer.from([0x04, 0x05]),
data: Buffer.of(0x04, 0x05),
});
expect(apdu.asBuffer()).toEqual(
Buffer.from([0x00, 0x01, 0x02, 0x03, 0x02, 0x04, 0x05])
Buffer.of(0x00, 0x01, 0x02, 0x03, 0x02, 0x04, 0x05)
);
});

Expand All @@ -51,7 +50,7 @@ test('CommandApdu data length validation', () => {
ins: 0x01,
p1: 0x02,
p2: 0x03,
data: Buffer.from(numericArray({ length: 256 })),
data: Buffer.alloc(256),
})
).toThrow('APDU data exceeds max command APDU data length');
});
Expand All @@ -61,7 +60,7 @@ test('CommandApdu as hex string', () => {
ins: 0xa1,
p1: 0xb2,
p2: 0xc3,
data: Buffer.from([0xd4, 0xe5]),
data: Buffer.of(0xd4, 0xe5),
});
expect(apdu.asHexString()).toEqual('00a1b2c302d4e5');
expect(apdu.asHexString(':')).toEqual('00:a1:b2:c3:02:d4:e5');
Expand All @@ -74,7 +73,7 @@ test('CardCommand with no data', () => {
p2: 0x03,
});
expect(command.asCommandApdus().map((apdu) => apdu.asBuffer())).toEqual([
Buffer.from([0x00, 0x01, 0x02, 0x03, 0x00]),
Buffer.of(0x00, 0x01, 0x02, 0x03, 0x00),
]);
});

Expand All @@ -83,17 +82,10 @@ test('CardCommand with data requiring a single APDU', () => {
ins: 0x01,
p1: 0x02,
p2: 0x03,
data: Buffer.from(numericArray({ length: 255 })),
data: Buffer.alloc(255),
});
expect(command.asCommandApdus().map((apdu) => apdu.asBuffer())).toEqual([
Buffer.from([
0x00,
0x01,
0x02,
0x03,
0xff,
...numericArray({ length: 255 }),
]),
Buffer.concat([Buffer.of(0x00, 0x01, 0x02, 0x03, 0xff), Buffer.alloc(255)]),
]);
});

Expand All @@ -102,53 +94,44 @@ test('CardCommand with data requiring multiple APDUs', () => {
ins: 0x01,
p1: 0x02,
p2: 0x03,
data: Buffer.from([
...numericArray({ length: 200, value: 1 }),
...numericArray({ length: 200, value: 2 }),
...numericArray({ length: 200, value: 3 }),
data: Buffer.concat([
Buffer.alloc(200, 1),
Buffer.alloc(200, 2),
Buffer.alloc(200, 3),
]),
});
expect(command.asCommandApdus().map((apdu) => apdu.asBuffer())).toEqual([
Buffer.from([
0x10,
0x01,
0x02,
0x03,
0xff,
...numericArray({ length: 200, value: 1 }),
...numericArray({ length: 55, value: 2 }),
Buffer.concat([
Buffer.of(0x10, 0x01, 0x02, 0x03, 0xff),
Buffer.alloc(200, 1),
Buffer.alloc(55, 2),
]),
Buffer.from([
0x10,
0x01,
0x02,
0x03,
0xff,
...numericArray({ length: 145, value: 2 }),
...numericArray({ length: 110, value: 3 }),
Buffer.concat([
Buffer.of(0x10, 0x01, 0x02, 0x03, 0xff),
Buffer.alloc(145, 2),
Buffer.alloc(110, 3),
]),
Buffer.from([
0x00,
0x01,
0x02,
0x03,
0x5a, // 90 (600 - 255 - 255) in hex
...numericArray({ length: 90, value: 3 }),
Buffer.concat([
Buffer.from([
0x00,
0x01,
0x02,
0x03,
0x5a, // 90 (600 - 255 - 255) in hex
]),
Buffer.alloc(90, 3),
]),
]);
});

test('constructTlv with Byte tag', () => {
const tlv = constructTlv(0x01, Buffer.from([0x02, 0x03]));
expect(tlv).toEqual(Buffer.from([0x01, 0x02, 0x02, 0x03]));
const tlv = constructTlv(0x01, Buffer.of(0x02, 0x03));
expect(tlv).toEqual(Buffer.of(0x01, 0x02, 0x02, 0x03));
});

test('constructTlv with Buffer tag', () => {
const tlv = constructTlv(
Buffer.from([0x01, 0x02]),
Buffer.from([0x03, 0x04])
);
expect(tlv).toEqual(Buffer.from([0x01, 0x02, 0x02, 0x03, 0x04]));
const tlv = constructTlv(Buffer.of(0x01, 0x02), Buffer.of(0x03, 0x04));
expect(tlv).toEqual(Buffer.of(0x01, 0x02, 0x02, 0x03, 0x04));
});

test.each<{ valueLength: number; expectedTlvLength: Byte[] }>([
Expand All @@ -161,16 +144,16 @@ test.each<{ valueLength: number; expectedTlvLength: Byte[] }>([
])(
'constructTlv value length handling - $valueLength',
({ valueLength, expectedTlvLength }) => {
const value = numericArray({ length: valueLength });
const value = Buffer.alloc(valueLength);
const tlv = constructTlv(0x01, Buffer.from(value));
expect(tlv).toEqual(Buffer.from([0x01, ...expectedTlvLength, ...value]));
}
);

test('constructTlv value length validation', () => {
expect(() =>
constructTlv(0x01, Buffer.from(numericArray({ length: 65536 })))
).toThrow('TLV value is too large');
expect(() => constructTlv(0x01, Buffer.alloc(65536))).toThrow(
'TLV value is too large'
);
});

test.each<{
Expand All @@ -180,45 +163,30 @@ test.each<{
}>([
{
tagAsByteOrBuffer: 0x01,
tlv: Buffer.from([0x01, 0x7f, ...numericArray({ length: 127 })]),
expectedOutput: [
Buffer.from([0x01]),
Buffer.from([0x7f]),
Buffer.from(numericArray({ length: 127 })),
],
tlv: Buffer.concat([Buffer.of(0x01, 0x7f), Buffer.alloc(127)]),
expectedOutput: [Buffer.of(0x01), Buffer.of(0x7f), Buffer.alloc(127)],
},
{
tagAsByteOrBuffer: 0x01,
tlv: Buffer.from([0x01, 0x81, 0xff, ...numericArray({ length: 255 })]),
expectedOutput: [
Buffer.from([0x01]),
Buffer.from([0x81, 0xff]),
Buffer.from(numericArray({ length: 255 })),
],
tlv: Buffer.concat([Buffer.of(0x01, 0x81, 0xff), Buffer.alloc(255)]),
expectedOutput: [Buffer.of(0x01), Buffer.of(0x81, 0xff), Buffer.alloc(255)],
},
{
tagAsByteOrBuffer: 0x01,
tlv: Buffer.from([
0x01,
0x82,
0xff,
0xff,
...numericArray({ length: 65535 }),
tlv: Buffer.concat([
Buffer.of(0x01, 0x82, 0xff, 0xff),
Buffer.alloc(65535),
]),
expectedOutput: [
Buffer.from([0x01]),
Buffer.from([0x82, 0xff, 0xff]),
Buffer.from(numericArray({ length: 65535 })),
Buffer.of(0x01),
Buffer.of(0x82, 0xff, 0xff),
Buffer.alloc(65535),
],
},
{
tagAsByteOrBuffer: Buffer.from([0x01, 0x02]),
tlv: Buffer.from([0x01, 0x02, 0x01, 0x00]),
expectedOutput: [
Buffer.from([0x01, 0x02]),
Buffer.from([0x01]),
Buffer.from([0x00]),
],
tagAsByteOrBuffer: Buffer.of(0x01, 0x02),
tlv: Buffer.of(0x01, 0x02, 0x01, 0x00),
expectedOutput: [Buffer.of(0x01, 0x02), Buffer.of(0x01), Buffer.of(0x00)],
},
])('parseTlv', ({ tagAsByteOrBuffer, tlv, expectedOutput }) => {
expect(parseTlv(tagAsByteOrBuffer, tlv)).toEqual(expectedOutput);
Expand Down
16 changes: 8 additions & 8 deletions libs/auth/src/apdu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export class CommandApdu {

asBuffer(): Buffer {
return Buffer.concat([
Buffer.from([this.cla, this.ins, this.p1, this.p2, this.lc]),
Buffer.of(this.cla, this.ins, this.p1, this.p2, this.lc),
this.data,
]);
}
Expand Down Expand Up @@ -227,7 +227,7 @@ export function constructTlv(
): Buffer {
const tag: Buffer = Buffer.isBuffer(tagAsByteOrBuffer)
? tagAsByteOrBuffer
: Buffer.from([tagAsByteOrBuffer]);
: Buffer.of(tagAsByteOrBuffer);

/**
* The convention for TLV length is as follows:
Expand All @@ -236,19 +236,19 @@ export function constructTlv(
* - 0x82 0xXX 0xXX if value length > 256 and < 65536 bytes
*
* For example:
* - 51 bytes --> Buffer.from([51]) --> 0x33 (33 is 51 in hex)
* - 147 bytes --> Buffer.from([0x81, 147]) --> 0x81 0x93 (93 is 147 in hex)
* - 3017 bytes --> Buffer.from([0x82, 11, 201]) --> 0x82 0x0b 0xc9 (bc9 is 3017 in hex)
* - 51 bytes --> Buffer.of(51) --> 0x33 (33 is 51 in hex)
* - 147 bytes --> Buffer.of(0x81, 147) --> 0x81 0x93 (93 is 147 in hex)
* - 3017 bytes --> Buffer.of(0x82, 11, 201) --> 0x82 0x0b 0xc9 (bc9 is 3017 in hex)
*/
let tlvLength: Buffer;
const valueNumBytes = value.length;
if (valueNumBytes < 128) {
tlvLength = Buffer.from([valueNumBytes]);
tlvLength = Buffer.of(valueNumBytes);
} else if (valueNumBytes < 256) {
tlvLength = Buffer.from([0x81, valueNumBytes]);
tlvLength = Buffer.of(0x81, valueNumBytes);
} else if (valueNumBytes < 65536) {
// eslint-disable-next-line no-bitwise
tlvLength = Buffer.from([0x82, valueNumBytes >> 8, valueNumBytes & 255]);
tlvLength = Buffer.of(0x82, valueNumBytes >> 8, valueNumBytes & 255);
} else {
throw new Error('TLV value is too large');
}
Expand Down
2 changes: 1 addition & 1 deletion libs/auth/src/artifact_authentication.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ function serializeArtifactSignatureBundle(
const { signature, signingMachineCert } = artifactSignatureBundle;
return Buffer.concat([
// ECC signature length can vary ever so slightly, hence the need to persist length metadata
Buffer.from([signature.length]),
Buffer.of(signature.length),
signature,
signingMachineCert,
]);
Expand Down
Loading

0 comments on commit 9b5ccf5

Please sign in to comment.