Skip to content

Conversation

@eddyashton
Copy link
Member

This util script makes heavy use of pipes (to avoid leaking intermediate secret values), but as a result the set -e is pretty useless.

For instance if you try to talk to a server that doesn't exist, it looks like you've blown up symcrypt:

# ./submit_recovery_share.sh https://127.0.0.1:8000 ... <other args>
curl: (7) Failed to connect to 127.0.0.1 port 8000 after 0 ms: Could not connect to server
Public Key operation error
4047F1AE73710000:error:41800069:SCOSSL::SCOSSL triggered SymCrypt failure:/usr/src/azl/BUILD/SymCrypt-OpenSSL-1.9.3/ScosslCommon/src/scossl_rsa.c:635:SymCryptRsaOaepDecrypt failed - SYMCRYPT_INVALID_ARGUMENT (0x800e)

But actually you failed way earlier, while trying to get a member's share, and just swallowed that error and passed some junk error message to openssl -decrypt.

Fix is simple - add set -o pipefail, so we fail early:

# ./submit_recovery_share.sh https://127.0.0.1:8000 ... <other args>
curl: (7) Failed to connect to 127.0.0.1 port 8000 after 0 ms: Could not connect to server

We could do the same in other scripts using set -e, but I haven't found any also relying on pipes, so have opted not to.

Copilot AI review requested due to automatic review settings January 7, 2026 13:45
@eddyashton eddyashton requested a review from a team as a code owner January 7, 2026 13:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves error handling in the submit_recovery_share.sh script by adding set -o pipefail to ensure that failures in piped commands are properly detected. Without this setting, the script's existing set -e directive would not catch errors occurring in the middle of pipe chains, leading to misleading error messages when earlier commands fail.

  • Adds set -o pipefail directive to catch pipeline failures early
  • Prevents silent failures in the extensive use of pipes for handling secret values
  • Improves error reporting by failing at the actual point of failure rather than downstream

@eddyashton eddyashton enabled auto-merge (squash) January 7, 2026 14:19
@achamayou
Copy link
Member

Perhaps #7559 will help prevent this in the future.

@eddyashton eddyashton merged commit 243593a into microsoft:main Jan 7, 2026
20 of 21 checks passed
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.

4 participants