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

OpenID Connect support #1631

Merged
merged 2 commits into from Apr 9, 2015

Conversation

Projects
None yet
4 participants
@liggitt
Contributor

liggitt commented Apr 7, 2015

1 - flattens oauth idp configs to this:

  identityProviders:
  - name: google
    challenge: false
    login: true
    provider:
      apiVersion: v1
      kind: GoogleIdentityProvider
      clientID: ...
      clientSecret: ...
  - name: github
    challenge: false
    login: true
    provider:
      apiVersion: v1
      kind: GitHubIdentityProvider
      clientID: ...
      clientSecret: ...

2 - adds generic OpenID Connect support. GoogleIdentityProvider could be configured like this instead:

  identityProviders:
  - name: google
    challenge: false
    login: true
    provider:
      apiVersion: v1
      kind: OpenIDIdentityProvider
      clientID: ...
      clientSecret: ...
      urls:
        authorize: https://accounts.google.com/o/oauth2/auth
        token: https://www.googleapis.com/oauth2/v3/token
        userInfo: https://www.googleapis.com/oauth2/v3/userinfo
      extraScopes:
      - email
      - profile
      claims:
        id:
        - sub
        preferredUsername:
        - preferred_username
        - email
        email:
        - email
        name:
        - name
        - email

A generic OpenID provider with a custom CA, standard scopes (openid), and just a user identity claim (sub) would look like this:

  identityProviders:
  - name: myidp
    challenge: false
    login: true
    provider:
      apiVersion: v1
      kind: OpenIDIdentityProvider
      ca: myidp-ca-bundle.crt
      clientID: ...
      clientSecret: ...
      claims:
        id:
        - sub
      urls:
        authorize: https://myidp.example.com/oauth2/authorize
        token: https://myidp.example.com/oauth2/token

@liggitt liggitt changed the title from WIP - OpenID to WIP - OpenID support Apr 7, 2015

@liggitt liggitt changed the title from WIP - OpenID support to WIP - OpenID Connect support Apr 7, 2015

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 8, 2015

Member

Very clever indeed

Member

smarterclayton commented Apr 8, 2015

Very clever indeed

@liggitt liggitt changed the title from WIP - OpenID Connect support to OpenID Connect support Apr 8, 2015

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Apr 8, 2015

Contributor

@deads2k review

Contributor

liggitt commented Apr 8, 2015

@deads2k review

@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot
Member

openshift-bot commented Apr 8, 2015

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1748/)

api.TypeMeta
// ClientID is the oauth client ID
ClientID string

This comment has been minimized.

@deads2k

deads2k Apr 8, 2015

Contributor

Why are we repeating these identical fields?

@deads2k

deads2k Apr 8, 2015

Contributor

Why are we repeating these identical fields?

Claims OpenIDClaims
}
type OpenIDURLs struct {

This comment has been minimized.

@deads2k

deads2k Apr 8, 2015

Contributor

Why make this a separate struct? Grouping urls?

@deads2k

deads2k Apr 8, 2015

Contributor

Why make this a separate struct? Grouping urls?

This comment has been minimized.

@liggitt

liggitt Apr 8, 2015

Contributor

yeah... I couldn't bring myself to name them "urlAuthorize", or to scatter the url fields throughout

@liggitt

liggitt Apr 8, 2015

Contributor

yeah... I couldn't bring myself to name them "urlAuthorize", or to scatter the url fields throughout

case (*api.GoogleIdentityProvider):
allErrs = append(allErrs, ValidateOAuthIdentityProvider(provider.ClientID, provider.ClientSecret, identityProvider.UseAsChallenger)...)
case (*api.OpenIDIdentityProvider):

This comment has been minimized.

@deads2k

deads2k Apr 8, 2015

Contributor

it's time for separate methods for the types.

@deads2k

deads2k Apr 8, 2015

Contributor

it's time for separate methods for the types.

This comment has been minimized.

@deads2k

deads2k Apr 8, 2015

Contributor

Don't we require claims?

@deads2k

deads2k Apr 8, 2015

Contributor

Don't we require claims?

This comment has been minimized.

@liggitt

liggitt Apr 8, 2015

Contributor

we use standard claims ("sub" is id, "name" is display name, "email" is email address, "preferred_username" is preferred username) unless they override them

@liggitt

liggitt Apr 8, 2015

Contributor

we use standard claims ("sub" is id, "name" is display name, "email" is email address, "preferred_username" is preferred username) unless they override them

This comment has been minimized.

@deads2k

deads2k Apr 8, 2015

Contributor

A fully specified config file should be fully specified. Saying that our file is fully specified except for these N fields that are filled in with default values takes us backwards and it makes it impossible to distinguish between "doesn't have this value" and "use your default".

@deads2k

deads2k Apr 8, 2015

Contributor

A fully specified config file should be fully specified. Saying that our file is fully specified except for these N fields that are filled in with default values takes us backwards and it makes it impossible to distinguish between "doesn't have this value" and "use your default".

This comment has been minimized.

@liggitt

liggitt Apr 8, 2015

Contributor

Required at least one ID claim

@liggitt

liggitt Apr 8, 2015

Contributor

Required at least one ID claim

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Apr 8, 2015

Contributor

Once the config file is fully specified, lgtm.

Contributor

deads2k commented Apr 8, 2015

Once the config file is fully specified, lgtm.

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Apr 8, 2015

Contributor
  • Renamed Scopes to ExtraScopes, which are added to the required openid scope
  • Removed claim defaulting
Contributor

liggitt commented Apr 8, 2015

  • Renamed Scopes to ExtraScopes, which are added to the required openid scope
  • Removed claim defaulting
@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Apr 8, 2015

Contributor

[merge]

Contributor

liggitt commented Apr 8, 2015

[merge]

@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Apr 8, 2015

Member

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1511/) (Image: devenv-fedora_1237)

Member

openshift-bot commented Apr 8, 2015

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1511/) (Image: devenv-fedora_1237)

@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Apr 8, 2015

Member

[Test]ing while waiting on the merge queue

Member

openshift-bot commented Apr 8, 2015

[Test]ing while waiting on the merge queue

@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Apr 8, 2015

Member

Evaluated for origin up to a840729

Member

openshift-bot commented Apr 8, 2015

Evaluated for origin up to a840729

openshift-bot added a commit that referenced this pull request Apr 9, 2015

@openshift-bot openshift-bot merged commit 9e304e5 into openshift:master Apr 9, 2015

2 of 3 checks passed

continuous-integration/openshift-jenkins/merge Testing
Details
continuous-integration/openshift-jenkins/test Tested
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@liggitt liggitt deleted the liggitt:openid branch Apr 9, 2015

@liggitt liggitt referenced this pull request Apr 11, 2015

Closed

config - make auth config #1339

jboyd01 added a commit to jboyd01/origin that referenced this pull request Feb 6, 2018

Squashed 'cmd/service-catalog/go/src/github.com/kubernetes-incubator/…
…service-catalog/' changes from d969acde90..b69b4a6c80

b69b4a6c80 origin build: modify hard coded path
527fac4d02 origin build: add origin tooling
545ffdb chart changes for v0.1.5 release (#1709)
4d9be8f Use userInfo for Originating-Identity so extras is correct. (#1702)
f358b99 Call destroy function on each storage interface (#1705)
36b5de9 refactor binding reconciliation functions (#1687)
5699360 Change binding_retrievable to bindingRetrievable
0d8bcfe thread through stopCh to DestroyFunc (#1671)
1c45aef Migrate from glide to dep for dependency management (#1670)
1cf0dd9 Add svcat to Makefile (#1683)
45b1013 make verify validates that versioned APIs contain json tags for fields, addresses #1303 (#1480)
0ee8398 Build the integration test binary before running any tests (#1666)
0fe0aa7 Update design.md (#1674)
1280d24 controller requires permission to update secrets (#1663)
129d98e Contribute svcat (#1664)
ff9739b Update dependencies to kubernetes-1.9.1 (#1633)
9c36019 chart changes for v0.1.4 release (#1669)
93319f6 move apiserver generation to script and verify (#1662)
385f0da refactor service instance provision/update/poll reconciliation (#1648)
e015212 run each integration test individually (#1661)
412e242 Tell people whether we're checking external hrefs (#1659)
ae05361 retry failed unbind requests (#1653)
7eae845 doc for setting up Service Catalog with Prometheus metrics (#1654)
0720cf9 minor README copy edit (#1656)
8bd347d run some integration subtests in parallel (#1637)
b83800c Use $ and console to indicate multi-command blocks
789c4b2 Use dynamic reaction to fix data race (#1650)
f1be763 only check external hrefs on master (#1652)
65c6d20 Controller-manager crash loops if API server is not available on startup (#1376) (#1591)
9225c92 embedded etcd is the way of the future for our tests (#1651)
605c952 Fix required fields in OpenAPI schema (#1602)
899ca21 Revert "Switch to wget for integration apiserver checks (#1384)" (#1585)
2f496ee Update code-of-conduct.md (#1635)
c1c69cf Build the e2e binary in CI (#1647)
4e2dcef Wait for successful API discovery (#1646)
5ae6d99 Bump copyright date in generated code (#1645)
8be5b05 Serve OpenAPI spec only when --serve-openapi-spec switch is enabled (#1612)
19fb30e silence go-restful logging output (#1622)
fdbabf0 Add walkthrough link back (#1620)
7c73e9a Add link to main k8s docs on service-catalog (#1627)
f59adc9 Overhauling the design document (#1619)
cd7b633 Updating the install documentation (#1616)
f6e5441 fix compilation error from updated util.WaitForBindingCondition() (#1629) (#1631)
54e57af Provide OSB Client proxy to instrument with metrics (#1615)
026b86f Disable test added in 1611 that contains data race (#1626)
cb735a6 Add integration tests for ServiceInstances (#1611)
67dbabb Cleaning up the docs README (#1618)
6bddc07 remove email from docker login during Travis deploy (#1614)
a604bc3 Use ConfigMaps for leader election (#1599)
c6f193a Add controller integration tests for ServiceInstance create and update (#1578)
26cf23b Rename OWNERS assignees: to approvers: (#1508)
1163edc expose Prometheus metrics from Controller (#677) (#1608)
2cd6554 Clean up docs/ folder (#1609)
1d7e96d Adding Service Binding Create Integration Tests (#1580)
6a4c469 Make the maximum polling backoff configurable (#1607)
31bbf55 Rename the imported package to avoid name conflict (#1603)
3cdd556 Add validation for broker spec to SAR admission controller (#1605)
a3408ce fix docker volume mount when building with docker under SELinux (#1500) (#1534)
307e747 Remove unneeded vendors of vendors (#1596)
770fc74 Make ups-instance.yaml in walkthrough to demonstrate good practices (#1592)
9112ba1 Add links to docs/README (#1589)
8902648 Add additional service to ups-broker to fix e2e (#1583)
0bcbc7d move instance update logic out of reconcileServiceInstanceDelete (#1584)
7ef5a3e Do not send Parameters field when there are no parameters from sourced secret (#1559)
4c51b25 Remove unneeded code that sets reason for provision/update call failure (#1561)
b122cb9 fix bind injection failure not being persisted in API server (#1546)
66421d5 Clear out current operation when starting reconciliation of a delete (#1564)
8cca70a Send an empty object for Parameters when deleting all parameters of a ServiceInstance (#1555)
270426c Add controllerTest type as a helper for running controller integration tests. (#1577)
e26c2d7 Ignore .vscode folder in project root (#1579)
REVERT: d969acde90 Add additional service to ups-broker to fix e2e (#1583)
REVERT: 1bcd53b684 origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: b69b4a6c8003f25d040e3087c7b1b16d1854a9e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment