Skip to content

Conversation

@rcernich
Copy link

@rcernich rcernich commented Mar 2, 2015

Adds support for using Keycloak as an external authentication handler. The handler is configured by loading a json formatted file containing the Keycloak client configuration (realm, auth url, client id, client secret, etc.). This information is generated by Keycloak, which simplifies creation of the file (i.e. copy/paste).

@rcernich rcernich force-pushed the keycloak_integration branch from 33a4b24 to a664959 Compare March 2, 2015 17:04
@rcernich
Copy link
Author

rcernich commented Mar 2, 2015

Not sure what the failure is coming from. Is the test case in question unstable, a timing issue perhaps? (it seems to have passed on the other two builds; other pr's seem to have similar problems with the test).

Copy link
Contributor

Choose a reason for hiding this comment

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

state.Encode() handles QueryEscape-ing the values

Copy link
Author

Choose a reason for hiding this comment

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

I was having an issue where the URL was not encoded and part of "then" was being processed through the main query, specifically request_type was not part of "then" but was seen as a separate parameter (e.g. state, then, request_type)

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can see, state is getting encoded correctly on our side. This is working with both Google and Github as external OAuth providers (queries wrapped for readability):

  1. Console redirects to OpenShift OAuth /authorize endpoint:

    GET https://localhost:8443/oauth/authorize?
    client_id=openshift-web-console&
    response_type=token&
    state=%2F&
    redirect_uri=https%3A%2F%2Flocalhost%3A8444%2Foauth

  2. OpenShift OAuth /authorize endpoint redirects to Google:

    < Location: https://accounts.google.com/o/oauth2/auth?
    access_type=offline&
    client_id=...&
    include_granted_scopes=true&
    redirect_uri=https%3A%2F%2Flocalhost%3A8443%2Foauth2callback%2Fgoogle&
    response_type=code&
    scope=profile+email&
    state=csrf%3D...%26then%3D%252Foauth%252Fauthorize%253Fclient_id%253Dopenshift-web-console%2526response_type%253Dtoken%2526state%253D%25252F%2526redirect_uri%253Dhttps%25253A%25252F%25252Flocalhost%25253A8444%25252Foauth

  3. Google does stuff

  4. Google redirects back to OpenShift OAuth callback:

    < Location: https://localhost:8443/oauth2callback/google?
    state=csrf%3D...%26then%3D%252Foauth%252Fauthorize%253Fclient_id%253Dopenshift-web-console%2526response_type%253Dtoken%2526state%253D%25252F%2526redirect_uri%253Dhttps%25253A%25252F%25252Flocalhost%25253A8444%25252Foauth&
    code=...&
    scope=https://www.googleapis.com/auth/userinfo.profile+https://www.googleapis.com/auth/userinfo.email

  5. OpenShift OAuth callback validates the code, parses the state, and redirects back to OpenShift /authorize:

    < Location: /oauth/authorize?
    client_id=openshift-web-console&
    response_type=token&
    state=%2F&
    redirect_uri=https%3A%2F%2Flocalhost%3A8444%2Foauth

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make sure keycloak is not altering the state passed to it? For example, the state sent to Google in step 2 is echoed back exactly in step 4. Seems like keycloak might be doing an extra decode, not treating state as an opaque value

Copy link
Author

Choose a reason for hiding this comment

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

I'll check that. I suspect you're right. In my tests, I was getting redirected to oauth/authorize?client_id=openshift-web-console (no other parameters).

Copy link
Contributor

Choose a reason for hiding this comment

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

For maximum compatibility, it might be worth changing our DefaultState impl to base64 encode and decode, just to avoid issues with external providers accidentally decoding state. Still, checking/fixing keycloak state handling as necessary should probably happen as well...

@rcernich
Copy link
Author

rcernich commented Mar 2, 2015

Thanks for the detailed feedback. I'll clean it up and investigate the issue with keycloak not preserving state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the keycloak config have a way to specify the trusted roots of the auth server? That wasn't an issue for github/google, but it likely will need to be able to be specified when hitting a self-hosted auth server. The Provider interface would need a new GetTransport() method which could return nil by default, and be overridden if trusted roots needed to be set.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also means that if the provider config will include a file reference for a trusted cert bundle, NewProviderFromConfigData would need a path to resolve file references relative to

Copy link
Author

Choose a reason for hiding this comment

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

Good point. The documentation doesn't cover any of this. I suspect this is left up to the user to ensure they've configured their key/trust stores appropriately. I'll walk through their ssl example and see what shakes out. I should probably also test ssl on the OS side of things as well.

Copy link
Author

Choose a reason for hiding this comment

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

I incorporated the other feedback, but was not able to get an https connection working. Looking at the code, it appears the auth transport is configured with the root certs for the OS instance. I was able to confirm that the cert used by the KC application was being picked up and included in that list and while I was able to get past some authorization errors (e.g. no SAN IPs), I was not able to get past "certificate signed by unknown authority." This was even when I used the OS CA cert to sign the cert used by KC. This is pretty far outsided my realm of expertise and it's possible I just haven't created the certificate properly (e.g. the SAN IPs issue above). Regardless, the change should be good to go, barring the fact that you can't use https for the KC auth server at this time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the outgoing request to the external OAuth server actually uses the system trusted roots, so I'm not surprised it's not picking up the OpenShift certs

@rcernich rcernich force-pushed the keycloak_integration branch from a664959 to 25bda52 Compare March 11, 2015 16:04
@rcernich
Copy link
Author

I'm not sure why the asset tests are failing, nor can I find the actual errors in the log (don't know what to look for).

Copy link
Contributor

Choose a reason for hiding this comment

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

use base64.URLEncoding to encode/decode since we're going to use the value in a url

@liggitt
Copy link
Contributor

liggitt commented Mar 11, 2015

looks good, thanks... we have a PR in progress that is moving some of the startup bits around, so I'll hold this one until that goes in.

@liggitt
Copy link
Contributor

liggitt commented Mar 11, 2015

don't worry about the asset tests, those were fixed in another branch

@rcernich rcernich force-pushed the keycloak_integration branch from 25bda52 to b26abff Compare March 12, 2015 17:48
@rcernich
Copy link
Author

Updated the PR based on your comments (URLEncoder, no fail if handler init fails, make sure realm name is encoded properly). Regarding the cert stuff, I thought this might have taken care of things, but it doesn't (https://github.com/openshift/origin/blob/master/pkg/cmd/server/origin/auth.go#L286). Thanks again for all the feedback.

@rcernich rcernich force-pushed the keycloak_integration branch from b26abff to 197519c Compare March 25, 2015 17:26
@rcernich
Copy link
Author

rebased and push -f'd. should be ready to go.

@liggitt
Copy link
Contributor

liggitt commented Mar 25, 2015

Thanks, this is on my list... should get to it soon...

@openshift-bot
Copy link
Contributor

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

@liggitt
Copy link
Contributor

liggitt commented Mar 31, 2015

@rcernich I haven't forgotten about this... we're working on externalizing OAuth config in #1426... once that's done, I'll look at pulling this in

@liggitt
Copy link
Contributor

liggitt commented Apr 7, 2015

FYI, pulled in base64 encoding in #1608

Since Keycloak has OpenID Connect support, I'd like to see if we can integrate with both Keycloak and Google using a single configurable OIDC identity provider.

@liggitt
Copy link
Contributor

liggitt commented Apr 9, 2015

#1631 should allow Keycloak to be supported as a generic OpenID Connect provider.

@rcernich can you see if you can sign in using your Keycloak server by doing the following:

1 - Generate a master config by running openshift start master --write-config --config=master.yaml

2 - Edit the master.yaml config file and replace the identityProviders: stanza with this:

  identityProviders:
  - name: keycloak
    challenge: false
    login: true
    provider:
      apiVersion: v1
      kind: OpenIDIdentityProvider
      ca: /path/to/keycloak-ca.crt
      clientID: YOUR_CLIENT_ID
      clientSecret: YOUR_CLIENT_SECRET
      claims:
        id:
        - sub
        preferredUsername:
        - preferred_username
        name:
        - name
        email:
        - email
      urls:
        authorize: https://YOUR_SERVER/auth/realms/YOUR_REALM/tokens/login
        token: https://YOUR_SERVER/auth/realms/YOUR_REALM/tokens/access/codes

3 - Start OpenShift with openshift start master --config=master.yaml

You should be able to log into the web console. If you can, I'd be curious to see the output of the following (run as cluster-admin):

  • osc get users -o json
  • osc get identities -o json

@liggitt
Copy link
Contributor

liggitt commented Apr 9, 2015

@liggitt liggitt closed this May 6, 2015
jboyd01 pushed a commit to jboyd01/origin that referenced this pull request Sep 14, 2017
…service-catalog/' changes from ef63307bdb..ae6b643caf

ae6b643caf Use oc adm instead of oadm which might not exist in various installations.
66a4eb2a2c Update instructions... will remove once documented elsewhere
1b704d1530 replace build context setup with init containers
ee4df18c7f hack/lib: dedup os::util::host_platform and os::build::host_platform
1cd6dfa998 origin: Switch out owners to Red Hatters
664f4d318f Add instructions for syncing repos
2f2cdd546b origin-build: delete files with colon in them
cdf8b12848 origin-build: don't build user-broker
ebfede9056 origin build: add _output to .gitignore
55412c7e3d origin build: make build-go and build-cross work
68c74ff4ae origin build: modify hard coded path
3d41a217f6 origin build: add origin tooling
a8fc27d Fix typos in walkthrough  (openshift#1224)
e77edbf openshift#1157: Limit the amount of time for reconciliations (openshift#1196)
1b1a749 temporarily disabled verify-links.sh from the verify target (openshift#1226)
acf8fab Send originating identity headers in OSB requests (openshift#1162)
821ba16 new admission controller to block updates to service instance updates that (openshift#1210)
d69c5e5 Minor improvement to godoc in binaries (openshift#1211)
5b81814 fix typos (openshift#1221)
836dc4a Adds how to download Helm chart (charts/catalog) (openshift#1219)
2fd0115 Fix "visit the project on github" link. (openshift#1217)
325e4b6 Add how to set $GOPATH. (openshift#1218)
68b775f Update the installation (openshift#1199)
6e3a3c1 v0.0.19 (openshift#1207)
8b69791 Removing errexit from TLS setup script (openshift#1206)
273260f Instance deletion lifecycle enhancements, issue openshift#820 (openshift#1159)
c050713 fix cleaning of build output for non-root users (openshift#1205)
5995df1 Merge branch 'pr/1204'
72f4802 Remove osb prefix from example ServiceClass (openshift#1201)
f9dbd4e pin all dependencies in glide to current version except for glog where we want to pick up the prior version to fix issue 1187.
f148bc5 v0.0.18 (openshift#1202)
b86ab8d Removing the helm install command (openshift#1185)
3cff482 Remove Alpha* prefix on all API fields for issue openshift#1180 (openshift#1184)
154b74d Fix gofmt issue (openshift#1192)
2ee894a do the clean before building an arch (openshift#1179)
b4976ef Fix bad URL (openshift#1189)
cd3dede Fix hrefs again (openshift#1190)
f066226 Design: Instance/Binding parameters (openshift#1075)
eb37682 This generated file is missing from master (openshift#1191)
28c0ae7 Use generation instead of checksum for Instances and InstanceCredentials (openshift#1151)
5cdd323 Fix bad href (openshift#1188)
8a892f0 handle lingering polling cases (openshift#1174)
f5fabd6 remove TPRs from Jenkins e2e pipeline (openshift#1175)
717df78 Add godoc explaining that Instance and InstanceCredential specs are immutable (openshift#1182)
REVERT: ef63307bdb origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: ae6b643cafd3a17412f173e70ed7c1a2e39ee549
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Feb 1, 2018
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.

3 participants