8000 Add additional parameter to oidc SkipEmailVerified to translate it to the dex by kirillbilchenko · Pull Request #6478 · concourse/concourse · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add additional parameter to oidc SkipEmailVerified to translate it to the dex #6478

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 2, 2021
Merged

Add additional parameter to oidc SkipEmailVerified to translate it to the dex #6478

merged 4 commits into from
Feb 2, 2021

Conversation

kirillbilchenko
Copy link
Contributor
@kirillbilchenko kirillbilchenko commented Jan 27, 2021 •
edited by aoldershaw
Loading

Explanation of issue can be found in this PR: dex

Add --oidc-force-email-verified flag to OIDC connector configure.

As we are using Azure as an identity provider, it's not producing this field.
Some providers return claims without "email_verified", when they had no usage of emails verification in enrollement process.

Signed-off-by: kirillbilchenko kirya7@gmail.com

What does this PR accomplish?

Bug Fix | Feature | Documentation

closes # .

Changes proposed by this PR:

Notes to reviewer:

Release Note

  • Some OIDC providers don't include the email_verified claim, which causes a validation error by default
  • To support these providers, you can set CONCOURSE_OIDC_SKIP_EMAIL_VERIFIED_VALIDATION to true

Contributor Checklist

Reviewer Checklist

  • Code reviewed
  • Tests reviewed
  • Documentation reviewed
  • Release notes reviewed
  • PR acceptance performed
  • New config flags added? Ensure that they are added to the
    BOSH and
    Helm packaging; otherwise, ignored for
    the integration
    tests
    (for example, if they are Garden configs that are not displayed in the
    --help text).

@kirillbilchenko
Add additional parameter to oidc SkipEmailVerified to translate it to…
dfd5490 
… dex

Signed-off-by: kirillbilchenko <kirya7@gmail.com>
@kirillbilchenko kirillbilchenko force-pushed the patch/email_verified_dex branch from 63fa51c to ac4a8b0 Compare January 27, 2021 17:03
@kirillbilchenko
Update
3d09adb 
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
@kirillbilchenko kirillbilchenko force-pushed the patch/email_verified_dex branch from ac4a8b0 to 3d09adb Compare January 27, 2021 17:03
@xtremerui
Copy link
Contributor

For a temporary solution, #6431 (comment)

@chenbh chenbh added the bug label Feb 2, 2021
chenbh
chenbh approved these changes Feb 2, 2021
View reviewed changes
Copy link
Contributor
@chenbh chenbh left a comment

Choose a reason for hiding this comment

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

lgtm, @kirillbilchenko you might also want to open an identical PR to backport this to the release/v6.7.x branch, master is currently targeted for v7.0.0.

@xtremerui what do you think of allowing configuring an --dex-config flag which gets marshalled into an oidc.Config struct and then merged with the config we construct?

The idea would be that if dex decides to add any new flags that we didn't notice, users can at least work around the regression without having to wait for a patch release.

chenbh
chenbh requested changes Feb 2, 2021
View reviewed changes
Copy link
Contributor
@chenbh chenbh left a comment

Choose a reason for hiding this comment

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

Actually, do you mind renaming the flag to InsecureSkipEmailVerified? It better describes what it actually does: ignore the email_verified value

@xtremerui
Copy link
8000 Contributor

@xtremerui what do you think of allowing configuring an --dex-config flag which gets marshalled into an oidc.Config struct and then merged with the config we construct?

I think it could be an additional config to all cmds in skymarshall so it will work for all connectors.

@kirillbilchenko
Copy link
Contributor Author

Actually, do you mind renaming the flag to InsecureSkipEmailVerified? It better describes what it actually does: ignore the email_verified value

will rename, @chenbh I will open the same PR to backport branch after we will merge this one

@kirillbilchenko
Merge branch 'master' of github.com:concourse/concourse into patch/em…
3f828bf 
…ail_verified_dex
@kirillbilchenko
Rename flag
90fe5bd 
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
@chenbh chenbh merged commit eb773ce into concourse:master Feb 2, 2021
kirillbilchenko added a commit to kirillbilchenko/concourse that referenced this pull request Feb 3, 2021
@kirillbilchenko
Backport concourse#6478 to 6.7.x branch
1bce499 
Signed-off-by: kirillbilchenko <kirya7@gmail.com>
@kirillbilchenko kirillbilchenko mentioned this pull request Feb 3, 2021
Merged
11 tasks
chenbh added a commit that referenced this pull request Feb 5, 2021
@chenbh
Merge pull request #6496 from kirillbilchenko/backport/pr_6478
c1f2fff 
Backport #6478 to 6.7.x branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenbh chenbh chenbh approved these changes

Assignees

@chenbh chenbh

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kirillbilchenko @xtremerui @chenbh
0