From 2148d776715b05884c660ff60997856348c36cc2 Mon Sep 17 00:00:00 2001 From: Noah Stride Date: Mon, 7 Oct 2024 10:12:44 +0100 Subject: [PATCH 1/3] Tweak Workload Attestation failure handling --- lib/tbot/service_spiffe_workload_api.go | 47 ++++++++++++++++-------- lib/tbot/spiffe/workloadattest/attest.go | 11 ++++-- 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/lib/tbot/service_spiffe_workload_api.go b/lib/tbot/service_spiffe_workload_api.go index 3747efc0f53b2..e3df708b99660 100644 --- a/lib/tbot/service_spiffe_workload_api.go +++ b/lib/tbot/service_spiffe_workload_api.go @@ -484,33 +484,48 @@ func filterSVIDRequests( return filtered } -func (s *SPIFFEWorkloadAPIService) authenticateClient(ctx context.Context) (*slog.Logger, workloadattest.Attestation, error) { - // The zero value of the attestation is equivalent to no attestation. - var att workloadattest.Attestation - +func (s *SPIFFEWorkloadAPIService) authenticateClient( + ctx context.Context, +) (*slog.Logger, workloadattest.Attestation, error) { p, ok := peer.FromContext(ctx) if !ok { - return nil, att, trace.BadParameter("peer not found in context") + return nil, workloadattest.Attestation{}, trace.BadParameter("peer not found in context") } log := s.log - authInfo, ok := p.AuthInfo.(uds.AuthInfo) - if ok && authInfo.Creds != nil { - var err error - att, err = s.attestor.Attest(ctx, authInfo.Creds.PID) - if err != nil { - return nil, att, trace.Wrap(err, "performing workload attestation") - } - log = log.With( - "workload", slog.LogValuer(att), - ) - } if p.Addr.String() != "" { log = log.With( slog.String("remote_addr", p.Addr.String()), ) } + authInfo, ok := p.AuthInfo.(uds.AuthInfo) + if !ok || authInfo.Creds == nil || authInfo.Creds.PID == 0 { + // This could be innocent - for example, if they are connecting via TCP + // we don't expect to see any UDS credentials. + // + // This can also occur if the caller is calling from another process + // namespace. + log.DebugContext(ctx, "Did not detect UDS credentials, will not complete workload attestation") + return log, workloadattest.Attestation{}, nil + } + + att, err := s.attestor.Attest(ctx, authInfo.Creds.PID) + if err != nil { + // Fail softly as there may be SVIDs configured that don't require any + // workload attestation and we should still issue those. + log.ErrorContext( + ctx, + "Workload attestation failed", + "error", err, + "pid", authInfo.Creds.PID, + ) + return log, workloadattest.Attestation{}, nil + } + log = log.With( + "workload", slog.LogValuer(att), + ) + return log, att, nil } diff --git a/lib/tbot/spiffe/workloadattest/attest.go b/lib/tbot/spiffe/workloadattest/attest.go index cd3d87cf65dc1..a50721b847ca1 100644 --- a/lib/tbot/spiffe/workloadattest/attest.go +++ b/lib/tbot/spiffe/workloadattest/attest.go @@ -27,6 +27,9 @@ import ( // Attestation holds the results of the attestation process carried out on a // PID by the attestor. +// +// The zero value of this type indicates that no attestation was performed or +// was successful. type Attestation struct { Unix UnixAttestation Kubernetes KubernetesAttestation @@ -82,10 +85,12 @@ func NewAttestor(log *slog.Logger, cfg Config) (*Attestor, error) { func (a *Attestor) Attest(ctx context.Context, pid int) (Attestation, error) { a.log.DebugContext(ctx, "Starting workload attestation", "pid", pid) - defer a.log.DebugContext(ctx, "Finished workload attestation complete", "pid", pid) + defer a.log.DebugContext(ctx, "Finished workload attestation", "pid", pid) - att := Attestation{} - var err error + var ( + att Attestation + err error + ) // We always perform the unix attestation first att.Unix, err = a.unix.Attest(ctx, pid) From b4f57df0c2cf6926b3510930a425d6dea20ed0ce Mon Sep 17 00:00:00 2001 From: Noah Stride Date: Mon, 7 Oct 2024 10:13:43 +0100 Subject: [PATCH 2/3] Add future TODO --- lib/tbot/service_spiffe_workload_api.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/tbot/service_spiffe_workload_api.go b/lib/tbot/service_spiffe_workload_api.go index e3df708b99660..f9b12cce223bf 100644 --- a/lib/tbot/service_spiffe_workload_api.go +++ b/lib/tbot/service_spiffe_workload_api.go @@ -506,6 +506,10 @@ func (s *SPIFFEWorkloadAPIService) authenticateClient( // // This can also occur if the caller is calling from another process // namespace. + // + // TODO(noah): We should probably consider skipping all of this code + // if we know that the listener is TCP and not UDS, so we can make this + // a bit noisier. log.DebugContext(ctx, "Did not detect UDS credentials, will not complete workload attestation") return log, workloadattest.Attestation{}, nil } From 11de623ab2adc0acf2b06986ff54711294d32b7d Mon Sep 17 00:00:00 2001 From: Noah Stride Date: Mon, 7 Oct 2024 10:23:55 +0100 Subject: [PATCH 3/3] Tidy up flow a little more explicitly --- lib/tbot/service_spiffe_workload_api.go | 26 ++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/lib/tbot/service_spiffe_workload_api.go b/lib/tbot/service_spiffe_workload_api.go index f9b12cce223bf..71e52698e542a 100644 --- a/lib/tbot/service_spiffe_workload_api.go +++ b/lib/tbot/service_spiffe_workload_api.go @@ -500,17 +500,21 @@ func (s *SPIFFEWorkloadAPIService) authenticateClient( } authInfo, ok := p.AuthInfo.(uds.AuthInfo) - if !ok || authInfo.Creds == nil || authInfo.Creds.PID == 0 { - // This could be innocent - for example, if they are connecting via TCP - // we don't expect to see any UDS credentials. - // - // This can also occur if the caller is calling from another process - // namespace. - // - // TODO(noah): We should probably consider skipping all of this code - // if we know that the listener is TCP and not UDS, so we can make this - // a bit noisier. - log.DebugContext(ctx, "Did not detect UDS credentials, will not complete workload attestation") + // We expect Creds to be nil/unset if the client is connecting via TCP and + // therefore there is no workload attestation that can be completed. + if !ok || authInfo.Creds == nil { + return log, workloadattest.Attestation{}, nil + } + + // For a UDS, sometimes we are unable to determine the PID of the calling + // workload. This can happen if the caller is calling from another process + // namespace. In this case, Creds will be non-nil but the PID will be 0. + // + // We should fail softly here as there could be SVIDs that do not require + // workload attestation. + if authInfo.Creds.PID == 0 { + log.DebugContext( + ctx, "Failed to determine the PID of the calling workload. TBot may be running in a different process namespace to the workload. Workload attestation will not be completed.") return log, workloadattest.Attestation{}, nil }