-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add keychain auth provider #3768
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
||
public protocol AuthorizationProvider { | ||
func authentication(for url: URL) -> (user: String, password: String)? | ||
mutating func addOrUpdate(for url: Foundation.URL, user: String, password: String, callback: @escaping (Result<Void, Error>) -> Void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the background to add the mutating aspect as part of the "Provider" protocol itself? do you see the downstream code (collections, registry, binary artifacts) needing to update the data (vs just reading it), or should this be a function of the concrete implementation only to allow CLI driven write operations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like having this mutating
either. The reason for it is because Netrc
's state changes with addOrUpdate
, and we need addOrUpdate
to support registry auth:
When credentials are provided, the corresponding object in the registries.json file includes a login key with the passed value. If the project's .netrc file has an existing entry for a given machine and login, it's updated with the new password; otherwise, a new entry is added. If no .netrc file exists, a new one is created and populated with the new entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this text is from SE-0292? which part of the code would be triggering the "write' in this case? I wonder maybe that code can interact with the Netrc utility directly to do write, and the provider protocol can stay readonly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what I am trying to say is that the "write" flow is not entirely clear to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this text is from SE-0292?
Yes
which part of the code would be triggering the "write' in this case?
In SwiftPackageRegistryTool I would think, and potentially add keychain as an option. cc @mattt
I guess what I am trying to say is that the "write" flow is not entirely clear to me
Same here, but it's clear that we'd need to support "write".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what I am trying to say is that the "write" flow is not entirely clear to me
Same here, but it's clear that we'd need to support "write".
I think for the sake of this PR, I would remove the write function from the provider API, but keep the implementation in the respective implementations, and then we can discuss with @mattt what he had in mind from a flow point of view. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like having this
mutating
either. The reason for it is becauseNetrc
's state changes withaddOrUpdate
, and we needaddOrUpdate
to support registry auth:
We decided to punt on this functionality for now (see #3647 (comment)). And we probably need to rethink this anyway if we're supporting both keychain and .netrc
, because it's unclear where those supplied credentials should be written.
I agree with @tomerd that we should remove the mutating function from the provider protocol. I have concerns about the plausibility of mutating .netrc
files so we may not want to keep that part, but the Keychain interaction code is solid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I've removed addOrUpdate
from the protocol.
one more thing to add here, or in a follow up PR is a |
First, bikeshedding the name... My main concern would be how to handle conflicts. For example, if we have a login
If we do 1 or 2, a user could pass I also worry that automatically pulling entries in the keychain could be too convenient, and lead to unexpected failures in production / CI environments. It's a kind of invisible global state that, without sufficient logging, the user may not even be aware of its existence. |
@@ -350,30 +350,13 @@ extension Workspace.Configuration { | |||
} | |||
|
|||
private static func load(_ path: AbsolutePath, fileSystem: FileSystem) throws -> AuthorizationProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workspace.Configuration.Netrc is designed to only concern itself with NetrcAuthorizationProvider
, so most of the code that was removed (moved to Basics really) here should be put back, and then in SwiftTool::getAuthorizationProvider
we would create the CompositeAuthorizationProvider
from the one returned from self.getNetrcConfig
+ the keychain one, i.e.
func getAuthorizationProvider() throws -> AuthorizationProvider? {
// currently only single provider: netrc
return try self.getNetrcConfig()?.get()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I undid the change in load
, but NetrcAuthorizationProvider
can be generic?
Quoting SE-0292:
If the user passes the --login and --password options to the set subcommand along with the --global option, the user-level .netrc file is updated instead. When Swift Package Manager connects to a custom registry, it first consults the project's .netrc file, if one exists. If no entry is found for the custom registry, Swift Package Manager then consults the user-level .netrc file, if one exists.
So potentially in SwiftTool::getAuthorizationProvider
we'd need to add another NetrcAuthorizationProvider
that points to the user-level netrc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amended 6cc7ece
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
making
NetrcAuthorizationProvider
generic makes sense -
I think the local vs. user netrc file logic could go into
func getNetrcConfig() -> Workspace.Configuration.Netrc? {
guard options.netrc else { return nil }
if let configuredPath = options.netrcFilePath {
guard localFileSystem.exists(configuredPath) else {
self.observabilityScope.emit(error: "Did not find .netrc file at \(configuredPath).")
return nil
}
return .init(path: configuredPath, fileSystem: localFileSystem)
} else {
let defaultPath = localFileSystem.homeDirectory.appending(component: ".netrc")
guard localFileSystem.exists(defaultPath) else { return nil }
return .init(path: defaultPath, fileSystem: localFileSystem)
}
}
which now loads the netrc by either a flag passed to the CLI or the user level, but this can be extended to also include the local case ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think that with this change Workspace.Configuration.Netrc
becomes redundant? any reason why not simply remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason why not simply remove it?
My original intent with this PR is to add keychain auth capability. I didn't plan to change existing behavior or cover new ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think that with this change Workspace.Configuration.Netrc becomes redundant?
Workspace.Configuration.Netrc
might be used elsewhere through libSwiftPM? It doesn't feel right to me that only SwiftTool
needs this logic.
I think the local vs. user netrc file logic could go into
That code is for SwiftTool
only and I assume SwiftPackageRegistryTool
will have to create its own AuthorizationProvider
in a similar way. I don't know if the logic for both tools is supposed to be the same, but my current understanding is that they are not.
AFAICT options.netrcFilePath
isn't necessarily a path within the workspace, so SwiftTool
uses either a custom netrc file location, which can be anywhere, or tries to find one in user's home directory. Regardless, it uses only one netrc file.
SwiftPackageRegistryTool
IIUC, looks for netrc file in both the workspace (same as project?) and user's home directory. i.e., it might use two netrc files.
If the tools were to share the same logic, then the code should be moved outside of SwiftTool
(to where?), and IMO this kind of logic doesn't belong in the tool/"UI" level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SwiftTool is a shared impl for all the CLI tools, so code there would be shared across them, but you are right that this is not needed to be dealt with in this PR - we can follow up with consolidating the logic as we make more progress
@swift-ci please smoke test |
Sources/Commands/SwiftTool.swift
Outdated
providers.append(workspaceNetrc) | ||
} | ||
#if canImport(Security) | ||
providers.append(KeychainAuthorizationProvider()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might consider adding a --no-keychain
flag to allow opt-out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a TODO and commented this out for now to avoid changing current behavior. I will create follow-up PR to consolidate auth logic.
@swift-ci please smoke test |
Motivation: Support keychain auth on macOS. Modifications: - Add `addOrUpdate` API to `AuthorizationProvider` protocol - Implement `KeychainAuthorizationProvider` - Move `NetrcAuthorizationProvider` to `Basics` and update conformance - Add tests rdar://83682028
@swift-ci please smoke test |
Motivation:
Support keychain auth on macOS.
Modifications:
addOrUpdate
API toAuthorizationProvider
protocolKeychainAuthorizationProvider
NetrcAuthorizationProvider
toBasics
and update conformancerdar://83682028