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
marcoandredinis
left a comment
There was a problem hiding this comment.
Partial pass
Looking good 👍
marcoandredinis
left a comment
There was a problem hiding this comment.
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.
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.
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.
| } | ||
| } | ||
|
|
||
| func TestDeduplicateAny(t *testing.T) { |
There was a problem hiding this comment.
nit: I'd be nice to also unit test the Deduplicate()
There was a problem hiding this comment.
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.KeyStorewithClientStore, which is made up of a key store, trusted certs store, and profile store.KeyStoreinterface with an FS and in-memory implementation, based on original key store.TrustedCertsStoreinterface with an FS and in-memory implementation, based on original key store.ProfileStoreinterface 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.ClientStoreontshstart.ClientStorepopulated from forwarded keys.auth.TrustedCerts.ClusterNameshould never be empty so that it can actually be useful for logic.auth.TrustedCerts.AuthorizedKeys, previously inaptly namedHostCertificates, should always be inauthorized_keysformat rather thanknown_hostsformat to maintain consistency across backends. We only convert toknown_hostswhen writing to~/.tshor identity file.