Skip to content
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

Identities offers both key and certificate when both are present #454

Merged
merged 3 commits into from
Mar 13, 2023

Conversation

chriseldredge
Copy link
Contributor

@chriseldredge chriseldredge commented Mar 11, 2023

This PR modifies Agent.identities() to return certificates in addition to keys, rather than returning only the certificate. This makes Secretive behave similarly to OpenSSH ssh-agent and addresses difficulties using the agent in scenarios where a private key can be used for authentication against a given remote server directly without the certificate.

Fixes #452.

@maxgoedjen
Copy link
Owner

@chriseldredge nice – thanks!

I'm not personally a certs user so I don't have direct experience in the mechanics of this one – is it correct to returns they key identities before the certs ones? My intuition would be that the certs should be offered first, but I'm really not sure there.

@chriseldredge
Copy link
Contributor Author

I’m not an expert either but can observe what happens in my configuration. As noted in #452, OpenSSH lists the public key first, so I mimicked that order.

I suspect this doesn’t matter much in practice, as most servers will either accept the certificate or the public key, usually not both, and in either situation the same private key gets used anyway.

@maxgoedjen
Copy link
Owner

I’m not an expert either but can observe what happens in my configuration. As noted in #452, OpenSSH lists the public key first, so I mimicked that order.

Good enough for me, thanks!

@maxgoedjen maxgoedjen enabled auto-merge (squash) March 13, 2023 00:42
@maxgoedjen maxgoedjen merged commit 0944d65 into maxgoedjen:main Mar 13, 2023
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.

Support for certificates and TrustedUserCAKeys
2 participants