-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Dynamic identity file reloading support for API Client #29139
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
strideynet
merged 15 commits into
master
from
strideynet/dynamic-identity-file-credential
Aug 28, 2023
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
4f8c22b
Skeleton of hot-swappable tls credentials for api client
strideynet 8b6eaf3
Start work on SSH support
strideynet 8afa3e5
Merge remote-tracking branch 'origin/master' into strideynet/dynamic-…
strideynet b461243
Add note to what needs doing
strideynet 94f1ef1
Support rotating ssh host cert
strideynet a412d9d
Tidy u implementation
strideynet 82bfb38
Merge remote-tracking branch 'origin/master' into strideynet/dynamic-…
strideynet 772250f
Further tidy dynamic credential implementation
strideynet 8a87796
Add godoc to new function
strideynet 5d077e9
Move code into credentials.go
strideynet e47a1ac
Add some more explanatory docs
strideynet 075d342
Fix panics intrdouced by refactor
strideynet 6b65945
Add tests & make fix-imports
strideynet f70693e
Add helper for extracting principal
strideynet 251986b
Merge remote-tracking branch 'origin/master' into strideynet/dynamic-…
strideynet File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: just personal preference. I would prefer calling
id.TLSConfigand taking cert and pool from there or maybe saving theidentityTLSConfigand using that later instead of "manually" doing it here.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.
I think I'd prefer to keep it as is. The reason for this is that
tls.Configis a poor way of passing just a key/cert/CAs around - it has other fields that the underlying implementation could start using instead of theCertificatesorRootCAsfields and I can easily see a future refactor to underlying parts breaking theDynamicIdentityFileCreds- the.CACertsand.Certsfields are much stricter and it's much more likely to be clearer that changing this risks breaking an upstream.I have a somewhat long-term plan to try and convince folks that we should stop returning
tls.Configandssh.ClientConfigfrom theclient.Credentialstype. These configs are for more than just carrying certs/keys around and it's creating a conflict of responsibility for what the Credential should do and what the Client should do.