<p><a href="https://go.iondynamics.net/statelessPassword">statelessPassword</a> is a derivation of <a href="https://en.wikipedia.org/wiki/Master_Password">Billemont's master password algorithm</a></p>
<p>It comes with a little CLI tool that generates passwords for you. <a href="https://github.com/ionDynamics/statelessPassword">GitHub</a><br/>
Due to the nature of the algorithm there is no need to sync password vaults between different devices.
You just have to remember your full name and your master password (if you stick to the defaults)</p>
<p>I'd highly appreciate some feedback :)</p>
<hr/>**评论：**<br/><br/>TheMerovius: <pre><p>I skipped a bit through your code and noticed a couple of things, in no particular order:</p>
<ul>
<li>What is variant?</li>
<li>Why are you exporting all those constants and variables (without any explanation what they're doing, no less)? I don't think they should be exported.</li>
<li>If you need to export them, use var/const blocks like this</li>
</ul>
<p>.</p>
<pre><code>var (
Foo = "x"
Bar = 42
)
</code></pre>
<ul>
<li>There's no need for two files here. It's less than 200 lines in total, that's completely manageable</li>
<li><code>uint32(seed[0])%uint32(len(templates))</code> is biased, it generates more small numbers than large numbers. Especially as <code>len(templates)</code> is probably small and probably doesn't divide 256. The correct way to create random numbers in a range is <a href="https://en.wikipedia.org/wiki/Rejection_sampling">rejection sampling</a>. It's non-trivial to implement here, but possible. Also, why converting to uint32? </li>
<li><a href="https://github.com/ionDynamics/statelessPassword/blob/master/algorithm.go#L63">this line</a> is unreadable. Split it up (and probably use a byte.Buffer or something).</li>
<li>The salt you add is relatively useless (from what I know). Salts need to be random strings to have the desired effect.</li>
<li>Also, why the conversion to uint32 again?</li>
<li>Make the whole passchars logic a <code>map[rune]string</code> or something, instead of exported variables and the humongous switch statement</li>
<li><code>uint32(seed[i+1])%uint32(len(passchars))</code> is also biased and doesn't need the <code>uint32</code> conversion.</li>
<li>Also, I would remove all that customization and variant stuff. Just choose the correct parameters and tick to them. People are in general idiots and given several choices will always take the worst possible one. That is especially true for cryptography.</li>
<li>It seems flawed to me, that I apparently need to remember the template I used to generate my password too.</li>
<li>In your <a href="https://github.com/ionDynamics/statelessPassword/blob/master/cmd/slpw/main.go">main.go</a> you seem keen to put the type of a variable into it's name. That's very unidiomatic.</li>
<li>No need for defVar, defVarUsed and devVarStr. Also, you shouldn't silently ignore invalid values, that will lead to strange errors for users. And no global variables that you only use in one place. I would rewrite the variant logic as:</li>
</ul>
<p>.</p>
<pre><code>variant := uint8(3)
if s := os.Getenv("ID_SLPW_DEFAULTVARIANT"); s != "" {
var err error
if variant, err = strconv.Atoi(s); err != nil {
log.Fatal("Illegal value for ID_SLPW_DEFAULTVARIANT")
}
}
</code></pre>
<ul>
<li>Implementing a <a href="https://github.com/ionDynamics/statelessPassword/blob/master/cmd/slpw/main.go#L48">"legacy mode"</a> inside the first released version of your software seems illogical to me. There is no legacy yet. This goes hand-in-hand with what I wrote above: Make the correct choice and stick to it.</li>
<li>You use Environment variables before you parse the flags (including the noEnv-flag). You also use the environment variable for the name of the user before using that flag. Also, you use Camelcase in flags, they should probably be dash-separated</li>
<li>You mix global flag variables with local ones. Pick a style, stick with it.</li>
<li>The documentation of <a href="https://godoc.org/bufio#Reader.ReadString">bufio.Reader.ReadString</a> tells you that a Scanner is probably more convenient. I agree.</li>
<li><code>fmt.Println("Found User:", fullname)</code> smells like debug-logging that has nothing to do in released code.</li>
<li>Don't use <code>syscall</code> for something like this. Rather use <code>os.Stdin.Fd()</code>.</li>
<li>I think your code has a subtle bug, namely that you use <code>bufio</code> to wrap stdin, but then continue to use it. This will blow up, when you do, e.g. <code>echo "foo\nbar\nbaz\nspam\neggs" | ./slpw</code> (i.e. don't use a terminal as an input). Also, you then use the wrapping Reader again afterwards. <code>*bufio.Reader</code> consumes input and buffers it, that's where the name comes from -- in particular, it will read more than you intend, given the chance.</li>
<li>Also, using the terminal package will blow up when not using a terminal as input.</li>
</ul>
<p>It's getting late here, so I'm going to bed for now :)</p>
<p>[edit] reddit code formatting sucks ballz. It doesn't format stuff as code, even though I use the four spaces <em>and</em> leave an extra line between the surrounding stuff -.-</p>
<p>[edit2] for now, this works. But blerghs, I would've expected more.</p></pre>KEANO_: <pre><p>That's really good. Thank You :)</p>
<p>Okay starting from the top:</p>
<ul>
<li><p>A variant is a preset of parameters for the scrypt key generation. I maybe should have named this "scrypt preset" </p></li>
<li><p>The constants don't need to be exported and shouldn't be but the original algorithm differentiates uppercased-only character sets by naming them uppercased. But this is a derivate of this algorithm. So I might change this</p></li>
<li><p>Going to change this one in the next commit/push (context: var/const blocks)</p></li>
<li><p>Same here (context: Two files) </p></li>
<li><p>uint32(seed[0])%uint32(len(templates)) is a direct translation from the original algorithm. The uint32 is inforced in Billemont's algo as well as the way of choosing the template. But again: This is a derivate</p></li>
<li><p>At the risk of repeating myself: Going to change this one in the next commit/push (context: unreadable line) </p></li>
<li><p>The salt doesn't has to be necessarily random. It should increase the increase the input data to prevent dictionary attacks. <a href="http://masterpasswordapp.com/algorithm.html" rel="nofollow">Billemont's website</a> states that the full name of the user provides "sufficiently high entropy while being (hopefully) impossible to forget by the user"</p></li>
<li><p>As mentioned above: Billemont explicitly wants unsigned 32bit integer</p></li>
<li><p>That's another very good point. Going to change this according to your feedback. (context: map[rune]string)</p></li>
<li><p>Again: This one is taken directly from Billemont... but slpw is still a derivate</p></li>
<li><p>Sadly to say but you are probably right about the people being idiots. I'm going to remove these custimization options from the cli tool. I'll keep them in the library for future hardware generations.</p></li>
<li><p>Yup. That you have to remember your template is annoying. But imagine a website that doesn't accept special characters in the password. So you have to tell the password generator that you don't want special characters. Therefore the templates</p></li>
<li><p>You mean naming a Pointer fooPtr?</p></li>
<li><p>This is going to be removed as I'm following your previous suggestion to stick to one variant. But still good to know for future projects.</p></li>
<li><p>The "legacy mode" should be named "Billemont mode". It sets the scrypt parameters to Billemont's original</p></li>
<li><p>Ack. Going to change this (context: env vars)</p></li>
<li><p>Same here (context: global vs local style. pick one)</p></li>
<li><p>I already stepped down from using ReadLine because the docs say "use ReadString/Byte instead" :D (Not to mention that in the very same paragraph they refer to some mysterious thing called Scanner)</p></li>
<li><p>Nope. Not a debug log. Just bad wording again :D I might change this to: "Generating passwords for {bla bla bla}". If slpw is used by more than one person on the same computer that could be handy.</p></li>
<li><p>Ack. (context: os.Stdin.Fd() instead of syscall)</p></li>
<li><p>I'll look into this and definitely change the order of using stdin and bufio.Reader(stdin). So I'll first get the master password from stdin via terminal package and than stuff stdin into bufio.Reader. (or Scanner :D)</p></li>
<li><p>The slpw command isn't really intended to be used with anything else than a terminal. I've used the terminal package because it can hide the master password input. Is there a better way to do this? </p></li>
</ul>
<p>That was a really good feedback. Thank you again.<br/>
TIL a couple of things :)</p>
<p>Edit: Struggling with the formatting, too</p>
<p>Edit2: Added context to some replies</p></pre>TheMerovius: <pre><blockquote>
<p>The constants don't need to be exported and shouldn't be but the original algorithm differentiates uppercased-only character sets by naming them uppercased. But this is a derivate of this algorithm. So I might change this</p>
</blockquote>
<p>I very strongly urge you :)</p>
<blockquote>
<p>uint32(seed[0])%uint32(len(templates)) is a direct translation from the original algorithm.</p>
</blockquote>
<p>Then the original is broken as well and needs to be fixed. And you might be suspicious of the skills of the original implementer when it comes to cryptography, as that is a very basic thing that they should have learned early on. It's so broken that you shouldn't even use it in code that <em>isn't</em> supposed to be secure in any way (say, a virtual dice-game).</p>
<p>You can convince yourself of the basic reasoning here with the following thought-experiment: Try to emulate an unbiased 6-sided die using only a coin-flip as a source of randomness. That's essentially what you are trying to do here (with every bit of the input/seed being one coin-flip and the number of sides of the die being <code>len(templates)</code>).</p>
<blockquote>
<p>The uint32 is inforced in Billemont's algo as well as the way of choosing the template</p>
</blockquote>
<p>I assume they do it because of some quirk of their implementation language. It has no effect whatsoever in go's case (except, of course, where they are needed to soothe the compiler). Use int (and don't convert the len-output).</p>
<blockquote>
<p>The salt doesn't has to be necessarily random. It should increase the increase the input data to prevent dictionary attacks. Billemont's website states that the full name of the user provides "sufficiently high entropy while being (hopefully) impossible to forget by the user"</p>
</blockquote>
<p>This is some shoddy reasoning right there. Names have next to no entropy at all. And salts don't make dictionary attacks any harder, really. They make reverse lookups harder, but not pure dictionary attacks.</p>
<p>I don't know, it <em>probably</em> doesn't matter a lot, but when it comes to crypto implementation details, I wouldn't trust "probably" or "a lot". I would just generate, for example, a random 6-character string (only letters) and expect the user to remember that too, which should give you a comparable amount of entropy but without the bias and actually secure. Or just generate a random string and publish that somewhere memorable. But ¯_(ツ)_/¯, as I said, it's probably fine.</p>
<p>Btw, an IMHO good introduction into password hashing and the like is <a href="https://crackstation.net/hashing-security.htm" rel="nofollow">this</a></p>
<blockquote>
<p>You mean naming a Pointer fooPtr?</p>
</blockquote>
<p>Yes, that :) Also, Str and stuff. In practice, it turns out, you don't normally forget this stuff and if you do your compiler will point it out. It's not a huge problem and readability benefits from having simple/short names.</p>
<blockquote>
<p>Not to mention that in the very same paragraph they refer to some mysterious thing called Scanner</p>
</blockquote>
<p><a href="https://godoc.org/bufio#Scanner" rel="nofollow">bufio.Scanner</a>. It's usefull. It handles all kinds of nastiness with the ReadString stuff for you :)</p>
<blockquote>
<p>Nope. Not a debug log. Just bad wording again :D I might change this to: "Generating passwords for {bla bla bla}". If slpw is used by more than one person on the same computer that could be handy.</p>
</blockquote>
<p>Hm. I disagree (I didn't say "smells like debug log" purely because of the wording, but also because that information seems unnecessarily verbose to me). But ¯_(ツ)_/¯, that's up to opinion.</p>
<blockquote>
<p>The slpw command isn't really intended to be used with anything else than a terminal. I've used the terminal package because it can hide the master password input. Is there a better way to do this?</p>
</blockquote>
<p>In that case I would make that explicit: Error out if it isn't used with a terminal. However, be advised that if your program is widely used, <a href="https://xkcd.com/1172/" rel="nofollow">there <em>will</em> be someone who wants to use it without a terminal and they <em>will</em> get angry at you</a> ;) Not that you necessarily need to care. In regards to better ways: That's a science in and off itself. There's a reason why gpg and ssh and stuff these days use separate agents with pluggable password-dialog software. For example it is very hard to prevent keylogging from linux and impossible from a terminal.</p>
<p>But it's probably fine to use a terminal, yes. Just don't create mysterious error conditions when people are trying to pipe stuff in.</p></pre>xkcd_transcriber: <pre><p><a href="http://imgs.xkcd.com/comics/workflow.png" rel="nofollow">Image</a></p>
<p><a href="http://m.xkcd.com/1172/" rel="nofollow">Mobile</a></p>
<p><strong>Title:</strong> Workflow</p>
<p><strong>Title-text:</strong> There are probably children out there holding down spacebar to stay warm in the winter! YOUR UPDATE MURDERS CHILDREN.</p>
<p><a href="http://www.explainxkcd.com/wiki/index.php/1172#Explanation" rel="nofollow">Comic Explanation</a></p>
<p><strong>Stats:</strong> This comic has been referenced 616 times, representing 0.6217% of referenced xkcds.</p>
<hr/>
<p><sup><a href="http://www.xkcd.com" rel="nofollow">xkcd.com</a></sup> <sup>|</sup> <sup><a href="http://www.reddit.com/r/xkcd/" rel="nofollow">xkcd sub</a></sup> <sup>|</sup> <sup><a href="http://www.reddit.com/r/xkcd_transcriber/" rel="nofollow">Problems/Bugs?</a></sup> <sup>|</sup> <sup><a href="http://xkcdref.info/statistics/" rel="nofollow">Statistics</a></sup> <sup>|</sup> <sup><a href="http://reddit.com/message/compose/?to=xkcd_transcriber&subject=ignore%20me&message=ignore%20me" rel="nofollow">Stop Replying</a></sup> <sup>|</sup> <sup><a href="http://reddit.com/message/compose/?to=xkcd_transcriber&subject=delete&message=delete%20t1_cztw05p" rel="nofollow">Delete</a></sup></p></pre>Growlizing: <pre><p>Interesting take, I have not seen this before.</p>
<p>I feel like this:</p>
<pre><code>Password(site string, version string, templates []string) (string, error)
</code></pre>
<p>Should be an interface, and the <em>type</em> Algorithm be unexported implementation of the interface.</p>
<p>It lends itself nicely to an interface.</p></pre>KEANO_: <pre><p>Going to change this to an interface with</p>
<pre><code>Init(fullname, masterpassword []byte, variant uint8)
Password(site string, version string, templates []string) (string, error)
</code></pre>
<p>Thanks for your feedback :)</p></pre>tech_tuna: <pre><p>This looks cool. Question - let's say I wanted to add your tool to my CI/CD pipeline. What would be the best way to do that? Obviously I need everything to be automated.</p>
<p>I've just added something like this to my pipeline using a combination of Jenkins encryption and ansible-vault. Thanks!</p></pre>KEANO_: <pre><p>Could you describe a more specific use case? Which problem do you want to solve with slpw?</p></pre>tech_tuna: <pre><p>Sure, I have a Jenkins job that pulls down source from Github, builds a Docker container and then runs tests which need to access various cloud services.</p>
<p>Right now what I have is a YAML secrets file containing my cloud API keys and passwords. I've encrypted the secrets file with ansible-vault. At runtime (build/test), I decrypt the secrets file (in memory only) using the ansible password which I store encrypted in Jenkins using its built-in credentials manager.</p>
<p>The weak point is that if someone hacked into my CI system, they could simply follow the logic in my build scripts and decrypt the secrets file by running my build. Maybe I'm an idealist but I feel like I can do better than that. </p></pre>KEANO_: <pre><p>Unfortunately slpw won't do any help in this case. slpw needs a secret as well.
Using slpw just shifts the attack vector from your CI system to your machine with slpw.</p>
<p>Edit: I don't think there is a much better approach than yours. It's pretty good to decrypt the passwords in RAM and don't store them anywhere in plaintext.
As you have to store them somewhere this location will always be a delicate target for an attacker. Make sure you don't get hacked and if you get hacked make sure you know as fast as possible that you were hacked. So you can regenerate your API keys</p>
<p>Edit2: Make sure you don't get hacked :D As it were that simple. But you get the point, right? </p></pre>tech_tuna: <pre><blockquote>
<p>Make sure you don't get hacked</p>
</blockquote>
<p>Best advice ever! :)</p>
<p>Thanks for the feedback. . . security is a pain.</p></pre>
