Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove checking for parent / grandparent process #299

Merged
merged 2 commits into from
Nov 1, 2024

Conversation

soraxas
Copy link
Contributor

@soraxas soraxas commented Oct 28, 2024

This is just a proposal, where is it needed to check for parent & grand parent process (for cache)?

This is is different than the standard ssh-agent process where ssh-add ~/.ssh/id_xxx will adds the key to memory, and then it works in other terminals (I don't think it checks for parent process).

I understand that perhaps this approach allows for more fine-grain control of prompting request. However, it doesn't quite works right most of the time. E.g.

  1. In plain ssh my_host the cache works well. But as soon as there are jump-host the cache no longer works
    • i.e., If your ~/.ssh/config has a host_b that jumps through host_a, it prompts for approval every time (probably because it spawns another ssh process, which makes parentProcess having different PID)
    • Reducing to JUST checking for grandParentProcess PID works, but then it fails again if we ssh to host_c via host_b via host_a, and so on.
  2. My git pull does not cache as well, where in the goldwarden log it shows
    [INF] [...] [Goldwarden > SSH] >>> SSH Agent connection from fish>git>git
    so perhaps the git is using some subprocess, which causes checking of parent process fails
  3. Again, even if we revert to checking for grandParentProcess, the above fails once we are using some wrapper process. I'm using yadm to manage my dotfile (which wraps git), and in goldwarden the process shows
    [INF] [...] [Goldwarden > SSH] >>> SSH Agent connection from yadm>git>git
    i.e., Checking for grandParentProcess will no longer cache the session.
  4. Essentially, the intended cache logic can probably works if we walk the process hierarchical structure by getting their parents until we reach some interactive terminal, but I reckon that'd be too fragile.

@soraxas soraxas marked this pull request as ready for review October 28, 2024 22:24
@soraxas soraxas marked this pull request as draft October 28, 2024 22:25
@quexten
Copy link
Owner

quexten commented Oct 30, 2024

Yeah, this was an experimental feature, but I've also found it unreliable, but don't know a better way to determine a program with an associated terminal or GUI cross platform. That's what I would have liked to cache by, and that is what 1Password does (I think).

I'm thinking of just moving to caching session wide to be honest, toggleable via an environment variable.

@quexten
Copy link
Owner

quexten commented Oct 30, 2024

Great analysis and thank you for the PR!

@quexten
Copy link
Owner

quexten commented Oct 30, 2024

^ I think the bypass should just be for ssh sessions for now, not vault access / pin entry. Other than that, I'm happy to merge.

@soraxas soraxas marked this pull request as ready for review October 31, 2024 23:59
@soraxas
Copy link
Contributor Author

soraxas commented Oct 31, 2024

^ I think the bypass should just be for ssh sessions for now, not vault access / pin entry. Other than that, I'm happy to merge.

Yep the new commit should accounts for that

@quexten quexten merged commit 490dfae into quexten:main Nov 1, 2024
10 of 11 checks passed
@soraxas soraxas deleted the feat/more-generic-cache branch November 3, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants