nixos/cosmic: migrate from gnome-keyring to oo7 - #539810
Conversation
|
|
This probably doesn't need |
Pandapip1
left a comment
There was a problem hiding this comment.
Probably worth someone else reviewing in case I missed anything, but nothing obvious that's blocking
| name = "oo7"; | ||
| enable = cfg.oo7.enable; | ||
| control = "optional"; | ||
| modulePath = "${pkgs.oo7-pam}/lib/security/pam_oo7.so"; |
There was a problem hiding this comment.
Probably worth using getLib here?
There was a problem hiding this comment.
Maybe could you share an example of where is getLib being used? I haven't seen that before so I don't know how to use it
There was a problem hiding this comment.
There was a problem hiding this comment.
Based on the documentation, using that method would evaluate to ${pkgs.oo7-pam}-lib
There was a problem hiding this comment.
No, it would evaluate to whichever output contained /lib, assuming the derivation was set up correctly. Basically, if oo7-pam were made into a multi-output derivation, this module would break unless it used getLib.
There was a problem hiding this comment.
This is part of #526624, I don't think this should be done. If oo7-pam ever becomes a multi output derivation it can be changed when it happens.
it would evaluate to whichever output contained /lib
This is wrong, it will evaluate to the lib output of a package, if it doesn't contain it, it will fallback to the out output.
There was a problem hiding this comment.
it will evaluate to the lib output of a package, if it doesn't contain it, it will fallback to the out output.
"the output that contains /lib" and "the lib output if it exists otherwise the out output" are effectiely the same, but people don't use getLib because of the latter, they use it because of the former
There was a problem hiding this comment.
my point is i don't think we should be using it for a package that doesn't have the lib output. I thought this was the standard, but after a quick grep it is not, so it should be fine to do it, though I don't really like it
| services.gnome.gnome-keyring.enable = lib.mkDefault (lib.versionOlder lib.version "26.11"); | ||
| services.oo7.enable = lib.mkDefault (lib.versionAtLeast lib.version "26.11"); |
There was a problem hiding this comment.
If we're going to predicate this on a version after all, why don't we use stateVersion instead?
There was a problem hiding this comment.
I'm also thinking if this version keeping is really necessary? As this change will only be present on 26.11 (as that is where master is at) so the gnome-keyring condition will always be false. And I don't know if the oo7 module alongside this changes would be backported into 26.05
0dd4b61 to
f985386
Compare
|
I tested the COSMIC/greetd oo7 migration locally on NixOS 26.11 with Electron apps using The important runtime detail is that COSMIC login goes through the The Secret Service collection then remains locked after login: $ busctl --user get-property org.freedesktop.secrets /org/freedesktop/secrets/collection/Login org.freedesktop.Secret.Collection Locked
b trueA working local configuration required The tested working auth/session shape for After switching and logging in again through COSMIC/greetd, oo7 unlocked correctly and VS Code/Goofcord stopped freezing with So I think the |
f985386 to
d87ee02
Compare
|
Thanks @caniko for your testing! I did some tests to and noticed the same issue where the keyring was not unlocked upon login (although I couldn't replicate the electron apps issue). I'm working on this already |
d87ee02 to
518fb57
Compare
|
Hi @caniko, could you test again your use case with the latest changes? On my testing the login keyring is successfully migrated on first login, and subsequent logins unlock the keyring automatically |
|
After testing it seems the previous keyring is migrated to oo7 and removed from the system, so if the user rollbacks to gnome-keyring, they will not be able to access their secrets. Should I add this to the warning message? And also avoid motivating users to go back to gnome-keyring as I do in the current warning message? |
Would it be possible for you to add a test for this to |
Sure, I'll try to do it. I haven't done any kind of testing using python, but it doesn't seem that complex as this case can be verified with just logs |
|
@salva09 I recommend the following flow (that's how I did it):
Thank you for working on this and all the best! :) |
|
Fresh-login validation refined the failure:
The generic fix belongs with the oo7 service/package work in #526624. The upstream oo7 fix is server: Resolve aliases in set_locked. PR #539810 should remain focused on COSMIC and greetd integration. Assisted by: GPT-5.6 Luna |
|
Now, that the module is merged, I guess we should wait until oo7 next release, after looking at the GNOME issue tracker there are some bugs in 0.6.0 that are fixed in the main branch, including the bug @caniko found |
|
In the meantime, I'll see if I'm able to write a cosmic test for this change |
Could we just carry that patch? I feel like we should follow upstream COSMIC as much as possible and if they are using oo7 as default so should we. |
ee64aee to
d322c90
Compare
I don't think we need to rush it, the release should not take too long |
ninelore
left a comment
There was a problem hiding this comment.
Changing the keyring usually means breaking saved passwords, logins, etc.
Unless there is a absolutely airtight migration I don't think we should touch existing systems, meaning to put it behind stateVersion
|
Hi @ninelore, upon first login after switching to oo7, the daemon attempts to migrate v0 keyrings (gnome-keyring) to v1. And starting on oo7 0.7.0, after a successful migration, the previous keyring no longer will be deleted and instead will be renamed.
Are there any considerations we should take into account to decide if this migration mechanism is enough to make the change directly or put it behind a |
|
Now, in my opinion, by having a backup of old keyring, it should be fine to not gatekeep it. Probably worth of adding a release note for let users know that they can roll back the change if they need to. |
|
Out of curiosity: Hows support for the gcr-ssh-agent or is there an ewuivalent feature? |
|
I haven't tested oo7 yet, I am waiting for 0.7.0, but I guess it should still work fine |
|
I also haven't tested that yet as I don't use it, but I think it should work fine. We just need to explicitly enable it so people making use of it don't lose access to ssh keys without notice |
|
I mean, writing a nixosTest to check this and also catch if this changes would probably satisfy everyone, if anyone has the time to write such a thing. |
|
I have it running on my system with this patch; works quite well |
ninelore
left a comment
There was a problem hiding this comment.
cant dismiss my request for changes, so im just gonna approve
d322c90 to
e119c2d
Compare
e119c2d to
4dc1e59
Compare
This pr depends on from #526624
I would appreciate if I could have a double check on the warning message as english is not my main language, thanks!
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.