pam: Set `PAM_AUTHTOK` on successful authentication by adombeck · Pull Request #1190 · canonical/authd

adombeck · 2026-01-13T15:51:08Z

By setting PAM_AUTHTOK the GNOME keyring is unlocked.

UDENG-8799

codecov-commenter · 2026-01-13T16:00:35Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.64%. Comparing base (01cd78c) to head (22bc3d5).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1190      +/-   ##
==========================================
+ Coverage   87.53%   87.64%   +0.11%     
==========================================
  Files          91       91              
  Lines        6231     6231              
  Branches      111      111              
==========================================
+ Hits         5454     5461       +7     
+ Misses        717      714       -3     
+ Partials       60       56       -4

☔ 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.

3v1n0 · 2026-01-13T16:27:08Z

This would be fine per the current broker, but we have the problem that we don't handle the change password phase well (as per #944 (comment)).

And so, without it we may just end up having the token just set once and then be broken by a password change.

Then, the problem, daemon side, is that we do not have the information to know whether the broker supports secrets or not to be sure if we should set this all the times.

So the token that we are setting may be:

Unset (if we login without local password, in potential brokers without it)
Change depending on the authentication method that has been chosen (imagine a broker supporting a PIN or a Password)

Thus again, this is something that IMHO we should not do without the broker providing us the AuthTok to set.

My idea to have something more generic was:

The broker defines an unique token that is invisible to the user
The token is sent to PAM on auth and we use it as the keyring password.

PRO: Independent from authentication mode being used
CONS: If the user logout from a keyring inside the session, currently we have no way to unlock it (we could do it through PAM, but it needs some UI work)

For the current situation, we could workaround the problem by keeping a similar structure, but sending back the very same secret that has been used by the user to authenticate.

In this way we still leave the control to the broker, but also we should be able to use the information only if it's possible.

However, in general I'd personally prefer to handle it all together (I mean, also once the passwd case is fixed), rather than having half-baked solutions, since this is the reason why we did not do this in the beginning.

adombeck · 2026-01-13T16:29:51Z

This would be fine per the current broker, but we have the problem that we don't handle the change password phase well (as per #944 (comment)).

I read that comment before opening the PR but I don't understand the issue. We only support changing the password after a successful device authentication, in which case the same code path is taken and we set PAM_AUTHTOK. I tested that after changing the password by doing device authentication again the keyring is still successfully unlocked.

3v1n0 · 2026-01-13T17:13:08Z

We only support changing the password after a successful device authentication, in which case the same code path is taken and we set PAM_AUTHTOK. I tested that after changing the password by doing device authentication again the keyring is still successfully unlocked.

I need to check again the keyring pam code, but that was expected I think when using passwd.

adombeck · 2026-01-13T17:15:15Z

I need to check again the keyring pam code, but that was expected I think when using passwd.

I also tried passwd already, also works fine.

3v1n0 · 2026-01-13T17:18:20Z

Also if the one authenticating to use passwd is not the same user we change the password for?

adombeck · 2026-01-13T17:19:27Z

Also if the one authenticating to use passwd is not the same user we change the password for?

I'll try that.

adombeck · 2026-01-13T17:21:01Z

Also if the one authenticating to use passwd is not the same user we change the password for?

It's always asking for the password of the user actually: #851

adombeck · 2026-01-13T17:23:44Z

Also if the one authenticating to use passwd is not the same user we change the password for?

Yes, the keyring is also unlocked when I change the password via sudo passwd in another user's session (which still requires the user's local password) and afterwards log in via GDM with the new password.

adombeck · 2026-02-02T14:40:15Z

With #1234 we will have the case that there is no local password. I think authd could instead generate a secret the first time the user authenticates and set PAM_AUTHTOK to that when the user authenticates successfully. We could use systemd credentials to encrypt the secret with the TPM.

That is similar to your suggestion @3v1n0, but I think it makes more sense to let authd generate that secret than the broker, because there's nothing broker-specific about that secret. WDYT?

codecov · 2026-02-03T18:53:48Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.56%. Comparing base (c86d7e2) to head (2dac51d).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1190      +/-   ##
==========================================
+ Coverage   80.57%   87.56%   +6.99%     
==========================================
  Files          20       91      +71     
  Lines         978     6232    +5254     
  Branches        0      111     +111     
==========================================
+ Hits          788     5457    +4669     
- Misses        190      719     +529     
- Partials        0       56      +56

☔ 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 · 2026-02-03T23:36:23Z

@3v1n0 and I discussed this today.

A reason to let the broker decide which secret to use is that it allows using the local password if there is one, which allows the user to unlock the keyring themself with that password in case that the keyring is locked during a session. That won't be possible without UI changes if we use an authd-generated secret instead. However, locking and unlocking the screen could be a workaround to also unlock the keyring in that case.

An issue if we do use the local password as the keyring secret is that it won't be updated if the disable_local_password setting is activated after a user already set a local password. That case would be fixed if we would always use an authd-generated secret instead.

adombeck · 2026-02-04T01:00:59Z

braindumping an idea:

local password enabled -> broker returns that with the auth result and PAM module sets PAM_AUTHTOK to it
local password is disabled -> broker sees that local password was disabled but that there is one already stored in the data dir (the hashed password file), so it knows that we need the local password to change the keyring password
2.1. broker only offers local password but sets next auth mode to device auth
2.2 auth result contains local password, authd generates a random secret and stores it in its data directory as a systemd credential
2.3 PAM module changes the keyring password from the local password to the generated secret
2.4 user continues with device authentication
2.5 broker deletes hashed password file

shiv-tyagi · 2026-02-05T16:58:23Z

2. broker sees that local password was disabled but that there is one already stored in the data dir (the hashed password file), so it knows that we need the local password to change the keyring password

What do we do in the other scenario where the local password is enabled back and there is a secret but no password file?

Do we perform the similar steps with password and secret swapped, i.e., changing the keyring password with secret and moving ahead with the local password?

3v1n0 · 2026-02-10T01:02:19Z

Do we perform the similar steps with password and secret swapped, i.e., changing the keyring password with secret and moving ahead with the local password?

That is technically possible, although risky (as the module may fail and us not discover it), but it's currently the only possibility.

Ideally having a keyring that supports multiple secrets would be nice.

shiv-tyagi · 2026-02-13T17:21:09Z

Ok. Please feel free to let me know if my help is needed there, I will be happy to help. I will rebase #1234 after that.

adombeck · 2026-04-17T11:53:10Z

@3v1n0, regarding our discussion, this would handle the MFA case you mentioned, right?

diff --git a/pam/internal/adapter/authentication.go b/pam/internal/adapter/authentication.go
index 9c920c52d..67eeefa91 100644
--- a/pam/internal/adapter/authentication.go
+++ b/pam/internal/adapter/authentication.go
@@ -349,6 +349,8 @@ func (m authenticationModel) Update(msg tea.Msg) (authModel authenticationModel,
 			var secret string
 			if msg.secret != nil {
 				secret = *msg.secret
+			} else if m.currentSecret != "" {
+				secret = m.currentSecret
 			} else {
 				log.Warningf(context.Background(), "authentication granted, but no secret returned, cannot set PAM_AUTHTOK")
 			}

By setting PAM_AUTHTOK the GNOME keyring is unlocked. UDENG-8799

3v1n0 · 2026-04-17T13:02:08Z

@3v1n0, regarding our discussion, this would handle the MFA case you mentioned, right?

diff --git a/pam/internal/adapter/authentication.go b/pam/internal/adapter/authentication.go
index 9c920c52d..67eeefa91 100644
--- a/pam/internal/adapter/authentication.go
+++ b/pam/internal/adapter/authentication.go
@@ -349,6 +349,8 @@ func (m authenticationModel) Update(msg tea.Msg) (authModel authenticationModel,
 			var secret string
 			if msg.secret != nil {
 				secret = *msg.secret
+			} else if m.currentSecret != "" {
+				secret = m.currentSecret
 			} else {
 				log.Warningf(context.Background(), "authentication granted, but no secret returned, cannot set PAM_AUTHTOK")
 			}

Yeah, unless the fact that if there are multiple fields, we may pick the wrong one (so I'd still use the last secret if that it's the one that provided the granted access)... But in case that there is one form and then a non-form entry, it would work.

It's tricky in the case that we have a Password authentication first and then an OTP for example, we can maybe do assumptions on the visibility of the form.. But well, as you know this is the reason why I liked the broker to drive all this.

adombeck · 2026-04-17T13:04:06Z

It's tricky in the case that we have a Password authentication first and then an OTP for example.

True, but changing the password is also broken in that case. Like I said in our discussion, I would leave solving that to later, when we actually have a broker that supports these kind of flows.

3v1n0 · 2026-04-17T13:05:28Z

Let's add some FIXME and TODO comments at least

adombeck · 2026-04-17T13:09:16Z

Let's add some FIXME and TODO comments at least

done

adombeck · 2026-04-17T13:19:05Z

@copilot review this PR

Copilot · 2026-04-17T13:20:42Z

@copilot review this PR

Here's a summary of the current PR changes:

return.go: Added AuthTok string field to PamSuccess struct.
authentication.go:
- On auth.Granted, selects the secret with priority: msg.secret (returned by broker) → m.currentSecret (last used secret, for MFA flows where the final step doesn't return a secret) → warning log if neither is available.
- Added TODO comments noting that the selected secret may be incorrect in multi-step flows where the last step uses a non-password secret (e.g., Password → OTP).
pam.go: Sets PAM_AUTHTOK via mTx.SetItem(pam.Authtok, exitStatus.AuthTok) when AuthTok is non-empty, which causes the GNOME keyring to be unlocked on successful authentication.

The MFA fallback (m.currentSecret) handles the case where multiple authentication steps are required and only some steps return a secret — in that case we fall back to the last known secret. The TODO comments document the known limitation for flows like Password→OTP where we'd pick the OTP secret instead of the password.

3v1n0 · 2026-04-17T13:49:02Z

+			var secret string
+			// TODO: This will not select the correct secret in case the last authentication step uses a secret
+			// which is not the local password (e.g. OTP).
+			if msg.secret != nil {


I don't remember if it's still available at this level, but I think that here we could at least rely on the currentModel type and basically only set this if this an hidden form model or a new password one.

3v1n0 · 2026-04-17T13:50:00Z

+			if msg.secret != nil {
+				secret = *msg.secret
+			} else if m.currentSecret != "" {
+				secret = m.currentSecret


Suggested change

secret = m.currentSecret

// FIXME: We should move this to the broker when all the stars align....... :)

secret = m.currentSecret

3v1n0 · 2026-04-17T13:50:44Z

+			} else if m.currentSecret != "" {
+				secret = m.currentSecret
+			} else {
+				log.Warningf(context.Background(), "authentication granted, but no secret returned, cannot set PAM_AUTHTOK")


Suggested change

log.Warningf(context.Background(), "authentication granted, but no secret returned, cannot set PAM_AUTHTOK")

log.Warningf(context.Background(), "authentication granted, but no secret is available, cannot set PAM_AUTHTOK")

3v1n0 · 2026-04-17T13:54:17Z

We should also add tests here, at least it would suffice to print the value (when set) in ./pam/tools/pam-runner/pam-runner.go's printPamResult, so that we can track the changes into the golden files.

3v1n0 · 2026-04-17T13:58:56Z

And testing the gdm model should also be easy, given that it's the main target (but not the only one, since we need the cli for password change anyways)

adombeck marked this pull request as ready for review January 13, 2026 16:09

adombeck requested a review from a team as a code owner January 13, 2026 16:09

adombeck requested a review from 3v1n0 January 13, 2026 16:09

adombeck mentioned this pull request Feb 2, 2026

Allow disabling local password through broker.conf #1234

Open

adombeck force-pushed the UDENG-8799-pam-set-authtok branch from 22bc3d5 to 2f5d0e1 Compare February 3, 2026 18:42

adombeck force-pushed the UDENG-8799-pam-set-authtok branch from 2f5d0e1 to 2dac51d Compare February 3, 2026 19:45

adombeck force-pushed the UDENG-8799-pam-set-authtok branch from 2dac51d to cfab192 Compare February 3, 2026 23:37

adombeck force-pushed the UDENG-8799-pam-set-authtok branch from cfab192 to d0544ad Compare April 16, 2026 13:38

pam: Set PAM_AUTHTOK on successful authentication

c44302e

By setting PAM_AUTHTOK the GNOME keyring is unlocked. UDENG-8799

adombeck force-pushed the UDENG-8799-pam-set-authtok branch from d0544ad to c44302e Compare April 17, 2026 11:54

pam: Add TODO comments

a01ea08

adombeck marked this pull request as draft April 17, 2026 13:16

adombeck marked this pull request as ready for review April 17, 2026 13:17

Copilot started work on behalf of adombeck April 17, 2026 13:19 View session

Copilot finished work on behalf of adombeck April 17, 2026 13:21

3v1n0 reviewed Apr 17, 2026

View reviewed changes

denisonbarbosa approved these changes Apr 17, 2026

View reviewed changes

	secret = m.currentSecret
	// FIXME: We should move this to the broker when all the stars align....... :)
	secret = m.currentSecret

	log.Warningf(context.Background(), "authentication granted, but no secret returned, cannot set PAM_AUTHTOK")
	log.Warningf(context.Background(), "authentication granted, but no secret is available, cannot set PAM_AUTHTOK")

Conversation

adombeck commented Jan 13, 2026

Uh oh!

codecov-commenter commented Jan 13, 2026

Codecov Report

Uh oh!

3v1n0 commented Jan 13, 2026

Uh oh!

adombeck commented Jan 13, 2026

Uh oh!

3v1n0 commented Jan 13, 2026

Uh oh!

adombeck commented Jan 13, 2026

Uh oh!

3v1n0 commented Jan 13, 2026

Uh oh!

adombeck commented Jan 13, 2026

Uh oh!

adombeck commented Jan 13, 2026

Uh oh!

adombeck commented Jan 13, 2026

Uh oh!

adombeck commented Feb 2, 2026

Uh oh!

codecov bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

adombeck commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adombeck commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shiv-tyagi commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

3v1n0 commented Feb 10, 2026

Uh oh!

shiv-tyagi commented Feb 13, 2026

Uh oh!

adombeck commented Apr 17, 2026

Uh oh!

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

Uh oh!

Uh oh!

adombeck commented Apr 17, 2026

Uh oh!

3v1n0 commented Apr 17, 2026

Uh oh!

adombeck commented Apr 17, 2026

Uh oh!

adombeck commented Apr 17, 2026

Uh oh!

Copilot AI commented Apr 17, 2026

Uh oh!

3v1n0 Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

3v1n0 Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

3v1n0 Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

3v1n0 commented Apr 17, 2026

Uh oh!

3v1n0 commented Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Feb 3, 2026 •

edited

Loading

adombeck commented Feb 3, 2026 •

edited

Loading

adombeck commented Feb 4, 2026 •

edited

Loading

shiv-tyagi commented Feb 5, 2026 •

edited

Loading

3v1n0 commented Apr 17, 2026 •

edited

Loading