Argo-cd: Better handling of base64 encoded passwords/credentials

[jannfis]

Summary

Argo CD should strip off any trailing CR and LF characters from passwords that are stored base64 encoded in Argo CD's configuration secrets.

Motivation

When Argo CD needs to store a base64 encoded somewhere (i.e. oAuth secrets, repository passwords, etc), users tend to base64 encode these passwords in the following way:

echo "somepassword" | base64

This will encode the trailing newline added by echo, and Argo CD will use the decoded string in a verbatim way, including any trailing CRLF characters, and pass them to the system requiring these credentials. This will obviously result in an authentication failure, because somepassword is different from `somepassword\n'.

Proposal

The best approach in my opinion would be to provide a new set of methods, i.e. DecodeBase64Credential(encoded string, verbatim bool) string, which should be used for decoding all base64 encoded credentials in the code base.

If verbatim is set to false, it will strip any trailing CRLF characters (or possibly, all white-space) from the decoded string before returning it. Otherwise, string will be returned as-is.

A contra argument would be, that user input should not be mutated, as the Kubernetes people decided to do (refer https://github.com/kubernetes/kubernetes/issues/23404). While this approach is very valid for Kubernetes, since they don't know how the data will be used, in Argo CD we have a lot more of contextual knowledge about where and how data from our secrets will be used.

[jsoref]

assuming there's a log, for initial setup, it's probably worth logging if the system encounters and strips these characters. -- This isn't for validating later, just the first time a given credential is used (assuming that's a thing).

[gaganhegde]

Hi @jannfis , had a query? is the boolean verbatim field expected in the argocd cm?

[jannfis]

Hey @gaganhegde, having this externally configurable is not required. It should just be an additional option for the programmatic interface for now, so that we can reuse it throughout the code base to decode base64 credentials.

[jannfis]

assuming there's a log, for initial setup, it's probably worth logging if the system encounters and strips these characters. -- This isn't for validating later, just the first time a given credential is used (assuming that's a thing).

I'm not sure whether it's viable to figure out whether it's the first the credential is used/decoded. But I agree printing a warning is a good thing, so the user will know and has the possibility to fix this small mistake. We could issue the warning when the credential is decoded, and needs to be trimmed from trailing CRLFs.

[gaganhegde]

Hey @jannfis , I did look into the issue and have some queries.

1. Under what circumstances do we get base64 encoded values? Do we assume that the user manually modifies the secret.yaml to enter a possible value with an erroneous trailing CRLFs in the data field. An example will be great.

2. I think the fix would be a modification to https://github.com/argoproj/argo-cd/blob/be718e2b6186294e2a84d7cd4789f096e44fc545/util/db/db.go#L112 , with a simple Trim of trailing CRLFs to the string received.

[gaganhegde]

Hey @jannfis , did you get a chance to look at the previous suggestions?

[jannfis]

Sorry for the delay, @gaganhegde. You are right, when we use K8s client API to get value from secrets, they'll already be decoded.

I think the code you pointed to is not the only place where we retrieve data from secrets, it's just a convenient way to unmarshal several values from several secrets into Go strings. Maybe it would make sense to modify the type of its input parameter to have it contain a toggle whether to strip trailing newline from it, with a default to false.

But then, we should also find the other places, i.e. where we pass credentials (client secret) to Dex.

[gaganhegde]

Thanks @jannfis, Maybe it would make sense to modify the type of its input parameter to have it contain a toggle whether to strip trailing newline from it, with a default to false., had a query regarding how this toggle default would be changed to true as in where would this input come from?

[jannfis]

@gaganhegde I'm always being a little over-complicated in my writings, pseudo-code is sometimes better :)

So currently, the unmarshalFromSecretsStr method accepts a map[*string]*v1.SecretKeySelector as parameter to define what value to map into *dst. I was thinking of a new struct type, e.g. something like the following (disclaimer: untested, just hacked down here):

type SecretMapperTransformFun func(inp string) string
type SecretMapperValue struct {
   Dest *string
   Transform SecretMapperTransformFun
}

We could define a transformation function like this in the consumer packages:

func stripNewlines(inp string) string {
  return strings.TrimRight(inp, "\r\n")
}

and then modify consumers of this method from

        err := db.unmarshalFromSecretsStr(map[*string]*apiv1.SecretKeySelector{
                &repo.Username:          repoInfo.UsernameSecret,
                &repo.Password:          repoInfo.PasswordSecret,
                &repo.SSHPrivateKey:     repoInfo.SSHPrivateKeySecret,
                &repo.TLSClientCertData: repoInfo.TLSClientCertDataSecret,
                &repo.TLSClientCertKey:  repoInfo.TLSClientCertKeySecret,
        }, make(map[string]*apiv1.Secret))

to something like

        err := db.unmarshalFromSecretsStr(map[*string]*apiv1.SecretKeySelector{
                SecretMapperValue{Dest: &repo.Username}:          repoInfo.UsernameSecret,
                SecretMapperValue{Dest: &repo.Password, Transform: stripNewlines}:          repoInfo.PasswordSecret,
                SecretMapperValue{Dest: &repo.SSHPrivateKey}:     repoInfo.SSHPrivateKeySecret,
                SecretMapperValue{Dest: &repo.TLSClientCertData}: repoInfo.TLSClientCertDataSecret,
                SecretMapperValue{Dest: &repo.TLSClientCertKey}:  repoInfo.TLSClientCertKeySecret,
        }, make(map[string]*apiv1.Secret))

finally, we need to adapt unmarshalFromSecretsStr at where you pointed to from

*dst = string(secret.Data[src.Key])

to

if dst.Transform != nil {
  *dst.Dest = dst.Transform(string(secret.Data[src.Key]))
} else {
  *dst.Dest = string(secret.Data[src.Key])
}

This might feel a little over engineered, but gives quite some flexibility. Not sure if it will work either. What do you think?

[gaganhegde]

Got it @jannfis , really thankful for the psuedo-code. This particular location trims the secret strings for repositories/helm-repos. Another use case you mentioned was dex config secrets, can you please point me out if there are any secrets besides these that will require to implement this interface (i.e SecretMapperValue)? thank you

[jannfis]

Sorry @gaganhegde for not coming back to you earlier.

There are a few other places - mainly Dex and OIDC related client secrets - that would benefit from trimming trailing newlines. But I think those would need a different, much simpler approach. Please refer to this code: https://github.com/argoproj/argo-cd/blob/b7b74582cbfa79f1bf090e50b46460975413b943/util/settings/settings.go#L1287

[gaganhegde]

Hi @jannfis , thanks for all the info. There is a PR raised wrt issue 4848. Kindly take a look when you find the time and please let me know if any changes are required.

[jannfis]

Awesome @gaganhegde, just seen your PR! :+1:

I will review it the next couple of days, possibly over the weekend,

[gaganhegde]

Thank you @jannfis .