Update auth logic #3789
Update auth logic #3789
Conversation
yim-lee
requested review from
abertelrud,
friedbunny,
neonichu and
tomerd
as code owners
|
haha, I also worked on some of this with #3790 happy to take yours and close mine |
Motivation:
SwiftPM should support using credentials stored in keychain for binary
download and registry authentication.
Modifications:
- Wire up `KeychainAuthorizationProvider` in `SwiftTool`. Add `--no-keychain` option which allows user to opt-out.
- Update netrc logic to either:
- Load from a user-defined `.netrc` file
- Default to `.netrc` file found in current workspace (if any) and/or
user's home directory.
- Deprecate `Workspace.Configuration.Netrc`
rdar://83682028
|
Rebased to include #3790 changes and resolved all review comments. |
|
@swift-ci please smoke test |
|
|
||
| return providers.isEmpty ? .none : CompositeAuthorizationProvider(providers, observabilityScope: self.observabilityScope) | ||
| } | ||
|
|
||
| func getNetrcConfigFile() throws -> AbsolutePath? { | ||
| func getNetrcAuthorizationProviders() throws -> [NetrcAuthorizationProvider] { |
There was a problem hiding this comment.
For .netrc files, I think that's a good option, yeah.
Alternatively, we could say:
- If you specify a
--netrc-path, use only that - Otherwise, consult project local (
./.netrc) and fallback to the user shared (~/.netrc)
This approach would give us the convenience of cascading, while being able to keep the behavior you described (for example, if we only wanted to load the local or shared .netrc file, we could pass it as --netrc-file).
The only downside is that this is more complex, and lacking sufficient diagnostics, could make it difficult for the user to understand where credentials are coming from.
There was a problem hiding this comment.
personally as a use I would expect the former (ie only load a single file, no fallback / merge with shared one). maybe we can investigate what other tools are doing?
There was a problem hiding this comment.
also cc @neonichu @abertelrud for opinions
There was a problem hiding this comment.
looks like curl only supports single location, either one specified with a flag or ~/.netrc: https://everything.curl.dev/usingcurl/netrc#the-netrc-file-format
I wonder if this is a good policy for us to due to security reasons - so users don't want to accidentally check in the .netrc file
@robertlacroix @andyp-apple opinions
There was a problem hiding this comment.
I wonder if this is a good policy for us to due to security reasons - so users don't want to accidentally check in the
.netrcfile
FWIW, we added .netrc to the default project template .gitignore file with #3511, which helps mitigate this.
There was a problem hiding this comment.
Alternatively, we could say:
- If you specify a --netrc-path, use only that
- Otherwise, consult project local (./.netrc) and fallback to the user shared (~/.netrc)
This is my understanding of the expected behavior described in SE-0292 and what I think I've implemented in this PR. My assumption is that local netrc would not have as many credentials as the user one (if both files exist), so for a given URL if credentials is not found in local netrc then looking for it in user netrc sounds reasonable to me.
It sounds like based on @tomerd and @mattt's comments that a single netrc file is fine. If that's the case I will remove this part of the changes and reduce the PR to simply add keychain auth.
There was a problem hiding this comment.
@yim-lee yes what you did is per spec, and correct. my question is more conceptual: if we got the spec right or if that would surprise users. there is also the option of not supporting project local at all IMO. in any case, I will leave it to @mattt to decide as he is the proposal author, unless we hear from the security folks something stronger.
There was a problem hiding this comment.
| @testable import Commands | ||
| import SPMTestSupport | ||
| import TSCBasic | ||
| import XCTest | ||
|
|
||
| final class SwiftToolTests: XCTestCase { | ||
| func testNetrcLocations() throws { | ||
| func testNetrcAuthorizationProviders() throws { |
|
@yim-lee good to merge? |
| do { | ||
| let localPath = packageRoot.appending(component: ".netrc") | ||
| // make sure there isn't a user home one | ||
| try localFileSystem.removeFileTree(userHomePath) |
There was a problem hiding this comment.
The tests probably shouldn't touch user's .netrc file
Motivation:
SwiftPM should support using credentials stored in keychain for binary
download and registry authentication.
Modifications:
KeychainAuthorizationProviderinSwiftTool. Add--no-keychainoption which allows user to opt-out..netrcfile.netrcfile found in current workspace (if any) and/oruser's home directory.
Workspace.Configuration.Netrcrdar://83682028