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

feat(api): basic support for openid-connect authentication provider #5393

Merged
merged 8 commits into from
Aug 27, 2020
Merged

feat(api): basic support for openid-connect authentication provider #5393

merged 8 commits into from
Aug 27, 2020

Conversation

phsym
Copy link
Contributor

@phsym phsym commented Aug 25, 2020

  1. Description
    This enables basic integration with openid-connect authentication providers like Keycloak or Hydra

  2. About tests
    Not sure yet how to automate the tests here, but for manual testing :

  • Setup Keycloak (more here)
    • Start a keycloak server in Docker with docker run -p 4040:8080 -e KEYCLOAK_USER=admin -e KEYCLOAK_PASSWORD=admin quay.io/keycloak/keycloak:11.0.1

    Map internal port 8080 to 4040 to avoid conflict with CDS UI, in case you run everything locally

    • Login to keycoak web UI as admin
    • Create a realm named "cds"
    • Create a user in this realm, add a password to it
    • Create an openid-connect client named "cds_client", set the CDS urls, and change it from public to confidential. Save, and copy the client secrets
    • Edit cds API config to add open id settings as following
    [api.auth.oidc]
          clientId = "cds_client"
          clientSecret = "<CDS CLIENT SECRET>"
          enabled = true
          signupDisabled = false
          url = "http://<KEYCLOAK ADDRESS>:4040/auth/realms/cds"

    <KEYCLOAK ADDRESS> can be localhost if you run everything locally

    • Login on CDS UI using the openid-connect button. You'll e redirected to keycloak login screen. After succesfull login, you'll be redirected to CDS screen.
  1. Mentions

@ovh/cds

Signed-off-by: phsym [email protected]

This enables basic integration with openid-connect authentication providers like Keycloak or Hydra

Signed-off-by: phsym <[email protected]>
Handle OIDC driver initialization error

Signed-off-by: phsym <[email protected]>
@richardlt
Copy link
Member

Hi, thanks for the PR can you provide some tips about how you test it ?

@phsym
Copy link
Contributor Author

phsym commented Aug 25, 2020

Hi, sure ! I just updated the PR description with the tips

@yesnault
Copy link
Member

@phsym Thank you!

Can you take this commit b9eca49 to add openid icon please?

Capture d’écran 2020-08-25 à 19 38 01
Capture d’écran 2020-08-25 à 19 41 59

@phsym
Copy link
Contributor Author

phsym commented Aug 25, 2020

@yesnault Done by merging your branch into mine, but it messed up the DCO check for some reason

EDIT: It's fixed now

yesnault and others added 2 commits August 25, 2020 20:13
Signed-off-by: Yvonnick Esnault <[email protected]>
Signed-off-by: phsym <[email protected]>
@phsym phsym closed this Aug 25, 2020
@phsym phsym reopened this Aug 25, 2020
@phsym
Copy link
Contributor Author

phsym commented Aug 25, 2020

Oops sorry, I missclicked on close

@phsym
Copy link
Contributor Author

phsym commented Aug 26, 2020

I just added a check on the "email_verified" OIDC claim

Copy link
Member

@fsamin fsamin left a comment

Choose a reason for hiding this comment

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

Thank you very much to your great contribution. Here are some change requests.

engine/api/authentication/oidc/openid.go Outdated Show resolved Hide resolved
engine/api/authentication/oidc/openid.go Outdated Show resolved Hide resolved
engine/api/authentication/oidc/openid.go Outdated Show resolved Hide resolved
engine/api/authentication/oidc/openid.go Outdated Show resolved Hide resolved
engine/api/authentication/oidc/openid.go Outdated Show resolved Hide resolved
@phsym
Copy link
Contributor Author

phsym commented Aug 26, 2020

@fsamin Corrections provided based on your comments

Copy link
Member

@fsamin fsamin left a comment

Choose a reason for hiding this comment

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

engine/api/authentication/oidc/openid.go Outdated Show resolved Hide resolved
engine/api/authentication/oidc/openid.go Outdated Show resolved Hide resolved
@phsym
Copy link
Contributor Author

phsym commented Aug 26, 2020

@fsamin Done

Copy link
Member

@fsamin fsamin left a comment

Choose a reason for hiding this comment

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

LGTM

@sguiheux sguiheux merged commit 7288e75 into ovh:master Aug 27, 2020
@phsym phsym deleted the auth_oidc branch August 27, 2020 13:39
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.

5 participants