Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(auth): handle ENOTDIR when opening cert config #10697

Merged

Conversation

dfinkel
Copy link
Contributor

@dfinkel dfinkel commented Aug 16, 2024

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

@dfinkel dfinkel requested a review from a team as a code owner August 16, 2024 18:49
@quartzmo quartzmo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 20, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 20, 2024
@codyoss codyoss added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 21, 2024
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see feedback on the linked issue.

@codyoss codyoss removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 21, 2024
@dfinkel dfinkel force-pushed the auth_ENOTDIR_fix_client_certificate_init branch 2 times, most recently from b4f7650 to eb7bb72 Compare August 22, 2024 14:14
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR and the test coverage 🎆

@codyoss codyoss added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 22, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 22, 2024
@codyoss codyoss changed the title auth: handle ENOTDIR when opening cert config fix(auth): handle ENOTDIR when opening cert config Aug 22, 2024
@dfinkel dfinkel force-pushed the auth_ENOTDIR_fix_client_certificate_init branch from 934674d to a60add3 Compare August 22, 2024 16:23
@codyoss codyoss added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 22, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 22, 2024
@codyoss
Copy link
Member

codyoss commented Aug 22, 2024

Failing test:

=== RUN   TestSecureConnectSource_ConfigMissingPerms
    secureconnect_cert_test.go:53: got cert: could not parse JSON in "/tmp/TestSecureConnectSource_ConfigMissingPerms1547290171/001/unreadable.json": unexpected end of JSON input, want certificate source is unavailable
--- FAIL: TestSecureConnectSource_ConfigMissingPerms (0.00s)

@dfinkel
Copy link
Contributor Author

dfinkel commented Aug 22, 2024

Oh, right!
That test won't work when running as root. 🤦.

I'll update it to skip that test when the UID is 0. (it seems a bit too complicated to check for CAP_FOWNER)

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
@dfinkel dfinkel force-pushed the auth_ENOTDIR_fix_client_certificate_init branch from e26a384 to 10b2435 Compare August 22, 2024 19:00
@dfinkel
Copy link
Contributor Author

dfinkel commented Aug 22, 2024

Ok, I've rebased and pushed a guard to skip the EPERM test when the UID is 0.
(I hope these tests sometimes run as something other than root -- it passes on my machine when running as a non-root user)

@codyoss codyoss added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 22, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 22, 2024
@codyoss codyoss merged commit 27f6ae6 into googleapis:main Aug 22, 2024
8 checks passed
@dfinkel dfinkel deleted the auth_ENOTDIR_fix_client_certificate_init branch August 29, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

auth: ENOTDIR not handled when user's HOME is not a directory
5 participants