Page MenuHomePhabricator

[lib] Introduce PKCS#7 padding
ClosedPublic

Authored by bartek on Mar 13 2023, 7:40 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Feb 27, 11:10 AM
Unknown Object (File)
Mon, Feb 19, 3:12 AM
Unknown Object (File)
Mon, Feb 19, 2:14 AM
Unknown Object (File)
Mon, Feb 19, 1:12 AM
Unknown Object (File)
Mon, Feb 19, 12:09 AM
Unknown Object (File)
Sun, Feb 18, 5:03 PM
Unknown Object (File)
Jan 30 2024, 4:11 AM
Unknown Object (File)
Jan 19 2024, 4:42 AM

Details

Summary

Part of ENG-3099

This diff implements standard PKCS#7 padding using JS typed arrays.

Test Plan

Added unit tests. Also tested using my CLI tool and hex editor / viewer (e.g. xxd tool)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Mar 13 2023, 8:09 AM
bartek removed a reviewer: anunay.
bartek added a subscriber: anunay.
lib/utils/pkcs7-padding.js
12

Nit: we don't use inline exports in the codebase except for Flow types and for files in lib/types (and other "types" files like that)

Can you move these to be a single export declaration at the bottom of the file?

Specifically adding @tomek as reviewer since he seemed to have some thoughts during the Encryption Sync (that might've been discussed offline?)

In D7059#210217, @atul wrote:

Specifically adding @tomek as reviewer since he seemed to have some thoughts during the Encryption Sync (that might've been discussed offline?)

Yes, I've met with @bartek and he clarified my concern. The thing which I was missing was the fact that the block padding is always added. Now I can't find any example that won't work.

Thanks for including unit tests!

This revision is now accepted and ready to land.Mar 17 2023, 1:01 PM
lib/utils/pkcs7-padding.js
13–14

Would it maybe make more sense to throw exceptions instead of using invariant?

CC @ashoat

lib/utils/pkcs7-padding.js
13–14

I think invariant is good for "programmer error" type scenarios like this but I don't feel strongly. Ultimately invariant just throws an exception so it doesn't really matter

lib/utils/pkcs7-padding.js
12

Sure, will do

13–14

I'd stick to invariant because it looks clean.

This revision was automatically updated to reflect the committed changes.