Skip to content

Commit 213f53d

Browse files
jsonmp-k8EItanya
jsonmp-k8
and
EItanya
authored
fix: support ssh auth for git-based skills (#1529)
## Summary - add `openssh-client` to the `skills-init` image so SSH host key discovery is available - derive SSH hosts from skill `gitRefs` and populate `known_hosts`, including custom `ssh://` ports - add translator, unit, and golden coverage for SSH-based git skill auth ### Behavioral change: dynamic host scanning replaces hardcoded defaults The old template unconditionally ran `ssh-keyscan github.com gitlab.com bitbucket.org`. This PR replaces that with dynamic host extraction from `gitRefs` URLs (both `ssh://` and scp-style like `git@github.com:...`). This is more precise — it avoids scanning unused hosts and correctly handles self-hosted instances with custom ports. The common case (github.com, gitlab.com, bitbucket.org via scp-style URLs) is still covered by the `gitSSHHost` parser. ## Testing - `go test ./core/internal/controller/translator/agent` - `CGO_ENABLED=0 make lint` Fixes #1523 --------- Signed-off-by: Jaison Paul <paul.jaison@gmail.com> Signed-off-by: Jaison Paul <46974812+jsonmp-k8@users.noreply.github.com> Co-authored-by: Eitan Yarmush <eitan.yarmush@solo.io>
1 parent 37b32d6 commit 213f53d

5 files changed

Lines changed: 241 additions & 67 deletions

File tree

‎go/core/internal/controller/translator/agent/adk_api_translator.go‎

Lines changed: 72 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1036,6 +1036,58 @@ func gitSkillName(ref v1alpha2.GitRepo) string {
10361036
return path.Base(u)
10371037
}
10381038

1039+
var (
1040+
scpLikeGitURLRegex = regexp.MustCompile(`^(?:[^@/]+@)?([^:/]+):.+$`)
1041+
1042+
// validHostPattern and validPortPattern are the security boundary that prevents
1043+
// shell injection when host/port values are interpolated into the ssh-keyscan
1044+
// commands in skills-init.sh.tmpl. Do NOT relax these patterns without auditing
1045+
// every template site that references .Host or .Port.
1046+
validHostPattern = regexp.MustCompile(`^[A-Za-z0-9.\-]+$`)
1047+
validPortPattern = regexp.MustCompile(`^[0-9]+$`)
1048+
)
1049+
1050+
func gitSSHHost(rawURL string) (sshHostData, bool) {
1051+
parsed, err := url.Parse(rawURL)
1052+
if err == nil {
1053+
switch parsed.Scheme {
1054+
case "ssh", "git+ssh":
1055+
host := parsed.Hostname()
1056+
if host == "" || !validHostPattern.MatchString(host) {
1057+
return sshHostData{}, false
1058+
}
1059+
port := parsed.Port()
1060+
if port == "22" {
1061+
port = "" // 22 is the SSH default; omit to avoid redundant -p flag
1062+
}
1063+
if port != "" && !validPortPattern.MatchString(port) {
1064+
return sshHostData{}, false
1065+
}
1066+
return sshHostData{
1067+
Host: host,
1068+
Port: port,
1069+
}, true
1070+
case "http", "https":
1071+
return sshHostData{}, false
1072+
}
1073+
}
1074+
1075+
if strings.Contains(rawURL, "://") {
1076+
return sshHostData{}, false
1077+
}
1078+
1079+
matches := scpLikeGitURLRegex.FindStringSubmatch(rawURL)
1080+
if len(matches) != 2 {
1081+
return sshHostData{}, false
1082+
}
1083+
host := matches[1]
1084+
if !validHostPattern.MatchString(host) {
1085+
return sshHostData{}, false
1086+
}
1087+
1088+
return sshHostData{Host: host}, true
1089+
}
1090+
10391091
// validateSubPath rejects subPath values that are absolute or contain ".." traversal segments.
10401092
func validateSubPath(p string) error {
10411093
if p == "" {
@@ -1126,35 +1178,10 @@ func prepareSkillsInitData(
11261178

11271179
if authSecretRef != nil {
11281180
data.AuthMountPath = "/git-auth"
1129-
seenHosts := make(map[string]bool)
1130-
hostPattern := regexp.MustCompile(`^[A-Za-z0-9\.\-:]+$`)
1131-
portPattern := regexp.MustCompile(`^[0-9]+$`)
1132-
for _, ref := range gitRefs {
1133-
u, err := url.Parse(ref.URL)
1134-
if err != nil || u.Scheme != "ssh" {
1135-
continue
1136-
}
1137-
host := u.Hostname()
1138-
if host == "" || !hostPattern.MatchString(host) {
1139-
continue
1140-
}
1141-
port := u.Port()
1142-
if port == "22" {
1143-
port = "" // 22 is the SSH default; omit to avoid -p flag
1144-
}
1145-
if port != "" && !portPattern.MatchString(port) {
1146-
continue
1147-
}
1148-
key := host + ":" + port
1149-
if seenHosts[key] {
1150-
continue
1151-
}
1152-
seenHosts[key] = true
1153-
data.SSHHosts = append(data.SSHHosts, sshHostData{Host: host, Port: port})
1154-
}
11551181
}
11561182

11571183
seen := make(map[string]bool)
1184+
seenSSHHosts := make(map[string]bool)
11581185

11591186
for _, ref := range gitRefs {
11601187
subPath := strings.TrimSuffix(ref.Path, "/")
@@ -1174,6 +1201,18 @@ func prepareSkillsInitData(
11741201
}
11751202
seen[name] = true
11761203

1204+
// SSH host collection is separate from the AuthMountPath block above
1205+
// because it runs per-ref inside the loop, not once at the top level.
1206+
if authSecretRef != nil {
1207+
if sshHost, ok := gitSSHHost(ref.URL); ok {
1208+
key := sshHost.Host + ":" + sshHost.Port
1209+
if !seenSSHHosts[key] {
1210+
seenSSHHosts[key] = true
1211+
data.SSHHosts = append(data.SSHHosts, sshHost)
1212+
}
1213+
}
1214+
}
1215+
11771216
data.GitRefs = append(data.GitRefs, gitRefData{
11781217
URL: ref.URL,
11791218
Ref: gitRef,
@@ -1196,6 +1235,13 @@ func prepareSkillsInitData(
11961235
})
11971236
}
11981237

1238+
slices.SortFunc(data.SSHHosts, func(a, b sshHostData) int {
1239+
if cmp := strings.Compare(a.Host, b.Host); cmp != 0 {
1240+
return cmp
1241+
}
1242+
return strings.Compare(a.Port, b.Port)
1243+
})
1244+
11991245
return data, nil
12001246
}
12011247

‎go/core/internal/controller/translator/agent/git_skills_test.go‎

Lines changed: 33 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package agent_test
22

33
import (
44
"context"
5-
"fmt"
65
"testing"
76

87
"github.com/stretchr/testify/assert"
@@ -46,14 +45,15 @@ func Test_AdkApiTranslator_Skills(t *testing.T) {
4645
name string
4746
agent *v1alpha2.Agent
4847
// assertions
49-
wantSkillsInit bool
50-
wantSkillsVolume bool
51-
wantContainsBranch string
52-
wantContainsCommit string
53-
wantContainsPath string
54-
wantContainsKrane bool
55-
wantAuthVolume bool
56-
wantSSHKeyscanHosts []string // substrings expected in the ssh-keyscan lines
48+
wantSkillsInit bool
49+
wantSkillsVolume bool
50+
wantContainsBranch string
51+
wantContainsCommit string
52+
wantContainsPath string
53+
wantContainsKrane bool
54+
wantAuthVolume bool
55+
wantAuthSecretName string
56+
wantScriptContains []string
5757
}{
5858
{
5959
name: "no skills - no init containers",
@@ -214,14 +214,18 @@ func Test_AdkApiTranslator_Skills(t *testing.T) {
214214
},
215215
},
216216
},
217-
wantSkillsInit: true,
218-
wantSkillsVolume: true,
219-
wantAuthVolume: true,
217+
wantSkillsInit: true,
218+
wantSkillsVolume: true,
219+
wantAuthVolume: true,
220+
wantAuthSecretName: "github-token",
221+
wantScriptContains: []string{
222+
"credential.helper",
223+
},
220224
},
221225
{
222226
name: "git skills with SSH URL and auth secret scans custom host",
223227
agent: &v1alpha2.Agent{
224-
ObjectMeta: metav1.ObjectMeta{Name: "agent-ssh", Namespace: namespace},
228+
ObjectMeta: metav1.ObjectMeta{Name: "agent-ssh-auth", Namespace: namespace},
225229
Spec: v1alpha2.AgentSpec{
226230
Type: v1alpha2.AgentType_Declarative,
227231
Declarative: &v1alpha2.DeclarativeAgentSpec{
@@ -234,17 +238,22 @@ func Test_AdkApiTranslator_Skills(t *testing.T) {
234238
},
235239
GitRefs: []v1alpha2.GitRepo{
236240
{
237-
URL: "ssh://git@gitea-ssh.gitea:22/gitops/ssh-skills-repo.git",
238-
Ref: "main",
241+
URL: "ssh://git@gitea-ssh.gitea:22/gitops/ssh-skills-repo.git",
242+
Ref: "main",
243+
Name: "ssh-skill",
239244
},
240245
},
241246
},
242247
},
243248
},
244-
wantSkillsInit: true,
245-
wantSkillsVolume: true,
246-
wantAuthVolume: true,
247-
wantSSHKeyscanHosts: []string{"gitea-ssh.gitea"},
249+
wantSkillsInit: true,
250+
wantSkillsVolume: true,
251+
wantAuthVolume: true,
252+
wantAuthSecretName: "gitea-ssh-credentials",
253+
wantScriptContains: []string{
254+
"ssh-keyscan",
255+
"gitea-ssh.gitea",
256+
},
248257
},
249258
{
250259
name: "git skill with custom name",
@@ -354,6 +363,10 @@ func Test_AdkApiTranslator_Skills(t *testing.T) {
354363
assert.Contains(t, script, "krane export")
355364
}
356365

366+
for _, want := range tt.wantScriptContains {
367+
assert.Contains(t, script, want)
368+
}
369+
357370
// Verify /skills volume mount exists
358371
hasSkillsMount := false
359372
for _, vm := range skillsInitContainer.VolumeMounts {
@@ -389,7 +402,7 @@ func Test_AdkApiTranslator_Skills(t *testing.T) {
389402
for _, v := range deployment.Spec.Template.Spec.Volumes {
390403
if v.Secret != nil && v.Name == "git-auth" {
391404
hasAuthVolume = true
392-
assert.Equal(t, tt.agent.Spec.Skills.GitAuthSecretRef.Name, v.Secret.SecretName, "auth volume should reference the correct secret")
405+
assert.Equal(t, tt.wantAuthSecretName, v.Secret.SecretName, "auth volume should reference the correct secret")
393406
}
394407
}
395408
assert.True(t, hasAuthVolume, "git-auth volume should exist")
@@ -403,20 +416,6 @@ func Test_AdkApiTranslator_Skills(t *testing.T) {
403416
}
404417
}
405418
assert.True(t, hasAuthMount, "skills-init container should mount auth secret")
406-
407-
// Verify script contains credential helper setup
408-
script := skillsInitContainer.Command[2]
409-
assert.Contains(t, script, "credential.helper")
410-
}
411-
412-
// Verify custom SSH hosts are scanned
413-
if len(tt.wantSSHKeyscanHosts) > 0 {
414-
require.NotNil(t, skillsInitContainer)
415-
script := skillsInitContainer.Command[2]
416-
for _, host := range tt.wantSSHKeyscanHosts {
417-
expected := fmt.Sprintf("ssh-keyscan %s", host)
418-
assert.Contains(t, script, expected, "script should ssh-keyscan custom host %q", host)
419-
}
420419
}
421420

422421
// Verify insecure flag for OCI skills

‎go/core/internal/controller/translator/agent/skills-init.sh.tmpl‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,17 @@ ENDVAL
66
)"
77
if [ -f "${_auth_mount}/ssh-privatekey" ]; then
88
mkdir -p ~/.ssh
9+
chmod 700 ~/.ssh
910
cp "${_auth_mount}/ssh-privatekey" ~/.ssh/id_rsa
1011
chmod 600 ~/.ssh/id_rsa
11-
ssh-keyscan github.com gitlab.com bitbucket.org >> ~/.ssh/known_hosts
12+
touch ~/.ssh/known_hosts
13+
chmod 600 ~/.ssh/known_hosts
1214
{{- range .SSHHosts }}
13-
{{- if .Port }}
14-
ssh-keyscan -p {{ .Port }} {{ .Host }} >> ~/.ssh/known_hosts
15-
{{- else }}
16-
ssh-keyscan {{ .Host }} >> ~/.ssh/known_hosts
17-
{{- end }}
15+
{{- if .Port }}
16+
ssh-keyscan -H -p "{{ .Port }}" "{{ .Host }}" >> ~/.ssh/known_hosts
17+
{{- else }}
18+
ssh-keyscan -H "{{ .Host }}" >> ~/.ssh/known_hosts
19+
{{- end }}
1820
{{- end }}
1921
elif [ -f "${_auth_mount}/token" ]; then
2022
git config --global credential.helper "!f() { echo username=x-access-token; echo password=\$(cat ${_auth_mount}/token); }; f"

0 commit comments

Comments
 (0)