OIDC support #144
OIDC support #144
Conversation
szh
force-pushed
the
oidc
branch
5 times, most recently
from
da2c77b to
52563e2
Compare
| "github.com/cyberark/conjur-api-go/conjurapi/response" | ||
| ) | ||
|
|
||
| // OidcProviderResponse contains information about an OIDC provider. | ||
| type OidcProviderResponse struct { |
There was a problem hiding this comment.
Wondering if there is a way to construct a provider information response without filtering it through a hard-coded struct.
Thinking about a case where the /providers endpoint needs to update its response content - this OidcProviderResponse struct would need to be kept in phase. If we used a generic string-to-string hashmap instead, maybe we could avoid all that, and decouple the Go API's response from the contained data.
There was a problem hiding this comment.
I see your point, but I think there's utility in having a struct, particularly so the CLI (or other API consumers) can know what fields to expect. I'll also point out that we do something similar in PolicyResponse.
conjurapi/authn.go
Outdated
| if err == nil { | ||
| c.cacheOidcToken(resp) | ||
| } |
There was a problem hiding this comment.
I might be missing the forest for the trees, but this doesn't seem specific to the API project, and instead could be the responsibility of the consuming client.
I can imagine a Go client that chooses to store the access token elsewhere (like Conjur UI, which uses an encrypted session cookie) and doesn't want/need the token saved to file elsewhere.
There was a problem hiding this comment.
So this is complicated. @doodlesbykumbi and I have been discussing whether the netrc storage should happen in the CLI or API. See this comment:
conjur-api-go/conjurapi/authn.go
Lines 244 to 245 in 52563e2
In the meantime, the practical reason I took this approach is that the CLI doesn't have access to the access token - it stays in the API library, and I didn't think it was such a great idea to change that. I'm open to more discussion though.
There was a problem hiding this comment.
Yup, I'm definitely remembering this topic now.
I'm concerned that if a user is only dealing with the API's public interface, they'll notice the OidcAuthenticate function returns an access token, and the token being written to a file could be either unintended or invisible.
Or maybe I'm thinking about this the wrong way, and the API is using the cached token to authenticate HTTP calls, so what is really "unintended" is returning the access token to a user at all!
Maybe this means that returning an access token and caching one should be two distinct function calls.
There was a problem hiding this comment.
That's an interesting point. In truth, we could make the Authenticate() and OidcAuthenticate() methods private, as far as the CLI is concerned. I suppose the issue then would be for clients that want to use the API in a different way.
Maybe the best solution would be to make a separate function for caching the token, but I would argue that by calling OidcAuthenticate() instead of Authenticate() it's implicit that this flow (including caching) should be used. OIDC generally can't be used for automation as the whole advantage it has over regular authn is that it requires a second factor, ie user interaction.
szh
force-pushed
the
oidc
branch
2 times, most recently
from
e678fa2 to
94a0f99
Compare
|
Thanks for the comments, LGTM! Just needs a rebase and I'll happily approve 😄 |
Desired Outcome
Add support for authn-oidc in the Conjur Golang API
Implemented Changes
Connected Issue/Story
CyberArk internal issue ID: ONYX-25484
Changelog
CHANGELOG update
Test coverage
changes, or
Documentation
READMEs) were updated in this PRBehavior
Security