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

fix bugs in team reverse sigs and beef up testing #67

Merged
merged 3 commits into from May 9, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 26 additions & 15 deletions lib/subkey.js

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

31 changes: 19 additions & 12 deletions lib/team.js

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

17 changes: 12 additions & 5 deletions src/subkey.iced
Expand Up @@ -33,7 +33,7 @@ exports.SubkeyBase = class SubkeyBase extends Base
esc = make_esc cb, "_v_generate"
if not @get_new_key_section()? and @get_new_km()?
obj = { reverse_sig: null }
obj[@sibkid_slot()] = @get_new_km().get_ekid().toString('hex')
obj[@sibkid_slot()] = @get_new_km().get_ekid().toString('hex') if @sibkid_slot()?
obj.parent_kid = @parent_kid if @parent_kid?
@set_new_key_section obj
if @get_new_km().can_sign()
Expand All @@ -51,8 +51,7 @@ exports.SubkeyBase = class SubkeyBase extends Base

_match_json : (outer, inner) ->
outer = json_cp outer
# body.sibkey.reverse_sig should be the only field different between the two
outer?.body?[@get_field()].reverse_sig = null
@_clear_reverse_sig(outer)
a = json_stringify_sorted outer
b = json_stringify_sorted inner
err = null
Expand All @@ -66,20 +65,28 @@ exports.SubkeyBase = class SubkeyBase extends Base
await @reverse_sig_check { json, new_km: @get_new_km() }, esc defer()
cb null

_get_reverse_sig : (json) ->
json?.body?[@get_field()]?.reverse_sig
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're in here, should we call get_field something more descriptive?

_get_new_sibkid : (json) ->
json?.body?[@get_field()]?[@sibkid_slot()]
_clear_reverse_sig : (outer) ->
# body.sibkey.reverse_sig should be the only field different between the two
outer?.body?[@get_field()].reverse_sig = null
Copy link
Contributor

Choose a reason for hiding this comment

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

Get rid of the ?'s?


reverse_sig_check : ({json, new_km, subkm}, cb) ->

# For historical reasons, some people call 'new_km' 'subkm'
new_km or= subkm

esc = make_esc cb, "SubkeyBase::reverse_sig_check"
err = null
if (sig = json?.body?[@get_field()]?.reverse_sig)? and new_km?
if (sig = @_get_reverse_sig(json))? and new_km?
eng = new_km.make_sig_eng()
await eng.unbox sig, esc defer raw
await a_json_parse raw, esc defer payload
rsk = new_km.get_ekid().toString('hex')
if (err = @_match_json json, payload)? then # noop
else if not streq_secure (a = json?.body?[@get_field()]?[@sibkid_slot()]), (b = rsk)
else if not streq_secure (a = @_get_new_sibkid(json)), (b = rsk)
err = new Error "Sibkey KID mismatch: #{a} != #{b}"
else
@reverse_sig_kid = rsk
Expand Down
9 changes: 6 additions & 3 deletions src/team.iced
Expand Up @@ -28,15 +28,18 @@ class TeamBase extends SubkeyBase
await super opts, defer err
cb err

get_field : () -> "per_team_key"
get_new_key_section : () -> @per_team_key
set_new_key_section : (m) ->
m.generation = @kms.generation
m.encryption_kid = @kms.encryption.get_ekid().toString('hex')
m.signing_kid = @kms.signing.get_ekid().toString('hex')
@per_team_key = m
get_new_km : () -> @kms?.signing # use the signing KM
sibkid_slot : () -> "signing_kid"
need_reverse_sig : (json) -> json?.body?.per_team_key?
need_reverse_sig : (json) -> json?.body?.team?.per_team_key?
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we're not implementing sibkid_slot anymore. Is it ok that we inherit the parent implementation that returns "kid"? It looks like we're signing ourselves up to override all parent methods that call sibkid_slot, but that sounds like a tricky contract?


_get_reverse_sig : (json) -> json?.body?.team?.per_team_key?.reverse_sig
_get_new_sibkid : (json) -> json?.body?.team?.per_team_key?.signing_kid
_clear_reverse_sig : (json) -> json.body.team.per_team_key.reverse_sig = null

_v_include_pgp_details : () -> false

Expand Down
57 changes: 57 additions & 0 deletions test/files/sibkey.iced
@@ -0,0 +1,57 @@
{alloc,Sibkey} = require '../../'
{EncKeyManager,KeyManager} = require('kbpgp').kb
{make_esc} = require 'iced-error'
{new_sig_arg} = require './util'
pgp_utils = require('pgp-utils')
{json_stringify_sorted} = pgp_utils.util

exports.test_sibkey_happy_path = (T,cb) ->
esc = make_esc cb, "test_sibkey_happy_path"
await KeyManager.generate {}, esc defer elder
await KeyManager.generate {}, esc defer sib
arg = new_sig_arg { km : elder }
arg.sibkm = sib
obj = new Sibkey arg
await obj.generate esc defer out
typ = out.inner.obj.body.type
obj2 = alloc typ, arg
varg = { armored : out.armored, skip_ids : true, make_ids : true, inner : out.inner.str }
await obj2.verify varg, esc defer()
cb null

round_trip_with_corrupted_reverse_sig = ({T, corrupt}, cb) ->
esc = make_esc cb, "round_trip_with_corrupted_reverse_sig"
await KeyManager.generate {}, esc defer elder
await KeyManager.generate {}, esc defer sib
arg = new_sig_arg { km : elder }
arg.sibkm = sib
obj = new Sibkey arg

obj._v_generate = (opts, cb) ->
esc = make_esc cb, "_v_generate"
x = { reverse_sig: null }
@set_new_key_section x
eng = @get_new_km().make_sig_eng()
await @generate_json { version : opts.version }, esc defer msg
msg2 = JSON.parse msg
msg2.foo = "bar" if corrupt
msg = json_stringify_sorted(msg2)
await eng.box msg, esc defer { armored, type }
x.reverse_sig = armored
cb null

await obj.generate esc defer out
typ = out.inner.obj.body.type
obj2 = alloc typ, arg
varg = { armored : out.armored, skip_ids : true, make_ids : true, inner : out.inner.str }
await obj2.verify varg, defer err
if corrupt
T.assert err?, "got an error back"
T.assert (err.message.indexOf('Reverse sig json mismatch') >= 0), "found right error message"
cb null

exports.test_corruption_mechanism = (T,cb) ->
round_trip_with_corrupted_reverse_sig { T, corrupt : false }, cb

exports.test_reverse_sig_failure = (T,cb) ->
round_trip_with_corrupted_reverse_sig { T, corrupt : true }, cb
41 changes: 41 additions & 0 deletions test/files/team.iced
Expand Up @@ -2,6 +2,8 @@
{EncKeyManager,KeyManager} = require('kbpgp').kb
{make_esc} = require 'iced-error'
{new_sig_arg} = require './util'
pgp_utils = require('pgp-utils')
{json_stringify_sorted} = pgp_utils.util

test_klass = ({T,arg, klass, keys}, cb) ->
esc = make_esc cb, "test_klass"
Expand Down Expand Up @@ -50,3 +52,42 @@ exports.test_bad_key_section = (T,cb) ->
T.equal err.message, "Need per_team_key.generation to be an integer > 0 (got undefined)", "right message"
cb null

round_trip_with_corrupted_reverse_sig = ({T, corrupt}, cb) ->
esc = make_esc cb, "test_bad_key_section"
await KeyManager.generate {}, esc defer km
arg = new_sig_arg { km }
arg.team = { members : { admin : ["a"] } }
arg.kms = {}
await EncKeyManager.generate {}, esc defer arg.kms.encryption
await KeyManager.generate {}, esc defer arg.kms.signing
arg.kms.generation = 10
obj = new team.RotateKey arg

obj._v_generate = (opts, cb) ->
esc = make_esc cb, "_v_generate"
x = { reverse_sig: null }
@set_new_key_section x
eng = @get_new_km().make_sig_eng()
await @generate_json { version : opts.version }, esc defer msg
msg2 = JSON.parse msg
msg2.foo = "bar" if corrupt
msg = json_stringify_sorted(msg2)
await eng.box msg, esc defer { armored, type }
x.reverse_sig = armored
cb null

await obj.generate_v2 esc defer out
typ = out.inner.obj.body.type
obj2 = alloc typ, arg
varg = { armored : out.armored, skip_ids : true, make_ids : true, inner : out.inner.str }
await obj2.verify_v2 varg, defer err
if corrupt
T.assert err?, "got an error back"
T.assert (err.message.indexOf('Reverse sig json mismatch') >= 0), "found right error message"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be asserting anything in the !corrupt case? It does look like we mean to test that case below.

cb null

exports.test_corruption_mechanism = (T,cb) ->
round_trip_with_corrupted_reverse_sig { T, corrupt : false }, cb

exports.test_reverse_sig_failure = (T,cb) ->
round_trip_with_corrupted_reverse_sig { T, corrupt : true }, cb