Add NGTS configuration + NGTS client#788
Conversation
There was a problem hiding this comment.
NB: this is pretty much copy+pasted from client_venafi_cloud.go because the logic is nearly identical. I refactored some of the shared logic out (util.go) but mostly this is the same thing with different names
pkg/client/client_ngts.go
Outdated
| claims["iss"] = c.credentials.ClientID | ||
| claims["iat"] = time.Now().Unix() | ||
| claims["exp"] = time.Now().Add(time.Minute).Unix() | ||
| claims["aud"] = path.Join(c.baseURL.Host, ngtsAccessTokenEndpoint) |
There was a problem hiding this comment.
I think the NGTS endpoint has not been updated yet to accept anything other than "api.venafi.cloud/v1/oauth/token/serviceaccount" here?
There was a problem hiding this comment.
Is this working for the NGTS endpoint?
There was a problem hiding this comment.
Thanks for the heads up - this was a guess and was untested (because none of this code is usable, testing is kinda 🤷 - this is just a first step to make future reviews smaller).
I've used the old aud for now. I'm working to test locally in a branch with helm changes
Spotted by @George-Yanev Signed-off-by: Ashley Davis <ashley.davis@cyberark.com>
This will add initial support for NGTS. Auth is based on the existing Venafi Cloud client using a keypair. Signed-off-by: Ashley Davis <ashley.davis@cyberark.com>
| c.lock.RLock() | ||
| needsUpdate := c.accessToken == nil || time.Now().Add(time.Minute).After(c.accessToken.expirationTime) | ||
| c.lock.RUnlock() | ||
|
|
||
| if needsUpdate { |
There was a problem hiding this comment.
note: @George-Yanev spotted that the locks weren't handled properly in the NGTS client, which is copied from here. So I fixed it here too.
|
@George-Yanev gave this a thumbs up in slack, so I'll merge now to be able to get the helm chart PR raised + reviewable. |
4 participants
This will add initial support for NGTS. Auth is based on the existing Venafi Cloud client using a keypair.
I'm not really able to test this effectively because of various issues with the test env, but I think this is safe enough to merge as-is because it's not customer-facing yet (needs helm chart support before this is realistically usable)
Note there are several TODOs in this PR. They need to be clarified before we can expose this functionality to customers, but I think they're fine for now.