fix: never sign client certificate requests in trustd

Talos worker nodes use `trustd` API on control plane nodes to issue
certificates for `apid` service. Access to the API is protected with the
Talos join token specified in the machine configuration.

There was no validation on what kind of request is requested, so
`trustd` could issue a certificate which is valid for client
authentication with any set of Talos API RBAC roles, including
`os:admin` role allowing full access to the Talos API on control plane
nodes.

See: GHSA-7hgc-php5-77qq
CVE: CVE-2022-36103

Signed-off-by: Andrey Smirnov <andrey.smirnov@talos-systems.com>

internal/app/trustd/internal/reg/reg.go

@@ -6,12 +6,19 @@ package reg
 
 import (
 	"context"
+	stdx509 "crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/pem"
+	"log"
 
 	"github.com/cosi-project/runtime/pkg/resource"
 	"github.com/cosi-project/runtime/pkg/safe"
 	"github.com/cosi-project/runtime/pkg/state"
 	"github.com/siderolabs/crypto/x509"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/peer"
+	"google.golang.org/grpc/status"
 
 	securityapi "github.com/talos-systems/talos/pkg/machinery/api/security"
 	"github.com/talos-systems/talos/pkg/machinery/resources/secrets"
@@ -33,17 +40,63 @@ func (r *Registrator) Register(s *grpc.Server) {
 }
 
 // Certificate implements the securityapi.SecurityServer interface.
+//
+// This API is called by Talos worker nodes to request a server certificate for apid running on the node.
+// Control plane nodes generate certificates (client and server) directly from machine config PKI.
 func (r *Registrator) Certificate(ctx context.Context, in *securityapi.CertificateRequest) (resp *securityapi.CertificateResponse, err error) {
+	remotePeer, ok := peer.FromContext(ctx)
+	if !ok {
+		return nil, status.Error(codes.PermissionDenied, "peer not found")
+	}
+
 	osRoot, err := safe.StateGet[*secrets.OSRoot](ctx, r.Resources, resource.NewMetadata(secrets.NamespaceName, secrets.OSRootType, secrets.OSRootID, resource.VersionUndefined))
 	if err != nil {
 		return nil, err
 	}
 
-	// TODO: Verify that the request is coming from the IP addresss declared in
+	// decode and validate CSR
+	csrPemBlock, _ := pem.Decode(in.Csr)
+	if csrPemBlock == nil {
+		return nil, status.Errorf(codes.InvalidArgument, "failed to decode CSR")
+	}
+
+	request, err := stdx509.ParseCertificateRequest(csrPemBlock.Bytes)
+	if err != nil {
+		return nil, status.Errorf(codes.InvalidArgument, "failed to parse CSR: %s", err)
+	}
+
+	log.Printf("received CSR signing request from %s: subject %s dns names %s addresses %s", remotePeer.Addr, request.Subject, request.DNSNames, request.IPAddresses)
+
+	// allow only server auth certificates
+	x509Opts := []x509.Option{
+		x509.KeyUsage(stdx509.KeyUsageDigitalSignature),
+		x509.ExtKeyUsage([]stdx509.ExtKeyUsage{stdx509.ExtKeyUsageServerAuth}),
+	}
+
+	// don't allow any certificates which can be used for client authentication
+	//
+	// we don't return an error here, as otherwise workers running old versions of Talos
+	// will fail to provision client certificate and will never launch apid
+	//
+	// instead, the returned certificate will be rejected when being used
+	if len(request.Subject.Organization) > 0 {
+		log.Printf("removing client auth organization from CSR: %s", request.Subject.Organization)
+
+		x509Opts = append(x509Opts, x509.OverrideSubject(func(subject *pkix.Name) {
+			subject.Organization = nil
+		}))
+	}
+
+	// TODO: Verify that the request is coming from the IP address declared in
 	// the CSR.
-	signed, err := x509.NewCertificateFromCSRBytes(osRoot.TypedSpec().CA.Crt, osRoot.TypedSpec().CA.Key, in.Csr)
+	signed, err := x509.NewCertificateFromCSRBytes(
+		osRoot.TypedSpec().CA.Crt,
+		osRoot.TypedSpec().CA.Key,
+		in.Csr,
+		x509Opts...,
+	)
 	if err != nil {
-		return
+		return nil, status.Errorf(codes.Internal, "failed to sign CSR: %s", err)
 	}
 
 	resp = &securityapi.CertificateResponse{

internal/app/trustd/internal/reg/reg_test.go

@@ -4,11 +4,98 @@
 
 package reg_test
 
-import "testing"
+import (
+	"context"
+	stdx509 "crypto/x509"
+	"net"
+	"net/netip"
+	"testing"
+	"time"
 
-func TestEmpty(t *testing.T) {
-	// added for accurate coverage estimation
-	//
-	// please remove it once any unit-test is added
-	// for this package
+	"github.com/cosi-project/runtime/pkg/state"
+	"github.com/cosi-project/runtime/pkg/state/impl/inmem"
+	"github.com/cosi-project/runtime/pkg/state/impl/namespaced"
+	"github.com/siderolabs/crypto/x509"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc/peer"
+
+	"github.com/talos-systems/talos/internal/app/trustd/internal/reg"
+	"github.com/talos-systems/talos/pkg/machinery/api/security"
+	"github.com/talos-systems/talos/pkg/machinery/config/types/v1alpha1/generate"
+	"github.com/talos-systems/talos/pkg/machinery/resources/secrets"
+	"github.com/talos-systems/talos/pkg/machinery/role"
+)
+
+func TestCertificate(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	resources := state.WrapCore(namespaced.NewState(inmem.Build))
+
+	ca, err := generate.NewTalosCA(time.Now())
+	require.NoError(t, err)
+
+	osRoot := secrets.NewOSRoot(secrets.OSRootID)
+	osRoot.TypedSpec().CA = &x509.PEMEncodedCertificateAndKey{
+		Crt: ca.CrtPEM,
+		Key: ca.KeyPEM,
+	}
+	require.NoError(t, resources.Create(ctx, osRoot))
+
+	ctx = peer.NewContext(ctx, &peer.Peer{
+		Addr: &net.TCPAddr{
+			IP:   netip.MustParseAddr("127.0.0.1").AsSlice(),
+			Port: 30000,
+		},
+	})
+
+	r := &reg.Registrator{
+		Resources: resources,
+	}
+
+	for _, tt := range []struct {
+		name       string
+		csrSetters []x509.Option
+	}{
+		{
+			name: "server certificate",
+			csrSetters: []x509.Option{
+				x509.IPAddresses([]net.IP{netip.MustParseAddr("10.5.0.4").AsSlice()}),
+				x509.DNSNames([]string{"talos-default-worker-1"}),
+				x509.CommonName("talos-default-worker-1"),
+			},
+		},
+		{
+			name: "attempt at client certificate",
+			csrSetters: []x509.Option{
+				x509.CommonName("talos-default-worker-1"),
+				x509.Organization(string(role.Impersonator)),
+			},
+		},
+	} {
+		tt := tt
+
+		t.Run(tt.name, func(t *testing.T) {
+			serverCSR, serverCert, err := x509.NewEd25519CSRAndIdentity(tt.csrSetters...)
+			require.NoError(t, err)
+
+			resp, err := r.Certificate(ctx, &security.CertificateRequest{
+				Csr: serverCSR.X509CertificateRequestPEM,
+			})
+			require.NoError(t, err)
+
+			assert.Equal(t, resp.Ca, ca.CrtPEM)
+
+			serverCert.Crt = resp.Crt
+
+			cert, err := serverCert.GetCert()
+			require.NoError(t, err)
+
+			assert.Equal(t, stdx509.KeyUsageDigitalSignature, cert.KeyUsage)
+			assert.Equal(t, []stdx509.ExtKeyUsage{stdx509.ExtKeyUsageServerAuth}, cert.ExtKeyUsage)
+			assert.Equal(t, "talos-default-worker-1", cert.Subject.CommonName)
+			assert.Equal(t, []string(nil), cert.Subject.Organization)
+		})
+	}
 }