Skip to content

fix(libstore): accept multi-@ SSH authorities in store URIs#15509

Open
znaniye wants to merge 1 commit intoNixOS:masterfrom
znaniye:feat/ssh-ng-uri-parsing
Open

fix(libstore): accept multi-@ SSH authorities in store URIs#15509
znaniye wants to merge 1 commit intoNixOS:masterfrom
znaniye:feat/ssh-ng-uri-parsing

Conversation

@znaniye
Copy link
Copy Markdown
Member

@znaniye znaniye commented Mar 17, 2026

Summary

  • Fix ssh-ng store URI parsing for the ShellHub addressing form, where device selection appears in userinfo (e.g. ssh-ng://freebotics@freedom.2c-cf-67-db-54-61@cloud.shellhub.io).
  • Before this change, StoreReference::parse failed early with Cannot parse Nix store ... because authorities with multiple @ were rejected unless inner @ were percent-encoded.
  • In practice, this forced ShellHub users to avoid direct URIs and rely on SSH indirection (NIX_SSHOPTS or ~/.ssh/config) to express the same target.
  • Add a lenient fallback only for SSH store schemes (ssh, ssh-ng, mounted-ssh-ng): treat the last @ as the userinfo/host separator and percent-encode earlier @ as %40.
  • Keep non-SSH schemes strict (e.g. https://user@service@example.org still throws UsageError).

Why this matters

  • ShellHub encodes device identity in an extended SSH userinfo form (user@device-id@gateway-host).
  • Supporting this directly in Nix store URIs removes the need for out-of-band SSH configuration workarounds while preserving strict URL parsing behavior outside SSH schemes.

@xokdvium
Copy link
Copy Markdown
Contributor

This has never worked (as in pre-2.31 too), right?

const static std::string userRegex = "(?:(?:" + unreservedRegex + "|" + pctEncoded + "|" + subdelimsRegex + "|:)*)";
const static std::string authorityRegex = "(?:" + userRegex + "@)?" + hostRegex + "(?::[0-9]+)?";

Not sure how I feel about making store reference syntax even more busted than it already is. What's wrong with just doing pct-encoding?

Comment thread src/libstore/store-reference.cc Outdated
@znaniye znaniye force-pushed the feat/ssh-ng-uri-parsing branch from 32e99bf to c4a72ce Compare March 18, 2026 15:17
@znaniye
Copy link
Copy Markdown
Member Author

znaniye commented Mar 18, 2026

This has never worked (as in pre-2.31 too), right?

Actually, before 2.31, we had some pretty cursed behavior. Nix wouldn't report any parsing error. Instead, when copying a closure to user@device@host, it failed to log in properly and fell back to a password prompt. When debugging with -vvv, we could see that the parser was prepending //, e.g:

debug1: Connecting to cloud.shellhub.io [5.161.34.26] port 22.
debug1: Connection established.
debug1: Local version string SSH-2.0-OpenSSH_10.0
debug1: Remote protocol version 2.0, remote software version Go
debug1: compat_banner: no match: Go
debug1: Authenticating to cloud.shellhub.io:22 as '//user@device'

Not sure how I feel about making store reference syntax even more busted than it already is. What's wrong with just doing pct-encoding?

That is indeed an option, but I though it would be nice if we could solve this on our side.

@xokdvium
Copy link
Copy Markdown
Contributor

That is indeed an option, but I though it would be nice if we could solve this on our side.

That is indeed nice, but what about any other characters? Are we going to special-case every exception we come across?

At least being consistent is somewhat sane. One particularly aggregious issue pre-2.31 was that one could specify hostnames with / (i.e. a "URL" with a path component) in them with everything before it being passed back in as-is, while everything after it was getting re-encoded with pct-encoding. This code is such a mess it's sad.

Move lenient authority normalization into libutil as an opt-in helper used by StoreReference parsing, replacing scheme-specific handling in libstore.

The fallback splits authority at the last '@', normalizes userinfo to RFC3986 form, preserves valid pct-encoding, rejects whitespace/control characters, and re-parses strictly. This keeps parseURL strict for other callers while handling legacy store references with multi-'@' userinfo.

Add libutil and libstore tests for multi-'@' authorities, generic invalid-character normalization, and whitespace rejection.
@znaniye znaniye force-pushed the feat/ssh-ng-uri-parsing branch from c4a72ce to 11cc196 Compare March 18, 2026 19:50
@znaniye
Copy link
Copy Markdown
Member Author

znaniye commented Mar 18, 2026

@xokdvium could you please review the rebased changes?
This is no longer a case-specific workaround.
We now apply a protocol-agnostic compatibility fallback that normalizes userinfo according to RFC3986 and then validates via strict parsing...

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