-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an validator to verify correctness of ProxyUrl string/secret #706
Conversation
ce71ddc
to
fd75261
Compare
src/agproxysecret/secret.go
Outdated
@@ -129,7 +129,7 @@ func (agProxySecretGenerator *ActiveGateProxySecretGenerator) proxyUrlFromUserSe | |||
func parseProxyUrl(proxy string) (host string, port string, username string, password string, err error) { | |||
proxyUrl, err := url.Parse(proxy) | |||
if err != nil { | |||
return "", "", "", "", err | |||
return "", "", "", "", fmt.Errorf("could not parse proxy URL") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use errors.New()
from the pkg/errors
library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/webhook/validation/activegate.go
Outdated
if k8serrors.IsNotFound(err) { | ||
return errorMissingActiveGateProxySecret | ||
} else if err != nil { | ||
return fmt.Sprintf("error occurred while reading PROXY secret indicated in the Dynakube specification (%s)", err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use errors.Wrap()
from the pkg/errors
libraries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/webhook/validation/activegate.go
Outdated
// proxyUrl is valid if | ||
// 1) encoded | ||
// 2) "`\ are escaped using \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does that mean it must be either escaped or encoded or must it be both?
If it must be both, would that then work with the dtclient, since it would take the password unencoded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my fault, characters can not be escaped
src/webhook/validation/activegate.go
Outdated
// 'eval' command is used by entrypoint.sh:readSecret function to return its result. | ||
// For this reason quotation mark ", backtick ` and backslash \ characters have to be escaped using backslash. | ||
func isEvalEscapeNeeded(str string) bool { | ||
previousChar := '\000' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
introduce a local constant for the null character just so it is more obvious that that magic number is supposed to be that character
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Characters can not be escaped due to dtclient (which uses the same password) so the logic is simpler - apostrophe and backtick are forbidden. The function has been renamed to evalIsStringInvalid
src/webhook/validation/activegate.go
Outdated
|
||
// 'eval' command is used by entrypoint.sh:readSecret function to return its result. | ||
// For this reason quotation mark ", backtick ` and backslash \ characters have to be escaped using backslash. | ||
func isEvalEscapeNeeded(str string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I understand, this function would effectively be a block-list, blocking the three known characters. It would be more secure to restructure it to an allow-list, checking that every character is one of the known ones, i.e., letters, numbers and tested special characters and then checking if they are appropriately escaped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my fault, characters can not be escaped because of dtclient. Finally, there are two invalid characters : apostrophe and backtick.
src/webhook/validation/activegate.go
Outdated
errorInvalidActiveGateProxyUrl = `The DynaKube's specification has an invalid Proxy URL value set. Make sure you correctly specify the URL in your custom resource.` | ||
errorInvalidEvalCharacter = `The DynaKube's specification has an invalid Proxy password value set. Make sure you correctly escape quotation mark, backtick and backslash characters using backslash.` | ||
|
||
errorMissingActiveGateProxySecret = `The Proxy secret indicated by the DynaKube specification doesn't exist.` | ||
|
||
errorInvalidProxySecretFormat = `The Proxy secret indicated by the DynaKube specification has an invalid format. Make sure you correctly creates the secret.` | ||
|
||
errorInvalidProxySecretUrl = `The Proxy secret indicated by the DynaKube specification has an invalid URL value set. Make sure you correctly specify the URL in the secret.` | ||
errorInvalidProxySecretEvalCharacter = `The Proxy secret indicated by the DynaKube specification has an invalid Proxy password value set. Make sure you correctly escape quotation mark, backtick and backslash characters using backslash.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With so many specifc errors (and also logic), this code and other proxy validation code should probably belong into a separate file, e.g., "validation/proxy.go"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
6361b3a
to
dff074c
Compare
src/webhook/validation/proxy_url.go
Outdated
// Finally, these 2 characters are forbidden. | ||
func evalIsStringInvalid(str string) bool { | ||
for _, char := range str { | ||
if char == '\'' || char == '`' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implements a blocklist again, please rewrite that function to return true anytime it encounters an unkown character
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this would implement an allowlist, only permitting the characters in the brackets to be used
func isStringValidForEval(str string) bool {
regex := regexp.MustCompile(`^[a-zA-Z0-9./?,*;\\ ]+$`)
return regex.MatchString(str)
}
func TestAllowList(t *testing.T) {
assert.True(t, isStringValidForEval("a simple password"))
assert.True(t, isStringValidForEval("a simple password w1th sp?cial ;; chara/cters"))
assert.False(t, isStringValidForEval("A ` backtick"))
assert.False(t, isStringValidForEval("A ' single quote"))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regex is used
(including user:password) so has to be replaced by less verbose message
c0aa044
to
96a2524
Compare
96a2524
to
b6944fa
Compare
assert.Equal(t, true, isStringValidForAG("password")) | ||
assert.Equal(t, true, isStringValidForAG("test~!@#^*()_-|}{[]\":;?><./pass")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I didn't see that during the first review. There is assert.True
and assert.False
for these kinds of assertions. Otherweise lgmt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, fixed
9413cd5
to
38b90fe
Compare
38b90fe
to
532208b
Compare
Description
ActiveGate Proxy URL has to be properly encoded URI. ActiveGate POD fails if Proxy URL contains special characters (for instance "?) unencoded. To improve user experience I propose to add an Validator to reject a CR if the Proxy value or Proxy secret is invalid.
How can this be tested?
Use any unencoded special character(s) in the Proxy Url (proxy.value, proxy secret) and try to apply your CR - should be rejected.
Checklist