From 10b24353d932ead11b629375c0d2c9ba39b2d946 Mon Sep 17 00:00:00 2001
From: David Finkel <davidf@vimeo.com>
Date: Fri, 16 Aug 2024 14:34:46 -0400
Subject: [PATCH] auth: treat all cert config read errors as missing

Some users explicitly have their home directories set to `/dev/null`.
When opening a file that's "under that directory", instead of getting
the normal `ENOENT`, one should expect to get `ENOTDIR`.

Unfortunately, due to the variety of cases under which `ENOTDIR` is
returned, the go team has declined to include ENOTDIR in `ErrNotExist`.
(https://golang.org/issues/18974)

The most robust solution in this case (following [1]) is to treat any
read error as `errSourceUnavailable`. This has the mixed-benefit of also
covering EPERM. (hopefully this doesn't make it too hard to track down
permissions problems for secure connect users)

Resolves: #10696

[1]: https://go-review.googlesource.com/c/oauth2/+/493695
---
 .../transport/cert/secureconnect_cert.go      | 10 +++----
 .../transport/cert/secureconnect_cert_test.go | 30 +++++++++++++++++++
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/auth/internal/transport/cert/secureconnect_cert.go b/auth/internal/transport/cert/secureconnect_cert.go
index 3227aba280c8..738cb21618e7 100644
--- a/auth/internal/transport/cert/secureconnect_cert.go
+++ b/auth/internal/transport/cert/secureconnect_cert.go
@@ -62,11 +62,11 @@ func NewSecureConnectProvider(configFilePath string) (Provider, error) {
 
 	file, err := os.ReadFile(configFilePath)
 	if err != nil {
-		if errors.Is(err, os.ErrNotExist) {
-			// Config file missing means Secure Connect is not supported.
-			return nil, errSourceUnavailable
-		}
-		return nil, err
+		// Config file missing means Secure Connect is not supported.
+		// There are non-os.ErrNotExist errors that may be returned.
+		// (e.g. if the home directory is /dev/null, *nix systems will
+		// return ENOTDIR instead of ENOENT)
+		return nil, errSourceUnavailable
 	}
 
 	var metadata secureConnectMetadata
diff --git a/auth/internal/transport/cert/secureconnect_cert_test.go b/auth/internal/transport/cert/secureconnect_cert_test.go
index 994d355d8775..c2375ecf5c9f 100644
--- a/auth/internal/transport/cert/secureconnect_cert_test.go
+++ b/auth/internal/transport/cert/secureconnect_cert_test.go
@@ -17,6 +17,8 @@ package cert
 import (
 	"bytes"
 	"errors"
+	"os"
+	"path/filepath"
 	"testing"
 )
 
@@ -30,6 +32,34 @@ func TestSecureConnectSource_ConfigMissing(t *testing.T) {
 	}
 }
 
+func TestSecureConnectSource_ConfigNotDirMissing(t *testing.T) {
+	source, err := NewSecureConnectProvider("/dev/null/missing.json")
+	if got, want := err, errSourceUnavailable; !errors.Is(err, errSourceUnavailable) {
+		t.Fatalf("got %v, want %v", got, want)
+	}
+	if source != nil {
+		t.Errorf("got %v, want nil source", source)
+	}
+}
+
+func TestSecureConnectSource_ConfigMissingPerms(t *testing.T) {
+	if os.Getuid() == 0 {
+		t.Skip("skipping permissions-related test because UID is 0 (reads never get EPERM while running as root in the current namespace)")
+	}
+	td := t.TempDir()
+	tmpFilePath := filepath.Join(td, "unreadable.json")
+	if wrErr := os.WriteFile(tmpFilePath, []byte{}, 0000); wrErr != nil {
+		t.Fatalf("failed to write temp file with permissions 000: %s", wrErr)
+	}
+	source, err := NewSecureConnectProvider(tmpFilePath)
+	if got, want := err, errSourceUnavailable; !errors.Is(err, errSourceUnavailable) {
+		t.Fatalf("got %v, want %v", got, want)
+	}
+	if source != nil {
+		t.Errorf("got %v, want nil source", source)
+	}
+}
+
 func TestSecureConnectSource_GetClientCertificateSuccess(t *testing.T) {
 	source, err := NewSecureConnectProvider("testdata/context_aware_metadata.json")
 	if err != nil {