Implement .netrc support#25
Conversation
NotAShelf
left a comment
There was a problem hiding this comment.
Thank you for the PR and the AI disclosure. I've left one quick comment. Other than this please:
- Reword your commits to patch the repository conventions (scoped commits)
- Add a quick section under documentation about the usage.
Load credentials from ~/.netrc, or from the path in NETRC when set. For upstreams without username in config, match credentialsby hostname and fall back to an optional default entry.
|
Restructured commit messages, added .netrc note under Upstream Authentication README section. I did not include an example of the netrc file, but I'll add it if needed |
| ) { | ||
| for upstream in upstreams { | ||
| // Skip s3 and explicitly configured credentials | ||
| if !upstream.username.is_empty() || upstream.s3.is_some() { |
There was a problem hiding this comment.
The guard (specifically !upstream.username.is_empty()) skips netrc lookup only when username is set. If only password is explicitly configured (no username), netrc will silently overwrite it anyway. We should also check upstream.password.is_some().
There was a problem hiding this comment.
Adding an upstream.password.is_some() guard here would result in effectively empty credentials, since password-only (or username-less) credentials are being filtered out at:
The docs and the upstream struct also imply that password is optional while username is required (for auth, at least).
I didn't add a password check initially because password-only configs were silently ignored before. I think it'd be better to issue a warning here and fall back to the username/password from .netrc to at least try to authenticate.
Or did you mean that an existing password shouldn't be overwritten by .netrc, and should instead give precedence to the one set in the config?
Please note, despite all my tries, I suck at rust and rust code was written with Claude 4.8 assistance. I cleaned excessive comments, simplified some functions it overengineered a bit and to my eye code looks reasonable.
I tested it myself and currently using this fork on my configuration.
Implements #23