internal/coordinator/remote: check for ownerID in principals of cert
This change checks that the owner ID which has been set in the
certificates principals correspond with the session being
authenticated. The certificate contains the session ID and
owner ID in the principals section. The session ID is checked
before the owner ID is checked.
For golang/go#52594
Change-Id: I5cedde248e01cbec22bf1c4c77aabf29a1edb2a7
Reviewed-on: https://go-review.googlesource.com/c/build/+/415679
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Carlos Amedee <carlos@golang.org>
Auto-Submit: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/internal/coordinator/remote/ssh.go b/internal/coordinator/remote/ssh.go
index 10e1405..702071d 100644
--- a/internal/coordinator/remote/ssh.go
+++ b/internal/coordinator/remote/ssh.go
@@ -532,7 +532,7 @@
return false
}
- _, err := sp.Session(sessionID)
+ ses, err := sp.Session(sessionID)
if err != nil {
log.Printf("HandleCertificateAuth: unable to retrieve session=%s: %s", sessionID, err)
return false
@@ -543,7 +543,13 @@
log.Printf("certChecker.CheckCert(%s, user_certificate) = %s", wantPrincipal, err)
return false
}
- return true
+ for _, principal := range cert.ValidPrincipals {
+ if principal == ses.OwnerID {
+ return true
+ }
+ }
+ log.Printf("HandleCertificateAuth: unable to verify ownerID in certificate principals")
+ return false
}
}
diff --git a/internal/coordinator/remote/ssh_test.go b/internal/coordinator/remote/ssh_test.go
index 45d89fb..fb4f610 100644
--- a/internal/coordinator/remote/ssh_test.go
+++ b/internal/coordinator/remote/ssh_test.go
@@ -224,6 +224,43 @@
}
})
+ t.Run("wrong owner", func(t *testing.T) {
+ ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer cancel()
+
+ addr, sp, s := setupSSHServer(t, ctx)
+ defer s.Close()
+
+ ownerID := "accounts.google.com:userIDvalue"
+ sessionID := sp.AddSession(ownerID, "maria", "linux-amd64", "xyz", &buildlet.FakeClient{})
+ certSigner := parsePrivateKey(t, []byte(devCertCAPrivate))
+ clientPubKey, err := SignPublicSSHKey(ctx, certSigner, []byte(devCertClientPublic), sessionID, ownerID+"WRONG", time.Minute)
+ if err != nil {
+ t.Fatalf("SignPublicSSHKey(...) = _, %s; want no error", err)
+ }
+ pubKey, _, _, _, err := ssh.ParseAuthorizedKey(clientPubKey)
+ if err != nil {
+ t.Fatalf("ParsePublicKey(...) = _, %s; want no error", err)
+ }
+ cert := pubKey.(*ssh.Certificate)
+ clientCertSigner := parsePrivateKey(t, []byte(devCertClientPrivate))
+ clientSigner, err := ssh.NewCertSigner(cert, clientCertSigner)
+ if err != nil {
+ t.Fatalf("NewCertSigner(...) = _, %s; want no error", err)
+ }
+ clientConfig := &ssh.ClientConfig{
+ User: sessionID,
+ Auth: []ssh.AuthMethod{
+ ssh.PublicKeys(clientSigner),
+ },
+ HostKeyCallback: ssh.InsecureIgnoreHostKey(),
+ Timeout: 5 * time.Second,
+ }
+ _, err = ssh.Dial("tcp", addr, clientConfig)
+ if err == nil {
+ t.Fatal("Dial(...) = _, nil; want error")
+ }
+ })
}
func setupSSHServer(t *testing.T, ctx context.Context) (addr string, sp *SessionPool, s *SSHServer) {
