Bcrypt password string to byte conversion

[straight-shoota]

While reviewing #15931 I noticed some oddities with the conversion from String to Bytes in Bcrypt.new and .hash_secret.

crystal/src/crypto/bcrypt.cr

Lines 73 to 86 in 08940fc

	# Creates a new `Crypto::Bcrypt` object from the given *password* with *salt* and *cost*.
	#
	# ```
	# require "crypto/bcrypt"
	#
	# password = Crypto::Bcrypt.new "secret", "salt_of_16_chars"
	# password.digest
	# ```
	def self.new(password : String, salt : String, cost = DEFAULT_COST)
	# We make a clone here to we don't keep a mutable reference to the original string
	passwordb = password.to_unsafe.to_slice(password.bytesize + 1).clone # include leading 0
	saltb = Base64.decode(salt, SALT_SIZE)
	new(passwordb, saltb, cost)
	end

The first oddity is that .clone call. There's a code comment that claims this is to avoid keeping a mutable reference to the original string. But AFAIK BCrypt does not mutate the password slice. It exposes it as #password which might make it available for other things. But it's still a read-only slice. So I don't think there's great danger here. We should be able to avoid the extra allocation.
The API docs for the other overload, which accepts Bytes explicitly demonstrates its use with "secret".to_slice.

Also, why does the byte slice include the trailing zero?
At first glance, this seems wrong because trailing zeros are an implementation detail of String.
I'd expect this to cause compatibility issues with other implementations. But this actually seems to work... Is a trailing zero expected in bcrypt passwords?

Related: #15276

[memiux]

The changes were introduced in the following commits: 13bc771, 2c48394.

[ysbaddaden]

Is a trailing zero expected in bcrypt passwords?

That indeed rings a bell. I tried to drop the trailing zero... and the test vectors are failing. I'd say that yes, bcrypt requires a trailing zero somehow.

The first oddity is that .clone call.

I guess we didn't have read-only slices at that point. Maybe we wanted to make sure that the slice (retained independently from the string) would be completely distinct from the string?

We wouldn't care if the API was only a couple methods as proposed in #15276.