feat: make oidc scopes configurable #49
Conversation
Why: enable service accounts without offline_access scope
pkg/cmd/login.go
Outdated
| @@ -51,6 +51,7 @@ func NewLoginCmd(ctx context.Context) *cobra.Command { | |||
| cmd.Flags().StringVar(&tenantCfg.OIDC.ClientSecret, "oidc.client-secret", "", "The OIDC client secret, see https://tools.ietf.org/html/rfc6749#section-2.3.") | |||
| cmd.Flags().StringVar(&tenantCfg.OIDC.ClientID, "oidc.client-id", "", "The OIDC client ID, see https://tools.ietf.org/html/rfc6749#section-2.3.") | |||
| cmd.Flags().StringVar(&tenantCfg.OIDC.Audience, "oidc.audience", "", "The audience for whom the access token is intended, see https://openid.net/specs/openid-connect-core-1_0.html#IDToken.") | |||
| cmd.Flags().StringSliceVar(&tenantCfg.OIDC.Scopes, "oidc.scopes", []string{"openid", "offline_access"}, "Optional scopes, must contain 'openid', see https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest") | |||
There was a problem hiding this comment.
If it must contain the "openid" scope, can we hardcode its addition to the list of scopes instead of having the risk of failing all authentications?
There was a problem hiding this comment.
Thanks, certainly. Changed it accordingly.
pkg/cmd/login.go
Outdated
| @@ -51,6 +51,7 @@ func NewLoginCmd(ctx context.Context) *cobra.Command { | |||
| cmd.Flags().StringVar(&tenantCfg.OIDC.ClientSecret, "oidc.client-secret", "", "The OIDC client secret, see https://tools.ietf.org/html/rfc6749#section-2.3.") | |||
| cmd.Flags().StringVar(&tenantCfg.OIDC.ClientID, "oidc.client-id", "", "The OIDC client ID, see https://tools.ietf.org/html/rfc6749#section-2.3.") | |||
| cmd.Flags().StringVar(&tenantCfg.OIDC.Audience, "oidc.audience", "", "The audience for whom the access token is intended, see https://openid.net/specs/openid-connect-core-1_0.html#IDToken.") | |||
| cmd.Flags().StringSliceVar(&tenantCfg.OIDC.Scopes, "oidc.scopes", []string{"offline_access"}, "Optional scopes, see https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest") | |||
There was a problem hiding this comment.
Thanks a lot for the update @RaphaelBut. I think we could have a different approach though. It seems like not only openid, but also offline_access scopes are required, correct?
We might also have to be careful about duplication of scopes, e.g. what if the user also provides the openid scope? We will end up appending another string with openid to the list of scopes. Could this break something? I think we should only append it when it's missing.
There was a problem hiding this comment.
Hi @douglascamata, thanks for the reviews btw
I am not sure if or in which use cases offline_access is required/desired but for service accounts it should explicitly not be required (or so I am told) and is the reason why I want this flag to override.
My understanding is without offline_access the application is not able to request a new token on its own and the user has to authenticate again if the token expires. If obsctl is used by a service account during a pipeline this wont be needed and is the reason why offline_access is not present on new service accounts.
Regarding the second point, I can update this to be cleaner and prevent duplicated scopes in the request but would like to keep the offline_access as default on the flag to not introduce any change.
(update: just tested it locally and it does not seem to change anything if we specify no scope or duplicates
maybe the golang.org/x/oauth2 package sorts it out under the hood 🤔 )
There was a problem hiding this comment.
Hi again :D
I thought about it a little more and maybe having this flag as a toggle for offline_access might be the most straight forward approach.
This way, we always have the required openid, keep the offline_access per default but can call with --oidc.offline-access=false in case we want to override for service accounts.
There was a problem hiding this comment.
Hey @RaphaelBut! I think what you say makes sense. I like the simplified option for the offline access scope. Thanks a lot for the research.
|
The test failures seem to be related to log changes and not related. Let's merge and iterate on those. |
Small change to make the scopes that are requested during login configurable per new flag 'oidc.scopes'.
Why: Service accounts without offline_access capabilities are unable to log in due to requesting offline_access without permissions. With this override one can omit 'offline_access' if required.
I kept the hardcoded values 'openid' and 'offline_access' as defaults on the flag to not introduce any breaking changes.