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.

Sign up for GitHub

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

automatically reset ssh sk permissions, unlock key #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

automatically reset ssh sk permissions, unlock key #23

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions scripts/ssh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
. "$(dirname "$0")/lib/init"

main() {
if [[ $(dd if=secrets/ssh/id_ed25519 bs=1 skip=1 | head -c 8) == "GITCRYPT" ]]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This process should already be automated. Did you git-crypt init?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't already automated - if you git-crypt lock it will not automatically unlock right now on the current version - and yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be part of the shellHook then, rather than part of a one-off SSH script. Or ideally, the user shouldn't git-crypt lock at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that continues the nix debate from #22 - which my point there was more that it doesn't make sense to nix-shell every time just to SSH. if you're gonna be terraforming something then yeah sure, but to just check on something quick via ssh isn't exactly a wonderful nix-shell use case. it's variable but it takes anywhere from 5 to 50 seconds on my laptop (NVMe, 4900hs, and connected to a fast and reliable network) to open a nix-shell when the packages have already been downloaded, for a quick ssh that's not ideal. as for not doing git-crypt lock, sure, although we should probably add another line to the README anyways saying to do git-crypt unlock (git-crypt init does not unlock the repo for me, only unlock does - init just prints a message saying the repo already uses git-crypt)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't make sense to nix-shell every time just to SSH

The way around this should be to use direnv, which automatically drops you into the Nix shell. In the future, more things may be required in the development environment to do anything at all, so I'm personally against supporting another way of interacting with this repo.

direnv also caches the shell, so entering the shell takes no time with it.

although we should probably add another line to the README anyways saying to do git-crypt unlock (git-crypt init does not unlock the repo for me, only unlock does - init just prints a message saying the repo already uses git-crypt)

This is certainly the odd behavior. A note in the README to run git-crypt unlock if the files aren't already unlocked would be a good idea, but git-crypt init, if played nicely, should already unlock the files automatically.

Tip

You can use the [!TIP] syntax to add a Tip box into the README for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when this is a pretty simple addition

This is true! It is a very simple addition, but said simple addition comes with the implication that we're either supporting a new way of interacting with the repository, or we're agreeing that it's a half-supported way to half-interact with it, which might very well be dropped in the near future (or even tomorrow for that matter).

With that, the question then becomes: is it worth it to support a new way of interacting with the repository when it comes with these costs? Is using Nix shell and/or Direnv that hard that it would justify the new way?

would it be better to classify this as a tip or as info though?

Either will work fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

said simple addition comes with the implication that we're either supporting a new way of interacting with the repository, or we're agreeing that it's a half-supported way to half-interact with it, which might very well be dropped in the near future (or even tomorrow for that matter)

I believe the latter would be true. There is a potential compromise solution: add chmod to the shellHook, and have instructions for non-nix-shell users. For instance, I currently have have the following exports in my zshrc, along with functions for faster ssh to 306/cirno and my home entry point:

# acmCSUF environment
set -o allexport
source ~/dev/acm-aws/.env
set +o allexport

function to306() {
  # ...
}

function tocirno() {
  # ...
}

function tosappho() {
  ssh [email protected]
}

With that, the question then becomes: is it worth it to support a new way of interacting with the repository when it comes with these costs? Is using Nix shell and/or Direnv that hard that it would justify the new way?

Is Direnv selectively enable-able? And is there a way we could automate Direnv setup in a non-destructive way for users if so? That's the only way I can see this being practical - I'd be fine using Direnv within the scope of acm-aws only, but due to hacky workarounds that may be present in other environments I would not want to use Direnv elsewhere. Ideally I also want to not create an even higher entry barrier for people - thin out the learning curve a bit, for instance.

Will it even be possible to ssh without nix-shell post-#21? I honestly don't know what the implications of transitioning to flakes are.

Either will work fine.

Alright - think if we move forward with that README addition, my recommendation would be info over tip; seems slightly closer to the intended point, plus (and this is purely subjective) info looks nicer than tip.

Also, on both my laptop (Fedora 39) and desktop (Arch), this is what init gets me (no decrypt):

❯ git-crypt init
Error: this repository has already been initialized with git-crypt.

In fact, for our use case - "decrypt this repo using the in-repo GPG-encrypted key" - the helppage says we should be using unlock, not init:

❯ git-crypt
Usage: git-crypt COMMAND [ARGS ...]

Common commands:
  init                 generate a key and prepare repo to use git-crypt
  status               display which files are encrypted
  lock                 de-configure git-crypt and re-encrypt files in work tree

GPG commands:
  add-gpg-user USERID  add the user with the given GPG user ID as a collaborator
  unlock               decrypt this repo using the in-repo GPG-encrypted key

I wonder if this could come from legacy git-crypt - since git-crypt init KEYFILE is listed as an alias for git-crypt unlock KEYFILE...

Copy link
Member

@diamondburned diamondburned Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it even be possible to ssh without nix-shell post-#21? I honestly don't know what the implications of transitioning to flakes are.

These are all great questions. I don't think I can answer them until the PR is finalized, and currently I'm just working on it. Everything could still be changed.

Issue #20 lists the upsides of migrating to Flakes, but Nix 4 (not real yet) is supposed to be Flakes-only, and a lot of core Nix people are adamant on encouraging use of Flakes, so it's best for us to try and heed that advice for maintainability.

Alright - think if we move forward with that README addition, my recommendation would be info over tip; seems slightly closer to the intended point, plus (and this is purely subjective) info looks nicer than tip.

This sounds good.

In fact, for our use case - "decrypt this repo using the in-repo GPG-encrypted key" - the helppage says we should be using unlock, not init

This makes sense. git-crypt's usage means we shouldn't even be worrying about having to decrypt things, because the git hooks are supposed to handle it. But the hooks are sometimes pretty fragile, e.g. they might not run until you do git status (unconfirmed), so this clarification is valid.

I believe git-crypt init is supposed to set up the git hooks, so it's weird that it would warn that it's "already been initialized" on the first run. If the hooks are missing, that would be pretty dangerous, since someone might try to push unencrypted secrets over.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all great questions. I don't think I can answer them until the PR is finalized, and currently I'm just working on it. Everything could still be changed.
Issue #20 lists the upsides of migrating to Flakes, but Nix 4 (not real yet) is supposed to be Flakes-only, and a lot of core Nix people are adamant on encouraging use of Flakes, so it's best for us to try and heed that advice for maintainability.

Alright, sounds good - how about we put this PR on hold until #21 is done?

I believe git-crypt init is supposed to set up the git hooks, so it's weird that it would warn that it's "already been initialized" on the first run. If the hooks are missing, that would be pretty dangerous, since someone might try to push unencrypted secrets over.

From my (albeit limited) experience the hooks appear to already be set in just fine - without doing git-crypt init it doesn't appear to push unencrypted secrets, but 🤷🏼‍♀️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, sounds good - how about we put this PR on hold until #21 is done?

SGTM.

git-crypt unlock
fi
# git doesn't save file mode properly - so we have to override it
# this is very common because it's also necessary after any git-crypt operation
chmod 600 secrets/ssh/id_ed25519
publicIP=$(./scripts/ip "$1")
ssh -o 'IdentitiesOnly=yes' -i ./secrets/ssh/id_ed25519 "root@${publicIP}"
}
Expand Down