Change saltLen to unit8 and add some tests

[dominikschulz]

Fixes #2116

RELEASE_NOTES=n/a

Signed-off-by: Dominik Schulz dominik.schulz@gauner.org

[AnomalRoil]

Are we sure we have the proper tests for saltLen as well?

[AnomalRoil]

It does seem like it was wrong already, but then isn't that really a breaking change that could change the resulting hashes for people?

[dominikschulz]

Do you mean "breaking change" in the context of semver?
If so I'd argue no, because it was broken before and did not work as intended. So this is a fix, not a breaking change.

The previously generated hashes were just invalid. They would use a random salt of length 32 (depending on the function) and either an empty string or the requested salt length as the password.

I don't know why this did never occur to anyone. Maybe nobody is using this?

I certainly did use it before, but not sure when. If I have some time I might bisect to find the breaking change. But not right now.

[AnomalRoil]

Yeah, I meant "breaking" as in "this could lead to different hashes now that we have patched".
But having now looked at its references in the code, it seems we are only using it to generate new values, so it's not going to cause different values on existing entries.

My guess is that it's dating from when we stopped using fixed size salts to use this saltLen function instead last year in that commit.

[dominikschulz]

Wow, good find.

Yes, indeed. That must be it.

I don't think I have used the feature since then.

Leaves the question how useful it is.
But I need it from time to time ...

[AnomalRoil]

@dominikschulz Wow, seems this just broke our CI on master 😱

https://github.com/gopasspw/gopass/runs/4851609036?check_suite_focus=true

[dominikschulz]

Why would that fail after the tests ran successfully on the PR? 🤔

[AnomalRoil]

I just ran it twice locally and it failed once and worked once...

[AnomalRoil]

$ go test ./internal/tpl/... -v   
    template_test.go:290: 
        	Error Trace:	template_test.go:290
        	Error:      	Received unexpected error:
        	            	can't validate: hash does not match password
        	Test:       	TestVars/{{.Content_|_md5crypt_"7"}}
        	Messages:   	{{.Content | md5crypt "7"}}
--- FAIL: TestVars (0.00s)

[dominikschulz]

So it's a flaky test. Hmm, I didn't expect that. Need to take a look later. #2121