Skip to content

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

Merged
merged 15 commits into from
Oct 5, 2021
Merged

Add keychain auth provider #3768

merged 15 commits into from
Oct 5, 2021

Conversation

yim-lee
Copy link
Contributor

@yim-lee yim-lee commented Sep 29, 2021

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


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)
Copy link
Contributor

@tomerd tomerd Sep 29, 2021

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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".

Copy link
Contributor

@tomerd tomerd Sep 30, 2021

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?

Copy link
Contributor

@mattt mattt Sep 30, 2021

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:

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.

Copy link
Contributor Author

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.

@tomerd
Copy link
Contributor

tomerd commented Sep 30, 2021

one more thing to add here, or in a follow up PR is a MultiAuthorizationProvider (in lack of a better name) that has an underlying array of AuthorizationProviders and when you call authentication(for url: URL) loops and fetches the auth info from its underlying. And so this is what we can "feed" the workspace, collections which means they dont need to care which provider the auth info came from. wdyt?

@mattt
Copy link
Contributor

mattt commented Sep 30, 2021

one more thing to add here, or in a follow up PR is a MultiAuthorizationProvider (in lack of a better name) that has an underlying array of AuthorizationProviders and when you call authentication(for url: URL) loops and fetches the auth info from its underlying. And so this is what we can "feed" the workspace, collections which means they dont need to care which provider the auth info came from. wdyt?

First, bikeshedding the name... CompositeAuthorizationProvider?

My main concern would be how to handle conflicts. For example, if we have a login mona for github.com in the keychain with one password, and a different password in a .netrc file, which of the following happens?

  1. Only the keychain is used
  2. Only the .netrc file is used
  3. Either the keychain or .netrc is used first, falling back on the other if that fails

If we do 1 or 2, a user could pass --no-keychain or --no-netrc (#3763), but that would be too coarse if credentials are spread equally across providers. If we do 3, we should provide the user a play-by-play with logging statements so that they can understand what's happening.

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.

@yim-lee
Copy link
Contributor Author

yim-lee commented Oct 2, 2021

@tomerd @mattt I added CompositeAuthorizationProvider which takes an array of AuthorizationProviders as argument. If multiple providers have credentials for a given URL, the first one found based on array ordering wins.

@@ -350,30 +350,13 @@ extension Workspace.Configuration {
}

private static func load(_ path: AbsolutePath, fileSystem: FileSystem) throws -> AuthorizationProvider {
Copy link
Contributor

@tomerd tomerd Oct 2, 2021

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()
    }

Copy link
Contributor Author

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.

Copy link
Contributor Author

@yim-lee yim-lee Oct 2, 2021

Choose a reason for hiding this comment

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

Amended 6cc7ece

Copy link
Contributor

@tomerd tomerd Oct 2, 2021

Choose a reason for hiding this comment

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

  1. making NetrcAuthorizationProvider generic makes sense

  2. 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 ?

Copy link
Contributor

@tomerd tomerd Oct 2, 2021

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

@yim-lee yim-lee Oct 2, 2021

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.

Copy link
Contributor

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

@yim-lee
Copy link
Contributor Author

yim-lee commented Oct 2, 2021

@swift-ci please smoke test

providers.append(workspaceNetrc)
}
#if canImport(Security)
providers.append(KeychainAuthorizationProvider())
Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@yim-lee
Copy link
Contributor Author

yim-lee commented Oct 4, 2021

@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
@yim-lee
Copy link
Contributor Author

yim-lee commented Oct 4, 2021

@swift-ci please smoke test

@yim-lee yim-lee merged commit 3874cdc into swiftlang:main Oct 5, 2021
@yim-lee yim-lee deleted the auth-provider branch October 5, 2021 00:38
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