Add support for keyring password storage #29

tedwardd · 2023-06-16T19:08:14Z

Add support for secure password storage in system keyring. Addresses #20

First pass just adds keyring support for Lemmy but it's a repeatable pattern. Might be better to consolidate in to a global method we can call to clean it up a little... might do that before merging...

tedwardd · 2023-06-16T19:17:35Z

I encountered a merge conflict with go.mod and go.sum... I did my best to resolve the conflict but I fear I may have goofed. Please test and make sure you can still build using these new files.

benonions · 2023-06-17T00:25:50Z

For what it's worth, I tested this branch locally (on MacOS) and worked great for me!

tedwardd · 2023-06-17T00:33:59Z

That's excellent news, thank you! I need someone with Windows and someone with a Linux system (that has a keyring installed) to test it. I may be able to test a Linux keyring (I don't currently have one installed) but I do not have a Windows system available at this time.

benonions · 2023-06-17T00:35:47Z

That's excellent news, thank you! I need someone with Windows and someone with a Linux system (that has a keyring installed) to test it. I may be able to test a Linux keyring (I don't currently have one installed) but I do not have a Windows system available at this time.

I might be able to test Ubuntu a little bit later, I've got it installed on a VM.

tedwardd · 2023-06-17T01:06:38Z

Do not attempt to build df1594d, it is broken, pushed only to sync end-of-day changes (I'll rebase this whole mess before merging).

benonions · 2023-06-17T01:16:58Z

Sadly didn't go as well on Ubuntu 23.04 with Gnome keyring.

Was able to log in and add the password to keyring, but starting neonmodem after that crashes.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x80 pc=0x8f194c]

goroutine 1 [running]:
github.com/mrusme/neonmodem/cmd.loadSystems(0x40001880a0)
	/home/ben/neonmodem/cmd/root.go:105 +0x9c
github.com/mrusme/neonmodem/cmd.glob..func1(0x138f240?, {0xa6a043?, 0x0?, 0x0?})
	/home/ben/neonmodem/cmd/root.go:122 +0xb4
github.com/spf13/cobra.(*Command).execute(0x138f240, {0x4000032240, 0x0, 0x0})
	/home/ben/go/pkg/mod/github.com/spf13/cobra@v1.7.0/command.go:944 +0x5ac
github.com/spf13/cobra.(*Command).ExecuteC(0x138f240)
	/home/ben/go/pkg/mod/github.com/spf13/cobra@v1.7.0/command.go:1068 +0x340
github.com/spf13/cobra.(*Command).Execute(...)
	/home/ben/go/pkg/mod/github.com/spf13/cobra@v1.7.0/command.go:992
github.com/mrusme/neonmodem/cmd.Execute(0x95a980?)
	/home/ben/neonmodem/cmd/root.go:140 +0x4c
main.main()
	/home/ben/neonmodem/neonmodem.go:13 +0x24

Went over it with delve, looks like system.New() returns a nil system interface here

The error is that The specified item could not be found in the keyring. However, I looked at the keyring myself and it's there. So not sure what the dealio is.

tedwardd · 2023-06-17T15:39:44Z

OK, after sleeping on it I have simpliefied the whole workflow with a fresh perspective. I've confirmed that it works on Linux without a keyring present and on MacOS with the MacOS keychain.

tedwardd · 2023-06-17T18:47:52Z

common/common.go

+
+	// Open system keyring object
+	ring, _ := keyring.Open(keyring.Config{
+		ServiceName: "NeonModem - Lemmy",


So that this is reusable, the keyring entry should probably be the url for the system being added.

Unfortunately it is a little bit more complicated than that. Check the following helpful interaction I had with the KeePassX guys a while ago: keepassxreboot/keepassxc#8475

tedwardd · 2023-06-17T18:50:17Z

common/common.go

+	// Attempt to save the password to system keyring
+	// If we can't, ask if should save it in clear text
+	err = ring.Set(keyring.Item{
+		Key:  "password",


Just thinking out loud... maybe we should also store the username in the keyring. I'm not super familiar with keyring integration on different systems but if we were using the system url AND we had username and password stored in the keyring, maybe we could check if the user already has their credentials saved for the url (from a web browser, for example)... thinking about how password managers do it... maybe food for later thought.

Yes, indeed, the complete login can be stored in the keyring.

tedwardd · 2023-06-17T18:51:49Z

common/common.go

+func SetPassword() (string, *PasswordError) {
+	// Prompt for password input
+	fmt.Println("Please enter your password (will not echo): ")
+	bytepw, err := term.ReadPassword(int(syscall.Stdin))


Will check to see if the promptui package has a feature for a no-echo prompt so this can be simplified and have a consistent feel to the yes/no prompt when a supported keyring is not available.

I believe bubbletea already has some bubbles that might accomplish what you're looking to do on the same Bubbletea basis that neonmodem is already using. :)

tedwardd · 2023-06-17T18:53:26Z

system/lemmy/lemmy.go

@@ -144,9 +145,25 @@ func (sys *System) Load() error {
 		credentials[k] = v.(string)
 	}

+	ring, _ := keyring.Open(keyring.Config{


Only initialize this if we find the password should be in the keyring below.

mrusme

Hey @tedwardd, thank you for taking the initiative, I appreciate it!

The keyring stuff seems a bit fiddly and I'm mainly worried that we might overcomplicate and thereby break stuff for individual platforms on which there is no keyring implementation available or for users that might not require it.

Hence my suggestion would be build an abstraction on top of the keyring library -- e.g. credentials.go -- and allow each system to use the Credentials service to store and retrieve credentials. The Credentials service in turn would implement different backends, one of which is the keyring library, the other one the plaintext config; Another one could be a command runner (e.g. for using pass ...). These backends would reside in dedicated files, which we could prefix with the respective // +build tag for Go. That way we wouldn't include the feature/dependency on platforms on which it isn't available anyway.

Wdyt?

mrusme · 2023-06-21T18:57:36Z

common/common.go

+func SetPassword() (string, *PasswordError) {
+	// Prompt for password input
+	fmt.Println("Please enter your password (will not echo): ")
+	bytepw, err := term.ReadPassword(int(syscall.Stdin))


I believe bubbletea already has some bubbles that might accomplish what you're looking to do on the same Bubbletea basis that neonmodem is already using. :)

mrusme · 2023-06-21T19:00:06Z

common/common.go

+
+	// Open system keyring object
+	ring, _ := keyring.Open(keyring.Config{
+		ServiceName: "NeonModem - Lemmy",


Unfortunately it is a little bit more complicated than that. Check the following helpful interaction I had with the KeePassX guys a while ago: keepassxreboot/keepassxc#8475

mrusme · 2023-06-21T19:00:45Z

common/common.go

+	// Attempt to save the password to system keyring
+	// If we can't, ask if should save it in clear text
+	err = ring.Set(keyring.Item{
+		Key:  "password",


Yes, indeed, the complete login can be stored in the keyring.

mrusme · 2023-06-21T19:03:01Z

common/common.go

+	})
+	if err != nil {
+		fmt.Println("Unable to save password to a keyring. Would you like to proceed to save the password in clear text in the neonmodem.toml?")
+		if resp, _ := YesNo(); resp != true {


This seems to be the only instance of YesNo, hence I'd probably even use scanner.Scan() in order to keep dependencies low. See the connect.go files of the systems for example.

mrusme · 2023-06-21T19:07:09Z

system/lemmy/lemmy.go

+
+	var password string
+
+	if credentials["password"] == "password_in_keyring" {


It might make sense to store the keyring identifier, prefixed with a special preamble/symbol here, so that it can reference a specific entry rather than just say password_in_keyring, e.g. @lemmy_myaccount123, where lemmy_myaccount123 is the identifier of the keyring item. However, this depends on the implementation of the keyring, which is yet tbc.

tedwardd added 4 commits June 16, 2023 21:04

Add Keyring Support

686f4d3

Update go modules

7e5a483

Add support for fetching password from keyring for lemmy

d7b2cb7

sync local changes upstream

df1594d

tedwardd force-pushed the keyring branch from 7adb7e8 to df1594d Compare June 17, 2023 01:04

tedwardd added 2 commits June 17, 2023 10:37

update go modules for new code

461fe33

Simplify common keyring code for system password

32ca96c

Clean up error handling for password validation

6013e82

tedwardd commented Jun 17, 2023

View reviewed changes

tedwardd mentioned this pull request Jun 19, 2023

Fix segfault when entering incorrect password #33

Merged

mrusme reviewed Jun 21, 2023

View reviewed changes

mrusme assigned tedwardd Jun 21, 2023

mrusme marked this pull request as draft June 21, 2023 19:23

mrusme mentioned this pull request Jul 12, 2023

changed how password is stored #44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for keyring password storage #29

Add support for keyring password storage #29

tedwardd commented Jun 16, 2023 •

edited

tedwardd commented Jun 16, 2023

benonions commented Jun 17, 2023

tedwardd commented Jun 17, 2023

benonions commented Jun 17, 2023

tedwardd commented Jun 17, 2023 •

edited

benonions commented Jun 17, 2023

tedwardd commented Jun 17, 2023

tedwardd Jun 17, 2023

mrusme Jun 21, 2023

tedwardd Jun 17, 2023

mrusme Jun 21, 2023

tedwardd Jun 17, 2023

mrusme Jun 21, 2023

tedwardd Jun 17, 2023

mrusme left a comment

mrusme Jun 21, 2023

mrusme Jun 21, 2023

mrusme Jun 21, 2023

mrusme Jun 21, 2023

mrusme Jun 21, 2023


		var password string

		if credentials["password"] == "password_in_keyring" {

Add support for keyring password storage #29

Are you sure you want to change the base?

Add support for keyring password storage #29

Conversation

tedwardd commented Jun 16, 2023 • edited

tedwardd commented Jun 16, 2023

benonions commented Jun 17, 2023

tedwardd commented Jun 17, 2023

benonions commented Jun 17, 2023

tedwardd commented Jun 17, 2023 • edited

benonions commented Jun 17, 2023

tedwardd commented Jun 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrusme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tedwardd commented Jun 16, 2023 •

edited

tedwardd commented Jun 17, 2023 •

edited