Fix generation of alphanumeric strings

[dbarranco]

Hi all!
I found an issue in the kubernetes helm charts due to the generation of alpha numeric strings.
Currently, the helm chart of Magento is using the randAlphaNum to generate the password of the application in the secrets.yaml:

magento-password: {{ randAlphaNum 10 | b64enc | quote }}

In each deploy Magento checks if the password of the application is alpha numeric, and following your library and making a simple test I can see that not every time the string generated is alpha numeric. Here you can see a simple script that generates random alpha numeric strings:

package main

import (
	"fmt"
	"goutils"
	"regexp"
)
func main() {
	for index := 0; index < 10000; index++ {
		a, err := goutils.RandomAlphaNumeric(10)
		if err != nil {
			fmt.Println("Err != nil")
		}
		match, _ := regexp.MatchString("([0-9]+)", a)
		if !match {
			fmt.Println("###### This string has no numbers: ")
		}
		fmt.Println("Random string: ", a)
	}
}

And one of the outputs of this test:

Random string:  9OyWQ7paZ6
Random string:  qQ47j1L8la
Random string:  AMabYevo22
Random string:  F6ohCHyjUs
Random string:  lTbrBkJ5dK
Random string:  ZliU9eaJPF
###### This string has no numbers:
Random string:  GOltNlZafR 
Random string:  wzy5RHUDiD
###### This string has no numbers:
Random string:  EBLPBCAYSm 
Random string:  chjWlJ494P

So each time the random string generated is not alpha numeric, you will see this error in the helm chart of Magento:

08:31:36 [testChart] nami    INFO  Initializing php
08:31:36 [testChart] nami    INFO  php successfully initialized
08:31:36 [testChart] nami    INFO  Initializing magento
08:31:36 [testChart] Error executing 'postInstallation': The admin password must contain both letters and numbers.

It would be nice to add this to the repo in order to generate always alpha numeric strings. Another solution of this will be concatenating a random string of numbers and a random string of characters in the helm template of the password of Magento, but that would decrease the entropy of the password, making it easier to hack.

Note: after adding the patch and testing the strings with 10000 random strings works as expected.
Also, this case can be extended to any other application with password validation in the installation process.

Best regards!

[technosophos]

I think it would be possible (though statistically rare) for the above to produce a string of all digits (with no alphabetic characters). Is that okay?

One way you could do the algorithm that might not need the for loop would be do do something like this:

• Generate the main string as-is
• Generate a random number between 0 and len(string) - 1. Call it "position"
• Change the character at "position" to a randomly generated digit

That way you can always be sure that at least one character is a digit, but it's not predictable which character is the digit, or which digit is in that space.

And thank you for finding and fixing this issue!

[dbarranco]

I'm currently changing the approach I was following to make it even simpler.
I will ping you when this is finished.

Sorry for the delay!!

[dbarranco]

Work in progress ⬆️

[dbarranco]

Fixed.
Now, after executing some tests:

I was going to generate..  KUTBJpUvtQYOehH
Generated position:  7
Random string:  KUTBJpU1tQYOehH
Random string:  lOjKaWxkh5iCH4Y
I was going to generate..  APgKkRPWoqmCngo
Generated position:  9
Random string:  APgKkRPWo0mCngo
Random string:  EOoGaALAv9m2Ibg
Random string:  NHIeIIx0t07IGIA
Random string:  9FwpXMzdM5izLUK
I was going to generate..  lRWYnyqwRLYWHaO
Generated position:  5
Random string:  lRWYn6qwRLYWHaO
Random string:  NY3mEneAzPIJhsd
Random string:  iA6CwmAwRdObdhD
I was going to generate..  oebxhHRgGsGsoih
Generated position:  6
Random string:  oebxhH8gGsGsoih
Random string:  YUVeIxqIVsYAn9W
Random string:  Tt9GxBuEYjtcKvc
Random string:  TbiBjFbDV6x2R8G
Random string:  t06UU33ydhTHOB4
Random string:  1NWhvVTQqmAfZ6f

I think now it's better, thanks for the tip @technosophos.
Could you please take a look ? Thanks in advance

[mattfarina]

The only thing that catches my attention about this approach is the use of _ to avoid dealing with the error. The error is returned if the regex doesn't compile which shouldn't be an issue. But, gometalinter (and obviously one of the linters it uses) notes the issue.

I don't know that I mind this approach since I would be shocked if this regex didn't compile somewhere. This is more a note for myself if I ever get this setup working through a linter.

[technosophos]

Yeah, a panic-on-error is probably a good approach here. But I don't know that that would necessarily prevent us from merging.

@dbarranco would you mind capturing the error here and just doing something like:

if err != nil {
  panic(err)
}

That way if another dev (ahem, most likely me) comes along and breaks the regexp, we'll get an early warning.

[dbarranco]

Thanks for the fast review @mattfarina @technosophos !

Just added your feedback, ping me if you find suitable add more warnings near the panic.

Regards!

[technosophos]

LGTM. Anything else @mattfarina ? Or are we ready to merge?

[mattfarina]

@dbarranco @technosophos I released v1.0.1 with this change.

[dbarranco]

Thanks! @mattfarina @technosophos

:)