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

enforce config.keys in TS and log an error when no keys are provided #67

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

enforce config.keys in TS and log an error when no keys are provided #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
345dc97
enforce config.keys in TS and log an error when no keys are provided
austinfrey Dec 20, 2020
5458be5
throw error for missing keys instead of logging
austinfrey Dec 21, 2020
f293838
allow a seed in lieu of keys
austinfrey Dec 21, 2020
4e0e1b3
remove unnecessary seed to key conversion
austinfrey Dec 22, 2020
dfbe7af
update type Config to use keys OR seed
austinfrey Dec 22, 2020
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
12 changes: 6 additions & 6 deletions package-lock.json
View file
Open in desktop

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

4 changes: 4 additions & 0 deletions src/plugins/shs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ export = {
name: 'multiserver-shs',
version: '1.0.0',
init (api: any, config: Config) {
if (!config.keys && !config.seed) {
throw new Error('Config object should contains SHS keys or a seed')
}
Copy link
Member

Choose a reason for hiding this comment

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

I like this. I would hazard you should probably throw instead of printing an error.

I'd also like to see a test around this - you may find you never see this log because toSodium could error loudly if things go wrong internally to that? (haven't looked at it)

Copy link
Author

Choose a reason for hiding this comment

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

@mixmix appreciate the review. the change has been made to throw rather than log. trying to think through the testing changes needed and could use some additional feedback.

this is what I've come up with so far to add a test case using the shs plugin directly, as catching the error when creating a new server catches the missing keys error but throws again when core.js goes to load and the shs transform is missing. Would the following suffice?

tape('throw error when no keys are supplied', function (t) {
  var shsPlugin = require('../lib/plugins/shs')
  var api = {}
  var config = {}

  // can't use create() here as catching the error for missing
  // keys causes another error downstream in ../lib/core where
  // the shs plugin is missing 'secret-stack needs at least 1 
  // transform protocol'
  try {
    shsPlugin.init(api, config)
  } catch (err) {
    t.ok(err, err.message)
  } 
  t.end()
})

Should this PR be enhanced to throw only when both keys and a seed are missing? That would save us from updating all the tests.

cc @staltz

Copy link
Member

Choose a reason for hiding this comment

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

I am not familiar enough with this code but expect that both keys need to be there... Or at least the public key.

Your test looks really scoped and sufficient to me. BTW you have essentially recreated tapes t.throws

You could write it like this

t.throws(
  () => plugin.init(ssb, config), 
  /shs plugin requires valid config.keys/
  "throws without keys"
)

Seconds arg is a regex which will be used to test error.message on the error which is thrown

Copy link
Author

Choose a reason for hiding this comment

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

This has been updated to include 2 new tests. One which throws an error when neither keys nor a seed are passed into with the config. A second test is included that shows the publicKey is being correctly populated when passing just a seed.

This is usually accomplished in multiserver where it would use the keys and expose the public key. Unfortunately we can't wait for this to make it down to multiserver when throwing for missing keys at this level, since the publicKey is needed right away.

This seems like a good idea to preserve some backward compatibility regarding the use of seeds, but open to being told otherwise :). This also introduces another dependency, secret-handshake, to use the toKeys method to convert the seed to a keypair but maybe using chloride directly would make more sense.


let timeoutHandshake: number | undefined
if (!isNaN(config.timers?.handshake as any)) {
timeoutHandshake = config.timers?.handshake!
Expand Down
29 changes: 19 additions & 10 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,13 @@ export type Transform = {
create: () => unknown;
};

export type Config = {
export interface BaseConfig {
// Cryptographic capability key
caps?: {
shs?: Buffer | string;
};
appKey?: Buffer | string;

// Cryptographic keys
keys?: {
public?: string;
private?: string;
id?: string;
};
seed?: unknown;

// Multiserver
connections?: {
incoming?: {
Expand All @@ -59,4 +51,21 @@ export type Config = {
// Legacy but still supported
host?: string;
port?: number;
};
};

export interface SeedConfig extends BaseConfig {
seed: unknown;
keys: never;
};

export interface KeysConfig extends BaseConfig {
seed: never;
// Cryptographic keys
keys: {
public: string;
private: string;
id: string;
};
};

export type Config = KeysConfig | SeedConfig;
Copy link
Author

Choose a reason for hiding this comment

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

@staltz I am attempting to create a Config type that requires one OR the other of a property. This makes it's way through the typescript compiler without issue, but wondering if there is a better solution.

20 changes: 20 additions & 0 deletions test/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,26 @@ var create = SecretStack({
var alice = create({ seed: seeds.alice })
var bob = create({ seed: seeds.bob })

tape('throw error when no seed or keys are supplied', function (t) {
var noop = () => {}
var shsPlugin = require('../lib/plugins/shs')
var api = {
multiserver: {
transform: noop
}
}
var config = {
caps: {shs: appkey}
}

t.throws(
() => shsPlugin.init(api, config),
/Config object should contains SHS keys/,
'SHS plugin throws without seed or keys'
)
t.end()
})

tape('alice connects to bob', function (t) {
alice.connect(bob.address(), function (err, rpc) {
if (err) throw err
Expand Down