Avoid polling token endpoint after login request cancelled #903

adombeck · 2025-04-30T05:19:05Z

If the context of the IsAuthenticated call from the PAM module to authd is cancelled, e.g. because the process of the PAM module exits, we want to automatically cancel the IsAuthenticated call to the broker, to avoid doing any further work towards authenticating the user (like sending requests to OAuth2 endpoints).

adombeck · 2025-04-30T05:47:07Z

I manually tested the changes by running sudo login ... and then cancelling the login via Ctrl+C when prompted to scan the QR code. The IsAuthenticated call to the broker is correctly cancelled, no more requests are sent to the token endpoint.

However, this comment:

authd/internal/brokers/dbusbroker.go

Line 102 in 5863e33

// We don’t want to cancel the context when the parent call is cancelled.

... indicates that the previous behavior was on purpose. I did some investigations to find out the reason.

The functionality to cancel IsAuthenticated calls from authd to the broker via a separate CancelIsAuthenticated method was introduced in #4 which closed UDENG-1107. That issue states "it was not possible to send a context through a dbus call". However, the godbus package does provide the CallWithContext method which automatically cancels the D-Bus call when the context is cancelled. I suspect that this was overlooked back then, and that there is not actually a good reason to not cancel the IsAuthenticated when the context is cancelled.

The first implementation actually used CallWithContext with the context, but that was changed in 53f1f92. The comment itself was added later in 4b312ca.

@denisonbarbosa, do you remember any more details? Am I missing something?

adombeck · 2025-04-30T07:05:17Z

Ok, I figured out what I was missing. The Ctrl+C to the PAM module causes an EndSession request to be sent to the broker, which then calls CancelIsAuthenticated. So the IsAuthenticated call is not cancelled because the context was cancelled on the client side, but because the client called EndSession. The CallWithContext provided by the Go dbus module does not actually cause the call to be cancelled on the server side, it only cancels the request on the client side. I'll add a code comment which explains all that, so that we don't waste time on it again.

3v1n0 · 2025-04-30T08:33:01Z

Yeah... Sorry for not mentioning it in the card, but I also did some experiments with that after I wrote the initial thing, login disconnecting doesn't bring anything new to us.

So I feel we'll have to do process monitoring or something instead.

The Go process started by the PAM module kept running even after the process of the PAM module exited, causing authd to continue with the authentication procedure.

This is needed to be able to check the underlying error with errors.Is or errors.As.

adombeck · 2025-04-30T11:15:32Z

With the new commits I pushed, cancellation also works when I run sudo login ... and let it time out.

D-Bus does not support cancelling ongoing method calls through context cancellation, in contrast to gRPC. So if the context of the IsAuthenticated call from the PAM module to authd is cancelled (e.g. because the PAM module exited unexpectedly), we need propagate that to the broker by calling the CancelIsAuthenticated method. We used to do that by calling IsAuthenticated in a goroutine and waiting for the context to be cancelled in the main goroutine, calling CancelIsAuthenticated when the context is cancelled. This commit simplifies that, by passing the context to BusObject.CallWithContext, and calling when the returned error is context.Cancelled. This is only done for the D-Bus broker, not the example broker. That's not a problem, because the example broker already cancels the IsAuthenticated call if the context is cancelled, without requiring a separate CancelIsAuthenticated call. This commit also returns auth.Cancelled if the context was cancelled, which was actually never used with the actual D-Bus broker, but with the example broker and the testutils.BrokerBusMock.

The first call was cancelled iff cancelFirstCall is false.

3v1n0 · 2025-04-30T11:55:29Z

pam/main-exec.go

+	if err := unix.Prctl(unix.PR_SET_PDEATHSIG, uintptr(syscall.SIGTERM), 0, 0, 0); err != nil {
+		log.Errorf(context.Background(), "failed to set PDEATHSIG: %v", err)
+		os.Exit(1)


Yeah, I was wondering to use the same too... But maybe we can handle it also client-side, on the dbus side... Let me check that too.

However this is fine.

The changes on broker instead I'd hold since I had some refactoring there for another branch, and that side is quite delicate sadly (as we may cancel too early).

denisonbarbosa · 2025-04-30T12:45:12Z

internal/brokers/dbusbroker.go

+		b.CancelIsAuthenticated(context.Background(), sessionID)
+		return auth.Cancelled, "", nil


Doesn't this create a race? CancelIsAuthenticated is not a blocking call, so (also considering that the "wait" was removed on the IsAuthenticated function) we can end up returning to PAM without the broker being "ready" to handle a new call, no?

CancelIsAuthenticated is not a blocking call

Really? It also uses BusObject.CallWithContext from the dbus package, which according to the documentation waits for a reply.

Yeah, the dbus call returns, but if you look at the CancelIsAuthenticated func on the broker side, it's not waiting for the cancellation to be done, it just requests a cancel and then the IsAuthenticated func that returns whether it was cancelled or not. Did it make sense? It's not easy to explain

codecov-commenter · 2025-04-30T13:16:13Z

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.38%. Comparing base (5863e33) to head (c576d00).

Files with missing lines	Patch %	Lines
pam/main-exec.go	42.85%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #903      +/-   ##
==========================================
- Coverage   85.51%   85.38%   -0.14%     
==========================================
  Files          82       82              
  Lines        5703     5707       +4     
  Branches      109      109              
==========================================
- Hits         4877     4873       -4     
- Misses        771      779       +8     
  Partials       55       55

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

adombeck added 2 commits April 30, 2025 12:36

pam: Make the Go process exit when the parent process exits

c5beec4

The Go process started by the PAM module kept running even after the process of the PAM module exited, causing authd to continue with the authentication procedure.

Implement Unwrap method of ToDisplayError

a461f10

This is needed to be able to check the underlying error with errors.Is or errors.As.

adombeck force-pushed the UDENG-6770-avoid-polling-after-login-request-cancelled branch from 631cd42 to afb8cea Compare April 30, 2025 11:14

adombeck added 3 commits April 30, 2025 13:45

Explain why we need to call CancelIsAuthenticated

26a86d4

Fix inverted behavior of cancelFirstCall option

791edc2

The first call was cancelled iff cancelFirstCall is false.

adombeck force-pushed the UDENG-6770-avoid-polling-after-login-request-cancelled branch from afb8cea to e44b939 Compare April 30, 2025 11:47

3v1n0 reviewed Apr 30, 2025

View reviewed changes

denisonbarbosa reviewed Apr 30, 2025

View reviewed changes

Improve error message

c576d00

adombeck force-pushed the UDENG-6770-avoid-polling-after-login-request-cancelled branch from e44b939 to c576d00 Compare April 30, 2025 13:02

3v1n0 mentioned this pull request Apr 30, 2025

pam/main-exec: Exit with failure if the the server connection is lost #904

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Avoid polling token endpoint after login request cancelled #903

Avoid polling token endpoint after login request cancelled #903

adombeck commented Apr 30, 2025

Uh oh!

adombeck commented Apr 30, 2025

Uh oh!

adombeck commented Apr 30, 2025 •

edited

Loading

Uh oh!

3v1n0 commented Apr 30, 2025 •

edited

Loading

Uh oh!

adombeck commented Apr 30, 2025

Uh oh!

3v1n0 Apr 30, 2025

Uh oh!

denisonbarbosa Apr 30, 2025 •

edited

Loading

Uh oh!

adombeck Apr 30, 2025

Uh oh!

denisonbarbosa Apr 30, 2025 •

edited

Loading

Uh oh!

codecov-commenter commented Apr 30, 2025

Uh oh!

Uh oh!

		b.CancelIsAuthenticated(context.Background(), sessionID)
		return auth.Cancelled, "", nil

Avoid polling token endpoint after login request cancelled #903

Are you sure you want to change the base?

Avoid polling token endpoint after login request cancelled #903

Conversation

adombeck commented Apr 30, 2025

Uh oh!

adombeck commented Apr 30, 2025

Uh oh!

adombeck commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

3v1n0 commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adombeck commented Apr 30, 2025

Uh oh!

3v1n0 Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

denisonbarbosa Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adombeck Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

denisonbarbosa Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 30, 2025

Codecov Report

Uh oh!

Uh oh!

adombeck commented Apr 30, 2025 •

edited

Loading

3v1n0 commented Apr 30, 2025 •

edited

Loading

denisonbarbosa Apr 30, 2025 •

edited

Loading

denisonbarbosa Apr 30, 2025 •

edited

Loading