Skip to content

Commit

Permalink
auth: treat all cert config read errors as missing
Browse files Browse the repository at this point in the history
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: googleapis#10696

[1]: https://go-review.googlesource.com/c/oauth2/+/493695
  • Loading branch information
dfinkel committed Aug 22, 2024
1 parent 5f06ae1 commit 10b2435
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 5 deletions.
10 changes: 5 additions & 5 deletions auth/internal/transport/cert/secureconnect_cert.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
30 changes: 30 additions & 0 deletions auth/internal/transport/cert/secureconnect_cert_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ package cert
import (
"bytes"
"errors"
"os"
"path/filepath"
"testing"
)

Expand All @@ -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 {
Expand Down

0 comments on commit 10b2435

Please sign in to comment.