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

Implement RFD 18: Agent Loading #5825

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Implement RFD 18: Agent Loading #5825

merged 1 commit into from
Mar 17, 2021

Conversation

xacrimon
Copy link
Contributor

@xacrimon xacrimon commented Mar 3, 2021

This PR implements RFD 18: Agent Loading.

This fixes #3169 and #4863.

@xacrimon xacrimon requested review from awly and russjones March 3, 2021 16:13
@xacrimon xacrimon self-assigned this Mar 3, 2021
@xacrimon xacrimon force-pushed the joel/rfd-18-impl branch 5 times, most recently from bc0fc7b to be43d80 Compare March 3, 2021 22:08
@xacrimon xacrimon requested a review from a-palchikov March 3, 2021 22:09
@xacrimon xacrimon force-pushed the joel/rfd-18-impl branch 2 times, most recently from 42a0c1a to 201f01d Compare March 5, 2021 17:03
lib/client/api.go Outdated Show resolved Hide resolved
lib/client/keyagent.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
lib/client/keyagent.go Outdated Show resolved Hide resolved
Copy link
Contributor

@awly awly left a comment

Choose a reason for hiding this comment

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

Please add test coverage for LocalKeyAgent changes

lib/client/keyagent.go Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
@xacrimon xacrimon requested review from awly and a-palchikov March 5, 2021 20:36
@xacrimon
Copy link
Contributor Author

xacrimon commented Mar 5, 2021

Please add test coverage for LocalKeyAgent changes

Added tests for my reworked non-leaky implementation.

lib/client/api.go Outdated Show resolved Hide resolved
lib/client/keyagent.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
lib/client/keystore.go Outdated Show resolved Hide resolved
@xacrimon xacrimon requested a review from awly March 5, 2021 21:41
Copy link
Contributor

@awly awly left a comment

Choose a reason for hiding this comment

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

LGTM
Just to confirm: did you test the different options against both gpg-agent and regular ssh-agent, and without a local agent at all?

@xacrimon
Copy link
Contributor Author

xacrimon commented Mar 8, 2021

Further testing is on my TO-DO today. I've only performed some light testing.

@xacrimon xacrimon force-pushed the joel/rfd-18-impl branch 3 times, most recently from 3dc1b84 to 11f12a0 Compare March 9, 2021 18:38
@xacrimon
Copy link
Contributor Author

xacrimon commented Mar 9, 2021

Tested now, run into one error and pushed a small patch to fix it. Requested a re-review. Could you also take a look @webvictim ?

@xacrimon xacrimon requested review from webvictim and awly March 9, 2021 21:10
@xacrimon
Copy link
Contributor Author

xacrimon commented Mar 9, 2021

I've manually tested all combinations of agents and flag values and everything seems to work according to RFD specifications now.

@xacrimon
Copy link
Contributor Author

xacrimon commented Mar 9, 2021

hmm, still some profile loading peculiaries with the -i flag, will investigate.

lib/client/keyagent.go Outdated Show resolved Hide resolved
lib/client/api.go Show resolved Hide resolved
lib/client/keyagent.go Outdated Show resolved Hide resolved
lib/client/keyagent.go Outdated Show resolved Hide resolved
lib/client/keyagent.go Outdated Show resolved Hide resolved
lib/client/keystore_test.go Outdated Show resolved Hide resolved
lib/client/keystore_test.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
@xacrimon xacrimon requested a review from a-palchikov March 12, 2021 16:43
@xacrimon xacrimon force-pushed the joel/rfd-18-impl branch 2 times, most recently from 52aa03b to b8a77f9 Compare March 12, 2021 18:51
lib/client/api.go Outdated Show resolved Hide resolved
lib/client/keystore.go Outdated Show resolved Hide resolved
lib/client/keystore.go Outdated Show resolved Hide resolved
@webvictim
Copy link
Contributor

@xacrimon I tested this on Windows both with ssh-agent and with no agent, it works fine. I'm afraid I didn't have time to set up gpg-agent and try that.

I did notice that even when we use --add-keys-to-agent only, we do still get a certs.pem file in the keys/<proxy> directory. This doesn't look to have any private key material in it, so should be fine

@xacrimon
Copy link
Contributor Author

xacrimon commented Mar 12, 2021

Yep, that certs.pem is needed. Contains CA certificates so we can verify certs it has issued.

Copy link
Contributor

@andrejtokarcik andrejtokarcik left a comment

Choose a reason for hiding this comment

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

Found two mistakes in my patch.

lib/client/keystore.go Outdated Show resolved Hide resolved
lib/client/keystore.go Outdated Show resolved Hide resolved
@xacrimon xacrimon added this to the 6.1 milestone Mar 15, 2021
lib/client/api.go Outdated Show resolved Hide resolved
lib/client/keyagent.go Outdated Show resolved Hide resolved
lib/client/keystore.go Show resolved Hide resolved
lib/client/keystore.go Outdated Show resolved Hide resolved
lib/client/keystore.go Outdated Show resolved Hide resolved

// DeleteKey removes a specific session key from a proxy.
func (s *MemLocalKeyStore) DeleteKey(proxyHost, username string, opts ...KeyOption) error {
delete(s.inMem, keyIndex{proxyHost: proxyHost, username: username})
Copy link
Contributor

Choose a reason for hiding this comment

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

should this also delete any keys where clusterName is set?

Copy link
Contributor

Choose a reason for hiding this comment

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

Once the opts handling is added here (see my comment below) leaving some clusterName leftovers shouldn't break anything as the only way to get a key with a clusterName is to go via an opt.

Alternatively the map's key could be just changed from [proxyHost, username, clusterName] to [proxyHost, username][clusterName].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be resolved now with the opts handling.

Copy link
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

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

Bot.

@xacrimon xacrimon merged commit d162b02 into master Mar 17, 2021
@xacrimon xacrimon deleted the joel/rfd-18-impl branch March 17, 2021 18:04
@Joerger Joerger restored the joel/rfd-18-impl branch February 1, 2022 20:31
@Joerger Joerger deleted the joel/rfd-18-impl branch February 1, 2022 20:34
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.

gpg-agent does not support SSH certificates, tsh should be aware of this
6 participants