update: add HOMEBREW_SEQUENTIAL_FETCH environment variable #13469

notfromstatefarm · 2022-06-24T02:06:25Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

I'm in the process of rolling out U2F SSH keys in my organization. Long story short, you have to tap on a physical key whenever you want to push or pull Git repos with your SSH key. This also includes private taps inside brew.

I made adding the tap work with my previous PR #13447, but then I realized that brew update was broken for U2F SSH keys due to the parallelization of git fetches. Unfortunately requests to security keys don't queue up. Only one request can be active at a time and other requests will fail with device not found. Therefor, if you have more than one brew tap that requires SSH, it will fail.

❯ brew update
sign_and_send_pubkey: signing failed for ECDSA-SK "/Users/jake/.ssh/id_ecdsa_sk": device not found
git@github.com: Permission denied (publickey).
fatal: Could not read from remote repository.

In order to make U2F SSH work with brew, I've added a --sequential-fetch flag to brew update along with the HOMEBREW_SEQUENTIAL_FETCH environment variable. This will cause the git fetches to happen sequentially instead of in parallel. While obviously much slower, it's unfortunately the only option I can think of to make U2F SSH work with Homebrew.

Appreciate your review! ❤️

EDIT:

PR has been updated to only have an environment variable instead of a flag so we don't pollute the update command with edge cases

MikeMcQuaid

I'm not sure what I think about this, I'm afraid. I don't really like that for this single feature/user we're adding a new SSH flag and a new way of running brew update. Honestly it makes me tempted to revert the previous SSH PR if it doesn't meet your use-case.

I'm also worried that even this more invasive approach will not fix things for you.

Honestly, I think that using SSH for your private repositories and authenticating in this way is just not a good fit for Homebrew.

MikeMcQuaid · 2022-06-24T06:03:52Z

docs/Manpage.md

@@ -2201,6 +2203,9 @@ example, run `export HOMEBREW_NO_INSECURE_REDIRECT=1` rather than just
 - `HOMEBREW_DEBUG`
  <br>If set, always assume `--debug` when running commands.

+- `HOMEBREW_SEQUENTIAL_FETCH`


Do you want to set this or use the flag? Feels like we only need one.

I'm not sure what I think about this, I'm afraid. I don't really like that for this single feature/user we're adding a new SSH flag and a new way of running brew update. Honestly it makes me tempted to revert the previous SSH PR if it doesn't meet your use-case.

I'm also worried that even this more invasive approach will not fix things for you.

Honestly, I think that using SSH for your private repositories and authenticating in this way is just not a good fit for Homebrew.

I see where you're coming from - adding a new flag for a small use case is a fast road to polluting the update command with god knows what else. Perhaps, then, the environment variable is enough. My midnight thoughts were that this would change the flow enough to be worthy of a flag.

The reason I added the environment variable to the docs too is because I noticed that sometimes other brew commands will call brew update too, without the flag of course. Now that I think about it more, that being the case, an environment variable is really the appropriate thing here.

I think the use case is significant enough to support it in Brew. Hardware SSH keys are (in my opinion) the safest way to secure access to a private Git repo. GitHub thought so too and explicitly added support for it last year: https://github.blog/2021-05-10-security-keys-supported-ssh-git-operations/

Brew update was the only command I found that ran fetch in parallel, so I think this PR should just about cover it.

Would a refactor that only has the environment variable be acceptable @MikeMcQuaid?

Thank you again!

I think the use case is significant enough to support it in Brew. Hardware SSH keys are (in my opinion) the safest way to secure access to a private Git repo. GitHub thought so too and explicitly added support for it last year: https://github.blog/2021-05-10-security-keys-supported-ssh-git-operations/

Honestly, it sounds to me like this user experience is pretty awful (press button once per private repo every time update is run automatically or manually) and very niche (you're the only person I've heard of that wants this) so I'm pretty reluctant to add this unless other @Homebrew/maintainers feel very strongly about it being included.

I've made some reverts in #13483 as it sounds like the previous PR isn't useful to you as-is.

If I can be convinced by other maintainers this should be included the changes I think are necessary are:

add back the forced SSH env and perhaps make this env more specifically refer to hardware keys and set the forced SSH env too, so people don't need to set both

remove the flag here

ensure that it's tested sufficiently that another follow-up PR isn't needed

I would strongly advise not working on the above until you get a 👍🏻 from me, though, because I want to make sure you don't waste your time ❤️

Sorry about all this, I know it must be frustrating but I have to balance your use-case with keeping this code maintainable and supporting millions of Homebrew users.

dawidd6

Looks good to me though.

smridge · 2022-06-25T03:18:02Z

manpages/brew.1

+\fBHOMEBREW_SEQUENTIAL_FETCH\fR
+.
+.br
+If set, use sequential git fetching instead of fetching in parallel\.


I think it could be helpful to reference why this would be used. Keeping this easier to debug and resolve.

MikeMcQuaid · 2022-06-28T08:38:12Z

Looks good to me though.

@dawidd6 Just to be clear: do you want/need to use this?

dawidd6 · 2022-06-28T08:43:03Z

@dawidd6 Just to be clear: do you want/need to use this?

Nope

MikeMcQuaid

Waiting for more feedback.

notfromstatefarm · 2022-06-28T11:55:23Z

@MikeMcQuaid absolutely understandable. The user experience in this case is awful and it is a niche use case, I cannot disagree there. The U2F SSH keys were not designed for Git operations, obviously. But it is a great way to keep things secure against malicious software on a laptop.

Thank you for your review and consideration again!

maxim-belkin

I think this is a useful change. As someone working in a shared-root env (yes, I know), this would be useful for me. Now, the big question is - is there ANY way to make this code testable in a CI env?

maxim-belkin · 2022-06-28T15:39:26Z

Library/Homebrew/cmd/update.sh

+    then
+      do_fetch
+    else
+      do_fetch &


Does unconditional do_fetch ${HOMEBREW_SEQUENTIAL_FETCH:-&} not work?

If it does, you might be also able to replace & (in ) & with that Bash beauty instead of creating a function. But it is not critical.

I learned some fancy Bash today! That actually does work!

I tried without the function, no dice. Maybe I'm missing the correct syntax. But we can get rid of a few lines with that oneliner, at least.

Try this

( ... ) & ${HOMEBREW_SEQUENTIAL_FETCH:+fg}

I'm not a big fan of defining functions inside functions. Plus, we have to pay attention to variable scopes (by default, variables in a subshell are local whereas in a function only those defined with the local keyword are).

maxim-belkin · 2022-06-28T15:40:45Z

Library/Homebrew/env_config.rb

@@ -365,6 +365,10 @@ module EnvConfig
        description: "If set, always assume `--debug` when running commands.",
        boolean:     true,
      },
+      HOMEBREW_SEQUENTIAL_FETCH:                 {
+        description: "If set, use sequential git fetching instead of fetching in parallel.",


Maybe mention use case (Use with U2F FIDO)

carlocab · 2022-06-28T16:28:22Z

I don't have immediate plans to use this, but I am wondering:

Is there no way to get ssh to request authentication and then cache those credentials? That way we can do it before do_fetch, and also not have to do it serially for this to work.

notfromstatefarm · 2022-06-28T16:42:51Z

I don't have immediate plans to use this, but I am wondering:

Is there no way to get ssh to request authentication and then cache those credentials? That way we can do it before do_fetch, and also not have to do it serially for this to work.

I found a way to only have to click it once, by using SSH connection caching.

Host *
    ControlMaster auto
    ControlPath ~/.ssh/S.%r@%h:%p
    ControlPersist 30s

However, even when this is used, because of the parallelization it's not going to work. The first time you authorize, it's going to be trying to pull from multiple SSH repos simultaneously. Unless we just do one pull sequentially and the rest in parallel.

MikeMcQuaid · 2022-06-29T12:29:29Z

I think this is a useful change. As someone working in a shared-root env (yes, I know), this would be useful for me.

@maxim-belkin can you elaborate on how/why? Do you plan on using this personally?

Is there no way to get ssh to request authentication and then cache those credentials? That way we can do it before do_fetch, and also not have to do it serially for this to work.

This seems like a sensible train of investigation.

However, even when this is used, because of the parallelization it's not going to work. The first time you authorize, it's going to be trying to pull from multiple SSH repos simultaneously. Unless we just do one pull sequentially and the rest in parallel.

@notfromstatefarm It feels like a wrapper for Homebrew would enable this to be handled without any changes to Homebrew. You could setup these SSH connections in advance to cache them and then terminate after update. This could also avoid needing to use a custom SSH too.

maxim-belkin · 2022-06-30T06:20:49Z

@maxim-belkin can you elaborate on how/why? Do you plan on using this personally?

The use case I'm interested in is the so-called "shared root" environment, where multiple users (20+) have root access to the system. As a result, no one is safe against a potential malicious actor. With U2F SSH, one gets the missing layer of protection by having to press a button on a physical device to authorize use of SSH keys. I haven't thought about that before, but U2F SSH seems like a must for such environments. If one participates in a "shared root" environment as a root, connection cache is, actually, a problem since another root will be able to reuse the connection, which is not great from the security perspective.

I think the proposed change is useful, given that it is so small code-wise (once my suggestion above is implemented) -- basically, it changes

( ... ) &

to

( ... ) & ${HOMEBREW_SEQUENTIAL_FETCH:+fg}

I tested ( sleep 10 ) & ${PATH:+fg} and ( sleep 10 ) & ${XXX:+fg} and both work flawlessly for me.

MikeMcQuaid · 2022-06-30T07:37:40Z

The use case I'm interested in is the so-called "shared root" environment, where multiple users (20+) have root access to the system.

Sorry to be a stickler here but: is this just something you see as useful/interesting or something you have a specific machine you plan on using this on?

notfromstatefarm · 2022-06-30T11:56:58Z

@maxim-belkin I tried

) &
    ${HOMEBREW_SEQUENTIAL_FETCH:+fg} >/dev/null

and everything works, except you can't SIGINT/Ctrl+C it.

maxim-belkin · 2022-07-07T11:38:00Z

Sorry to be a stickler here but:

😄 No worries! I respect that.

is this just something you see as useful/interesting or something you have a specific machine you plan on using this on?

This is definitely more than just interesting to me and I have a system where I would like to use U2F FIDO but I have to investigate all the requirements of U2F FIDO as my personal needs are related to remote systems (the ones I SSH into).

At any rate, this feature makes Homebrew compatible with a more secure way of using SSH keys, so I think we should be discussing why wouldn't Homebrew want this. Yes, associated user experience is not great, but we all know that 'security' and 'user experience' are practically polar opposites.

I tried
) &
    ${HOMEBREW_SEQUENTIAL_FETCH:+fg} >/dev/null
and everything works, except you can't SIGINT/Ctrl+C it.

You might have to update the trap ... SIGINT statements on lines 539--540 (and 686) -- maybe wrap them in if statements. Same applies to the wait statement on line 685.

MikeMcQuaid · 2022-07-07T14:48:33Z

At any rate, this feature makes Homebrew compatible with a more secure way of using SSH keys, so I think we should be discussing why wouldn't Homebrew want this.

the user experience sucks
it's more code to maintain/document/support
Homebrew's private tap experience is not good and I don't particularly want to spend time or effort making it better when most taps could be public, even when distributing private resources
HTTPS works fine

github-actions · 2022-07-29T00:06:40Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

notfromstatefarm force-pushed the feature/sequential-flag-for-update branch 2 times, most recently from 02bbf95 to 920b4dd Compare June 24, 2022 04:37

notfromstatefarm changed the title ~~update: add sequential flag~~ update: add sequential-fetch flag Jun 24, 2022

update: add sequential-fetch flag

7165f1f

notfromstatefarm force-pushed the feature/sequential-flag-for-update branch from 920b4dd to 7165f1f Compare June 24, 2022 04:40

MikeMcQuaid reviewed Jun 24, 2022

View reviewed changes

remove sequential flag, only have env variable

c40da99

notfromstatefarm changed the title ~~update: add sequential-fetch flag~~ update: add HOMEBREW_SEQUENTIAL_FETCH environment variable Jun 24, 2022

notfromstatefarm requested a review from MikeMcQuaid June 24, 2022 14:11

dawidd6 approved these changes Jun 24, 2022

View reviewed changes

smridge reviewed Jun 25, 2022

View reviewed changes

MikeMcQuaid mentioned this pull request Jun 28, 2022

Remove HOMEBREW_FORCE_BREWED_SSH #13483

Merged

MikeMcQuaid requested changes Jun 28, 2022

View reviewed changes

maxim-belkin approved these changes Jun 28, 2022

View reviewed changes

github-actions bot added the stale No recent activity label Jul 29, 2022

github-actions bot closed this Aug 5, 2022

github-actions bot added the outdated PR was locked due to age label Sep 5, 2022

github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update: add HOMEBREW_SEQUENTIAL_FETCH environment variable #13469

update: add HOMEBREW_SEQUENTIAL_FETCH environment variable #13469

notfromstatefarm commented Jun 24, 2022 •

edited

MikeMcQuaid left a comment

MikeMcQuaid Jun 24, 2022

notfromstatefarm Jun 24, 2022 •

edited

MikeMcQuaid Jun 28, 2022

dawidd6 left a comment

smridge Jun 25, 2022

MikeMcQuaid commented Jun 28, 2022

dawidd6 commented Jun 28, 2022

MikeMcQuaid left a comment

notfromstatefarm commented Jun 28, 2022

maxim-belkin left a comment

maxim-belkin Jun 28, 2022

maxim-belkin Jun 28, 2022

notfromstatefarm Jun 28, 2022

notfromstatefarm Jun 28, 2022

maxim-belkin Jun 28, 2022

maxim-belkin Jun 28, 2022

carlocab commented Jun 28, 2022

notfromstatefarm commented Jun 28, 2022 •

edited

MikeMcQuaid commented Jun 29, 2022

maxim-belkin commented Jun 30, 2022

MikeMcQuaid commented Jun 30, 2022

notfromstatefarm commented Jun 30, 2022

maxim-belkin commented Jul 7, 2022

MikeMcQuaid commented Jul 7, 2022

github-actions bot commented Jul 29, 2022

update: add HOMEBREW_SEQUENTIAL_FETCH environment variable #13469

update: add HOMEBREW_SEQUENTIAL_FETCH environment variable #13469

Conversation

notfromstatefarm commented Jun 24, 2022 • edited

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notfromstatefarm Jun 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawidd6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Jun 28, 2022

dawidd6 commented Jun 28, 2022

MikeMcQuaid left a comment

Choose a reason for hiding this comment

notfromstatefarm commented Jun 28, 2022

maxim-belkin 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

carlocab commented Jun 28, 2022

notfromstatefarm commented Jun 28, 2022 • edited

MikeMcQuaid commented Jun 29, 2022

maxim-belkin commented Jun 30, 2022

MikeMcQuaid commented Jun 30, 2022

notfromstatefarm commented Jun 30, 2022

maxim-belkin commented Jul 7, 2022

MikeMcQuaid commented Jul 7, 2022

github-actions bot commented Jul 29, 2022

notfromstatefarm commented Jun 24, 2022 •

edited

notfromstatefarm Jun 24, 2022 •

edited

notfromstatefarm commented Jun 28, 2022 •

edited