Implement reference key server

[vqhuy]

• Implement registration function

[arlolra]

Can remove the true here.

[arlolra]

fallthrough doesn't seem right

[vqhuy]

I think we could do ./main -genconfig -genkey=sign,vrf. Does it make sense?

[arlolra]

No, fallthrough just falls through, it doesn't check the second condition.

[vqhuy]

Oops :( Thanks!

[vqhuy]

I just tried with coniksserver -genconfig and it generated the config file only. So I think it worked as expected?

[arlolra]

Only because genkey doesn't have a default value, right. The code still gets executed but on the empty string, so nothing happens.

I guess it's ok with the fallthrough then, but maybe leave a comment saying that we're relying on that. Or not.

[vqhuy]

Yup, I got it. Thanks!

[arlolra]

boolStringFlag is no longer used.

[arlolra]

If you want to keep it, go back and look at,

keyTypes = strings.Split(keys, ",")
log.Println(keyTypes)

[arlolra]

I wonder if this should be a path to dir?

[vqhuy]

No, it's the path to config.toml (https://github.com/coniks-sys/coniks-go/blob/cdaf4bf18b7f801149e6766f7239a301ec0c2365/keyserver/server.go#L60-L64)

[arlolra]

I know, I was just thinking about,

cd keyserver/coniksserver/
mkdir config
./coniksserver -dir config -genconfig -genkey "sign,vrf"
./conkisserver -dir config   // to run

But, ya, nevermind.

[vqhuy]

Yeah, sounds good!

[arlolra]

λ (registration *) ./coniksserver -genkey "sign"
2016/08/15 11:58:06 {true sign,vrf}

boolStringFlag still doesn't seem to be working.

[vqhuy]

boolStringFlag is a bool type flag. The reason is I want to have a call like ./coniksserver -genkey with default value. I'm not sure if we want to change it to string type flag.

[arlolra]

Ok, but right now it doesn't support generating the keys individually. So there's no point in doing all the string split orchestration.

[vqhuy]

Did you try -genkey=sign. I think it should work.

[arlolra]

Did you try -genkey=sign. I think it should work.

Well that's embarrassing. Yup, that works. You've even documented it correctly. Although, it's slightly unintuitive that ./coniksserver -genkey "sign" doesn't work, but at least I see why now. It shorts to bool and "sign" ends up being another argument.

Ok, sorry for being slow here. What you have is fine.

[arlolra]

Maybe a helper for this boilerplate?

func resolvePath(file, other string) other string {
  if !filename.IsAbs(other) {
    file = filepath.Join(filepath.Dir(file), other)
  }
} 

vrfPath := util.resolvePath(file, conf.Policies.VRFKeyPath)

[arlolra]

Maybe NewConiksServer could take an optional db?

[vqhuy]

Maybe it's unnecessary, since this's just a modification for testing.

[arlolra]

Sorry, move this below the err check.

[masomel]

I'm still reviewing this pull, and was going through every single commit. Please don't merge this yet.

[masomel]

What does this image represent?

[arlolra]

I think it's an ascii onion. But, yeah, we should probably stick to CONIKS branding.

[vqhuy]

Yes, it's Tor logo ascii. I hope you don't get mad at this :(

[arlolra]

was going through every single commit.

Sorry for clearing the history, I can restore it if you want :(

[masomel]

Don't worry about it :) It's much easier to review a single commit and I had only made one small comment (which I've added back in).

[arlolra]

Phew

[masomel]

I think the panic should be on err not e, no? Otherwise, if the registration request resulted in an error as well, we'll end up panicking on that instead.

[arlolra]

panic seems very wrong here.

[vqhuy]

I think the panic should be on err not e, no?

Yes, it should panic on err.

panic seems very wrong here.

I think if there is any error here, it must be our bug?

[masomel]

I think if there is any error here, it must be our bug?

Not necessarily a bug? I'm not sure. But definitely internal server error (e.g. ErrorDirectory)

panic seems very wrong here.

@arlolra do you think we shouldn't be panicking here?

[vqhuy]

But definitely internal server error (e.g. ErrorDirectory)

No, it is panic(err) which is an error returned by json.Marshal. That's why I said it's maybe our bug.

[masomel]

No, it is panic(err) which is an error returned by json.Marshal. That's why I said it's maybe our bug.

Oh yeah you're right, it's definitely a bug in this case because the response might be malformed. My previous response more for the general case.

[arlolra]

Sorry, I hastily threw in the above comment. Don't mind me here.

it's definitely a bug in this case because the response might be malformed

Sounds right.

Thanks for digging into the error propagation so thoroughly.

[arlolra]

That's Tor Messenger specific. We should keep this to CONIKS only. Since there's no logo, what if you just did "CONIKS" in ascii art?

Pick your favourite http://patorjk.com/software/taag/#p=display&f=Rectangles&t=CONIKS

 _____ _____ _____ _____ _____ _____ 
|     |     |   | |     |  |  |   __|
|   --|  |  | | | |-   -|    -|__   |
|_____|_____|_|___|_____|__|__|_____|

[vqhuy]

Yup, I probably should remove it :D. Sorry for the noise :D

[masomel]

Nice!

[masomel]

LGTM :)