Bug: character '@' in registry username is unexpectedly banned.

[LemonPrefect]

When using a Aliyun RAM(Resource Access Management) account to config a private docker registry, a username like below will be given.

Reference for RAM account format: https://www.alibabacloud.com/help/zh/ram/support/faq-about-ram-users

<UserName>@<AccountAlias>.onaliyun.com

Hence, a @ is needed for the registry username. But in the source code of K8sService.cs, this character was banned for injection proof. As I guess, it was prepared for the code in line 306, which concat strings to serialize a json as below.

// lines starts at 305 of K8sService.cs
            var auth = Codec.Base64.Encode($"{registry.UserName}:{registry.Password}");
            var dockerjson = $"{{\"auths\":{{\"{registry.ServerAddress}\":{{\"auth\":\"{auth}\"," +
                $"\"username\":\"{registry.UserName}\",\"password\":\"{registry.Password}\"}}}}}}";
            var dockerjsonBytes = Encoding.ASCII.GetBytes(dockerjson);

And if it's the purpose of these code, maybe we can do better. Using System.Text.Json to serialize the json which will not result in the injection in the string formatting.

var dockerjson = new {
    auths = new {
        registry.ServerAddress = new {
            auth = auth,
            username = registry.UserName,
            password = registry.Password
        }
    }
};
string jsonString = JsonSerializer.Serialize(dockerjson);

FYI, the codes above should work here.

[GZTimeWalker]

@LemonPrefect Sure! Good solution!

[LemonPrefect]

UPDATE: There's a dynamic key in the json, so an extra dependency might has to be needed to serialize this object. The codes below should work in the sitaution. I choose Newtonsoft.Json here.

using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

      var authJObject = new JObject();
      authJObject[registry.ServerAddress] = JToken.FromObject(new{
          auth,
          username = registry.UserName,
          password = registry.Password
      });
      var dockerJObject = JObject.FromObject(new{
          auths = authJObject
      });
      string jsonString = dockerJObject.ToString(Formatting.None);

[GZTimeWalker]

fixed in https://github.com/GZTimeWalker/GZCTF/commit/49b912f38a3e9c43d6b375f89209d7be0f908907

[LemonPrefect]

LGTM, but the code generating AuthSecretName prevent the metadata.name from being a valid RFC 1123 string. Maybe we'll need a new form of AuthSecretName here.

https://github.com/GZTimeWalker/GZCTF/blob/49b912f38a3e9c43d6b375f89209d7be0f908907/src/GZCTF/Services/K8sService.cs#L51

When there is a '@' in the registry.UserName, it will be put into the AuthSecretName which shouldn't have a character rather than alphanumeric and .- by the demand of K8s which defines the secretName as a RFC 1123 domain string. Therefore, in order to prevent the malform of secretName, some changes has to be made.

After replacing the character '@' into - and casting all the string formatted into lower case here, I have temporarily managed to make it work. If the name has to be easy to recognize by human beings, how about just replace the non-RFC-1123-domain characters into - and cast every characters into lower cases?

FYI, a valid secretName should consist with the regular expression below.

[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

[GZTimeWalker]

@LemonPrefect Indeed, this should be a consideration. May we use ServerAddress for this name?

[LemonPrefect]

@LemonPrefect Indeed, this should be a consideration. May we use ServerAddress for this name?

ServerAddress may always be the same for some private registry service such as aliyun. It may solve this problem but will make the AuthSecretName ambigious for human being to tell the difference between two registry secrets. If it's not the point, I think the ServerAddress is a good choice.

[GZTimeWalker]

ServerAddress may always be the same for some private registry service such as aliyun. It may solve this problem but will make the AuthSecretName ambigious for human being to tell the difference between two registry secrets. If it's not the point, I think the ServerAddress is a good choice.

maybe...

public static string ToValidRFC1123String(this string str)
{
    var sb = new StringBuilder();
    foreach (var c in str)
    {
        sb.Append(char.IsLetterOrDigit(c) ? c.toLowerInvariant() : '-');
    }
    return sb.ToString();
}

or...

Regex.Replace(str, "[^a-zA-Z0-9]", "-").ToLowerInvariant()

[LemonPrefect]

ServerAddress may always be the same for some private registry service such as aliyun. It may solve this problem but will make the AuthSecretName ambigious for human being to tell the difference between two registry secrets. If it's not the point, I think the ServerAddress is a good choice.

maybe...

public static string ToValidRFC1123String(this string str)
{
    var sb = new StringBuilder();
    foreach (var c in str)
    {
        sb.Append(char.IsLetterOrDigit(c) ? c.toLowerInvariant() : '-');
    }
    return sb.ToString();
}

or...

Regex.Replace(str, "[^a-zA-Z0-9]", "-").ToLowerInvariant()

Maybe use the second one and ensure the string starts with alphabet letters? For example, add a word "secret" to simply prevent the string to start with numbers.

AuthSecretName = Regex.Replace($"secret-{registry.UserName}-{padding}", "[^a-z0-9]", "-").ToLowerInvariant();

[GZTimeWalker]

done in https://github.com/GZTimeWalker/GZCTF/commit/d6d6f096d7838b059ac9e57b213f9692281f5f20