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 1 commit
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
7 changes: 6 additions & 1 deletion src/plugins/shs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ export = {
name: 'multiserver-shs',
version: '1.0.0',
init (api: any, config: Config) {
const keys = config.keys && toSodiumKeys(config.keys)
if (!keys) {
console.error(new Error('Config object should contains SHS keys'))
}
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 All @@ -40,7 +45,7 @@ export = {
}

const shs = Shs({
keys: config.keys && toSodiumKeys(config.keys),
keys,
seed: config.seed,
appKey: toBuffer(shsCap),
timeout: timeoutHandshake,
Expand Down
8 changes: 4 additions & 4 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ export type Config = {
appKey?: Buffer | string;

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

Expand Down