pass command instead of password in zuliprc #1581

Gopinath-Mahendiran · 2025-04-10T11:22:11Z

What does this PR do, and why?

This PR updates the default config generation logic in zulip-term to support a more secure way of managing Zulip API keys. Instead of writing the API key directly into the zuliprc file, we now generate:
• a zuliprc file with a passcmd field
• a zulip_key file containing just the API key
• zulip_key: A temporary file that contains only the API key.
• zuliprc: [api]: contains Zulip login configuration (email, server URL, and now passcmd)

Here’s how the new auto-generated zuliprc file looks:

[api]
email=abc@gmail.com
# Fill the passcmd field with a command that outputs the API key.
# The API key is temporarily stored in the 'zulip_key' file.
# After storing the key in a password manager, please delete the 'zulip_key' file.
passcmd=cat zulip_key
site=https://chat.zulip.org

If you’re downloading a zuliprc file from your Zulip account (which includes the key field), you’re encouraged to manually replace the key line with a passcmd, like this:

passcmd=cat zulip_key

You can (and should) later replace cat zulip_key with a command tied to your preferred secret storage tool

Outstanding aspect(s)

[ ]

External discussion & connections

Discussed in #zulip-terminal in topic
Fully fixes pass command instead of password in zuliprc? #1502
Partially fixes issue #
Builds upon previous unmerged work in PR #
Is a follow-up to work in PR #
Requires merge of PR #
Merge will enable work on #

How did you test this?

Manually - Behavioral changes
Manually - Visual changes
Adapting existing automated tests
Adding automated tests for new behavior (or missing tests)
Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

It is a minimal coherent idea
It has a commit summary following the documented style (title & body)
It has a commit summary describing the motivation and reasoning for the change
It individually passes linting and tests
It contains test additions for any new behavior
It flows clearly from a previous branch commit, and/or prepares for the next commit

zulipbot · 2025-04-10T11:22:16Z

Hello @Gopinath-Mahendiran, it seems like you have referenced #1502 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #1502..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

Gopinath-Mahendiran · 2025-04-13T19:34:00Z

@neiljp,@dkwo review the pr and kindly give the feedback

neiljp

@Gopinath-Mahendiran It looks like you have the right idea here now 👍

I've left some inline comments, since there are a few adjustments still to be made, but it's easier to point them out in the code now.

We can include this only in Terminal, but as Tim said, you could add it to the general Zulip python library instead. That would of course affect where the initial changes are made - but we could still document how it works for those using it in Terminal as a follow-up here, or perhaps wrap options in command-line flags.

You've updated some test code to match the code, but where you've added code it'd be useful to have new tests to demonstrate the functionality. I may have missed that - the last commit is rather large and full of unrelated changes.

neiljp · 2025-04-15T04:09:01Z

zulipterminal/core.py

@@ -137,6 +139,25 @@ def raise_exception_in_main_thread(
        self._exception_info = exc_info
        self._critical_exception = critical
        os.write(self._exception_pipe, b"1")
+
+    def get_api_key(self,config_file: str) -> Optional[str]:


If we're including this in the Terminal app, this should be integrated into the existing code in run.py, where we handle elements in the config file.

Alternatively, as discussed on chat.zulip.org, you could add this to the zulip package in the python-zulip-api repository, since it could then load it transparently in the Client initializer when we use it in Terminal.

Moved the logic for retrieving the API key (get_api_key) to the python-zulip-api repository as part of PR #86

neiljp · 2025-04-15T04:17:05Z

zulipterminal/cli/run.py

-            f.write(f"[api]\nemail={login_id}\nkey={api_key}\nsite={server_url}")
+            f.write(f"[api]\nemail={login_id}\n# Fill the passcmd field with a command that outputs the API key.\n# The API key is temporarily stored in the 'zulip_key' file at {os.path.dirname(os.path.abspath(to_path))}.\n# After storing the key in a password manager, replace the cmd.\npasscmd=cat zulip_key\nsite={server_url}")


Automatic creation is likely to be a rare enough feature, that we can leave this element for now, but if the commit is tidied up we can move it to the end to more easily merge the earlier commit(s).

Removed the implementation responsible for automatic creation

neiljp · 2025-04-15T04:18:29Z

zulipterminal/cli/run.py

        return ""
    except FileExistsError:
        return f"zuliprc already exists at {to_path}"
    except OSError as ex:
        return f"{ex.__class__.__name__}: zuliprc could not be created at {to_path}"
+
+def _write_zulip_key(to_path: str, *, api_key: str) -> str:


This function is very similar to the function above. Instead of adding a near-duplicate, it is preferable to refactor the existing code and then use that new version for the feature commit.

refactored the function by removing duplicate-lines

neiljp · 2025-04-15T04:19:28Z

zulipterminal/cli/run.py

+    if not save_zulipkey_failure:
+        print(f"Generated API key saved at {zulip_key_path}")
+    else:
+        exit_with_error(save_zuliprc_failure)
+
+    if not save_zuliprc_failure :
+        print(f"Generated config file saved at {zuliprc_path}")
    else:
        exit_with_error(save_zuliprc_failure)


If this saved only one file, what would you expect to happen?

could you explain this in detail?

Does it make sense to (only) save one file, if there is an error saving the other?

Ensured that if an error occurs during the creation of one file, the other files are not created, maintaining consistency.

neiljp · 2025-04-15T04:20:34Z

README.md

@@ -257,6 +265,7 @@ transparency=disabled
 # editor: nano
 ```

+


Please check for unnecessary additions.

neiljp · 2025-04-15T04:58:14Z

README.md

+configuration conssist of two file:
+- zulip_key, file contains the api_key
+- zuliprc, file consist of login configurations
+
+The `zulip_key`contains only the api_key.
+


The zulip_key (or similar) file is intended to be optional, so that zuliprc files in a previous format (and current, from the web app) work correctly.

For that reason anything like this part should likely be further down.

Since this documents the way the zuliprc file works, whether only for the Terminal app, or for all apps using the python zulip package (when we use the updated package, if so), it should be clear that it refers to the [api] part of the config.

neiljp · 2025-04-15T05:00:19Z

README.md

-key=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
+passcmd=cat zulip_key


Since this needs to be optional, and if in Terminal only will be specific to us, it needs to be briefly documented in the sample file, and commented out - see how we document the Terminal options further below.

neiljp · 2025-04-15T05:13:15Z

README.md

@@ -207,6 +207,12 @@ to get the hang of things.

 ## Configuration


This last commit is not just a docs update. If there are changes to tidy other commits, they belong combined with those commits. That will ensure that all the linting and tests passes on each commit individually, rather than just over the branch.

If you have other changes, they belong in new commits specific to those improvements.

neiljp · 2025-04-15T05:13:27Z

pyproject.toml

+markers = [
+    "wsl: marks tests as WSL specific",
+    "quoted_content: marks tests dealing with quoted content rendering",
+]


This looks unrelated.

Gopinath-Mahendiran · 2025-04-16T06:39:46Z

I will work towards the changes

Gopinath-Mahendiran · 2025-04-25T06:09:16Z

@neiljp, updated the code as discussed. Let me know if it looks good.

Gopinath-Mahendiran added 2 commits April 10, 2025 14:59

core: Add logic to run psscmd and extract key.

7f9f798

setup: Add logic to write zuliprc and zulip_key.

6da7b0d

zulipbot added the size: L [Automatic label added by zulipbot] label Apr 10, 2025

Gopinath-Mahendiran changed the title ~~Gopinath mahendiran~~ pass command instead of password in zuliprc Apr 10, 2025

Gopinath-Mahendiran force-pushed the Gopinath-Mahendiran branch 2 times, most recently from c165d04 to 0e5ca88 Compare April 13, 2025 17:28

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Apr 13, 2025

Gopinath-Mahendiran force-pushed the Gopinath-Mahendiran branch from 0e5ca88 to 4ea856e Compare April 13, 2025 17:54

docs: Update README to explain passcmd usage in zuliprc.

a1132b8

Gopinath-Mahendiran force-pushed the Gopinath-Mahendiran branch from 4ea856e to a1132b8 Compare April 13, 2025 19:10

neiljp reviewed Apr 15, 2025

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback enhancement New feature or request area: config area: login labels Apr 15, 2025

Gopinath-Mahendiran mentioned this pull request Apr 24, 2025

Add function to process the passcmd for API key retrieval. zulip/python-zulip-api#866

Open

12 tasks

Gopinath-Mahendiran added 2 commits April 24, 2025 21:04

refactor: Remove redundant code in run.py.

6b126db

refactor: Move get_api_key functionality to python-zulip-api repo.

72e5e0d

Gopinath-Mahendiran force-pushed the Gopinath-Mahendiran branch 2 times, most recently from e516da9 to daba4fd Compare April 24, 2025 17:43

test: Update tests according to new changes.

c872d7d

Gopinath-Mahendiran force-pushed the Gopinath-Mahendiran branch from daba4fd to c872d7d Compare April 24, 2025 17:46

Gopinath-Mahendiran added 2 commits April 25, 2025 11:12

test: Update tests according to new changes.

2575028

chore: Remove unnecessary code and cleanup files.

828885a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pass command instead of password in zuliprc #1581

pass command instead of password in zuliprc #1581

Gopinath-Mahendiran commented Apr 10, 2025 •

edited

Loading

zulipbot commented Apr 10, 2025

Gopinath-Mahendiran commented Apr 13, 2025 •

edited

Loading

neiljp left a comment

neiljp Apr 15, 2025

Gopinath-Mahendiran Apr 24, 2025

neiljp Apr 15, 2025

Gopinath-Mahendiran Apr 24, 2025

neiljp Apr 15, 2025

Gopinath-Mahendiran Apr 24, 2025

neiljp Apr 15, 2025

Gopinath-Mahendiran Apr 20, 2025

neiljp Apr 20, 2025

Gopinath-Mahendiran Apr 24, 2025

neiljp Apr 15, 2025

neiljp Apr 15, 2025

neiljp Apr 15, 2025

neiljp Apr 15, 2025

neiljp Apr 15, 2025

Gopinath-Mahendiran commented Apr 16, 2025

Gopinath-Mahendiran commented Apr 25, 2025

		f.write(f"[api]\nemail={login_id}\nkey={api_key}\nsite={server_url}")
		f.write(f"[api]\nemail={login_id}\n# Fill the passcmd field with a command that outputs the API key.\n# The API key is temporarily stored in the 'zulip_key' file at {os.path.dirname(os.path.abspath(to_path))}.\n# After storing the key in a password manager, replace the cmd.\npasscmd=cat zulip_key\nsite={server_url}")

		@@ -207,6 +207,12 @@ to get the hang of things.

		## Configuration

pass command instead of password in zuliprc #1581

Are you sure you want to change the base?

pass command instead of password in zuliprc #1581

Conversation

Gopinath-Mahendiran commented Apr 10, 2025 • edited Loading

What does this PR do, and why?

Outstanding aspect(s)

External discussion & connections

How did you test this?

Self-review checklist for each commit

zulipbot commented Apr 10, 2025

Gopinath-Mahendiran commented Apr 13, 2025 • edited Loading

neiljp 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gopinath-Mahendiran commented Apr 16, 2025

Gopinath-Mahendiran commented Apr 25, 2025

Gopinath-Mahendiran commented Apr 10, 2025 •

edited

Loading

Gopinath-Mahendiran commented Apr 13, 2025 •

edited

Loading