Skip to content
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

Allow "scope" to be set in the token #32

Merged

Conversation

lindseyburnett
Copy link
Contributor

@lindseyburnett lindseyburnett commented Sep 18, 2023

We've come across an observatorium instance that needs to have a token including scope in the payload, or else it fails to authenticate.

Verified change by including --scope="openid offline_access --file="token.out" and verifying the "token.out" file contains a jwt. When decoded, the payload data includes:

  "scope": "openid offline_access",

Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Generally LGTM, just one comment!

main.go Outdated
@@ -82,6 +83,7 @@ func parseFlags() (*config, error) {
flag.StringVar(&cfg.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.")
flag.StringVar(&cfg.oidc.username, "oidc.username", "", "The username to use for OIDC authentication. If both username and password are set then grant_type is set to password.")
flag.StringVar(&cfg.oidc.password, "oidc.password", "", "The password to use for OIDC authentication. If both username and password are set then grant_type is set to password.")
flag.StringVar(&cfg.scope, "scope", "", "The scope to be included in the payload data of the token.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a good idea to use flag.StringSliceVar here, as there might be multiple scopes to include?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple scopes are supported by a single "scope" key, and the value being a string with each scope separated by spaces. If you simply change to StringSliceVar and the corresponding data types to []string, it tries to send multiple scope parameters, you get the following error:

level=error name=token-refresher ts=2023-09-18T15:08:05.355519306Z caller=main.go:244 msg="failed to get token" err="oauth2: cannot fetch token: 400 Bad Request\nResponse: {\"error\":\"invalid_request\",\"error_description\":\"duplicated parameter\"}"

My latest change now supports multiple scopes in the following ways:

--scope="openid offline_access"
--scope="openid,offline_access"
--scope="openid" --scope="offline_access"

main.go Outdated
@@ -82,6 +83,7 @@ func parseFlags() (*config, error) {
flag.StringVar(&cfg.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.")
flag.StringVar(&cfg.oidc.username, "oidc.username", "", "The username to use for OIDC authentication. If both username and password are set then grant_type is set to password.")
flag.StringVar(&cfg.oidc.password, "oidc.password", "", "The password to use for OIDC authentication. If both username and password are set then grant_type is set to password.")
flag.StringSliceVar(&cfg.scope, "scope", []string{""}, "The scope to be included in the payload data of the token. Scopes can either be comma-separated or space-separated.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
flag.StringSliceVar(&cfg.scope, "scope", []string{""}, "The scope to be included in the payload data of the token. Scopes can either be comma-separated or space-separated.")
flag.StringSliceVar(&cfg.scope, "scope", []string{""}, "The scope to be included in the payload data of the token. Scopes can either be comma-separated or space-separated.")

Copy link
Member

@squat squat Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lindseyburnett I don't think that the values to the flag can be space-separated. The shell will interpret the second or third scopes as separate arguments to the program and Go will parse them as additional arguments that are not associated to the flag. Perhaps they can be space-separated if the scopes are wrapped in quotes to indicate that it's one single value. pflag definitely allows for comma-separation and specifying the flag multiple times, e.g. --scope X --scope Y --scope Z

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right. Space-separated will need to be wrapped in quotes. All three of these variations worked for me:

--scope="openid offline_access"
--scope=openid,offline_access
--scope=openid --scope=offline_access

Personally, my first choice would be to use the first variation because it's so similar to how I would manually get a token via the command line.

TOKEN=$(curl --request POST \
    --url https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token \
    --header 'content-type: application/x-www-form-urlencoded' \
    --data scope='openid offline_access' \
    --data grant_type=client_credentials \
    --data client_id=REDACTED \
    --data client_secret=REDACTED | jq .access_token | tr -d '"')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, as per StringSliceVar goDoc comment all of these are valid. :)

lindseyburnett and others added 3 commits September 18, 2023 11:38
Ignore jetbrains IDE generated subdir
Co-authored-by: Lucas Servén Marín <lserven@gmail.com>
squat
squat previously approved these changes Sep 19, 2023
main.go Outdated Show resolved Hide resolved
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@saswatamcode saswatamcode merged commit f5e3403 into observatorium:master Sep 20, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants