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

Use SSO to create CLI token #598

Draft
wants to merge 34 commits into
base: staging
Choose a base branch
from
Draft

Conversation

yuriadams
Copy link
Contributor

@yuriadams yuriadams commented Mar 23, 2023

What is the feature/fix?

Docs:

configure

Configure necessary fields to make the authentication using login command.
Fields:

  • Provider: Which identity provider will be used to authenticate. Supported providers: okta.
  • Client ID: The OpenID Connect client ID provided by your Identity Provider.
  • Client secret: The OpenID Connect client secret provided by your Identity Provider.
  • Issuer: A unique string that identifies the provider issuing a request.
  • Callback URL: A unique string that identifies the callback url that the idp will redirect the request after authorize the customer.

Usage

    convox sso configure

or

    convox sso configure -p PROVIDER -c CLIENT_ID -s CLIENT_SECRET -i ISSUER -u CALLBACK_URL

or we can add following environment variables:

SSO_PROVIDER
SSO_CLIENT_ID
SSO_CLIENT_SECRET
SSO_ISSUER
SSO_CALLBACK_URL

Examples

    convox sso configure
    SSO Provider: 
    SSO Client ID: 
    SSO Client Secret: 
    SSO Issuer:
    SSO Callback URL: 
    OK

login

Authenticate your CLI using a identity provider.

Usage

    convox sso login

You will be redirected to the browser to your fill the credentials in your identity provider.

Requirements

Is necessary to configure the identity provider to add the convoxID that refers with the user id in the console. Also, is required to add this field in the token claim.
Also, configure the callback endpoint in the customer application, for example, http://localhost:8090/authorization-code/callback. Port and the path can be customized by the customer.

Does it has a breaking change?

No

Video Record

https://convox602-my.sharepoint.com/:u:/r/personal/yuri_adams_convox_com/Documents/Convox%20-%20SSO%20Demo.zip?csf=1&web=1&e=Qqro5W

Checklist

  • New coverage tests
  • Unit tests passing
  • E2E tests passing
  • E2E downgrade/update test passing
  • Documentation updated
  • No warnings or errors on Deepsource/Codecov

@yuriadams yuriadams requested a review from Twsouza March 23, 2023 17:30
@yuriadams yuriadams changed the title Feat 1204056522119209 sso Use SSO to create CLI token Mar 23, 2023
Copy link
Collaborator

@nightfury1204 nightfury1204 left a comment

Choose a reason for hiding this comment

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

@yuriadams nice job.

but userid as convoxID poses a security issue. it should't be a user id. since we once someone knows the user id, they can easily login in. also its hard to change if it gets compromised since it's act as an identifier of the user in our system and all the data associated with it.

also can every user in the sso okta can configure identity provider?

@yuriadams
Copy link
Contributor Author

yuriadams commented Mar 28, 2023

@yuriadams nice job.

but userid as convoxID poses a security issue. it should't be a user id. since we once someone knows the user id, they can easily login in. also its hard to change if it gets compromised since it's act as an identifier of the user in our system and all the data associated with it.

also can every user in the sso okta can configure identity provider?

@nightfury1204 thank you very much for your feedback.

First question:

This is a great point that I don't have the answer what's the the best way. A improvement I did this morning was add the convoxID as a token claim. So, this token will be sent to the console and the console will be responsable to check if the id is there, so the cli won't "know" the convox id because it will be in the token payload. So, your concern is more the person who's configuring the okta knows the user id? Do you have a suggestion about how can we make this link between the okta and our console? Create like a new token for the user and this token to be included in okta ui maybe?

Second:
No, actually the customer needs to configure his own okta application, we don't control. In the video record I just show which requirements the customer needs to do before configure our CLI.

@yuriadams yuriadams requested a review from ntner March 28, 2023 12:37
pkg/cli/sso.go Outdated Show resolved Hide resolved
pkg/cli/sso.go Outdated Show resolved Hide resolved
pkg/cli/sso.go Outdated Show resolved Hide resolved
pkg/cli/sso.go Outdated Show resolved Hide resolved
pkg/cli/sso.go Outdated Show resolved Hide resolved
pkg/cli/sso.go Outdated Show resolved Hide resolved
pkg/cli/sso.go Outdated Show resolved Hide resolved
nightfury1204
nightfury1204 previously approved these changes Mar 31, 2023
Copy link
Contributor

@Twsouza Twsouza left a comment

Choose a reason for hiding this comment

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

the code looks good, just some errors message that could be more helpful to add more context.

pkg/cli/sso.go Show resolved Hide resolved
pkg/cli/sso.go Outdated Show resolved Hide resolved
pkg/cli/sso.go Outdated Show resolved Hide resolved
pkg/cli/sso.go Outdated Show resolved Hide resolved
pkg/cli/sso.go Outdated Show resolved Hide resolved
@yuriadams yuriadams removed the request for review from ntner April 18, 2023 12:14
Twsouza
Twsouza previously approved these changes Apr 18, 2023
nightfury1204
nightfury1204 previously approved these changes Apr 18, 2023
nightfury1204 and others added 8 commits May 1, 2023 17:05
* update to k8s 1.24

* Update golang to 1.17

* Update hashicorp/kubernetes version

* add eks addon dependency

* Fix for do

* Add dependency
…Set. (#627)

* Change rack name (#589)

* Add rack_name to the vars

It will be used to set the rack name

* Add rack name to the nanespace label

* Change the rack name before it's moved to the console or locally

* Fix tests about terraform

* Add rack_name to all providers

* fix: Add fix for aws gov cloud (#609)

* Add fix for aws gov cloud

* Fix attribute not found error when terraform destroy

* Remove outdated code and unused lib (#611)

* Set default value for rack_name on tf files

* Add fix for route table and bucket acl not allowed error

* Store rack install & rack install update telemetry data (#608)

* Cli command to activate or desactivate cli telemetry

* unit test for the cli cmd

* Send telemetry params on rack install and rack params set

* CLI docs

* Changing approach: The telemetry data will be send by the terraform after all infra will be provisioned

* Add resource telemetry on all providers

* Extract telemetry for a different module

* add telemetry param

* Fix unit test

* Define a k8s configmap with the values on vars.json.

* Get params from configmap and sync with metrics app

* Anonymize params

* Redact rack_name

* refactoring to improve quality on the function to create sync cm

* set telemetry var as true as default

* Fix deepsource terraform warnings

* Fix unit tests

* Fix deepsource go warnings

* Change telemetry type to boolean

* create the configmap just if the settings dir is set (#622)

* Try to restrict settings variable to only new releases

* Fix minor version check for telemetry support

* remove prints

* rollback go syntax: init toSync map

* rollback release version check

* Fix deepsource report

* Fix unit tests

* Remove diff strategy

* rack_params as a prefix

* Put telemetry off as default for next release

* fix unit tests

* Fix deepsource

* not necessary update sync configmap anymore

* Put the right minor version supported

* refactoring check if release is supported

* try a patch release

---------

Co-authored-by: Taynan <Twsouza@users.noreply.github.com>
Co-authored-by: Md. Nure Alam Nahid <knightnahid@gmail.com>
Co-authored-by: Taynan Souza <taynan.tws@gmail.com>
* Add spot and on_demand instance count in telemetry

* Put the login in seperate funtion
* Azure fix

* Fix build security context bug

* Update buildkit version
@yuriadams yuriadams dismissed stale reviews from nightfury1204 and Twsouza via a31bd39 May 15, 2023 18:13
@Twsouza
Copy link
Contributor

Twsouza commented May 18, 2023

@yuriadams is this good to review?

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