Credential Handling Improvements #205
Merged
Conversation
This file contains 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
…nt creds The client credentials were previously discarded and client credentials configured via the configuration file where never used.
To stay consistent with the naming in the UI. This shouldn't be a breaking change as the file based client credential configuration was previously broken anyways.
This will allow to cache user, service principal and workload identity provider tokens in the same way. It will also provide a common way to refresh and fetch new tokens if the cached token is expired. This new cache will replace the user specific session and cache solution that is currently used.
The config value was only used in that function and constructing the config inside of the function makes it easier to understand and reason about.
…Source It will be necessary to have access to the configuration to be able to cache the resulting tokens with the identity provider's resource name.
This enables caching for service-principal and workload identity provider tokens and uses the same caching mechanism for all types of credentials.
The functionality is now implemented by the common cache.
Without this the cache could keep collecting expired tokens for service principals and workload identity providers.
With the switch to the common cache it is not possible to return this typed error anymore. The error does however not seem to be used anywhere currently and this change should not be breaking.
This option can be used by CLIs to implement a `login` command that will force a browser login (or in the future client credentials based login) to occur. The tokens received during the login will be cached for future invocations.
This makes it easier to understand how the HCP config is used in the http client and allows to play around with different HCP config options in the example code.
The previous name hcp-sdk-go-client could be interpreted as it being a usable client.
Without this it wouldn't be possible to test the token source logic.
To make it easier to find the logic and to make sure that it works as intended.
tobias-hashicorp
changed the title
tobias-hashicorp
force-pushed
the
tobias/plat-1424/credential-handling-improvements
branch
2 times, most recently
from
010b37d
to
74903bf
Compare
They were not needed anymore and the linter was complaining.
tobias-hashicorp
force-pushed
the
tobias/plat-1424/credential-handling-improvements
branch
from
74903bf
to
249a701
Compare
dadgar
reviewed
Oct 26, 2023
| // SecretID is the secret id of an HCP Service Principal | ||
| SecretID string `json:"secret_id,omitempty"` | ||
| // ClientSecret is the client secret of an HCP Service Principal | ||
| ClientSecret string `json:"client_secret,omitempty"` |
There was a problem hiding this comment.
@marianoasselborn Just a heads up the credential file format will change a bit b/c of this
| // Read the cache information from the file, if it exists | ||
| cachedTokens, err := readCache(source.cacheFile) | ||
| if err != nil { | ||
| log.Println(err) |
There was a problem hiding this comment.
Should we special case no file existing and not log and on other errors should it delete the cache?
Also is the right way to log, should we pass a logger through the entire Config?
Was going to comment this but NVM, I see that it isn't plumbed that way yet. Future enhancement
There was a problem hiding this comment.
I have added a check for "NoExist".
I don't think the cache needs to be explicitly deleted as it will automatically overwrite the existing file on the write operation.
| log.Printf("failed to refresh the token: %s\n", err) | ||
| } | ||
|
|
||
| if source.oauthTokenSource != nil { |
There was a problem hiding this comment.
Shouldn't this be preferred to the raw oauthConfig?
There was a problem hiding this comment.
No. It's intentional that it is this way around. The oauthConfig will perform a refresh, the pure token source will not be able to do so as it does not know the cached refresh token. I have added a comment and modified the code a bit to make this more prominent.
As it is the main interface for the browser login functionality.
tobias-hashicorp
force-pushed
the
tobias/plat-1424/credential-handling-improvements
branch
from
cdd1961
to
351ddd1
Compare
This makes it easier to create folder and files in a consistent way.
It previously tried to refresh a token even if it didn't have a RefreshToken field. This change should also make it more clear why the oauth config takes precedence over the token source.
The function does a lot, but the goal is to have a valid token. Evaluate is quite generic and getValidToken should be more expressive.
The timeout was previously unset and the refresh would have timed out right away.
Without this the token source could return tokens that are very close to expiry.
Having no cache file is the default initial state and it is not problematic and doesn't need to be logged.
It could otherwise be assumed that the expired entries will be removed from the cache file.
It was initially planned to support service-principal login, but since that is not implemented right now the distinction between source types can rely on the sourceType and the complexity can be spared. In the future a flag might need to be added to differencate between user and sp based login.
The documentation was not consistent with the current implementatio anymore.
To make it more consistent with the real implementation.
tobias-hashicorp
force-pushed
the
tobias/plat-1424/credential-handling-improvements
branch
from
f3199f7
to
8767256
Compare
The file would otherwise only get updated when a new token needs to get cached.
This is needed to be able to test the library without the need for a real token exchange.
dadgar
approved these changes
Oct 27, 2023
|
|
||
| // Garbage collect expired tokens | ||
| cachedTokens.removeExpiredTokens() | ||
| _ = cachedTokens.write(source.cacheFile) |
There was a problem hiding this comment.
Do we not want to log in case the file permissions are wrong, so the user sees the issue.
There was a problem hiding this comment.
I wasn't worried about this particular operation failing as it would just leave the expired tokens around and writing of new tokens failing should still be sufficient signal to know that writing is not possible.
tobias-hashicorp
deleted the
tobias/plat-1424/credential-handling-improvements
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🛠️ Description
All credentials (login, service-principal and workload identity provider) are now cached.
The cache file has moved to
creds-cache.jsonto not interfere with applications that rely on the previous cache filestructure.
It is further now possible to enforce an interactive login (via a configuration flag), which can be used to implement a
cli loginfunctionality.There are some minor breaking changes:
ErrorNoLocalCredsFoundis not returned anymoreWithSessionis not supported anymore👍 Definition of Done