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.

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

Beginning ofTypescript migration & S2K Gnu case refactor #1709

Open
wants to merge 13 commits into
base: v6
Choose a base branch
from

Conversation

Realtin
Copy link

@Realtin Realtin commented Dec 12, 2023

This is the beginning of the migration to Typescript. The typescript and rollup configuration are adjusted to support the incremental addition of typescript modules, so it also supports javascript moduls.
All typescript files need to be imported with it's explicit .ts file endings.

  • S2K Module: migrated to typescript
  • S2K Module: case for gnu types is moved to its own class

Copy link
Collaborator

@larabr larabr left a comment

Choose a reason for hiding this comment

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

Thanks for working on this 🙂
I left a few comments, mostly based on the usages of the s2k modules, which in turn affect some typing choices.
The types can also be stricted when dealing with e.g. enums.s2k values, instead of using plain numbers -- we have the relevant type declarations in openpgp.d.ts.

Finally, I mentioned a logic change we'd like to make with the GnuS2K class & instantiations, but we can also work on that at a later time if it's easier 👌

Comment on lines 36 to 57
algorithm: number;
type: string;
c: number;
/**
* @param {Object} [config] - Full configuration, defaults to openpgp.config
*/
constructor(config = defaultConfig) {
/**
* Hash function identifier, or 0 for gnu-dummy keys
* @type {module:enums.hash | 0}
*/
this.algorithm = enums.hash.sha256;
/**
* enums.s2k identifier or 'gnu-dummy'
* @type {String}
*/
this.type = 'gnu';
/**
* @type {Integer}
*/
this.c = config.s2kIterationCountByte;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
algorithm: number;
type: string;
c: number;
/**
* @param {Object} [config] - Full configuration, defaults to openpgp.config
*/
constructor(config = defaultConfig) {
/**
* Hash function identifier, or 0 for gnu-dummy keys
* @type {module:enums.hash | 0}
*/
this.algorithm = enums.hash.sha256;
/**
* enums.s2k identifier or 'gnu-dummy'
* @type {String}
*/
this.type = 'gnu';
/**
* @type {Integer}
*/
this.c = config.s2kIterationCountByte;
}
type: 'gnu' = 'gnu';
gnuType: 'gnu-dummy';

None of the rest should be needed anymore 🙂
And, after talking to Daniel, we'd also like to introduce gnuType, instead of reassigning type = 'gnu-dummy', as such value that does not match a enums.s2k .

In turn, that'd require updating some usages in the rest of the code (where we do e.g. s2k.type === 'gnu-dummy' -> s2k.gnuType === 'gnu-dummy').

@@ -33,7 +35,7 @@ export function newS2KFromType(type, config = defaultConfig) {
* @returns {Object} New s2k object
* @throws {Error} for unknown or unsupported types
*/
export function newS2KFromConfig(config) {
export function newS2KFromConfig(config = defaultConfig) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export function newS2KFromConfig(config = defaultConfig) {
export function newS2KFromConfig(config: Config) {

we want the caller to not forget to pass this explicitly in this case 🙂

Comment on lines 104 to 116
const arr: number[] = [];
let rlength = 0;

while (rlength < numBytes) {
if (this.type !== 'gnu') {
throw new Error('Unknown s2k type.');
} else {
throw new Error('GNU s2k type not supported.');
}
}

return util.concatUint8Array(arr).subarray(0, numBytes);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const arr: number[] = [];
let rlength = 0;
while (rlength < numBytes) {
if (this.type !== 'gnu') {
throw new Error('Unknown s2k type.');
} else {
throw new Error('GNU s2k type not supported.');
}
}
return util.concatUint8Array(arr).subarray(0, numBytes);
}
throw new Error('Gnu S2K does not support producing keys');

✂️

let loadArgonWasmModule;
let argon2Promise;
let loadArgonWasmModule: Function;
let argon2Promise: Promise<Function>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The argon2 module is typed, I'd take advantage of that

import type { default as loadArgonWasmModuleType, computeHashType } from 'argon2id';

let loadArgonWasmModule: typeof loadArgonWasmModuleType;
let argon2Promise: Promise<computeHashType>; // or `ReturnType<typeof loadArgonWasmModuleType>

Comment on lines 29 to 33
type: string;
salt: Uint8Array | null;
t: number;
p: number;
encodedM: number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type: string;
salt: Uint8Array | null;
t: number;
p: number;
encodedM: number;
type: 'argon2';
private salt: Uint8Array | null;
private t: number;
private p: number;
private encodedM: number;

I think we can be quite strict here 🙂

Comment on lines 18 to 28
/**
* Implementation of the String-to-key specifier
*
* {@link https://tools.ietf.org/html/rfc4880#section-3.7|RFC4880 3.7}:
* String-to-key (S2K) specifiers are used to convert passphrase strings
* into symmetric-key encryption/decryption keys. They are used in two
* places, currently: to encrypt the secret part of private keys in the
* private keyring, and to convert passphrases to encryption keys for
* symmetrically encrypted messages.
* @module type/s2k
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

These docs don't apply to the new class -- I leave this just as reminder to either remove the comments, or update them 🙂

Copy link
Author

Choose a reason for hiding this comment

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

I'll remove the comment

/**
* @param {Object} [config] - Full configuration, defaults to openpgp.config
*/
constructor(s2kType, config = defaultConfig) {
constructor(s2kType: number, config = defaultConfig) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
constructor(s2kType: number, config = defaultConfig) {
constructor(s2kType: enums.s2k.simple | enums.s2k.salted | enums.s2k.iterated, config = defaultConfig) {

Comment on lines 37 to 40
algorithm: number;
type: string;
c: number;
salt: Uint8Array | null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
algorithm: number;
type: string;
c: number;
salt: Uint8Array | null;
private algorithm: enums.hash;
type: enums.s2k.simple | enums.s2k.salted | enums.s2k.iterated;
private c: number;
private salt: Uint8Array | null;

and also the getCount() method can be private 🙂

const arr = [new Uint8Array([enums.write(enums.s2k, this.type), this.algorithm])];

switch (this.type) {
case 'simple':
break;
case 'salted':
arr.push(this.salt);
this.salt && arr.push(this.salt);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to throw here (same for the 'iterated' case that follows), or I think it gives the wrong impression that the salt is optional 🙂

Suggested change
this.salt && arr.push(this.salt);
if (!salt) { throw Error('Salt was not set') }
arr.push(this.salt);

import { UnsupportedError } from '../../packet/packet';
import util from '../../util';

class GnuS2k {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
class GnuS2k {
class GnuS2K {

@ayhamthemayhem
Copy link

Hey 👋 We have addressed almost every comment on this PR 🙂 but I did notice one thing in argon2.ts which is using the argon2id package and you mentioned it would be a good idea to leverage the types from the in your comment here but I noticed we are passing ARGON2_TYPE and ARGON2_VERSION to Argon2idParams object which are not used or declared in the Argon2idParams type :/ and even commenting out those values does not break the build or the tests. I added a ts-ignore to not have typescript throw errors on us for now until this is more clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants