Skip to content

Commit 230cdc4

Browse files
committed
fix: support ssh auth for git-based skills
- add openssh-client to the skills-init image - extract gitSSHHost to derive SSH hosts from skill gitRefs URLs (ssh://, git+ssh://, and scp-style), with strict hostname/port validation to prevent command injection - normalize default port 22 to avoid redundant ssh-keyscan -p flag - gate SSH host collection behind authSecretRef != nil - add ssh-keyscan failure warning to stderr in init script - add unit, translator, and golden test coverage Fixes #1523 Signed-off-by: Jaison Paul <paul.jaison@gmail.com>
1 parent 8f1971e commit 230cdc4

6 files changed

Lines changed: 234 additions & 68 deletions

File tree

docker/skills-init/Dockerfile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,5 +14,5 @@ RUN CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -o /build/krane .
1414

1515
FROM alpine:3.21
1616

17-
RUN apk add --no-cache git
17+
RUN apk add --no-cache git openssh-client
1818
COPY --from=krane-builder /build/krane /usr/local/bin/krane

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

Lines changed: 64 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1568,6 +1568,55 @@ func gitSkillName(ref v1alpha2.GitRepo) string {
15681568
return path.Base(u)
15691569
}
15701570

1571+
var (
1572+
scpLikeGitURLRegex = regexp.MustCompile(`^(?:[^@/]+@)?([^:/]+):.+$`)
1573+
// validHostPattern rejects shell metacharacters in hostnames and ports to prevent
1574+
// command injection when values are interpolated into ssh-keyscan shell commands.
1575+
validHostPattern = regexp.MustCompile(`^[A-Za-z0-9.\-]+$`)
1576+
validPortPattern = regexp.MustCompile(`^[0-9]+$`)
1577+
)
1578+
1579+
func gitSSHHost(rawURL string) (sshHostData, bool) {
1580+
parsed, err := url.Parse(rawURL)
1581+
if err == nil {
1582+
switch parsed.Scheme {
1583+
case "ssh", "git+ssh":
1584+
host := parsed.Hostname()
1585+
if host == "" || !validHostPattern.MatchString(host) {
1586+
return sshHostData{}, false
1587+
}
1588+
port := parsed.Port()
1589+
if port == "22" {
1590+
port = "" // 22 is the SSH default; omit to avoid redundant -p flag
1591+
}
1592+
if port != "" && !validPortPattern.MatchString(port) {
1593+
return sshHostData{}, false
1594+
}
1595+
return sshHostData{
1596+
Host: host,
1597+
Port: port,
1598+
}, true
1599+
case "http", "https":
1600+
return sshHostData{}, false
1601+
}
1602+
}
1603+
1604+
if strings.Contains(rawURL, "://") {
1605+
return sshHostData{}, false
1606+
}
1607+
1608+
matches := scpLikeGitURLRegex.FindStringSubmatch(rawURL)
1609+
if len(matches) != 2 {
1610+
return sshHostData{}, false
1611+
}
1612+
host := matches[1]
1613+
if !validHostPattern.MatchString(host) {
1614+
return sshHostData{}, false
1615+
}
1616+
1617+
return sshHostData{Host: host}, true
1618+
}
1619+
15711620
// validateSubPath rejects subPath values that are absolute or contain ".." traversal segments.
15721621
func validateSubPath(p string) error {
15731622
if p == "" {
@@ -1658,35 +1707,10 @@ func prepareSkillsInitData(
16581707

16591708
if authSecretRef != nil {
16601709
data.AuthMountPath = "/git-auth"
1661-
seenHosts := make(map[string]bool)
1662-
hostPattern := regexp.MustCompile(`^[A-Za-z0-9\.\-:]+$`)
1663-
portPattern := regexp.MustCompile(`^[0-9]+$`)
1664-
for _, ref := range gitRefs {
1665-
u, err := url.Parse(ref.URL)
1666-
if err != nil || u.Scheme != "ssh" {
1667-
continue
1668-
}
1669-
host := u.Hostname()
1670-
if host == "" || !hostPattern.MatchString(host) {
1671-
continue
1672-
}
1673-
port := u.Port()
1674-
if port == "22" {
1675-
port = "" // 22 is the SSH default; omit to avoid -p flag
1676-
}
1677-
if port != "" && !portPattern.MatchString(port) {
1678-
continue
1679-
}
1680-
key := host + ":" + port
1681-
if seenHosts[key] {
1682-
continue
1683-
}
1684-
seenHosts[key] = true
1685-
data.SSHHosts = append(data.SSHHosts, sshHostData{Host: host, Port: port})
1686-
}
16871710
}
16881711

16891712
seen := make(map[string]bool)
1713+
seenSSHHosts := make(map[sshHostData]bool)
16901714

16911715
for _, ref := range gitRefs {
16921716
subPath := strings.TrimSuffix(ref.Path, "/")
@@ -1706,6 +1730,13 @@ func prepareSkillsInitData(
17061730
}
17071731
seen[name] = true
17081732

1733+
if authSecretRef != nil {
1734+
if sshHost, ok := gitSSHHost(ref.URL); ok && !seenSSHHosts[sshHost] {
1735+
seenSSHHosts[sshHost] = true
1736+
data.SSHHosts = append(data.SSHHosts, sshHost)
1737+
}
1738+
}
1739+
17091740
data.GitRefs = append(data.GitRefs, gitRefData{
17101741
URL: ref.URL,
17111742
Ref: gitRef,
@@ -1728,6 +1759,13 @@ func prepareSkillsInitData(
17281759
})
17291760
}
17301761

1762+
slices.SortFunc(data.SSHHosts, func(a, b sshHostData) int {
1763+
if cmp := strings.Compare(a.Host, b.Host); cmp != 0 {
1764+
return cmp
1765+
}
1766+
return strings.Compare(a.Port, b.Port)
1767+
})
1768+
17311769
return data, nil
17321770
}
17331771

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"
@@ -45,14 +44,15 @@ func Test_AdkApiTranslator_Skills(t *testing.T) {
4544
name string
4645
agent *v1alpha2.Agent
4746
// assertions
48-
wantSkillsInit bool
49-
wantSkillsVolume bool
50-
wantContainsBranch string
51-
wantContainsCommit string
52-
wantContainsPath string
53-
wantContainsKrane bool
54-
wantAuthVolume bool
55-
wantSSHKeyscanHosts []string // substrings expected in the ssh-keyscan lines
47+
wantSkillsInit bool
48+
wantSkillsVolume bool
49+
wantContainsBranch string
50+
wantContainsCommit string
51+
wantContainsPath string
52+
wantContainsKrane bool
53+
wantAuthVolume bool
54+
wantAuthSecretName string
55+
wantScriptContains []string
5656
}{
5757
{
5858
name: "no skills - no init containers",
@@ -213,14 +213,18 @@ func Test_AdkApiTranslator_Skills(t *testing.T) {
213213
},
214214
},
215215
},
216-
wantSkillsInit: true,
217-
wantSkillsVolume: true,
218-
wantAuthVolume: true,
216+
wantSkillsInit: true,
217+
wantSkillsVolume: true,
218+
wantAuthVolume: true,
219+
wantAuthSecretName: "github-token",
220+
wantScriptContains: []string{
221+
"credential.helper",
222+
},
219223
},
220224
{
221225
name: "git skills with SSH URL and auth secret scans custom host",
222226
agent: &v1alpha2.Agent{
223-
ObjectMeta: metav1.ObjectMeta{Name: "agent-ssh", Namespace: namespace},
227+
ObjectMeta: metav1.ObjectMeta{Name: "agent-ssh-auth", Namespace: namespace},
224228
Spec: v1alpha2.AgentSpec{
225229
Type: v1alpha2.AgentType_Declarative,
226230
Declarative: &v1alpha2.DeclarativeAgentSpec{
@@ -233,17 +237,22 @@ func Test_AdkApiTranslator_Skills(t *testing.T) {
233237
},
234238
GitRefs: []v1alpha2.GitRepo{
235239
{
236-
URL: "ssh://git@gitea-ssh.gitea:22/gitops/ssh-skills-repo.git",
237-
Ref: "main",
240+
URL: "ssh://git@gitea-ssh.gitea:22/gitops/ssh-skills-repo.git",
241+
Ref: "main",
242+
Name: "ssh-skill",
238243
},
239244
},
240245
},
241246
},
242247
},
243-
wantSkillsInit: true,
244-
wantSkillsVolume: true,
245-
wantAuthVolume: true,
246-
wantSSHKeyscanHosts: []string{"gitea-ssh.gitea"},
248+
wantSkillsInit: true,
249+
wantSkillsVolume: true,
250+
wantAuthVolume: true,
251+
wantAuthSecretName: "gitea-ssh-credentials",
252+
wantScriptContains: []string{
253+
"ssh-keyscan",
254+
"gitea-ssh.gitea",
255+
},
247256
},
248257
{
249258
name: "git skill with custom name",
@@ -353,6 +362,10 @@ func Test_AdkApiTranslator_Skills(t *testing.T) {
353362
assert.Contains(t, script, "krane export")
354363
}
355364

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

421420
// 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 2>/dev/null || echo "WARNING: ssh-keyscan failed for {{ .Host }}:{{ .Port }}" >&2
17+
{{- else }}
18+
ssh-keyscan -H "{{ .Host }}" >> ~/.ssh/known_hosts 2>/dev/null || echo "WARNING: ssh-keyscan failed for {{ .Host }}" >&2
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)