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

Add default GitHub API URLs to SSO connector #31397

Merged
merged 1 commit into from
Sep 5, 2023
Merged

Conversation

tcsc
Copy link
Contributor

@tcsc tcsc commented Sep 4, 2023

tctl sso configure github was producing an untestable connector spec, or at least a spec that would always fail when tested with tctl sso test, due to missing GitHub endpoints in the generated spec.

This change adds the default GitHub endpoint URLs as default values for the endpoint flags in tctl sso configure github so that the produces spec is valis and testable.

Note: Our WebUI appears to magically add these values when saving
a connector, so this issue only really effects tctl sso test

Includes doc update to match new output.

Fixes: #31396
Changelog: tctl sso configure github now includes default Github endpoints

`tctl sso configure github` was producing an untestable connector
spec, or at least a spec that would _always fail_ when tested with
`tctl ssl test`, due to missing GitHub endpoints in the generated
spec.

This change adds the default GitHub endpoint URLs as default values
for the endpoint flags in `tctl sso configure github` so that the
produces spec is valis and testable.

Note: Our WebUI appears to magically add these values when saving
      a connector, so this issue only really effects `tctl sso test`

Includes doc update to match new output.

Fixes: #31396
Changelog: `tctl sso configure github` now includes default Github endpoints
@@ -279,12 +279,12 @@ func (c *GithubConnectorV3) SetDisplay(display string) {

// GetEndpointURL returns the endpoint URL
func (c *GithubConnectorV3) GetEndpointURL() string {
return githubURL
return GithubURL
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is anyone else curious why this always returns a constant? Is this because SSO for self-hosted github is an enterprise-only feature?

@smallinsky smallinsky requested a review from Tener September 4, 2023 06:48
@Tener
Copy link
Contributor

Tener commented Sep 4, 2023

I've given the old code a cursory look before and from my understanding the validation is being too strict here; in particular, you can expect there to be some existing configs that are correct (using public GitHub, so these fields can be empty) but will now be rejected.

Sounds wrong, and I'd be curious to see where exactly the error is being thrown:

$ tctl sso configure github --id $(cat ./github.client-id) --secret $(cat ./github.secret) --teams-to-roles GGUXGXM3Q5QDN6CD,Admin,editor -r GGUXGXM3Q5QDN6CD,everyone,access | tctl sso test
INFO [CLIENT]    RedirectURL empty, resolving automatically.
INFO [CLIENT]    RedirectURL set to "https://sso.dev-trent.net:443/v1/webapi/github/callback"
ERROR: Failed to create auth request. Check the auth connector definition for errors. Error: rpc error: code = Unknown desc = url scheme is empty

@tcsc
Copy link
Contributor Author

tcsc commented Sep 4, 2023

the validation is being too strict here

I don't think this is necessarily a validation problem, and I certainly didn't add any - I just changed the generated connector so that it works in the test.

Sounds wrong, and I'd be curious to see where exactly the error is being thrown

Given your comments that it sounded wrong, I dug a bit deeper. Turns out this is what happens when you use an Enterprise tctl to test the sso connector. Using an OSS tctl works as expected.

The error was coming from deep inside the auth server. I suspect that the connector was being treated as a private GitHub one so not using the global defaults I flagged in the code review above. This ends up with the auth server being given empty endpoint URLs and failing when trying to parse them.

All in all it's a bit confusing confusing as a user, as both flavours of tctl look largely identical to the outside world.

@Tener
Copy link
Contributor

Tener commented Sep 4, 2023

All in all it's a bit confusing confusing as a user, as both flavours of tctl look largely identical to the outside world.

Agreed. I think @capnspacehook may have the best idea of what needs fixing in this context.

@zmb3 zmb3 changed the title Adds default Github API urls to SSO connector Add default GitHub API URLs to SSO connector Sep 4, 2023
sub.Flag("endpoint-url", "Endpoint URL for GitHub instance.").
PlaceHolder("URL").
Default(types.GithubURL).
StringVar(&spec.EndpointURL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's a URL, should we use URLVar instead of StringVar?

@tcsc tcsc added this pull request to the merge queue Sep 5, 2023
@tcsc tcsc removed this pull request from the merge queue due to a manual request Sep 5, 2023
@tcsc tcsc added this pull request to the merge queue Sep 5, 2023
Merged via the queue into master with commit 4524974 Sep 5, 2023
@tcsc tcsc deleted the tcsc/gihub-connector branch September 5, 2023 23:44
@public-teleport-github-review-bot

@tcsc See the table below for backport results.

Branch Result
branch/v11 Failed
branch/v12 Failed
branch/v13 Create PR
branch/v14 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tctl sso configure github produces an untestable connector spec
5 participants