-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Client store generalization #19420
Client store generalization #19420
Conversation
80e9d77
to
fdb3d53
Compare
- Generalize KeyStore logic to support non-disk stores in a streamlined way - Replace custom identity file logic with NewMemLocalKeyStoreFromIdentityFile - Add NonSessionKeyStore interface and an in-memory implementation - Fix inconsistencies in Host CA handling across different types of keys and key stores. e.g. Missing Host certs, missing cluster name.
…ecker with key.CheckHostKey; cleanup and fix tests.
c31fe63
to
152c499
Compare
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.
Partial pass
Looking good 👍
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.
Most changes look good 👍
There are a lot of changes that are just moving around methods/structs to different files
I wonder if grouping those changes together would make this PR easier to review
// Deduplicate deduplicates list of strings | ||
func Deduplicate(in []string) []string { | ||
// Deduplicate deduplicates list of comparable values. | ||
func Deduplicate[T comparable](in []T) []T { |
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.
nit: I'm not sure about the performance differences, but you could use DeduplicateAny()
in the implementation here:
func Deduplicate[T comparable](in []T) []T {
return DeduplicateAny(in, func(t T, t2 T) bool {
return t == t2
})
}
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.
Deduplicate[T comparable]
has better performance since it can use a map[T]bool
for O(1) existence checks, so I think it's better not to generalize all uses to the current DeduplicateAny
with O(n) existence checks. Let me know if you disagree since the performance is pretty negligible to be honest.
I was considering changing DeduplicateAny
to func[T any](in []T, hashFunc func(T) string) []T
so Deduplicate
could simply call that, but I don't think it's worth the extra complexity right now.
@@ -37,3 +38,19 @@ func TestDeduplicate(t *testing.T) { | |||
}) | |||
} | |||
} | |||
|
|||
func TestDeduplicateAny(t *testing.T) { |
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.
nit: I'd be nice to also unit test the Deduplicate()
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.
These tests already exist above here, no change was necessary for the generification.
Co-authored-by: Jakub Nyckowski <jakub.nyckowski@goteleport.com>
It looks like this change broke adding in a new cluster in Connect. I fixed that in #20263 and added an integration test which would've caught this issue. |
This PR generalizes client store logic to enable more operability between client backends (
~/.tsh
, identity file, in-memory, agent forwarded keys). This lays the groundwork for #19421.KeyStore
withClientStore
, which is made up of a key store, trusted certs store, and profile store.KeyStore
interface with an FS and in-memory implementation, based on original key store.TrustedCertsStore
interface with an FS and in-memory implementation, based on original key store.ProfileStore
interface with an FS and in-memory implementation.tsh --add-keys-to-agent=only
, which keeps keys in memory while writing trusted certs and profiles to disk.ClientStore
ontsh
start.ClientStore
populated from forwarded keys.auth.TrustedCerts.ClusterName
should never be empty so that it can actually be useful for logic.auth.TrustedCerts.AuthorizedKeys
, previously inaptly namedHostCertificates
, should always be inauthorized_keys
format rather thanknown_hosts
format to maintain consistency across backends. We only convert toknown_hosts
when writing to~/.tsh
or identity file.